[JEP-222] WebSocket-based agents #357
Conversation
…r implementations.
…a hard dependency on Socket.
…hough its classes & services remain unshaded for now.
People seem to really like being able to control connectivity, particularly for complex things like agents, from a single, central location. I'm not sure that's a real requirement, though. Will there be some use case or scenario in which that capability is required? I'm not coming up with one -- it's probably best to not bother until we identify the need. |
Well we offer controls about listening on ports, for good reason. But |
Co-Authored-By: Jeff Thompson <[email protected]>
…or about multiple -url.
…ain.CuiListener etc. already exit the JVM in this case.
There was a problem hiding this comment.
This looks nice. Everything is in good shape and this will make for a great addition to the agent capabilities.
The code all looks good. I've reviewed the latest changes.
I've tested the capabilities with agents. Things behave fine. My testing has been mostly happy-path. I've tested out some conflicting or incorrect parameters but I've only tested on a reliable network with straightforward configurations and use. I haven't done any robustness testing. I haven't tested the reconnect capabilities.
|
We should probably wait a day or two to see if anyone else wants to review but after that I'm satisfied that we can merge this and release it whenever we want. |
There was a problem hiding this comment.
It looks good overall, just comments about better rollout (beta feature, 4.0).
I have not checked the code in runtime, but it might make sense that the WebSocket mode works well with thread UnhandledException failover for NIo threads in JNLP4. Can be just verified by looking at the list of running threads in the websocket mode
Not sure what you are saying. |
This was suggested for the removal of the deprecated protocols, but too late. I have no strong opinion, and anyway consider the version numbers in this repository to be more or less meaningless—it is just a bundled component of Jenkins. Will leave it to @jeffret-b to decide this sort of thing. |
|
The version numbers here really are mostly meaningless. It's a bundled Jenkins component. It's recommended to keep agent versions current with the server. There have been bigger user-facing changes in the past that haven't rolled the major version, such as upgrading Java versions or the deprecation of the older protocols. Changing the minimum version for the server is a bigger breaking change. If this were a generic library incorporated into other tools, it would make sense to follow semver. I'll go ahead and update it to 4.x, though. (It kind of looks like we update for adding major new functionality but not for breaking dependencies or changes.) |
|
I have been planning to release this later today if I don't get distracted with something else. @oleg-nenashev, if you want to clarify your comment about "UnhandledException failover for NIo threads" I can delay. Especially since you've approved, it seems like that could be handled in a follow-on if we discover something needed there. |
jeffret-b
added
ready-to-merge
major-rfe
* Playing with WebSocket connections. * Pluggable handlers. * errorWithoutStack * Server-side keepalive pings. * Redesigned to work as an HttpResponse. * Unused dep. * Comment on JnlpSlaveRestarterInstaller. * Sketch of WebSocket-based endpoint for JNLPLauncher. * Comment. * Test enhancement. * Making it more obvious from JNLPLauncherTest where remoting.jar is being loaded from. * Reworked protocol to negotiate remote capabilities. * Unhelpful comment. * Simplifying handshake to use HTTP headers. * Moving code into top-level classes and otherwise prettifying. * Linking to upstream PRs. * Timestamped snapshot + incremental. * Picking up incrementalified build of winstone. * Working around jenkinsci/lib-access-modifier#17. * Finally have an incrementalified build of remoting. * Use -webSocket option. * Handling some close and error methods. * Sending X-Remoting-Minimum-Version. * If hudson.remoting.jnlp.Main._main fails, show the error. * Comment. * GUI analogue of jenkinsci/remoting@86cea5b. * Missing since tags. * Capitalization. * WebSockets.isSupported * Rather than hiding JNLPLauncher.DescriptorImpl when the TCP port is disabled, always display it, but show form validation appropriate to WebSocket or TCP mode. * Form validation fixes. * Minor test improvements. * Reworked WebSocketAgents to be compatible with JnlpAgentReceiver. * Removing WebSocketSession.keepAlive in favor of a global setting applicable to all services. * Add a -webSocket option to the Jenkins CLI. * Using a snapshot deployment of Remoting, pending INFRA-2379. * After #3838 there is no reason to recheck authentication after parsing CLICommand arguments. * Advertise the -webSocket option. * Adapt to newer HtmlUnit. * HtmlUnit/htmlunit#29 seems to have been incompatible. WebClient.addRequestHeader will no longer override a header in a WebRequest, such as when the same WebClient/WebRequest was previously used with different headers. * Tracked down a behavioral change in passing through URL-encoded path characters. HtmlUnit/htmlunit@2c49568 picks up apache/httpcomponents-client@8c04c6a which is the actual change. * Disabled TCP port does not matter in WebSocket mode. * Shade dependencies needed for jenkins-cli.jar. * Help text edit. * jenkinsci/winstone#79 was released as 5.5. * jenkinsci/jenkins-test-harness#183 released as 2.59. * #4387 (comment) * Bumping remoting to a new deployed snapshot. * Need jenkinsci/winstone#86. * jenkinsci/winstone#86 released as 5.6. * Introduced constant for X-Remoting-Minimum-Version. * s/slave/agent/ in GUI * Removing in-JVM test, as it was no longer useful after introducing shading anyway. * Bump. * No need to check for anonymous CONNECT here. * s/jenkins.slaves/jenkins.agents/g for new code. * Restrict the diagnostic endpoint to administrators. * Fixed Javadoc import after package move. * jenkinsci/remoting#357 released as 4.0.
|
@jglick @oleg-nenashev @jeffret-b Hi! |
|
@Asgoret file an issue in JIRA with steps to reproduce. |
|
@jglick and what's the link to the jira? |
See JEP-222.