diff --git a/src/main/java/hudson/plugins/ec2/EC2SpotSlave.java b/src/main/java/hudson/plugins/ec2/EC2SpotSlave.java index 7256dfe57..c4b1a77ab 100644 --- a/src/main/java/hudson/plugins/ec2/EC2SpotSlave.java +++ b/src/main/java/hudson/plugins/ec2/EC2SpotSlave.java @@ -10,6 +10,7 @@ import hudson.slaves.NodeProperty; import java.io.IOException; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.concurrent.CompletableFuture; import java.util.concurrent.Future; @@ -310,13 +311,44 @@ public String getSpotInstanceRequestId() { public String getInstanceId() { if (StringUtils.isEmpty(instanceId)) { SpotInstanceRequest sr = getSpotRequest(); - if (sr != null) { + if (sr != null && StringUtils.isNotEmpty(sr.instanceId())) { instanceId = sr.instanceId(); + // Tag the instance immediately when it becomes available + // Note: Only tag the instance itself, not volumes, as volumes may not be attached yet + tagInstanceOnFulfillment(); } } return instanceId; } + private void tagInstanceOnFulfillment() { + if (tags == null || tags.isEmpty()) { + return; + } + + try { + LOGGER.info("Tagging spot instance " + instanceId + " immediately on fulfillment"); + HashSet instTags = new HashSet<>(); + + for (EC2Tag t : tags) { + instTags.add(software.amazon.awssdk.services.ec2.model.Tag.builder() + .key(t.getName()) + .value(t.getValue()) + .build()); + } + + // Only tag the instance itself, not volumes (volumes may not be attached yet) + software.amazon.awssdk.services.ec2.model.CreateTagsRequest tagRequest = + software.amazon.awssdk.services.ec2.model.CreateTagsRequest.builder() + .resources(instanceId) + .tags(instTags) + .build(); + getCloud().connect().createTags(tagRequest); + } catch (Exception e) { + LOGGER.log(Level.WARNING, "Failed to tag spot instance " + instanceId + " on fulfillment", e); + } + } + @Override public void onConnected() { // The spot request has been fulfilled and is connected. If the Spot diff --git a/src/main/java/hudson/plugins/ec2/SlaveTemplate.java b/src/main/java/hudson/plugins/ec2/SlaveTemplate.java index 8c0c12bde..dfc569022 100644 --- a/src/main/java/hudson/plugins/ec2/SlaveTemplate.java +++ b/src/main/java/hudson/plugins/ec2/SlaveTemplate.java @@ -2768,6 +2768,13 @@ private List provisionSpot(Image image, int number, EnumSet tagList = new ArrayList<>(); + tagList.add(TagSpecification.builder() + .tags(instTags) + .resourceType(ResourceType.SPOT_INSTANCES_REQUEST) + .build()); + spotRequestBuilder.tagSpecifications(tagList); + RequestSpotInstancesResponse reqResult; try { // Make the request for a new Spot instance @@ -2836,6 +2843,11 @@ private List provisionSpot(Image image, int number, EnumSet instTags) { + try { + LOGGER.info("Tagging spot instance " + instanceId + " at creation time"); + List resources = new ArrayList<>(); + resources.add(instanceId); + + ec2.createTags(CreateTagsRequest.builder() + .resources(resources) + .tags(instTags) + .build()); + } catch (AwsServiceException e) { + LOGGER.log( + Level.WARNING, + "Failed to tag spot instance " + instanceId + " at creation: " + + e.awsErrorDetails().errorMessage(), + e); + } + } + /** * Get a list of security group ids for the agent */ diff --git a/src/test/java/hudson/plugins/ec2/EC2SpotSlaveUnitTest.java b/src/test/java/hudson/plugins/ec2/EC2SpotSlaveUnitTest.java new file mode 100644 index 000000000..3d8665ab0 --- /dev/null +++ b/src/test/java/hudson/plugins/ec2/EC2SpotSlaveUnitTest.java @@ -0,0 +1,245 @@ +package hudson.plugins.ec2; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.*; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.logging.Handler; +import java.util.logging.LogRecord; +import java.util.logging.Logger; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.junit.jupiter.WithJenkins; +import org.mockito.ArgumentCaptor; +import software.amazon.awssdk.services.ec2.Ec2Client; +import software.amazon.awssdk.services.ec2.model.CreateTagsRequest; +import software.amazon.awssdk.services.ec2.model.CreateTagsResponse; +import software.amazon.awssdk.services.ec2.model.DescribeSpotInstanceRequestsRequest; +import software.amazon.awssdk.services.ec2.model.DescribeSpotInstanceRequestsResponse; +import software.amazon.awssdk.services.ec2.model.SpotInstanceRequest; + +@WithJenkins +class EC2SpotSlaveUnitTest { + + private JenkinsRule r; + private Logger logger; + private TestHandler handler; + + @BeforeEach + void setUp(JenkinsRule rule) { + r = rule; + handler = new TestHandler(); + logger = Logger.getLogger(EC2SpotSlave.class.getName()); + logger.addHandler(handler); + } + + private EC2SpotSlave createSpotSlave(String spotRequestId, List tags) throws Exception { + return new EC2SpotSlave( + "test-slave", + spotRequestId, + "test description", + "/tmp", + 1, + hudson.model.Node.Mode.NORMAL, + "initScript", + "tmpDir", + "label", + Collections.emptyList(), + "remoteAdmin", + EC2AbstractSlave.DEFAULT_JAVA_PATH, + "-Xmx1g", + "30", + tags, + "test-cloud", + 300, + new UnixData("", null, null, "22", null), + ConnectionStrategy.PRIVATE_IP, + -1); + } + + @Test + void testGetInstanceIdTagsInstanceOnFulfillment() throws Exception { + Ec2Client ec2 = mock(Ec2Client.class); + EC2Cloud cloud = mock(EC2Cloud.class); + when(cloud.connect()).thenReturn(ec2); + + SpotInstanceRequest spotRequest = SpotInstanceRequest.builder() + .spotInstanceRequestId("sir-12345") + .instanceId("i-abcdef") + .build(); + + DescribeSpotInstanceRequestsResponse response = DescribeSpotInstanceRequestsResponse.builder() + .spotInstanceRequests(spotRequest) + .build(); + when(ec2.describeSpotInstanceRequests(any(DescribeSpotInstanceRequestsRequest.class))) + .thenReturn(response); + when(ec2.createTags(any(CreateTagsRequest.class))) + .thenReturn(CreateTagsResponse.builder().build()); + + List tags = new ArrayList<>(); + tags.add(new EC2Tag("Name", "my-spot-instance")); + tags.add(new EC2Tag("Team", "platform")); + + EC2SpotSlave slave = spy(createSpotSlave("sir-12345", tags)); + doReturn(cloud).when(slave).getCloud(); + + String instanceId = slave.getInstanceId(); + + assertEquals("i-abcdef", instanceId); + + ArgumentCaptor captor = ArgumentCaptor.forClass(CreateTagsRequest.class); + verify(ec2).createTags(captor.capture()); + + CreateTagsRequest tagRequest = captor.getValue(); + assertEquals(Collections.singletonList("i-abcdef"), tagRequest.resources()); + assertEquals(2, tagRequest.tags().size()); + assertTrue(tagRequest.tags().stream() + .anyMatch(t -> "Name".equals(t.key()) && "my-spot-instance".equals(t.value()))); + assertTrue(tagRequest.tags().stream().anyMatch(t -> "Team".equals(t.key()) && "platform".equals(t.value()))); + } + + @Test + void testGetInstanceIdDoesNotTagWhenInstanceIdEmpty() throws Exception { + Ec2Client ec2 = mock(Ec2Client.class); + EC2Cloud cloud = mock(EC2Cloud.class); + when(cloud.connect()).thenReturn(ec2); + + SpotInstanceRequest spotRequest = SpotInstanceRequest.builder() + .spotInstanceRequestId("sir-12345") + .instanceId("") + .build(); + + DescribeSpotInstanceRequestsResponse response = DescribeSpotInstanceRequestsResponse.builder() + .spotInstanceRequests(spotRequest) + .build(); + when(ec2.describeSpotInstanceRequests(any(DescribeSpotInstanceRequestsRequest.class))) + .thenReturn(response); + + List tags = new ArrayList<>(); + tags.add(new EC2Tag("Name", "my-spot-instance")); + + EC2SpotSlave slave = spy(createSpotSlave("sir-12345", tags)); + doReturn(cloud).when(slave).getCloud(); + + String instanceId = slave.getInstanceId(); + + assertEquals("", instanceId); + verify(ec2, never()).createTags(any(CreateTagsRequest.class)); + } + + @Test + void testGetInstanceIdDoesNotTagWhenSpotRequestNull() throws Exception { + Ec2Client ec2 = mock(Ec2Client.class); + EC2Cloud cloud = mock(EC2Cloud.class); + when(cloud.connect()).thenReturn(ec2); + + EC2SpotSlave slave = spy(createSpotSlave(null, Collections.singletonList(new EC2Tag("Name", "test")))); + doReturn(cloud).when(slave).getCloud(); + + String instanceId = slave.getInstanceId(); + + assertEquals("", instanceId); + verify(ec2, never()).createTags(any(CreateTagsRequest.class)); + } + + @Test + void testGetInstanceIdDoesNotTagWhenTagsEmpty() throws Exception { + Ec2Client ec2 = mock(Ec2Client.class); + EC2Cloud cloud = mock(EC2Cloud.class); + when(cloud.connect()).thenReturn(ec2); + + SpotInstanceRequest spotRequest = SpotInstanceRequest.builder() + .spotInstanceRequestId("sir-12345") + .instanceId("i-abcdef") + .build(); + + DescribeSpotInstanceRequestsResponse response = DescribeSpotInstanceRequestsResponse.builder() + .spotInstanceRequests(spotRequest) + .build(); + when(ec2.describeSpotInstanceRequests(any(DescribeSpotInstanceRequestsRequest.class))) + .thenReturn(response); + + EC2SpotSlave slave = spy(createSpotSlave("sir-12345", Collections.emptyList())); + doReturn(cloud).when(slave).getCloud(); + + String instanceId = slave.getInstanceId(); + + assertEquals("i-abcdef", instanceId); + verify(ec2, never()).createTags(any(CreateTagsRequest.class)); + } + + @Test + void testGetInstanceIdHandlesTaggingException() throws Exception { + Ec2Client ec2 = mock(Ec2Client.class); + EC2Cloud cloud = mock(EC2Cloud.class); + when(cloud.connect()).thenReturn(ec2); + + SpotInstanceRequest spotRequest = SpotInstanceRequest.builder() + .spotInstanceRequestId("sir-12345") + .instanceId("i-abcdef") + .build(); + + DescribeSpotInstanceRequestsResponse response = DescribeSpotInstanceRequestsResponse.builder() + .spotInstanceRequests(spotRequest) + .build(); + when(ec2.describeSpotInstanceRequests(any(DescribeSpotInstanceRequestsRequest.class))) + .thenReturn(response); + when(ec2.createTags(any(CreateTagsRequest.class))).thenThrow(new RuntimeException("AWS API failure")); + + List tags = new ArrayList<>(); + tags.add(new EC2Tag("Name", "my-spot-instance")); + + EC2SpotSlave slave = spy(createSpotSlave("sir-12345", tags)); + doReturn(cloud).when(slave).getCloud(); + + String instanceId = slave.getInstanceId(); + + assertEquals("i-abcdef", instanceId); + assertTrue(handler.getRecords().stream() + .anyMatch(r -> r.getMessage().contains("Failed to tag spot instance i-abcdef"))); + } + + @Test + void testGetInstanceIdDoesNotRetagWhenAlreadySet() throws Exception { + Ec2Client ec2 = mock(Ec2Client.class); + EC2Cloud cloud = mock(EC2Cloud.class); + when(cloud.connect()).thenReturn(ec2); + + List tags = new ArrayList<>(); + tags.add(new EC2Tag("Name", "my-spot-instance")); + + EC2SpotSlave slave = spy(createSpotSlave("sir-12345", tags)); + doReturn(cloud).when(slave).getCloud(); + + slave.instanceId = "i-already-set"; + + String instanceId = slave.getInstanceId(); + + assertEquals("i-already-set", instanceId); + verify(ec2, never()).describeSpotInstanceRequests(any(DescribeSpotInstanceRequestsRequest.class)); + verify(ec2, never()).createTags(any(CreateTagsRequest.class)); + } + + static class TestHandler extends Handler { + private final List records = new ArrayList<>(); + + @Override + public void close() throws SecurityException {} + + @Override + public void flush() {} + + @Override + public void publish(LogRecord record) { + records.add(record); + } + + public List getRecords() { + return records; + } + } +} diff --git a/src/test/java/hudson/plugins/ec2/SlaveTemplateUnitTest.java b/src/test/java/hudson/plugins/ec2/SlaveTemplateUnitTest.java index d71b717e7..31e34f048 100644 --- a/src/test/java/hudson/plugins/ec2/SlaveTemplateUnitTest.java +++ b/src/test/java/hudson/plugins/ec2/SlaveTemplateUnitTest.java @@ -1243,6 +1243,187 @@ void testChooseSemicolonDelimitedSubnetId() { assertEquals("subnet-123", subnet3); } + @Test + void testTagSpotInstanceSuccess() throws Exception { + List capturedRequests = new ArrayList<>(); + Ec2Client ec2 = new Ec2Client() { + @Override + public CreateTagsResponse createTags(CreateTagsRequest createTagsRequest) { + capturedRequests.add(createTagsRequest); + return CreateTagsResponse.builder().build(); + } + + @Override + public void close() {} + + @Override + public String serviceName() { + return "AmazonEC2"; + } + }; + + SlaveTemplate template = + new SlaveTemplate( + "ami1", + EC2AbstractSlave.TEST_ZONE, + null, + "default", + "foo", + InstanceType.M1_LARGE.toString(), + false, + "ttt", + Node.Mode.NORMAL, + "foo ami", + "bar", + "bbb", + "aaa", + "10", + "fff", + null, + EC2AbstractSlave.DEFAULT_JAVA_PATH, + "-Xmx1g", + false, + "subnet 456", + null, + null, + 0, + 0, + null, + "", + false, + true, + "", + false, + "", + false, + false, + false, + ConnectionStrategy.PRIVATE_IP, + -1, + Collections.emptyList(), + null, + Tenancy.Default, + EbsEncryptRootVolume.DEFAULT, + EC2AbstractSlave.DEFAULT_METADATA_ENDPOINT_ENABLED, + EC2AbstractSlave.DEFAULT_METADATA_TOKENS_REQUIRED, + EC2AbstractSlave.DEFAULT_METADATA_HOPS_LIMIT, + EC2AbstractSlave.DEFAULT_METADATA_SUPPORTED, + EC2AbstractSlave.DEFAULT_ENCLAVE_ENABLED) { + @Override + protected Object readResolve() { + return null; + } + }; + + ArrayList instTags = new ArrayList<>(); + instTags.add(Tag.builder().key("Name").value("test-instance").build()); + instTags.add(Tag.builder().key("Environment").value("dev").build()); + + Method tagSpotInstance = SlaveTemplate.class.getDeclaredMethod( + "tagSpotInstance", Ec2Client.class, String.class, Collection.class); + tagSpotInstance.setAccessible(true); + tagSpotInstance.invoke(template, ec2, "i-12345", instTags); + + assertEquals(1, capturedRequests.size()); + CreateTagsRequest request = capturedRequests.get(0); + assertEquals(Collections.singletonList("i-12345"), request.resources()); + assertEquals(2, request.tags().size()); + assertTrue(request.tags() + .contains(Tag.builder().key("Name").value("test-instance").build())); + assertTrue(request.tags() + .contains(Tag.builder().key("Environment").value("dev").build())); + } + + @Test + void testTagSpotInstanceHandlesAwsServiceException() throws Exception { + Ec2Client ec2 = new Ec2Client() { + @Override + public CreateTagsResponse createTags(CreateTagsRequest createTagsRequest) { + throw AwsServiceException.builder() + .message("Instance not found") + .awsErrorDetails(AwsErrorDetails.builder() + .errorCode("InvalidInstanceID.NotFound") + .errorMessage("The instance ID 'i-99999' does not exist") + .build()) + .build(); + } + + @Override + public void close() {} + + @Override + public String serviceName() { + return "AmazonEC2"; + } + }; + + SlaveTemplate template = + new SlaveTemplate( + "ami1", + EC2AbstractSlave.TEST_ZONE, + null, + "default", + "foo", + InstanceType.M1_LARGE.toString(), + false, + "ttt", + Node.Mode.NORMAL, + "foo ami", + "bar", + "bbb", + "aaa", + "10", + "fff", + null, + EC2AbstractSlave.DEFAULT_JAVA_PATH, + "-Xmx1g", + false, + "subnet 456", + null, + null, + 0, + 0, + null, + "", + false, + true, + "", + false, + "", + false, + false, + false, + ConnectionStrategy.PRIVATE_IP, + -1, + Collections.emptyList(), + null, + Tenancy.Default, + EbsEncryptRootVolume.DEFAULT, + EC2AbstractSlave.DEFAULT_METADATA_ENDPOINT_ENABLED, + EC2AbstractSlave.DEFAULT_METADATA_TOKENS_REQUIRED, + EC2AbstractSlave.DEFAULT_METADATA_HOPS_LIMIT, + EC2AbstractSlave.DEFAULT_METADATA_SUPPORTED, + EC2AbstractSlave.DEFAULT_ENCLAVE_ENABLED) { + @Override + protected Object readResolve() { + return null; + } + }; + + ArrayList instTags = new ArrayList<>(); + instTags.add(Tag.builder().key("Name").value("test-instance").build()); + + Method tagSpotInstance = SlaveTemplate.class.getDeclaredMethod( + "tagSpotInstance", Ec2Client.class, String.class, Collection.class); + tagSpotInstance.setAccessible(true); + + // Should not throw - exception is caught internally and logged as warning + tagSpotInstance.invoke(template, ec2, "i-99999", instTags); + + assertTrue(handler.getRecords().stream() + .anyMatch(r -> r.getMessage().contains("Failed to tag spot instance i-99999"))); + } + @Issue("JENKINS-59460") @Test void testConnectionStrategyDeprecatedFieldsAreExported() {