Skip to content

Commit

Permalink
Choose subnets randomly when there are equivalent options
Browse files Browse the repository at this point in the history 
The current placement strategy is not really ideal, but when there are capacity issues we
can probably improve things by not constantly trying to select the same subnet to allocate an instance in.
Randomising equivalent choices may probably help a bit, even if the strategy is still "dumb".
  • Loading branch information
@chadlwilson
chadlwilson committed Dec 12, 2024
1 parent 0ffe4de commit 10adb06
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 34 deletions.
2 changes: 1 addition & 1 deletion build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ group = 'com.thoughtworks.gocd'

gocdPlugin {
id = 'com.thoughtworks.gocd.elastic-agent.ecs'
pluginVersion = '8.0.0'
pluginVersion = '8.0.1'
goCdVersion = '21.4.0'
name = 'GoCD Elastic Agent Plugin for Amazon ECS'
description = 'GoCD Elastic Agent Plugin for Amazon Elastic Container Service allow for more efficient use of instances'
Expand Down
26 changes: 21 additions & 5 deletions src/main/java/com/thoughtworks/gocd/elasticagent/ecs/aws/SubnetSelector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,15 @@
import com.google.common.base.Joiner;
import com.thoughtworks.gocd.elasticagent.ecs.domain.PluginSettings;
import com.thoughtworks.gocd.elasticagent.ecs.exceptions.SubnetNotAvailableException;
import org.apache.commons.lang3.RandomUtils;

import java.util.*;
import java.util.stream.Collectors;

import static com.thoughtworks.gocd.elasticagent.ecs.ECSElasticPlugin.LOG;
import static java.text.MessageFormat.format;
import static java.util.stream.Collectors.collectingAndThen;
import static java.util.stream.Collectors.toList;

public class SubnetSelector {
public Subnet selectSubnetWithMinimumEC2Instances(PluginSettings pluginSettings, Collection<String> subnetIds, List<Instance> instances) {
Expand All @@ -40,7 +43,7 @@ public Subnet selectSubnetWithMinimumEC2Instances(PluginSettings pluginSettings,
final List<Subnet> subnets = availableSubnets(pluginSettings, subnetIds);

if (instances.isEmpty()) {
return subnets.get(0);
return randomFrom(subnets);
}

return findSubnetWithMinimumInstances(subnets, instances);
Expand All @@ -52,13 +55,26 @@ private Subnet findSubnetWithMinimumInstances(List<Subnet> subnets, List<Instanc
// Can happen if all found instances happen to be from other elastic agent profiles (which may or may not
// be sharing a profile with our plugin configuration
if (instancePerSubnet.isEmpty()) {
return subnets.get(0);
return randomFrom(subnets);
}

Long minCount = instancePerSubnet.values().stream().min(Long::compareTo).orElse(0L);

return subnets.stream()
.filter(subnet -> !instancePerSubnet.containsKey(subnet))
.findFirst()
.orElse(Collections.min(instancePerSubnet.entrySet(), Comparator.comparingDouble(Map.Entry::getValue)).getKey());
.collect(collectingAndThen(toList(), this::optionallyRandomFrom))
.orElse(instancePerSubnet.entrySet().stream()
.filter(e -> minCount.equals(e.getValue()))
.map(Map.Entry::getKey)
.collect(collectingAndThen(toList(), this::randomFrom)));
}

private Optional<Subnet> optionallyRandomFrom(List<Subnet> subnets) {
return subnets.isEmpty() ? Optional.empty() : Optional.of(randomFrom(subnets));
}

private Subnet randomFrom(List<Subnet> subnets) {
return subnets.get(RandomUtils.insecure().randomInt(0, subnets.size()));
}

private Map<Subnet, Long> instancePerSubnet(List<Subnet> subnets, List<Instance> instances) {
Expand All @@ -72,7 +88,7 @@ private Map<Subnet, Long> instancePerSubnet(List<Subnet> subnets, List<Instance>
private List<Subnet> availableSubnets(PluginSettings pluginSettings, Collection<String> subnetIds) {
final List<Subnet> subnets = allSubnets(pluginSettings, subnetIds).stream()
.filter(this::isSubnetAvailable)
.collect(Collectors.toList());
.collect(toList());

if (subnets.isEmpty()) {
throw new SubnetNotAvailableException(format("None of the subnet available to launch ec2 instance from list {0}", Joiner.on(",").join(subnetIds)));
Expand Down
63 changes: 35 additions & 28 deletions src/test/java/com/thoughtworks/gocd/elasticagent/ecs/aws/SubnetSelectorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.stream.IntStream;

import static java.util.Collections.emptyList;
import static java.util.stream.Collectors.toSet;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.any;
Expand Down Expand Up @@ -64,7 +66,7 @@ void shouldReturnNullIfSubnetIdsNotConfiguredInPluginSettings() {

@Test
void shouldErrorOutIfNoSubnetAvailable() {
when(pluginSettings.getSubnetIds()).thenReturn(Arrays.asList("subnet-1"));
when(pluginSettings.getSubnetIds()).thenReturn(List.of("subnet-1"));
when(ec2Client.describeSubnets(any(DescribeSubnetsRequest.class))).thenReturn(new DescribeSubnetsResult().withSubnets(
new Subnet().withSubnetId("subnet-1").withState("pending")
));
Expand All @@ -75,61 +77,66 @@ void shouldErrorOutIfNoSubnetAvailable() {
}

@Test
void shouldReturnFirstAvailableSubnetIdIfNoContainerInstanceRunning() {
void shouldReturnRandomSubnetIdIfNoContainerInstanceRunningAndMultipleSubnetOptions() {
final ArgumentCaptor<DescribeSubnetsRequest> argumentCaptor = ArgumentCaptor.forClass(DescribeSubnetsRequest.class);

when(pluginSettings.getSubnetIds()).thenReturn(Arrays.asList("subnet-1"));
when(pluginSettings.getSubnetIds()).thenReturn(List.of("subnet-1", "subnet-2"));
when(ec2Client.describeSubnets(argumentCaptor.capture())).thenReturn(new DescribeSubnetsResult().withSubnets(
new Subnet().withSubnetId("subnet-1").withState("available")
new Subnet().withSubnetId("subnet-1").withState("available"),
new Subnet().withSubnetId("subnet-2").withState("available")
));

final Subnet subnet = subnetSelector.selectSubnetWithMinimumEC2Instances(pluginSettings, pluginSettings.getSubnetIds(), emptyList());

assertThat(subnet.getSubnetId()).isEqualTo("subnet-1");
assertThat(argumentCaptor.getValue().getSubnetIds()).isEqualTo(Arrays.asList("subnet-1"));
assertThat(IntStream.range(0, 10)
.mapToObj(i -> subnetSelector.selectSubnetWithMinimumEC2Instances(pluginSettings, pluginSettings.getSubnetIds(), emptyList()).getSubnetId())
.collect(toSet()))
.containsExactly("subnet-1", "subnet-2");
}

@Test
void shouldReturnFirstAvailableSubnetIdIfContainerInstanceRunningInUnrelatedSubnet() {
void shouldReturnRandomSubnetIdIfContainerInstanceRunningInUnrelatedSubnetAndMultipleSubnetOptions() {
final ArgumentCaptor<DescribeSubnetsRequest> argumentCaptor = ArgumentCaptor.forClass(DescribeSubnetsRequest.class);

when(pluginSettings.getSubnetIds()).thenReturn(Arrays.asList("subnet-1"));
when(pluginSettings.getSubnetIds()).thenReturn(List.of("subnet-1", "subnet-2"));
when(ec2Client.describeSubnets(argumentCaptor.capture())).thenReturn(new DescribeSubnetsResult().withSubnets(
new Subnet().withSubnetId("subnet-1").withState("available")
new Subnet().withSubnetId("subnet-1").withState("available"),
new Subnet().withSubnetId("subnet-2").withState("available")
));

Instance instanceInOtherSubnet = new Instance().withSubnetId("some-other-subnet");

final Subnet subnet = subnetSelector.selectSubnetWithMinimumEC2Instances(pluginSettings, pluginSettings.getSubnetIds(), List.of(instanceInOtherSubnet));

assertThat(subnet.getSubnetId()).isEqualTo("subnet-1");
assertThat(argumentCaptor.getValue().getSubnetIds()).isEqualTo(Arrays.asList("subnet-1"));
assertThat(IntStream.range(0, 10)
.mapToObj(i -> subnetSelector.selectSubnetWithMinimumEC2Instances(pluginSettings, pluginSettings.getSubnetIds(), List.of(instanceInOtherSubnet)).getSubnetId())
.collect(toSet()))
.containsExactly("subnet-1", "subnet-2");
}

@Test
void shouldReturnSubnetIdWhichIsHavingMinimumEC2InstanceRunning() {
final ArgumentCaptor<DescribeSubnetsRequest> argumentCaptor = ArgumentCaptor.forClass(DescribeSubnetsRequest.class);

when(pluginSettings.getSubnetIds()).thenReturn(Arrays.asList("subnet-1"));
when(pluginSettings.getSubnetIds()).thenReturn(List.of("subnet-1", "subnet-2", "subnet-3", "subnet-4", "subnet-5"));
when(ec2Client.describeSubnets(argumentCaptor.capture())).thenReturn(new DescribeSubnetsResult().withSubnets(
new Subnet().withSubnetId("subnet-1").withState("pending"),
new Subnet().withSubnetId("subnet-2").withState("available"),
new Subnet().withSubnetId("subnet-3").withState("available"),
new Subnet().withSubnetId("subnet-4").withState("available")
new Subnet().withSubnetId("subnet-4").withState("available"),
new Subnet().withSubnetId("subnet-5").withState("available")
));

final List<Instance> instances = Arrays.asList(
new Instance().withInstanceId("instance-1").withSubnetId("subnet-1"),
new Instance().withInstanceId("instance-2").withSubnetId("subnet-2"),
new Instance().withInstanceId("instance-3").withSubnetId("subnet-2"),
new Instance().withInstanceId("instance-4").withSubnetId("subnet-3"),
new Instance().withInstanceId("instance-5").withSubnetId("subnet-4"),
new Instance().withInstanceId("instance-6").withSubnetId("subnet-4"),
new Instance().withInstanceId("instance-7").withSubnetId("subnet-4")
final List<Instance> instances = List.of(
new Instance().withInstanceId("instance-1.1").withSubnetId("subnet-1"),
new Instance().withInstanceId("instance-2.1").withSubnetId("subnet-2"),
new Instance().withInstanceId("instance-2.2").withSubnetId("subnet-2"),
new Instance().withInstanceId("instance-3.1").withSubnetId("subnet-3"),
new Instance().withInstanceId("instance-4.1").withSubnetId("subnet-4"),
new Instance().withInstanceId("instance-4.2").withSubnetId("subnet-4"),
new Instance().withInstanceId("instance-4.3").withSubnetId("subnet-4"),
new Instance().withInstanceId("instance-5.1").withSubnetId("subnet-5")
);

final Subnet subnet = subnetSelector.selectSubnetWithMinimumEC2Instances(pluginSettings, pluginSettings.getSubnetIds(), instances);

assertThat(subnet.getSubnetId()).isEqualTo("subnet-3");
assertThat(IntStream.range(0, 10)
.mapToObj(i -> subnetSelector.selectSubnetWithMinimumEC2Instances(pluginSettings, pluginSettings.getSubnetIds(), instances).getSubnetId())
.collect(toSet()))
.containsExactly("subnet-3", "subnet-5");
}
}

0 comments on commit 10adb06

Please sign in to comment.