Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-44100] - Detach JNLP protocol management logic to a plugin #2875

Conversation

@oleg-nenashev
Copy link
Member

oleg-nenashev commented May 7, 2017

This is a "weekend hack project" attempt to move some boilerplate code outside the core in order to make it more maintainable and to allow applying changes to earlier core versions. Pull request on another side: oleg-nenashev/agent-remoting-support-plugin#1

See JENKINS-44100.

Details:

  • JENKINS-44100 - Detach JNLP and CLI protocol handlers and aux stuff to the plugin
  • Provide new extension points, which allow retaining the original behavior of the core when the detached plugin is installed.
  • Add a last resort NoOpComputerLauncher, which is being used as a default fallback when the detached plugin is not installed
  • JENKINS-44101 - provide API for shipping custom Remoting.jar to agent endpoints like jnlpJars/slave.jar (to be implemented in the detached plugin)

TODOs:

  • Decide which other components need to be detached. E.g. some bits of TcpSlaveAgentListener, PingProtocol and Pinging logic, PingThread, etc. could be candidates
  • Decide if the CLI stuff should go to another detached plugin (Remoting CLI Support?). It seems reasonable, because this mode is deprecated
  • Hide NoOpComputerLauncher from UI
  • Register the plugin as a detached plugin

Changelog entries

Proposed changelog entries:

  • Entry 1: TBD
  • ...

Submitter checklist

  • JIRA issue is well described
  • Link to JIRA ticket in description, if appropriate
  • Appropriate autotests or explanation to why this change has no tests
  • For new API and extension points: Link to the reference implementation in open-source (or example in Javadoc)

Desired reviewers

@reviewbybees, esp. @stephenc

@mention

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented May 7, 2017

Also CC @batmat, because some bits of https://github.com/jenkinsci/versioncolumn-plugin can be probabl moved to this plugin

@jglick

This comment has been minimized.

Copy link
Member

jglick commented May 8, 2017

I do not see why the CLIProtocol and CLIProtocol2 are included here. Seem unrelated to the rest of the change, which is clearly about JNLP agents. Yes they both happen to implement AgentProtocol and make use of hudson.remoting.* but that is about it.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented May 8, 2017

@jglick

I do not see why the CLIProtocol and CLIProtocol2 are included here. Seem unrelated to the rest of the change, which is clearly about JNLP agents. Yes they both happen to implement AgentProtocol and make use of hudson.remoting.* but that is about it.

I agree, see my TODO list. I consider creating a detached "Remoting CLI Plugin", which would rely on the "Agent Remoting Support Plugin". This dependency is not ideal, but IMHO it is fine since this CLI Plugin immediately becomes "deprecated"

Copy link
Member

jglick left a comment

Too much API debris being left behind in core. If you are going to move the JNLP launcher to a plugin, make a clean break of it.

