Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[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)
  • Loading branch information
deadmoose authored and alvarolobato committed Nov 29, 2016
1 parent a05d942 commit d563062
Show file tree
Hide file tree
Showing 4 changed files with 212 additions and 30 deletions.
13 changes: 9 additions & 4 deletions core/pom.xml
Expand Up @@ -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>
Expand Down
104 changes: 79 additions & 25 deletions core/src/main/java/hudson/slaves/ChannelPinger.java
Expand Up @@ -35,11 +35,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
Expand All @@ -50,16 +50,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 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
Expand All @@ -68,54 +95,80 @@ 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("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());
}

// 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. */
Expand All @@ -141,6 +194,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 });
}
}
117 changes: 117 additions & 0 deletions 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<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();
}
}
8 changes: 7 additions & 1 deletion pom.xml
Expand Up @@ -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>
Expand Down Expand Up @@ -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>
Expand Down

0 comments on commit d563062

Please sign in to comment.