From 01b8ed8c71dee97d661bd5bf4b8e4bc5ecd636ae Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Wed, 3 Jan 2018 20:59:11 +0100 Subject: [PATCH] [JENKINS-48761] - Address review comments from @jglick --- .../jenkins/slaves/remoting-info.properties | 4 ++-- core/src/main/java/hudson/Launcher.java | 3 +-- .../java/hudson/TcpSlaveAgentListener.java | 2 +- core/src/main/java/hudson/model/Computer.java | 4 +--- .../main/java/hudson/slaves/ChannelPinger.java | 2 +- .../main/java/hudson/slaves/SlaveComputer.java | 3 +-- .../security/MasterToSlaveCallable.java | 18 ++++++++---------- .../jenkins/slaves/RemotingVersionInfo.java | 11 +++++------ .../jenkins/slaves/StandardOutputSwapper.java | 2 +- .../slaves/RemotingVersionInfoTest.java | 6 +++--- pom.xml | 4 ++-- test/pom.xml | 2 +- .../jenkins/slaves/OldRemotingAgentTest.java | 9 ++++----- 13 files changed, 31 insertions(+), 39 deletions(-) diff --git a/core/src/filter/resources/jenkins/slaves/remoting-info.properties b/core/src/filter/resources/jenkins/slaves/remoting-info.properties index 5a3a6ef14a..6ac78f4ad4 100644 --- a/core/src/filter/resources/jenkins/slaves/remoting-info.properties +++ b/core/src/filter/resources/jenkins/slaves/remoting-info.properties @@ -2,5 +2,5 @@ # This version MAY differ from what is really classloaded (see the "Pluggable Core Components", JENKINS-41196) remoting.embedded.version=${remoting.version} -# Minimal Remoting version on external agents which is supported by the core -remoting.minimal.supported.version=${remoting.minimal.supported.version} +# Minimum Remoting version on external agents which is supported by the core +remoting.minimum.supported.version=${remoting.minimum.supported.version} diff --git a/core/src/main/java/hudson/Launcher.java b/core/src/main/java/hudson/Launcher.java index 1c5e884a4d..432a5afe24 100644 --- a/core/src/main/java/hudson/Launcher.java +++ b/core/src/main/java/hudson/Launcher.java @@ -44,7 +44,6 @@ import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; import javax.annotation.CheckForNull; -import javax.annotation.concurrent.GuardedBy; import java.io.BufferedOutputStream; import java.io.File; import java.io.IOException; @@ -1289,7 +1288,7 @@ public abstract class Launcher { } public RemoteProcess call() throws IOException { - final Channel channel = _getOpenChannelOrFail(); + final Channel channel = getOpenChannelOrFail(); Launcher.ProcStarter ps = new LocalLauncher(listener).launch(); ps.cmds(cmd).masks(masks).envs(env).stdin(in).stdout(out).stderr(err).quiet(quiet); if(workDir!=null) ps.pwd(workDir); diff --git a/core/src/main/java/hudson/TcpSlaveAgentListener.java b/core/src/main/java/hudson/TcpSlaveAgentListener.java index e6e2a7807d..03455a7753 100644 --- a/core/src/main/java/hudson/TcpSlaveAgentListener.java +++ b/core/src/main/java/hudson/TcpSlaveAgentListener.java @@ -290,7 +290,7 @@ public final class TcpSlaveAgentListener extends Thread { try { Writer o = new OutputStreamWriter(s.getOutputStream(), "UTF-8"); - //TODO: expose version about minimal supported Remoting version (JENKINS-48766) + //TODO: expose version about minimum supported Remoting version (JENKINS-48766) if (header.startsWith("GET / ")) { o.write("HTTP/1.0 200 OK\r\n"); o.write("Content-Type: text/plain;charset=UTF-8\r\n"); diff --git a/core/src/main/java/hudson/model/Computer.java b/core/src/main/java/hudson/model/Computer.java index b802f9e4d6..43fa2d31d7 100644 --- a/core/src/main/java/hudson/model/Computer.java +++ b/core/src/main/java/hudson/model/Computer.java @@ -32,7 +32,6 @@ import hudson.Launcher.ProcStarter; import hudson.slaves.Cloud; import jenkins.util.SystemProperties; import hudson.Util; -import hudson.cli.declarative.CLIMethod; import hudson.cli.declarative.CLIResolver; import hudson.console.AnnotatedLargeText; import hudson.init.Initializer; @@ -85,7 +84,6 @@ import org.kohsuke.stapler.HttpRedirect; import org.kohsuke.stapler.WebMethod; import org.kohsuke.stapler.export.Exported; import org.kohsuke.stapler.export.ExportedBean; -import org.kohsuke.args4j.Option; import org.kohsuke.stapler.interceptor.RequirePOST; import javax.annotation.OverridingMethodsMustInvokeSuper; @@ -1426,7 +1424,7 @@ public /*transient*/ abstract class Computer extends Actionable implements Acces private static final class DumpExportTableTask extends MasterToSlaveCallable { public String call() throws IOException { - final Channel ch = _getChannelOrFail(); + final Channel ch = getChannelOrFail(); StringWriter sw = new StringWriter(); try (PrintWriter pw = new PrintWriter(sw)) { ch.dumpExportTable(pw); diff --git a/core/src/main/java/hudson/slaves/ChannelPinger.java b/core/src/main/java/hudson/slaves/ChannelPinger.java index b791df2813..85d6d78675 100644 --- a/core/src/main/java/hudson/slaves/ChannelPinger.java +++ b/core/src/main/java/hudson/slaves/ChannelPinger.java @@ -134,7 +134,7 @@ public class ChannelPinger extends ComputerListener { @Override public Void call() throws IOException { // No sense in setting up channel pinger if the channel is being closed - setUpPingForChannel(_getOpenChannelOrFail(), null, pingTimeoutSeconds, pingIntervalSeconds, false); + setUpPingForChannel(getOpenChannelOrFail(), null, pingTimeoutSeconds, pingIntervalSeconds, false); return null; } diff --git a/core/src/main/java/hudson/slaves/SlaveComputer.java b/core/src/main/java/hudson/slaves/SlaveComputer.java index 029565f137..9b87bf5dec 100644 --- a/core/src/main/java/hudson/slaves/SlaveComputer.java +++ b/core/src/main/java/hudson/slaves/SlaveComputer.java @@ -44,7 +44,6 @@ import hudson.remoting.VirtualChannel; import hudson.security.ACL; import hudson.slaves.OfflineCause.ChannelTermination; import hudson.util.Futures; -import hudson.util.IOUtils; import hudson.util.NullStream; import hudson.util.RingBufferLogHandler; import hudson.util.StreamTaskListener; @@ -869,7 +868,7 @@ public class SlaveComputer extends Computer { } try { - _getChannelOrFail().setProperty("slave",Boolean.TRUE); // indicate that this side of the channel is the slave side. + getChannelOrFail().setProperty("slave",Boolean.TRUE); // indicate that this side of the channel is the slave side. } catch (ChannelClosedException e) { throw new IllegalStateException(e); } diff --git a/core/src/main/java/jenkins/security/MasterToSlaveCallable.java b/core/src/main/java/jenkins/security/MasterToSlaveCallable.java index 9dbe3904a4..60ac25bfbc 100644 --- a/core/src/main/java/jenkins/security/MasterToSlaveCallable.java +++ b/core/src/main/java/jenkins/security/MasterToSlaveCallable.java @@ -7,8 +7,6 @@ import org.jenkinsci.remoting.RoleChecker; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; -import javax.annotation.Nonnull; - /** * Convenient {@link Callable} meant to be run on agent. @@ -25,9 +23,9 @@ public abstract class MasterToSlaveCallable implements C checker.check(this,Roles.SLAVE); } - //TODO: replace by Callable#getChannelOrFail() once Minimal supported Remoting version is 3.15 or above - @Restricted(NoExternalUse.class) - protected static Channel _getChannelOrFail() throws ChannelClosedException { + //TODO: remove once Minimum supported Remoting version is 3.15 or above + @Override + public Channel getChannelOrFail() throws ChannelClosedException { final Channel ch = Channel.current(); if (ch == null) { throw new ChannelClosedException(new IllegalStateException("No channel associated with the thread")); @@ -35,11 +33,11 @@ public abstract class MasterToSlaveCallable implements C return ch; } - //TODO: replace by Callable#getOpenChannelOrFail() once Minimal supported Remoting version is 3.15 or above - @Restricted(NoExternalUse.class) - protected static Channel _getOpenChannelOrFail() throws ChannelClosedException { - final Channel ch = _getChannelOrFail(); - if (ch.isClosingOrClosed()) { // TODO: Since Remoting 2.33, we still need to explicitly declare minimal Remoting version + //TODO: remove once Callable#getOpenChannelOrFail() once Minimaumsupported Remoting version is 3.15 or above + @Override + public Channel getOpenChannelOrFail() throws ChannelClosedException { + final Channel ch = getChannelOrFail(); + if (ch.isClosingOrClosed()) { // TODO: Since Remoting 2.33, we still need to explicitly declare minimum Remoting version throw new ChannelClosedException(new IllegalStateException("The associated channel " + ch + " is closing down or has closed down", ch.getCloseRequestCause())); } return ch; diff --git a/core/src/main/java/jenkins/slaves/RemotingVersionInfo.java b/core/src/main/java/jenkins/slaves/RemotingVersionInfo.java index e4ac3b71c7..e2c2211591 100644 --- a/core/src/main/java/jenkins/slaves/RemotingVersionInfo.java +++ b/core/src/main/java/jenkins/slaves/RemotingVersionInfo.java @@ -37,9 +37,8 @@ import java.util.logging.Logger; // TODO: Make the API public (JENKINS-48766) /** - * Provides information about Remoting versions used withing the core. + * Provides information about Remoting versions used within the core. * @author Oleg Nenashev - * @since TODO */ @Restricted(NoExternalUse.class) public class RemotingVersionInfo { @@ -51,7 +50,7 @@ public class RemotingVersionInfo { private static VersionNumber EMBEDDED_VERSION; @CheckForNull - private static VersionNumber MINIMAL_SUPPORTED_VERSION; + private static VersionNumber MINIMUM_SUPPORTED_VERSION; private RemotingVersionInfo() {} @@ -66,7 +65,7 @@ public class RemotingVersionInfo { } EMBEDDED_VERSION = tryExtractVersion(props, "remoting.embedded.version"); - MINIMAL_SUPPORTED_VERSION = tryExtractVersion(props, "remoting.minimal.supported.version"); + MINIMUM_SUPPORTED_VERSION = tryExtractVersion(props, "remoting.minimum.supported.version"); } @CheckForNull @@ -98,7 +97,7 @@ public class RemotingVersionInfo { } @CheckForNull - public static VersionNumber getMinimalSupportedVersion() { - return MINIMAL_SUPPORTED_VERSION; + public static VersionNumber getMinimumSupportedVersion() { + return MINIMUM_SUPPORTED_VERSION; } } diff --git a/core/src/main/java/jenkins/slaves/StandardOutputSwapper.java b/core/src/main/java/jenkins/slaves/StandardOutputSwapper.java index 5c0966e8a8..9a8d10a12f 100644 --- a/core/src/main/java/jenkins/slaves/StandardOutputSwapper.java +++ b/core/src/main/java/jenkins/slaves/StandardOutputSwapper.java @@ -39,7 +39,7 @@ public class StandardOutputSwapper extends ComputerListener { private static final class ChannelSwapper extends MasterToSlaveCallable { public Boolean call() throws Exception { if (File.pathSeparatorChar==';') return false; // Windows - Channel c = _getOpenChannelOrFail(); + Channel c = getOpenChannelOrFail(); StandardOutputStream sos = (StandardOutputStream) c.getProperty(StandardOutputStream.class); if (sos!=null) { swap(sos); diff --git a/core/src/test/java/jenkins/slaves/RemotingVersionInfoTest.java b/core/src/test/java/jenkins/slaves/RemotingVersionInfoTest.java index 3e894b404c..5f629028ed 100644 --- a/core/src/test/java/jenkins/slaves/RemotingVersionInfoTest.java +++ b/core/src/test/java/jenkins/slaves/RemotingVersionInfoTest.java @@ -40,8 +40,8 @@ public class RemotingVersionInfoTest { } @Test - public void shouldLoadMinimalSupportedVersionByDefault() { - assertThat("Remoting Minimal supported version is not defined", - RemotingVersionInfo.getMinimalSupportedVersion(), notNullValue()); + public void shouldLoadMinimumSupportedVersionByDefault() { + assertThat("Remoting Minimum supported version is not defined", + RemotingVersionInfo.getMinimumSupportedVersion(), notNullValue()); } } diff --git a/pom.xml b/pom.xml index d0f11bbe36..c5f550a5af 100644 --- a/pom.xml +++ b/pom.xml @@ -104,9 +104,9 @@ THE SOFTWARE. 3.0.0 - + 3.15 - 2.60 + 2.60 diff --git a/test/pom.xml b/test/pom.xml index 931dbb0562..f6e20856ea 100644 --- a/test/pom.xml +++ b/test/pom.xml @@ -239,7 +239,7 @@ THE SOFTWARE. org.jenkins-ci.main remoting - ${remoting.minimal.supported.version} + ${remoting.minimum.supported.version} jar ${project.build.outputDirectory}/old-remoting remoting-minimal-supported.jar diff --git a/test/src/test/java/jenkins/slaves/OldRemotingAgentTest.java b/test/src/test/java/jenkins/slaves/OldRemotingAgentTest.java index adb17cf9ce..9dff0ae75c 100644 --- a/test/src/test/java/jenkins/slaves/OldRemotingAgentTest.java +++ b/test/src/test/java/jenkins/slaves/OldRemotingAgentTest.java @@ -44,7 +44,6 @@ import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.SimpleCommandLauncher; -import javax.annotation.CheckForNull; import java.io.File; import java.io.IOException; import java.lang.reflect.Method; @@ -70,7 +69,7 @@ public class OldRemotingAgentTest { @Before public void extractAgent() throws Exception { agentJar = new File(tmpDir.getRoot(), "old-agent.jar"); - FileUtils.copyURLToFile(getClass().getResource("/old-remoting/remoting-minimal-supported.jar"), agentJar); + FileUtils.copyURLToFile(OldRemotingAgentTest.class.getResource("/old-remoting/remoting-minimal-supported.jar"), agentJar); } @Test @@ -81,7 +80,7 @@ public class OldRemotingAgentTest { boolean isUnix = agent.getComputer().isUnix(); assertThat("Received wrong agent version. A minimal supported version is expected", agent.getComputer().getSlaveVersion(), - equalTo(RemotingVersionInfo.getMinimalSupportedVersion().toString())); + equalTo(RemotingVersionInfo.getMinimumSupportedVersion().toString())); // Just ensure we are able to run something on the agent FreeStyleProject project = j.createFreeStyleProject("foo"); @@ -122,8 +121,8 @@ public class OldRemotingAgentTest { try { monitorMethod = AbstractAsyncNodeMonitorDescriptor.class.getDeclaredMethod("monitor", Computer.class); } catch (NoSuchMethodException ex) { - System.out.println("Cannot invoke monitor " + monitor + ", no monitor(Computer.class) method in the Descriptor. It will be ignored. " + ex.getMessage()); - return; + //TODO: make the API visible for testing? + throw new AssertionError("Cannot invoke monitor " + monitor + ", no monitor(Computer.class) method in the Descriptor. It will be ignored. ", ex); } try { monitorMethod.setAccessible(true);