Skip to content
Permalink
Browse files

[JENKINS-48761] - Address review comments from @jglick

  • Loading branch information...
oleg-nenashev committed Jan 3, 2018
1 parent 8a34983 commit 01b8ed8c71dee97d661bd5bf4b8e4bc5ecd636ae
@@ -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}
@@ -44,7 +44,6 @@
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 Launcher getInner() {
}

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);
@@ -290,7 +290,7 @@ private void respondHello(String header, Socket s) throws IOException {
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");
@@ -32,7 +32,6 @@
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.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 void doDumpExportTable( StaplerRequest req, StaplerResponse rsp ) throws

private static final class DumpExportTableTask extends MasterToSlaveCallable<String,IOException> {
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);
@@ -134,7 +134,7 @@ public void install(Channel channel) {
@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;
}

@@ -44,7 +44,6 @@
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 Void call() {
}

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);
}
@@ -7,8 +7,6 @@
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,21 +23,21 @@ public void checkRoles(RoleChecker checker) throws SecurityException {
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"));
}
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;
@@ -37,9 +37,8 @@

// 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 @@
private static VersionNumber EMBEDDED_VERSION;

@CheckForNull
private static VersionNumber MINIMAL_SUPPORTED_VERSION;
private static VersionNumber MINIMUM_SUPPORTED_VERSION;

private RemotingVersionInfo() {}

@@ -66,7 +65,7 @@ private 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 static VersionNumber getEmbeddedVersion() {
}

@CheckForNull
public static VersionNumber getMinimalSupportedVersion() {
return MINIMAL_SUPPORTED_VERSION;
public static VersionNumber getMinimumSupportedVersion() {
return MINIMUM_SUPPORTED_VERSION;
}
}
@@ -39,7 +39,7 @@ public void preOnline(Computer c, Channel channel, FilePath root, TaskListener l
private static final class ChannelSwapper extends MasterToSlaveCallable<Boolean,Exception> {
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);
@@ -40,8 +40,8 @@ public void shouldLoadEmbeddedVersionByDefault() {
}

@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());
}
}
@@ -104,9 +104,9 @@ THE SOFTWARE.

<maven-war-plugin.version>3.0.0</maven-war-plugin.version> <!-- JENKINS-47127 bump when 3.2.0 is out. Cf. MWAR-407 -->

<!-- Minimal Remoting version, which is tested for API compatibility -->
<!-- Minimum Remoting version, which is tested for API compatibility -->
<remoting.version>3.15</remoting.version>
<remoting.minimal.supported.version>2.60</remoting.minimal.supported.version>
<remoting.minimum.supported.version>2.60</remoting.minimum.supported.version>

</properties>

@@ -239,7 +239,7 @@ THE SOFTWARE.
<artifactItem>
<groupId>org.jenkins-ci.main</groupId>
<artifactId>remoting</artifactId>
<version>${remoting.minimal.supported.version}</version>
<version>${remoting.minimum.supported.version}</version>
<type>jar</type>
<outputDirectory>${project.build.outputDirectory}/old-remoting</outputDirectory>
<destFileName>remoting-minimal-supported.jar</destFileName>
@@ -44,7 +44,6 @@
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 @@
@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 void shouldBeAbleToConnectAgentWithMinimalSupportedVersion() throws Excep
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 @@ static void assertMonitor(NodeMonitor monitor, Computer c) throws AssertionError
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);

0 comments on commit 01b8ed8

Please sign in to comment.
You can’t perform that action at this time.