*/
@Deprecated
@Extension @Symbol("cli")
public class CliProtocol extends AgentProtocol {

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Member

I think this should be left alone.

*/
@Deprecated
@Extension @Symbol("cli2")
public class CliProtocol2 extends CliProtocol {

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Member

Ditto this.

@@ -348,9 +349,14 @@ public FilePath getRootPath() {
/**
* Web-bound object used to serve jar files for JNLP.
*/
public static final class JnlpJar implements HttpResponse {
public static class JnlpJar implements HttpResponse {

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Member

Why? I see no subclasses here or in the paired PR.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 8, 2017

Author Member

It is for https://issues.jenkins-ci.org/browse/JENKINS-44101, see the TODO in the paired PR

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Member

I am not sure that use case is a good idea, but even if it is, you do not need to do this. The JNLP file produced by the plugin can simply point to a custom endpoint which serves a different JAR.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 11, 2017

Author Member

Yeah. But the idea is to tweak the default endpoint. In such case there will be no changes required on the agent side, docker images, documentation, etc.

@@ -172,7 +173,7 @@ public boolean isAcceptingTasks() {
* @since 1.498
*/
public String getJnlpMac() {
return JnlpSlaveAgentProtocol.SLAVE_SECRET.mac(getName());
return AgentProtocol.AGENT_SECRET.mac(getName());

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Member

I think the method should rather be deprecated and made to throw UnsupportedOperationException.

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Member

(Or simply deleted.)

// Ignore
return false;
}
return jnlpLauncherClass.isAssignableFrom(launcher.getClass());

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Member

More simply:

return getClass().getName().equals("hudson.slaves.JNLPLauncher");

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 8, 2017

Author Member

No since JNLPLauncher is not final

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Member

I know, but there are no expected subclasses. Anyway this is good enough if verbose.

this.hub = null;
}
}
@Restricted(NoExternalUse.class)

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Member

Hmm, overzealous rename detection from Git?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 8, 2017

Author Member

yep. New class

}
@Override
public void launch(SlaveComputer computer, TaskListener listener) throws IOException, InterruptedException {
throw new IOException("Cannot launch computer " + computer + ", because a default NoOpComputerLauncher is specified");

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Member

Use AbortException.

* Singleton holder of {@link IOHub}
*
* @since 2.27
* Computer Launcher, which does nothing.

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Member

I do not think this really needs to exist. Allow SlaveComputer.launcher to be null and handle that directly when trying to call the launcher.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 11, 2017

Author Member

Maybe

@@ -52,7 +52,8 @@

@Issue("JENKINS-23517")
@Test public void offlineNodeForJDK() throws Exception {
Node slave = new DumbSlave("disconnected-slave", null, "/wherever", "1", Node.Mode.NORMAL, null, new JNLPLauncher(), RetentionStrategy.NOOP, Collections.<NodeProperty<?>>emptyList());
Node slave = new DumbSlave("disconnected-slave", null, "/wherever", "1", Node.Mode.NORMAL, null,
DefaultComputerLauncherProvider.findDefaultLauncher(DumbSlave.class), RetentionStrategy.NOOP, Collections.<NodeProperty<?>>emptyList());

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Member

This makes no sense. Leave the test alone and add the new plugin as a test-scoped dependency. Or, switch to CommandLauncher.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 11, 2017

Author Member

I just wanted to get some free coverage of the extension point. But yeah, maybe it is a bad idea

@@ -80,7 +80,8 @@ public Object call() {
* @see #launchJnlpSlave(Slave)
*/
public DumbSlave createJnlpSlave(String name) throws Exception {
DumbSlave s = new DumbSlave(name, "", System.getProperty("java.io.tmpdir") + '/' + name, "2", Mode.NORMAL, "", new JNLPLauncher(), RetentionStrategy.INSTANCE, Collections.EMPTY_LIST);
DumbSlave s = new DumbSlave(name, "", System.getProperty("java.io.tmpdir") + '/' + name, "2", Mode.NORMAL, "",
DefaultComputerLauncherProvider.findDefaultLauncher(DumbSlave.class), RetentionStrategy.INSTANCE, Collections.EMPTY_LIST);

This comment has been minimized.

Copy link
@jglick

jglick May 8, 2017

Member

Ditto.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented May 8, 2017

creating a detached "Remoting CLI Plugin"

Why bother? The code is deprecated, it can just sit in core unless and until we find a way to separate the whole CLI system to a plugin (already a filed task but tricky).

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented May 8, 2017

Why bother? The code is deprecated, it can just sit in core unless and until we find a way to separate the whole CLI system to a plugin (already a filed task but tricky).

The code depends on NioChannelSelector, which I would like to move to the detached plugin. Will have to leave it in the core otherwise

@jglick

This comment has been minimized.

Copy link
Member

jglick commented May 8, 2017

The code depends on NioChannelSelector, which I would like to move

Ah, got it. OK, so move it and leave it deprecated, no need to ever move it elsewhere.

@jglick jglick dismissed their stale review May 11, 2017

Waiting to see where this goes.

@batmat batmat removed their request for review Sep 21, 2017
@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Mar 4, 2018

This pull request is not recoverable, somebody needs to do it from scratch after lots of related code changes. I do not anticipate to get time for such Remoting-related work in next 2-3 months, so I am just closing it.

https://issues.jenkins-ci.org/browse/JENKINS-44100 has been updated to reflect the state and to point to pull requests

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jan 11, 2019

creating a detached "Remoting CLI Plugin"

Simpler to delete without replacement! #3838

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.