Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JENKINS-23792] Do not terminate stopped EC2 nodes that were just booted #632

Merged
merged 8 commits into from
Jun 18, 2021
Merged

[JENKINS-23792] Do not terminate stopped EC2 nodes that were just booted #632

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8867fe6
regression, do not terminate stopped EC2 nodes that were just booted …
unicolet Jun 14, 2021
9a6d9e3
get launch time the right way
unicolet Jun 14, 2021
62c3dc9
move test into its own test case
unicolet Jun 14, 2021
61667eb
remove old test case, which was moved into its own method a few lines…
unicolet Jun 14, 2021
9fa24ba
rename launchedAtMs to getLaunchTime
unicolet Jun 14, 2021
1f188ef
provide valid test case for issue fixed by this PR https://issues.jen…
unicolet Jun 14, 2021
90592b8
wording
unicolet Jun 14, 2021
3bcb58c
fix reason for assertion
unicolet Jun 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/hudson/plugins/ec2/EC2RetentionStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ private long internalCheck(EC2Computer computer) {
}
}

final long idleMilliseconds = this.clock.millis() - computer.getIdleStartMilliseconds();
final long idleMilliseconds = this.clock.millis() - Math.max(computer.getIdleStartMilliseconds(), uptime);
unicolet marked this conversation as resolved.
Show resolved Hide resolved


if (idleTerminationMinutes > 0) {
Expand Down
15 changes: 15 additions & 0 deletions src/test/java/hudson/plugins/ec2/EC2RetentionStrategyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ private EC2Computer computerWithIdleTime(final int minutes, final int seconds) t
* the computer returns the value established.
*/
private EC2Computer computerWithIdleTime(final int minutes, final int seconds, final Boolean isOffline, final Boolean isConnecting) throws Exception {
idleTimeoutCalled.set(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

required, since the state is otherwise never reset across tests. Admittedly, this could be fixed better, but I wanted to keep the PR scope as small as possible to make it easy to review :-)

final EC2AbstractSlave slave = new EC2AbstractSlave("name", "id", "description", "fs", 1, null, "label", null, null, "init", "tmpDir", new ArrayList<NodeProperty<?>>(), "remote", "jvm", false, "idle", null, "cloud", false, Integer.MAX_VALUE, null, ConnectionStrategy.PRIVATE_IP, -1) {
@Override
public void terminate() {
Expand Down Expand Up @@ -221,6 +222,20 @@ public void testTerminateOfflineComputerIfNotConnecting() throws Exception {
rs.check(computer2);
assertThat("The computer is terminated, it should not accept more tasks", idleTimeoutCalled.get(), equalTo(true));
assertThat(logging.getMessages(), hasItem(containsString("offline but not connecting, will check if it should be terminated because of the idle time configured")));

// A computer that just booted should not be terminated. Let's wait
// until up to termination minutes before terminating it
final Instant inOneMinute = Instant.now().plus(Duration.ofSeconds(60));
unicolet marked this conversation as resolved.
Show resolved Hide resolved
rs = new EC2RetentionStrategy(
"5",
Clock.fixed(inOneMinute.plusSeconds(1), zoneId),
inOneMinute.toEpochMilli()
);
final EC2Computer booting = computerWithIdleTime(0, 0, null, false);
// We terminate this one
rs.check(booting);
assertThat("The computer is terminated, but should not be", idleTimeoutCalled.get(), equalTo(false));
assertThat(logging.getMessages(), hasItem(containsString("offline but not connecting, will check if it should be terminated because of the idle time configured")));
}

@Test
Expand Down