guacamole-client
guacamole-client copied to clipboard
apache
Allow SSH-Key with at least 4096 bits
error before updating mysql table : 12:41:39.036 [http-nio-8080-exec-10] ERROR o.a.g.rest.RESTExceptionWrapper - Unexpected internal error:
Error updating database. Cause: com.mysql.jdbc.MysqlDataTruncation: Data truncation: Data too long for column 'parameter_value' at row 2
The error may involve org.apache.guacamole.auth.jdbc.connection.ConnectionParameterMapper.insert-Inline
The error occurred while setting parameters
SQL: INSERT INTO guacamole_connection_parameter ( connection_id, parameter_name, parameter_value ) VALUES (?, ?, ?) , (?, ?, ?) , (?, ?, ?) , (?, ?, ?) , (?, ?, ?) , (?, ?, ?) , (?, ?, ?) , (?, ?, ?) , (?, ?, ?) , (?, ?, ?)
Cause: com.mysql.jdbc.MysqlDataTruncation: Data truncation: Data too long for column 'parameter_value' at row 2
Hi @alexandregaltier,
Please see our contribution guidelines. In particular, absolutely every change to Guacamole needs a corresponding issue in JIRA, and that issue needs to be tagged in the commit message (see the commit history of the repository for examples there).
As for the changes itself:
`parameter_value` varchar(65353) NOT NULL,
65353 is awfully large. I have not checked myself, but I know from tangential experience that some databases have issues with large columns that cause the row to be stored inefficiently, slowing down any queries which deal with the table. Have you looked into whether MySQL or PostgreSQL are affected in such a way? Any particular reason why you chose that specific value?
Assuming there are no issues with increasing the size to that degree, beware the same change will need to be made to the PostgreSQL schema, as well. This change is incomplete if it only touches the MySQL schema.
Also beware that these changes would also need to be made in an incremental way in an upgrade script (see the upgrade scripts for past releases for examples). Users of Guacamole which use an older version of the schema need a non-destructive upgrade path that allows them to continue using their existing data.
65353 is awfully large.
I have a 4096 bit SSH key, and the private key totals 3326 characters - the public key is 750. I don't know off the top of my head how much that character count will vary, but pushing to 65K characters seems a little on the side of overkill.
7927 will be enough :
id_rsa_1024 891
id_rsa_2048 1679
id_rsa_3072 2459
id_rsa_4096 3243
id_rsa_5120 4023
id_rsa_6144 4803
id_rsa_7168 5587
id_rsa_8192 6363
id_rsa_9216 7147
id_rsa_10240 7927
On 10/07/2017 15:26, necouchman wrote:
65353 is awfully large. I have a 4096 bit SSH key, and the private key totals 3326 characters - the public key is 750. I don't know off the top of my head how much that character count will vary, but pushing to 65K characters seems a little on the side of overkill.— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apache/incubator-guacamole-client/pull/175#issuecomment-314104686, or mute the thread https://github.com/notifications/unsubscribe-auth/AI7fKw50JJ1KDSj7NuDJVizbtUp45cn-ks5sMiaEgaJpZM4OSDYk.
Okay, so I'll throw out maybe make it 8192 characters to make it nice factor of 2 and provide a little padding? Can you make that change and commit it, and also address the PostgreSQL side of things, and hopefully we can get this into the master branch?
I concur on the nice power-of-two, BUT: we definitely need to look into whether this is going to kill query performance for anything dealing with guacamole_connection_parameter.
In MySQL there's an automatic limit where if a varchar column is longer than 768 bytes it will automatically be treated like a text column. Postgre has a similar clause, though I couldn't find the limit for that.
So in this case, query performance will not be affected. Typing 65536 is no different than typing 30000 or 80000 or 150000. This is just a theoretical limit for the column, the column will actually be saved with variable length outside the main table, as a text or blob column.
Transfer time is also negligible, you usally don't have your MySQL server on a slow connection to your appserver.
Another thing: tomcat goes up to 20% cpu usage (for me, at least) when a lot of stuff is moving in my RDP session. Compared to this, running a query ONCE per session (to retrieve keys) is surely negligible, isn't it?
https://www.postgresql.org/docs/9.1/static/datatype-character.html
http://www.mysqlperformanceblog.com/2010/02/09/blob-storage-in-innodb/
So in this case, query performance will not be affected.
OK, then. If queries in general will not be affected, and the only possible performance hit will be to queries specifically involving the large values, then sounds fine.
Another thing: tomcat goes up to 20% cpu usage (for me, at least) when a lot of stuff is moving in my RDP session. Compared to this, running a query ONCE per session (to retrieve keys) is surely negligible, isn't it?
No, not necessarily. In this case, there are two systems in question: the remote desktop translation and processing system, which is inherently processing-intensive compared to other things, and a database, which should not be processing-intensive. It is expected that large Guacamole deployments will require a guacd cluster, but if changes result in significantly reducing the scalability of the database portion of a typical deployment, then that's a bad change.
That said, from what you're saying here, it sounds like that isn't the case.
@alexandregaltier: Looks like this is just about ready to go, just need the JIRA issue, need to change the title of the pull request to include the JIRA issue, and need to make sure the commits are properly tagged with the JIRA issue.
@mike-jumper Well, my problem is really not guacd. guacd has moderate usage throughout (playing a youtube video shows really high activity, which is expected, but staying consistently very low in the single digit decimals during idle) but tomcat just stays at a higher usage than guacd through the entirety of the connection (like 5% during the whole time, 10% on load). This is all with a single connection only.
That's for another topic, but I have a fleeting thought that the Java server probably has a small performance issue
Hi,
I created issue GUACAMOLE-350
Regards,
On 19/07/2017 20:51, sinni800 wrote:
@mike-jumper https://github.com/mike-jumper Well, my problem is really not guacd. guacd has moderate usage throughout but tomcat just stays at a higher usage than guacd through the entirety of the connection (like 5% during the whole time). That's for another topic, but I have a fleeting thought that the Java server probably has some performance issues
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apache/incubator-guacamole-client/pull/175#issuecomment-316481864, or mute the thread https://github.com/notifications/unsubscribe-auth/AI7fK-qhjAPxUHqun3i-GRq_xLJcKtSKks5sPlAngaJpZM4OSDYk.
Well, my problem is really not guacd. guacd has moderate usage throughout (playing a youtube video shows really high activity, which is expected, but staying consistently very low in the single digit decimals during idle) but tomcat just stays at a higher usage than guacd through the entirety of the connection (like 5% during the whole time, 10% on load). This is all with a single connection only.
I suggest modifying Tomcat's server.xml to use the NIO connector - that should reduce load due purely to Tomcat's processing of traffic. I recently ran some load tests across Guacamole clusters of various sizes, and found that Tomcat would be a bottleneck in some situations if the NIO connector was not in use.
If the above doesn't help, we should probably continue this portion of the discussion on the mailing list. It's no longer relevant to this particular set of changes.
@alexandregaltier:
I created issue GUACAMOLE-350
Great! As soon as you edit the pull request and commit messages and prepend the GUACAMOLE-350, and add a new commit to change 65535 to 8192, we'll be ready to merge this!
Hi, Thx for reminding. At last i'm on Holliday. I will try to do it this week. Have a nice day.
On August 11, 2017 11:14:24 PM GMT+02:00, necouchman [email protected] wrote:
@alexandregaltier: Ping - any progress here?
-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/apache/incubator-guacamole-client/pull/175#issuecomment-321920542
-- Sent from my Android device with K-9 Mail. Please excuse my brevity.
@alexandregaltier Any chance you can finish this up so we can get it merged??
NOTE: Per recent discussion on the mailing lists, the release process for 0.9.14-incubating has begun, and the issue associated with this change (GUACAMOLE-350) is within scope. @alexandregaltier, if this PR is alive, please update the merge base of this PR to point to staging/0.9.14-incubating.
If dead, the PR which takes the place of this one will use the staging/0.9.14-incubating merge base.
@alexandregaltier Pinging one more time to try to get this finished up for the 0.9.14 release.
NOTE: Per discussion on GUACAMOLE-350 in JIRA: as all other 0.9.14 issues have been completed, GUACAMOLE-350 has been removed from scope to allow the release process to move forward.
Please disregard my previous message asking that the PR be based against staging/0.9.14. It would be nice to see forward progress here, but it is no longer blocking 0.9.14.