Skip to content

Commit

Permalink
Merge pull request #2645 from alvarolobato/JENKINS-28245
Browse files Browse the repository at this point in the history 
[FIX JENKINS-28245] - Allow defining agent ping interval and ping timeout in seconds
  • Loading branch information
@oleg-nenashev
oleg-nenashev committed Dec 14, 2016
2 parents e7e51ee + 6dea3c3 commit 1868c84
Show file tree
Hide file tree
Showing 4 changed files with 222 additions and 29 deletions.
13 changes: 9 additions & 4 deletions core/pom.xml
View file
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
114 changes: 90 additions & 24 deletions core/src/main/java/hudson/slaves/ChannelPinger.java
View file
Expand Up @@ -36,10 +36,9 @@

import java.io.IOException;
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 @@ -49,17 +48,40 @@
*/
@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;
static final int PING_TIMEOUT_SECONDS_DEFAULT = 4 * 60;
static final int PING_INTERVAL_SECONDS_DEFAULT = 5 * 60;

private static final Logger LOGGER = Logger.getLogger(ChannelPinger.class.getName());
private static final String TIMEOUT_SECONDS_PROPERTY = ChannelPinger.class.getName() + ".pingTimeoutSeconds";
private static final String INTERVAL_MINUTES_PROPERTY_DEPRECATED = ChannelPinger.class.getName() + ".pingInterval";
private static final String INTERVAL_SECONDS_PROPERTY = ChannelPinger.class.getName() + ".pingIntervalSeconds";

/**
* Timeout for the ping in seconds.
*/
private int pingTimeoutSeconds = SystemProperties.getInteger(TIMEOUT_SECONDS_PROPERTY, PING_TIMEOUT_SECONDS_DEFAULT, Level.WARNING);

/**
* Interval for the ping in minutes.
* Interval for the ping in seconds.
*/
private final int pingInterval;
private int pingIntervalSeconds = PING_INTERVAL_SECONDS_DEFAULT;

public ChannelPinger() {
pingInterval = SystemProperties.getInteger(SYS_PROPERTY_NAME, DEFAULT_PING_INTERVAL_MIN);

Integer interval = SystemProperties.getInteger(INTERVAL_SECONDS_PROPERTY, null, Level.WARNING);

// if interval wasn't set we read the deprecated property in minutes
if (interval == null) {
interval = SystemProperties.getInteger(INTERVAL_MINUTES_PROPERTY_DEPRECATED,null, Level.WARNING);
if (interval != null) {
LOGGER.warning(INTERVAL_MINUTES_PROPERTY_DEPRECATED + " property is deprecated, " + INTERVAL_SECONDS_PROPERTY + " should be used");
interval *= 60; //to seconds
}
}

if (interval != null) {
pingIntervalSeconds = interval;
}
}

@Override
Expand All @@ -68,54 +90,97 @@ public void preOnline(Computer c, Channel channel, FilePath root, TaskListener l
}

public void install(Channel channel) {
if (pingInterval < 1) {
LOGGER.fine("Agent ping is disabled");
if (pingTimeoutSeconds < 1 || pingIntervalSeconds < 1) {
LOGGER.warning("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> {
static class SetUpRemotePing extends MasterToSlaveCallable<Void, IOException> {
private static final long serialVersionUID = -2702219700841759872L;
private int pingInterval;
public SetUpRemotePing(int pingInterval) {
this.pingInterval = pingInterval;
@Deprecated
private transient int pingInterval;
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 int hashCode() {
final int prime = 31;
int result = 1;
result = prime * result + pingIntervalSeconds;
result = prime * result + pingTimeoutSeconds;
return result;
}

@Override
public boolean equals(Object obj) {
if (this == obj) {
return true;
}
if (obj == null) {
return false;
}
if (getClass() != obj.getClass()) {
return false;
}
SetUpRemotePing other = (SetUpRemotePing) obj;
if (pingIntervalSeconds != other.pingIntervalSeconds) {
return false;
}
if (pingTimeoutSeconds != other.pingTimeoutSeconds) {
return false;
}
return true;
}

protected Object readResolve() {
if (pingInterval != 0) {
return new SetUpRemotePing(PING_TIMEOUT_SECONDS_DEFAULT, pingInterval * 60);
}
return this;
}
}

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) {
LOGGER.log(Level.FINE, "setting up ping on {0} with a {1} seconds interval and {2} seconds timeout", new Object[] {channel.getName(), intervalSeconds, timeoutSeconds});
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) {
@Override
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 +206,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} seconds interval and a {2} seconds timeout",
new Object[] { channel, intervalSeconds, timeoutSeconds });
}
}
116 changes: 116 additions & 0 deletions core/src/test/java/hudson/slaves/ChannelPingerTest.java
View file
@@ -0,0 +1,116 @@
package hudson.slaves;

import static org.mockito.Matchers.eq;
import static org.mockito.Mockito.verify;
import static org.powermock.api.mockito.PowerMockito.mockStatic;
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.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(ChannelPinger.PING_TIMEOUT_SECONDS_DEFAULT,
ChannelPinger.PING_INTERVAL_SECONDS_DEFAULT)));
verifyStatic();
ChannelPinger.setUpPingForChannel(mockChannel, ChannelPinger.PING_TIMEOUT_SECONDS_DEFAULT,
ChannelPinger.PING_INTERVAL_SECONDS_DEFAULT, true);
}

@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, true);
}

@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(ChannelPinger.PING_TIMEOUT_SECONDS_DEFAULT, 420)));
verifyStatic();
ChannelPinger.setUpPingForChannel(mockChannel, ChannelPinger.PING_TIMEOUT_SECONDS_DEFAULT, 420, true);
}

@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(ChannelPinger.PING_TIMEOUT_SECONDS_DEFAULT, 73)));
verifyStatic();
ChannelPinger.setUpPingForChannel(mockChannel, ChannelPinger.PING_TIMEOUT_SECONDS_DEFAULT, 73, true);
}

@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
View file
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 1868c84

Please sign in to comment.