Skip to content

Commit

Permalink
[fix] Terminate scheduled instances ONLY IF idle
Browse files Browse the repository at this point in the history 
jenkinsci#363
  • Loading branch information
@pdk27
pdk27 committed Jun 27, 2023
1 parent 53fb6b0 commit d8d107b
Show file tree
Hide file tree
Showing 2 changed files with 119 additions and 7 deletions.
35 changes: 28 additions & 7 deletions src/main/java/com/amazon/jenkins/ec2fleet/EC2FleetCloud.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import com.amazonaws.services.ec2.model.Instance;
import com.amazonaws.services.ec2.model.InstanceStateName;
import com.cloudbees.jenkins.plugins.awscredentials.AWSCredentialsHelper;
import com.google.common.collect.Sets;
import hudson.Extension;
import hudson.model.Computer;
import hudson.model.Descriptor;
Expand Down Expand Up @@ -51,6 +52,7 @@
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.logging.SimpleFormatter;
import java.util.stream.Collectors;

/**
* @see CloudNanny
Expand Down Expand Up @@ -212,7 +214,7 @@ public EC2FleetCloud(final String name,
this.minSize = Math.max(0, minSize);
this.maxSize = maxSize;
this.minSpareSize = Math.max(0, minSpareSize);
this.maxTotalUses = StringUtils.isBlank(maxTotalUses) ? -1 : Integer.parseInt(maxTotalUses);
this.maxTotalUses = StringUtils.isBlank(maxTotalUses) ? DEFAULT_MAX_TOTAL_USES : Integer.parseInt(maxTotalUses);
this.numExecutors = Math.max(numExecutors, 1);
this.addNodeOnlyIfRunning = addNodeOnlyIfRunning;
this.restrictUsage = restrictUsage;
Expand Down Expand Up @@ -284,10 +286,6 @@ public String getEndpoint() {
return endpoint;
}

public int getMaxTotalUses() {
return maxTotalUses == null ? DEFAULT_MAX_TOTAL_USES : maxTotalUses;
}

@Override
public String getFleet() {
return fleet;
Expand Down Expand Up @@ -382,6 +380,11 @@ synchronized void setStats(final FleetStateStats stats) {
this.stats = stats;
}

// make maxTotalUses inaccessible from cloud for safety. Use {@link EC2FleetNode#maxTotalUses} and {@link EC2FleetNode#usesRemaining} instead.
public boolean hasUnlimitedUsesForNodes() {
return maxTotalUses == -1;
}

@Override
public synchronized boolean hasExcessCapacity() {
if(stats == null) {
Expand Down Expand Up @@ -502,7 +505,25 @@ public FleetStateStats update() {
}
}
currentToAdd = toAdd;
currentInstanceIdsToTerminate = new HashMap<>(instanceIdsToTerminate);

// for computers currently busy doing work, wait until next update cycle to terminate corresponding instances (issue#363).
final Jenkins j = Jenkins.get();
currentInstanceIdsToTerminate = instanceIdsToTerminate.entrySet()
.stream()
.filter(e -> {
final Node node = j.getNode(e.getKey());
if (node != null) {
final Computer comp = node.toComputer();
return comp == null || comp.isIdle();
}
return true;
})
.collect(Collectors.toMap(Map.Entry::getKey,Map.Entry::getValue));

final Set<String> filteredOutNonIdleIds = Sets.difference(instanceIdsToTerminate.keySet(), currentInstanceIdsToTerminate.keySet());
if (filteredOutNonIdleIds.size() > 0) {
info("Skipping termination of the following instances as they are still busy doing some work: %s", filteredOutNonIdleIds);
}
}

currentState = updateByState(currentToAdd, currentInstanceIdsToTerminate, currentState);
Expand Down Expand Up @@ -829,7 +850,7 @@ private void addNewSlave(final AmazonEC2 ec2, final Instance instance, FleetStat
final Node.Mode nodeMode = restrictUsage ? Node.Mode.EXCLUSIVE : Node.Mode.NORMAL;
final EC2FleetNode node = new EC2FleetNode(instanceId, "Fleet slave for " + instanceId,
effectiveFsRoot, effectiveNumExecutors, nodeMode, labelString, new ArrayList<NodeProperty<?>>(),
this, computerLauncher, getMaxTotalUses());
this, computerLauncher, maxTotalUses);

// Initialize our retention strategy
node.setRetentionStrategy(new EC2RetentionStrategy());
Expand Down
91 changes: 91 additions & 0 deletions src/test/java/com/amazon/jenkins/ec2fleet/EC2FleetCloudTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
Expand Down Expand Up @@ -1227,6 +1228,44 @@ public void update_shouldUpdateStateWithFleetTargetCapacityMinusToTerminate() th
Assert.assertEquals(4, fleetCloud.getStats().getNumDesired());
}

/**
* For context, see https://github.com/jenkinsci/ec2-fleet-plugin/issues/363
*/
@Test
public void update_shouldTerminateIdleInstancesOnly() {
// given
Node nodeMock1 = mock(Node.class);
Node nodeMock2 = mock(Node.class);
Computer compMock1 = mock(Computer.class);
Computer compMock2 = mock(Computer.class);

PowerMockito.when(ec2Fleet.getState(anyString(), anyString(), anyString(), anyString()))
.thenReturn(new FleetStateStats("", 0, FleetStateStats.State.active(),
Collections.<String>emptySet(), Collections.<String, Double>emptyMap()));
when(jenkins.getNode("i-0")).thenReturn(nodeMock1);
when(jenkins.getNode("i-1")).thenReturn(nodeMock2);
doReturn(compMock1).when(nodeMock1).toComputer();
doReturn(compMock2).when(nodeMock2).toComputer();
doReturn(true).when(compMock1).isIdle();
doReturn(false).when(compMock2).isIdle();

EC2FleetCloud fleetCloud = new EC2FleetCloud(null, null, "credId", null, "region",
"", "fleetId", "", null, PowerMockito.mock(ComputerConnector.class), false,
false, 0, 0, 10, 0,1, false,
true, "-1", false,
0, 0, true, 10, false);

fleetCloud.scheduleToTerminate("i-0", false, EC2AgentTerminationReason.IDLE_FOR_TOO_LONG);
fleetCloud.scheduleToTerminate("i-1", true, EC2AgentTerminationReason.MAX_TOTAL_USES_EXHAUSTED);

// when
fleetCloud.update();

// then
verify(ec2Api).terminateInstances(amazonEC2, new HashSet<>(Arrays.asList("i-0")));
Assert.assertEquals(Collections.singleton("i-1"), fleetCloud.getStats().getInstances());
}

