Only reconnect offline agent if it is not idle#2020
Conversation
| // The idle timeout still expires and the instance is idle-terminated. | ||
| assertThat( | ||
| "The mock sets `idleTimeoutCalled` to true, if EC2AbstractSlave#idleTimeout is called", | ||
| idleTimeoutCalled.get(), | ||
| is(true)); |
There was a problem hiding this comment.
This is the best we can do atm with the existing test setup - though it doesn't prove that an agent is created inside Jenkins and deleted,
but uses mock computers that are anonymous inner classes in this test class. This assertion works because the mock here sets field idleTimeoutCalled for calling EC2AbstractSlave#idleTimeout
ec2-plugin/src/test/java/hudson/plugins/ec2/EC2RetentionStrategyTest.java
Lines 452 to 455 in 86eb364
There was a problem hiding this comment.
Note: this assertion won't fail even without the patch, because computer#connect doesn't work in this mock setup.
However the log assertion immediately below this demonstrates the failure the reconnect attempt, and fails the assertion.
| && (InstanceState.STOPPED.equals(state) || InstanceState.STOPPING.equals(state))) { | ||
| if (computer.isOnline()) { | ||
| LOGGER.info("External Stop of " + computer.getName() + " detected - disconnecting. instance status" | ||
| LOGGER.info("External Stop of " + computer.getName() + " detected - disconnecting. instance status " |
There was a problem hiding this comment.
Pull request overview
This PR adjusts EC2 agent retention behavior so that an agent marked offline is only automatically reconnected if it is not idle, allowing idle-termination to proceed for offline/idle agents (addressing #1752 / JENKINS-61789). It also reorders the reconnect condition to avoid unnecessary AWS API calls when local conditions already rule out reconnection.
Changes:
- Update
EC2RetentionStrategy#attemptReconnectIfOfflineto skip reconnects for offline idle agents and to evaluate local state before callingcomputer.getState(). - Add a unit test ensuring offline/idle agents still trigger idle-timeout termination and do not log reconnect attempts.
- Minor log message formatting tweak.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/main/java/hudson/plugins/ec2/EC2RetentionStrategy.java |
Changes reconnect gating to avoid reconnecting offline/idle agents and reduces unnecessary EC2 state lookups. |
src/test/java/hudson/plugins/ec2/EC2RetentionStrategyTest.java |
Adds regression test coverage for offline/idle termination behavior tied to issue #1752. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private void attemptReconnectIfOffline(EC2Computer computer) { | ||
| try { | ||
| if (computer.getState() == InstanceState.RUNNING && computer.isOffline()) { | ||
| if (computer.isOffline() && !computer.isIdle() && computer.getState() == InstanceState.RUNNING) { | ||
| LOGGER.warning("EC2Computer " + computer.getName() + " is offline"); | ||
| if (!computer.isConnecting()) { |
There was a problem hiding this comment.
Without the patch the new test submitted fails,
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 3.460 s <<< FAILURE! -- in hudson.plugins.ec2.EC2RetentionStrategyTest
[ERROR] hudson.plugins.ec2.EC2RetentionStrategyTest.testDoNotReconnectOfflineIdleComputer -- Time elapsed: 3.443 s <<< FAILURE!
java.lang.AssertionError:
No reconnection logs
Expected: <true>
but: was <false>
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at hudson.plugins.ec2.EC2RetentionStrategyTest.testDoNotReconnectOfflineIdleComputer(EC2RetentionStrategyTest.java:712)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
[INFO]
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] EC2RetentionStrategyTest.testDoNotReconnectOfflineIdleComputer:712 No reconnection logs
Expected: <true>
but: was <false>
[INFO]
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
jglick
left a comment
jglick
left a comment
There was a problem hiding this comment.
Sounds reasonable. I did not take the time to study the test.
| private void attemptReconnectIfOffline(EC2Computer computer) { | ||
| try { | ||
| if (computer.getState() == InstanceState.RUNNING && computer.isOffline()) { | ||
| if (computer.isOffline() && !computer.isIdle() && computer.getState() == InstanceState.RUNNING) { |
There was a problem hiding this comment.
Is there a reason to order the checks like this (avoiding work in short-circuit evaluation), or would the smaller diff
| if (computer.isOffline() && !computer.isIdle() && computer.getState() == InstanceState.RUNNING) { | |
| if (computer.getState() == InstanceState.RUNNING && computer.isOffline() && !computer.isIdle()) { |
work just as well?
There was a problem hiding this comment.
yes, it was to avoid the work in short-circuit. I had noticed in a thread dump the getState makes a network call; not exactly in this place but same file another method -
...
at software.amazon.awssdk.core.client.handler.SdkSyncClientHandler.execute(SdkSyncClientHandler.java:45)
at software.amazon.awssdk.awscore.client.handler.AwsSyncClientHandler.execute(AwsSyncClientHandler.java:53)
at software.amazon.awssdk.services.ec2.DefaultEc2Client.describeInstances(DefaultEc2Client.java:21130)
at hudson.plugins.ec2.CloudHelper.getInstance(CloudHelper.java:58)
at hudson.plugins.ec2.CloudHelper.getInstanceWithRetry(CloudHelper.java:31)
at hudson.plugins.ec2.EC2Computer.describeInstance(EC2Computer.java:194)
at hudson.plugins.ec2.EC2Computer.getState(EC2Computer.java:215)
at hudson.plugins.ec2.EC2RetentionStrategy.internalCheck(EC2RetentionStrategy.java:181)
at hudson.plugins.ec2.EC2RetentionStrategy.runHeavyCheck(EC2RetentionStrategy.java:145)
at hudson.plugins.ec2.EC2RetentionStrategy.lambda$check$1(EC2RetentionStrategy.java:128)
at hudson.plugins.ec2.EC2RetentionStrategy$$Lambda/0x000000f001899ca0.run(Unknown Source)
| logging.getMessages().stream().noneMatch(m -> m.contains("Attempting to reconnect")), | ||
| equalTo(true)); |
There was a problem hiding this comment.
There is probably a better matcher for this that would report the messages if the assertion failed.
There was a problem hiding this comment.
right, fixed now.
now shows all the log entries,
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 2.976 s <<< FAILURE! -- in hudson.plugins.ec2.EC2RetentionStrategyTest
[ERROR] hudson.plugins.ec2.EC2RetentionStrategyTest.testDoNotReconnectOfflineIdleComputer -- Time elapsed: 2.960 s <<< FAILURE!
java.lang.AssertionError:
No reconnection logs
Expected: not a collection containing a string containing "Attempting to reconnect"
but: was <[EC2Computer name is offline, Attempting to reconnect EC2Computer name, Computer id offline but not connecting, will check if it should be terminated because of the idle time configured, Idle timeout of name after 5 idle minutes, instance statusRUNNING]>
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at hudson.plugins.ec2.EC2RetentionStrategyTest.testDoNotReconnectOfflineIdleComputer(EC2RetentionStrategyTest.java:713)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
[INFO]
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] EC2RetentionStrategyTest.testDoNotReconnectOfflineIdleComputer:713 No reconnection logs
Expected: not a collection containing a string containing "Attempting to reconnect"
but: was <[EC2Computer name is offline, Attempting to reconnect EC2Computer name, Computer id offline but not connecting, will check if it should be terminated because of the idle time configured, Idle timeout of name after 5 idle minutes, instance statusRUNNING]>
[INFO]
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
Fixes #1752, checked the reconnection logic introduced in #1142 that it was intended for reconnecting busy agent that faced intermittent network failure.
Re-ordered the conditional logic to do the local check before making AWS API call in
computer.getState().Previously such agents would never hit the idle termination because the
Computer#connectwould bump its connect time, thus never allowing to expire the idle timeout (described in #1752 (comment))I believe it will also address the agents not being deleted during ssh key failure #1990 (comment). Note: this PR doesn't address the ssh key failure, but just that agent deletion would now start to happen when that issue is faced.
Testing
Automated Testing
Added a new unit test that using the existing machinery.
Manual Testing
Idle agent gets deleted
kept the
Maximum Total Usesto the default-1so the agent is not deleted after build is done.configured
Idle termination timeto3triggered simple build
node('ec2') { sh 'date' }marked the agent offline manually
after 3 minutes agent is deleted.
logs
Busy agent facing intermittent network issue is reconnected (behaves as before)
same setup was above, but changed the pipeline to
node('ec2') { sh 'sleep 240' }while the build was in progress, went to the ssh console of the VM (amazon linux), and restarted the ssh service with a 1 minute delay.
jenkins showed connection drop logs,
logs
Build succeeded
build log
This online agent becomes idle and gets deleted as expected too. (same behavior as before)
logs
Submitter checklist