Skip to content

Commit

Permalink
[FIXED JENKINS-28245] Allow finer-grained tuning of ChannelPinger.
Browse files Browse the repository at this point in the history 
 * 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
deadmoose committed May 18, 2015
1 parent 98a6c6f commit 3743bfe
Show file tree
Hide file tree
Showing 4 changed files with 203 additions and 26 deletions.
12 changes: 8 additions & 4 deletions core/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -585,10 +585,14 @@ 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>
</dependency>
<!-- Overriding Stapler’s 1.1.3 version to diagnose JENKINS-20618: -->
<dependency>
<groupId>com.jcraft</groupId>
Expand Down
91 changes: 70 additions & 21 deletions core/src/main/java/hudson/slaves/ChannelPinger.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import jenkins.slaves.PingFailureAnalyzer;

import java.io.IOException;
import java.util.Arrays;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.logging.Logger;

Expand All @@ -49,22 +50,41 @@
@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.warning("Ignoring " + INTERVAL_MINUTES_PROPERTY + "=" + intervalMinutes + " because " + INTERVAL_SECONDS_PROPERTY + "=" + intervalSeconds);
} else {
intervalSeconds = intervalMinutes * 60;
}
}

pingIntervalSeconds = intervalSeconds == null ? 5 * 60 : intervalSeconds;

if (pingIntervalSeconds < pingTimeoutSeconds) {
LOGGER.warning("Ping interval (" + pingIntervalSeconds + ") is less than ping timeout (" + pingTimeoutSeconds + ")");
}
}

@Override
Expand All @@ -73,39 +93,67 @@ 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());
}

// 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<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);
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()) {
Expand Down Expand Up @@ -136,6 +184,7 @@ public void onClosed(Channel channel, IOException cause) {
});

t.start();
LOGGER.fine("Ping thread started for " + channel + " with a " + interval + " minute interval");
LOGGER.fine("Ping thread started for " + channel + " with a " +
intervalSeconds + " second interval and a " + timeoutSeconds + " second timeout.");
}
}
117 changes: 117 additions & 0 deletions core/src/test/java/hudson/slaves/ChannelPingerTest.java
View file
Original file line number Diff line number Diff line change
@@ -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();
}
}
9 changes: 8 additions & 1 deletion pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,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.7.1</maven-plugin.version>
<matrix-project.version>1.4.1</matrix-project.version>
Expand Down Expand Up @@ -182,7 +183,13 @@ 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>
<scope>test</scope>
</dependency>

<dependency>
Expand Down

0 comments on commit 3743bfe

Please sign in to comment.