diff --git a/core/pom.xml b/core/pom.xml index 0f63887062f6..f85e960e8175 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -585,10 +585,15 @@ THE SOFTWARE. /usr/local/yjp/lib/yjp.jar - - com.google.guava - guava - + + com.google.guava + guava + + + com.google.guava + guava-testlib + test + com.jcraft diff --git a/core/src/main/java/hudson/slaves/ChannelPinger.java b/core/src/main/java/hudson/slaves/ChannelPinger.java index 49ae7c1cf997..aa81b5b3bf6c 100644 --- a/core/src/main/java/hudson/slaves/ChannelPinger.java +++ b/core/src/main/java/hudson/slaves/ChannelPinger.java @@ -34,11 +34,11 @@ import jenkins.slaves.PingFailureAnalyzer; import java.io.IOException; +import java.util.Arrays; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.logging.Level; import java.util.logging.Logger; -import static java.util.logging.Level.*; - /** * Establish a periodic ping to keep connections between {@link Slave slaves} * and the main Jenkins node alive. This prevents network proxies from @@ -49,22 +49,43 @@ @Extension public class ChannelPinger extends ComputerListener { private static final Logger LOGGER = Logger.getLogger(ChannelPinger.class.getName()); - private static final String SYS_PROPERTY_NAME = ChannelPinger.class.getName() + ".pingInterval"; + private static final String TIMEOUT_SECONDS_PROPERTY = ChannelPinger.class.getName() + ".pingTimeoutSeconds"; + private static final String INTERVAL_MINUTES_PROPERTY = ChannelPinger.class.getName() + ".pingInterval"; + private static final String INTERVAL_SECONDS_PROPERTY = ChannelPinger.class.getName() + ".pingIntervalSeconds"; /** - * Interval for the ping in minutes. + * Timeout for the ping in seconds. */ - private int pingInterval = 5; + private final int pingTimeoutSeconds; + + /** + * Interval for the ping in seconds. + */ + private final int pingIntervalSeconds; public ChannelPinger() { - String interval = System.getProperty(SYS_PROPERTY_NAME); - if (interval != null) { - try { - pingInterval = Integer.valueOf(interval); - } catch (NumberFormatException e) { - LOGGER.warning("Ignoring invalid " + SYS_PROPERTY_NAME + "=" + interval); + pingTimeoutSeconds = Integer.getInteger(TIMEOUT_SECONDS_PROPERTY, 4 * 60); + + // A little extra hoop-jumping to migrate from the old system property + Integer intervalSeconds = Integer.getInteger(INTERVAL_SECONDS_PROPERTY); + Integer intervalMinutes = Integer.getInteger(INTERVAL_MINUTES_PROPERTY); + if (intervalMinutes != null) { + LOGGER.warning("Property '" + INTERVAL_MINUTES_PROPERTY + "' is deprecated. Please migrate to '" + INTERVAL_SECONDS_PROPERTY + "'"); + + if (intervalSeconds != null) { + LOGGER.log(Level.WARNING, "Ignoring {0}={1} because {2}={3}", + new Object[] { INTERVAL_MINUTES_PROPERTY, intervalMinutes, INTERVAL_SECONDS_PROPERTY, intervalSeconds }); + } else { + intervalSeconds = intervalMinutes * 60; } } + + pingIntervalSeconds = intervalSeconds == null ? 5 * 60 : intervalSeconds; + + if (pingIntervalSeconds < pingTimeoutSeconds) { + LOGGER.log(Level.WARNING, "Ping interval ({0}) is less than ping timeout ({1})", + new Object[] { pingIntervalSeconds, pingTimeoutSeconds }); + } } @Override @@ -73,13 +94,13 @@ public void preOnline(Computer c, Channel channel, FilePath root, TaskListener l } public void install(Channel channel) { - if (pingInterval < 1) { + if (pingTimeoutSeconds < 1 || pingIntervalSeconds < 1) { LOGGER.fine("Slave ping is disabled"); return; } try { - channel.call(new SetUpRemotePing(pingInterval)); + channel.call(new SetUpRemotePing(pingTimeoutSeconds, pingIntervalSeconds)); LOGGER.fine("Set up a remote ping for " + channel.getName()); } catch (Exception e) { LOGGER.severe("Failed to set up a ping for " + channel.getName()); @@ -87,38 +108,66 @@ public void install(Channel channel) { // set up ping from both directions, so that in case of a router dropping a connection, // both sides can notice it and take compensation actions. - setUpPingForChannel(channel, pingInterval); + setUpPingForChannel(channel, pingTimeoutSeconds, pingIntervalSeconds); } - private static class SetUpRemotePing extends MasterToSlaveCallable { - private static final long serialVersionUID = -2702219700841759872L; - private int pingInterval; - public SetUpRemotePing(int pingInterval) { - this.pingInterval = pingInterval; + static class SetUpRemotePing extends MasterToSlaveCallable { + private final int pingTimeoutSeconds; + private final int pingIntervalSeconds; + + SetUpRemotePing(int pingTimeoutSeconds, int pingIntervalSeconds) { + this.pingTimeoutSeconds = pingTimeoutSeconds; + this.pingIntervalSeconds = pingIntervalSeconds; } + @Override public Void call() throws IOException { - setUpPingForChannel(Channel.current(), pingInterval); + setUpPingForChannel(Channel.current(), pingTimeoutSeconds, pingIntervalSeconds); return null; } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + + SetUpRemotePing that = (SetUpRemotePing) o; + return this.pingTimeoutSeconds == that.pingTimeoutSeconds + && this.pingIntervalSeconds == that.pingIntervalSeconds; + } + + @Override + public int hashCode() { + // TODO(deadmoose): switch to Objects.hash once Java 7's fully available + return Arrays.hashCode(new Object[] { pingTimeoutSeconds, pingIntervalSeconds }); + } + + @Override + public String toString() { + return "SetUpRemotePing(" + pingTimeoutSeconds + "," + pingIntervalSeconds + ")"; + } } - private static void setUpPingForChannel(final Channel channel, int interval) { + static void setUpPingForChannel(final Channel channel, int timeoutSeconds, int intervalSeconds) { final AtomicBoolean isInClosed = new AtomicBoolean(false); - final PingThread t = new PingThread(channel, interval * 60 * 1000) { + final PingThread t = new PingThread(channel, timeoutSeconds * 1000L, intervalSeconds * 1000L) { protected void onDead(Throwable cause) { try { for (PingFailureAnalyzer pfa : PingFailureAnalyzer.all()) { pfa.onPingFailure(channel,cause); } if (isInClosed.get()) { - LOGGER.log(FINE,"Ping failed after the channel "+channel.getName()+" is already partially closed.",cause); + LOGGER.log(Level.FINE,"Ping failed after the channel "+channel.getName()+" is already partially closed.",cause); } else { - LOGGER.log(INFO,"Ping failed. Terminating the channel "+channel.getName()+".",cause); + LOGGER.log(Level.INFO,"Ping failed. Terminating the channel "+channel.getName()+".",cause); channel.close(cause); } } catch (IOException e) { - LOGGER.log(SEVERE,"Failed to terminate the channel "+channel.getName(),e); + LOGGER.log(Level.SEVERE,"Failed to terminate the channel "+channel.getName(),e); } } protected void onDead() { @@ -136,6 +185,7 @@ public void onClosed(Channel channel, IOException cause) { }); t.start(); - LOGGER.fine("Ping thread started for " + channel + " with a " + interval + " minute interval"); + LOGGER.log(Level.FINE, "Ping thread started for {0} with a {1} second interval and a {2} second timeout.", + new Object[] { channel, intervalSeconds, timeoutSeconds }); } } diff --git a/core/src/test/java/hudson/slaves/ChannelPingerTest.java b/core/src/test/java/hudson/slaves/ChannelPingerTest.java new file mode 100644 index 000000000000..9018ea55ee81 --- /dev/null +++ b/core/src/test/java/hudson/slaves/ChannelPingerTest.java @@ -0,0 +1,117 @@ +package hudson.slaves; + +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.powermock.api.mockito.PowerMockito.mockStatic; +import static org.powermock.api.mockito.PowerMockito.verifyNoMoreInteractions; +import static org.powermock.api.mockito.PowerMockito.verifyStatic; + +import com.google.common.testing.EqualsTester; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; +import org.powermock.api.mockito.PowerMockito; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; +import hudson.remoting.Channel; + +import java.util.Map; +import java.util.HashMap; + +@RunWith(PowerMockRunner.class) +@PrepareForTest({ ChannelPinger.class }) +public class ChannelPingerTest { + + @Mock private Channel mockChannel; + + private Map savedSystemProperties = new HashMap(); + + @Before + public void setUp() throws Exception { + MockitoAnnotations.initMocks(this); + mockStatic(ChannelPinger.class); + } + + @Before + public void preserveSystemProperties() throws Exception { + preserveSystemProperty("hudson.slaves.ChannelPinger.pingInterval"); + preserveSystemProperty("hudson.slaves.ChannelPinger.pingIntervalSeconds"); + preserveSystemProperty("hudson.slaves.ChannelPinger.pingTimeoutSeconds"); + } + + @After + public void restoreSystemProperties() throws Exception { + for (Map.Entry entry : savedSystemProperties.entrySet()) { + if (entry.getValue() != null) { + System.setProperty(entry.getKey(), entry.getValue()); + } else { + System.clearProperty(entry.getKey()); + } + } + } + + private void preserveSystemProperty(String propertyName) { + savedSystemProperties.put(propertyName, System.getProperty(propertyName)); + System.clearProperty(propertyName); + } + + @Test + public void testDefaults() throws Exception { + ChannelPinger channelPinger = new ChannelPinger(); + channelPinger.install(mockChannel); + + verify(mockChannel).call(eq(new ChannelPinger.SetUpRemotePing(240, 300))); + verifyStatic(); + ChannelPinger.setUpPingForChannel(mockChannel, 240, 300); + } + + @Test + public void testFromSystemProperties() throws Exception { + System.setProperty("hudson.slaves.ChannelPinger.pingTimeoutSeconds", "42"); + System.setProperty("hudson.slaves.ChannelPinger.pingIntervalSeconds", "73"); + + ChannelPinger channelPinger = new ChannelPinger(); + channelPinger.install(mockChannel); + + verify(mockChannel).call(new ChannelPinger.SetUpRemotePing(42, 73)); + verifyStatic(); + ChannelPinger.setUpPingForChannel(mockChannel, 42, 73); + } + + @Test + public void testFromOldSystemProperty() throws Exception { + System.setProperty("hudson.slaves.ChannelPinger.pingInterval", "7"); + + ChannelPinger channelPinger = new ChannelPinger(); + channelPinger.install(mockChannel); + + verify(mockChannel).call(eq(new ChannelPinger.SetUpRemotePing(240, 420))); + verifyStatic(); + ChannelPinger.setUpPingForChannel(mockChannel, 240, 420); + } + + @Test + public void testNewSystemPropertyTrumpsOld() throws Exception { + System.setProperty("hudson.slaves.ChannelPinger.pingIntervalSeconds", "73"); + System.setProperty("hudson.slaves.ChannelPinger.pingInterval", "7"); + + ChannelPinger channelPinger = new ChannelPinger(); + channelPinger.install(mockChannel); + + verify(mockChannel).call(eq(new ChannelPinger.SetUpRemotePing(240, 73))); + verifyStatic(); + ChannelPinger.setUpPingForChannel(mockChannel, 240, 73); + } + + @Test + public void testSetUpRemotePingEquality() { + new EqualsTester() + .addEqualityGroup(new ChannelPinger.SetUpRemotePing(1, 2), new ChannelPinger.SetUpRemotePing(1, 2)) + .addEqualityGroup(new ChannelPinger.SetUpRemotePing(2, 3), new ChannelPinger.SetUpRemotePing(2, 3)) + .testEquals(); + } +} diff --git a/pom.xml b/pom.xml index 2a83ea812a17..4cfad9a7652b 100644 --- a/pom.xml +++ b/pom.xml @@ -96,6 +96,7 @@ THE SOFTWARE. https://api.github.com jenkins-jira + 11.0.1 1.7.7 2.7.1 1.4.1 @@ -182,7 +183,12 @@ THE SOFTWARE. com.google.guava guava - 11.0.1 + ${guavaVersion} + + + com.google.guava + guava-testlib + ${guavaVersion}