[FIXED JENKINS-28245] Allow finer-grained tuning of ChannelPinger.

 * Allows customization in seconds, not minutes
 * Allows customization of the ping timeout (before, you could set a
   custom interval, but the timeout would always be PingThread's 4
   minute default)

This also drops the serialVersionUID from ChannelPinger.SetUpRemotePing;
without one provided, the JVM will generate one on demand which is
sufficient for the purposes here since these are never persisted and
master & slave run the same compiled code. (And it demonstrably works
since countless other MasterToSlaveCallables fail to specify their own
custom IDs)

core/pom.xml

@@ -606,10 +606,15 @@ THE SOFTWARE.
       <systemPath>/usr/local/yjp/lib/yjp.jar</systemPath>
     </dependency-->
 
-      <dependency>
-        <groupId>com.google.guava</groupId>
-        <artifactId>guava</artifactId>
-      </dependency>
+    <dependency>
+      <groupId>com.google.guava</groupId>
+      <artifactId>guava</artifactId>
+    </dependency>
+    <dependency>
+      <groupId>com.google.guava</groupId>
+      <artifactId>guava-testlib</artifactId>
+      <scope>test</scope>
+    </dependency>
     <!-- Overriding Stapler’s 1.1.3 version to diagnose JENKINS-20618: -->
     <dependency>
       <groupId>com.jcraft</groupId>

core/src/main/java/hudson/slaves/ChannelPinger.java

@@ -35,11 +35,11 @@ import jenkins.security.MasterToSlaveCallable;
 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
@@ -50,16 +50,43 @@ import static java.util.logging.Level.*;
 @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 int DEFAULT_PING_INTERVAL_MIN = 5;
-    
+    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";
+
+    /**
+     * Timeout for the ping in seconds.
+     */
+    private final int pingTimeoutSeconds;
+
     /**
-     * Interval for the ping in minutes.
+     * Interval for the ping in seconds.
      */
-    private final int pingInterval;
+    private final int pingIntervalSeconds;
 
     public ChannelPinger() {
-        pingInterval = SystemProperties.getInteger(SYS_PROPERTY_NAME, DEFAULT_PING_INTERVAL_MIN);
+        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
@@ -68,13 +95,13 @@ public class ChannelPinger extends ComputerListener {
     }
 
     public void install(Channel channel) {
-        if (pingInterval < 1) {
+        if (pingTimeoutSeconds < 1 || pingIntervalSeconds < 1) {
             LOGGER.fine("Agent 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());
@@ -82,40 +109,66 @@ public class ChannelPinger extends ComputerListener {
 
         // 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, true);
+        setUpPingForChannel(channel, pingTimeoutSeconds, pingIntervalSeconds, true);
     }
 
-    private static class SetUpRemotePing extends MasterToSlaveCallable<Void, IOException> {
-        private static final long serialVersionUID = -2702219700841759872L;
-        private int pingInterval;
-        public SetUpRemotePing(int pingInterval) {
-            this.pingInterval = pingInterval;
+    static class SetUpRemotePing extends MasterToSlaveCallable<Void, IOException> {
+        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, false);
+            setUpPingForChannel(Channel.current(), pingTimeoutSeconds, pingIntervalSeconds, false);
             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, final boolean analysis) {
-        LOGGER.log(FINE, "setting up ping on {0} at interval {1}m", new Object[] {channel.getName(), interval});
+    static void setUpPingForChannel(final Channel channel, int timeoutSeconds, int intervalSeconds, final boolean analysis) {
         final AtomicBoolean isInClosed = new AtomicBoolean(false);
-        final PingThread t = new PingThread(channel, interval * 60 * 1000) {
-            @Override
+        final PingThread t = new PingThread(channel, timeoutSeconds * 1000L, intervalSeconds * 1000L) {
             protected void onDead(Throwable cause) {
                 try {
                     if (analysis) {
                         analyze(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);
                 }
             }
             /** Keep in a separate method so we do not even try to do class loading on {@link PingFailureAnalyzer} from an agent JVM. */
@@ -141,6 +194,7 @@ public class ChannelPinger extends ComputerListener {
         });
 
         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 });
     }
 }

core/src/test/java/hudson/slaves/ChannelPingerTest.java

+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<String, String> savedSystemProperties = new HashMap<String, String>();
+
+    @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<String, String> 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();
+    }
+}

pom.xml

@@ -91,6 +91,7 @@ THE SOFTWARE.
     <project.patchManagement.url>https://api.github.com</project.patchManagement.url>
     <patch.tracker.serverId>jenkins-jira</patch.tracker.serverId>
 
+    <guavaVersion>11.0.1</guavaVersion>
     <slf4jVersion>1.7.7</slf4jVersion> <!-- < 1.6.x version didn't specify the license (MIT) -->
     <maven-plugin.version>2.14</maven-plugin.version>
     <matrix-project.version>1.4.1</matrix-project.version>
@@ -186,7 +187,12 @@ THE SOFTWARE.
       <dependency>
         <groupId>com.google.guava</groupId>
         <artifactId>guava</artifactId>
-        <version>11.0.1</version>
+        <version>${guavaVersion}</version>
+      </dependency>
+      <dependency>
+        <groupId>com.google.guava</groupId>
+        <artifactId>guava-testlib</artifactId>
+        <version>${guavaVersion}</version>
       </dependency>
 
       <dependency>