Skip to content

Commit e739886

Browse files
olivergondzaolivergondza
committed
[FIX JENKINS-46680] Reset SlaveComputer channel before closing it on ping timeout
1 parent 320174e commit e739886

File tree

3 files changed

+45
-26
lines changed

3 files changed

+45
-26
lines changed

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

+37-16
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
*/
2424
package hudson.slaves;
2525

26+
import com.google.common.annotations.VisibleForTesting;
2627
import hudson.Extension;
2728
import hudson.FilePath;
2829
import jenkins.util.SystemProperties;
@@ -34,6 +35,7 @@
3435
import jenkins.security.MasterToSlaveCallable;
3536
import jenkins.slaves.PingFailureAnalyzer;
3637

38+
import javax.annotation.CheckForNull;
3739
import java.io.IOException;
3840
import java.util.concurrent.atomic.AtomicBoolean;
3941
import java.util.logging.Level;
@@ -86,28 +88,42 @@ public ChannelPinger() {
8688

8789
@Override
8890
public void preOnline(Computer c, Channel channel, FilePath root, TaskListener listener) {
89-
install(channel);
91+
SlaveComputer slaveComputer = null;
92+
if (c instanceof SlaveComputer) {
93+
slaveComputer = (SlaveComputer) c;
94+
}
95+
install(channel, slaveComputer);
9096
}
9197

98+
/**
99+
* @deprecated Use {@link #install(Channel, SlaveComputer)} instead.
100+
*/
101+
@Deprecated
92102
public void install(Channel channel) {
103+
install(channel, null);
104+
}
105+
106+
@VisibleForTesting
107+
/*package*/ void install(Channel channel, @CheckForNull SlaveComputer c) {
93108
if (pingTimeoutSeconds < 1 || pingIntervalSeconds < 1) {
94109
LOGGER.warning("Agent ping is disabled");
95110
return;
96111
}
97112

113+
// set up ping from both directions, so that in case of a router dropping a connection,
114+
// both sides can notice it and take compensation actions.
98115
try {
99116
channel.call(new SetUpRemotePing(pingTimeoutSeconds, pingIntervalSeconds));
100117
LOGGER.fine("Set up a remote ping for " + channel.getName());
101118
} catch (Exception e) {
102-
LOGGER.severe("Failed to set up a ping for " + channel.getName());
119+
LOGGER.log(Level.SEVERE, "Failed to set up a ping for " + channel.getName(), e);
103120
}
104121

105-
// set up ping from both directions, so that in case of a router dropping a connection,
106-
// both sides can notice it and take compensation actions.
107-
setUpPingForChannel(channel, pingTimeoutSeconds, pingIntervalSeconds, true);
122+
setUpPingForChannel(channel, c, pingTimeoutSeconds, pingIntervalSeconds, true);
108123
}
109124

110-
static class SetUpRemotePing extends MasterToSlaveCallable<Void, IOException> {
125+
@VisibleForTesting
126+
/*package*/ static class SetUpRemotePing extends MasterToSlaveCallable<Void, IOException> {
111127
private static final long serialVersionUID = -2702219700841759872L;
112128
@Deprecated
113129
private transient int pingInterval;
@@ -121,7 +137,7 @@ static class SetUpRemotePing extends MasterToSlaveCallable<Void, IOException> {
121137

122138
@Override
123139
public Void call() throws IOException {
124-
setUpPingForChannel(Channel.current(), pingTimeoutSeconds, pingIntervalSeconds, false);
140+
setUpPingForChannel(Channel.current(), null, pingTimeoutSeconds, pingIntervalSeconds, false);
125141
return null;
126142
}
127143

@@ -163,30 +179,35 @@ protected Object readResolve() {
163179
}
164180
}
165181

166-
static void setUpPingForChannel(final Channel channel, int timeoutSeconds, int intervalSeconds, final boolean analysis) {
182+
@VisibleForTesting
183+
/*package*/ static void setUpPingForChannel(final Channel channel, final SlaveComputer computer, int timeoutSeconds, int intervalSeconds, final boolean analysis) {
167184
LOGGER.log(Level.FINE, "setting up ping on {0} with a {1} seconds interval and {2} seconds timeout", new Object[] {channel.getName(), intervalSeconds, timeoutSeconds});
168185
final AtomicBoolean isInClosed = new AtomicBoolean(false);
169186
final PingThread t = new PingThread(channel, timeoutSeconds * 1000L, intervalSeconds * 1000L) {
170187
@Override
171188
protected void onDead(Throwable cause) {
172-
try {
173189
if (analysis) {
174190
analyze(cause);
175191
}
176-
if (isInClosed.get()) {
192+
boolean inClosed = isInClosed.get();
193+
// Disassociate computer channel before closing it
194+
if (computer != null) {
195+
computer.disconnect(); // TODO specify cause
196+
}
197+
if (inClosed) {
177198
LOGGER.log(Level.FINE,"Ping failed after the channel "+channel.getName()+" is already partially closed.",cause);
178199
} else {
179200
LOGGER.log(Level.INFO,"Ping failed. Terminating the channel "+channel.getName()+".",cause);
180-
channel.close(cause);
181201
}
182-
} catch (IOException e) {
183-
LOGGER.log(Level.SEVERE,"Failed to terminate the channel "+channel.getName(),e);
184-
}
185202
}
186203
/** Keep in a separate method so we do not even try to do class loading on {@link PingFailureAnalyzer} from an agent JVM. */
187-
private void analyze(Throwable cause) throws IOException {
204+
private void analyze(Throwable cause) {
188205
for (PingFailureAnalyzer pfa : PingFailureAnalyzer.all()) {
189-
pfa.onPingFailure(channel,cause);
206+
try {
207+
pfa.onPingFailure(channel, cause);
208+
} catch (IOException ex) {
209+
LOGGER.log(Level.WARNING, "Ping failure analyzer failed for " + channel.getName(), ex);
210+
}
190211
}
191212
}
192213
@Deprecated

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

+8-8
Original file line numberDiff line numberDiff line change
@@ -59,12 +59,12 @@ private void preserveSystemProperty(String propertyName) {
5959
@Test
6060
public void testDefaults() throws Exception {
6161
ChannelPinger channelPinger = new ChannelPinger();
62-
channelPinger.install(mockChannel);
62+
channelPinger.install(mockChannel, null);
6363

6464
verify(mockChannel).call(eq(new ChannelPinger.SetUpRemotePing(ChannelPinger.PING_TIMEOUT_SECONDS_DEFAULT,
6565
ChannelPinger.PING_INTERVAL_SECONDS_DEFAULT)));
6666
verifyStatic();
67-
ChannelPinger.setUpPingForChannel(mockChannel, ChannelPinger.PING_TIMEOUT_SECONDS_DEFAULT,
67+
ChannelPinger.setUpPingForChannel(mockChannel, null, ChannelPinger.PING_TIMEOUT_SECONDS_DEFAULT,
6868
ChannelPinger.PING_INTERVAL_SECONDS_DEFAULT, true);
6969
}
7070

@@ -74,23 +74,23 @@ public void testFromSystemProperties() throws Exception {
7474
System.setProperty("hudson.slaves.ChannelPinger.pingIntervalSeconds", "73");
7575

7676
ChannelPinger channelPinger = new ChannelPinger();
77-
channelPinger.install(mockChannel);
77+
channelPinger.install(mockChannel, null);
7878

7979
verify(mockChannel).call(new ChannelPinger.SetUpRemotePing(42, 73));
8080
verifyStatic();
81-
ChannelPinger.setUpPingForChannel(mockChannel, 42, 73, true);
81+
ChannelPinger.setUpPingForChannel(mockChannel, null, 42, 73, true);
8282
}
8383

8484
@Test
8585
public void testFromOldSystemProperty() throws Exception {
8686
System.setProperty("hudson.slaves.ChannelPinger.pingInterval", "7");
8787

8888
ChannelPinger channelPinger = new ChannelPinger();
89-
channelPinger.install(mockChannel);
89+
channelPinger.install(mockChannel, null);
9090

9191
verify(mockChannel).call(eq(new ChannelPinger.SetUpRemotePing(ChannelPinger.PING_TIMEOUT_SECONDS_DEFAULT, 420)));
9292
verifyStatic();
93-
ChannelPinger.setUpPingForChannel(mockChannel, ChannelPinger.PING_TIMEOUT_SECONDS_DEFAULT, 420, true);
93+
ChannelPinger.setUpPingForChannel(mockChannel, null, ChannelPinger.PING_TIMEOUT_SECONDS_DEFAULT, 420, true);
9494
}
9595

9696
@Test
@@ -99,11 +99,11 @@ public void testNewSystemPropertyTrumpsOld() throws Exception {
9999
System.setProperty("hudson.slaves.ChannelPinger.pingInterval", "7");
100100

101101
ChannelPinger channelPinger = new ChannelPinger();
102-
channelPinger.install(mockChannel);
102+
channelPinger.install(mockChannel, null);
103103

104104
verify(mockChannel).call(eq(new ChannelPinger.SetUpRemotePing(ChannelPinger.PING_TIMEOUT_SECONDS_DEFAULT, 73)));
105105
verifyStatic();
106-
ChannelPinger.setUpPingForChannel(mockChannel, ChannelPinger.PING_TIMEOUT_SECONDS_DEFAULT, 73, true);
106+
ChannelPinger.setUpPingForChannel(mockChannel, null, ChannelPinger.PING_TIMEOUT_SECONDS_DEFAULT, 73, true);
107107
}
108108

109109
@Test

‎test/src/test/java/hudson/slaves/PingThreadTest.java

-2
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,6 @@ public void failedPingThreadResetsComputerChannel() throws Exception {
8484
// Expected
8585
}
8686

87-
Thread.sleep(10000);
88-
8987
assertNull(slave.getComputer().getChannel());
9088
assertNull(computer.getChannel());
9189
} finally {

0 commit comments

Comments
 (0)