@Test
public void update_shouldUpdateStateWithMinSpare() throws IOException {
// given
Expand Down Expand Up @@ -1968,6 +2007,58 @@ public void create_numExecutorsLessThenOneShouldUpgradedToOne() {
assertEquals(1, ec2FleetCloud.getNumExecutors());
}

@Test
public void hasUnlimitedUsesForNodes_shouldReturnTrueWhenUnlimited() {
final int maxTotalUses = -1;
EC2FleetCloud ec2FleetCloud = new EC2FleetCloud(
"CloudName", null, null, null, null, null, null,
null, null, null, false,
false, null, 0, 1, 0,
0, true, false, String.valueOf(maxTotalUses), false
, 0, 0, false,
45, false);
assertTrue(ec2FleetCloud.hasUnlimitedUsesForNodes());
}

@Test
public void hasUnlimitedUsesForNodes_shouldReturnDefaultTrueForNull() {
final String maxTotalUses = null;
EC2FleetCloud ec2FleetCloud = new EC2FleetCloud(
"CloudName", null, null, null, null, null, null,
null, null, null, false,
false, null, 0, 1, 0,
0, true, false, maxTotalUses, false
, 0, 0, false,
45, false);
assertTrue(ec2FleetCloud.hasUnlimitedUsesForNodes());
}

@Test
public void hasUnlimitedUsesForNodes_shouldReturnDefaultTrueForEmptyString() {
final String maxTotalUses = "";
EC2FleetCloud ec2FleetCloud = new EC2FleetCloud(
"CloudName", null, null, null, null, null, null,
null, null, null, false,
false, null, 0, 1, 0,
0, true, false, maxTotalUses, false
, 0, 0, false,
45, false);
assertTrue(ec2FleetCloud.hasUnlimitedUsesForNodes());
}

@Test
public void hasUnlimitedUsesForNodes_shouldReturnFalseWhenLimited() {
final int maxTotalUses = 5;
EC2FleetCloud ec2FleetCloud = new EC2FleetCloud(
"CloudName", null, null, null, null, null, null,
null, null, null, false,
false, null, 0, 1, 0,
0, true, false, String.valueOf(maxTotalUses), false
, 0, 0, false,
45, false);
assertFalse(ec2FleetCloud.hasUnlimitedUsesForNodes());
}

private void mockNodeCreatingPart() {
when(jenkins.getNodesObject()).thenReturn(mock(Nodes.class));

Expand Down

0 comments on commit d8d107b

Please sign in to comment.