fix(cli): ecs hotswap deployment waits correctly for success or failu…

…re (aws#28448) Reraising aws#27943 as it was incorrectly closed as stale. See original PR for details. Closes aws#27882. See linked issue for further details. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
go-to-k · Mar 26, 2024 · 5c30255 · 5c30255
1 parent e77ce26
commit 5c30255
Show file tree

Hide file tree

Showing 4 changed files with 153 additions and 7 deletions.
diff --git a/packages/@aws-cdk-testing/cli-integ/lib/aws.ts b/packages/@aws-cdk-testing/cli-integ/lib/aws.ts
@@ -18,6 +18,7 @@ export class AwsClients {
   public readonly cloudFormation: AwsCaller<AWS.CloudFormation>;
   public readonly s3: AwsCaller<AWS.S3>;
   public readonly ecr: AwsCaller<AWS.ECR>;
+  public readonly ecs: AwsCaller<AWS.ECS>;
   public readonly sns: AwsCaller<AWS.SNS>;
   public readonly iam: AwsCaller<AWS.IAM>;
   public readonly lambda: AwsCaller<AWS.Lambda>;
@@ -34,6 +35,7 @@ export class AwsClients {
     this.cloudFormation = makeAwsCaller(AWS.CloudFormation, this.config);
     this.s3 = makeAwsCaller(AWS.S3, this.config);
     this.ecr = makeAwsCaller(AWS.ECR, this.config);
+    this.ecs = makeAwsCaller(AWS.ECS, this.config);
     this.sns = makeAwsCaller(AWS.SNS, this.config);
     this.iam = makeAwsCaller(AWS.IAM, this.config);
     this.lambda = makeAwsCaller(AWS.Lambda, this.config);

diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js
@@ -4,6 +4,7 @@ var constructs = require('constructs');
 if (process.env.PACKAGE_LAYOUT_VERSION === '1') {
   var cdk = require('@aws-cdk/core');
   var ec2 = require('@aws-cdk/aws-ec2');
+  var ecs = require('@aws-cdk/aws-ecs');
   var s3 = require('@aws-cdk/aws-s3');
   var ssm = require('@aws-cdk/aws-ssm');
   var iam = require('@aws-cdk/aws-iam');
@@ -17,6 +18,7 @@ if (process.env.PACKAGE_LAYOUT_VERSION === '1') {
     DefaultStackSynthesizer,
     LegacyStackSynthesizer,
     aws_ec2: ec2,
+    aws_ecs: ecs,
     aws_s3: s3,
     aws_ssm: ssm,
     aws_iam: iam,
@@ -357,6 +359,60 @@ class LambdaHotswapStack extends cdk.Stack {
   }
 }
 
+class EcsHotswapStack extends cdk.Stack {
+  constructor(parent, id, props) {
+    super(parent, id, props);
+
+    // define a simple vpc and cluster
+    const vpc = new ec2.Vpc(this, 'vpc', {
+      natGateways: 0,
+      subnetConfiguration: [
+        {
+          cidrMask: 24,
+          name: 'Public',
+          subnetType: ec2.SubnetType.PUBLIC,
+        },
+      ],
+      maxAzs: 1,
+    });
+    const cluster = new ecs.Cluster(this, 'cluster', {
+      vpc,
+    });
+
+    // allow stack to be used to test failed deployments
+    const image =
+      process.env.USE_INVALID_ECS_HOTSWAP_IMAGE == 'true'
+        ? 'nginx:invalidtag'
+        : 'nginx:alpine';
+
+    // deploy basic service
+    const taskDefinition = new ecs.FargateTaskDefinition(
+      this,
+      'task-definition'
+    );
+    taskDefinition.addContainer('nginx', {
+      image: ecs.ContainerImage.fromRegistry(image),
+      environment: {
+        SOME_VARIABLE: process.env.DYNAMIC_ECS_PROPERTY_VALUE ?? 'environment',
+      },
+      healthCheck: {
+        command: ['CMD-SHELL', 'exit 0'], // fake health check to speed up deployment
+        interval: cdk.Duration.seconds(5),
+      },
+    });
+    const service = new ecs.FargateService(this, 'service', {
+      cluster,
+      taskDefinition,
+      assignPublicIp: true, // required without NAT to pull image
+      circuitBreaker: { rollback: false },
+      desiredCount: 1,
+    });
+
+    new cdk.CfnOutput(this, 'ClusterName', { value: cluster.clusterName });
+    new cdk.CfnOutput(this, 'ServiceName', { value: service.serviceName });
+  }
+}
+
 class DockerStack extends cdk.Stack {
   constructor(parent, id, props) {
     super(parent, id, props);
@@ -532,6 +588,7 @@ switch (stackSet) {
 
     new LambdaStack(app, `${stackPrefix}-lambda`);
     new LambdaHotswapStack(app, `${stackPrefix}-lambda-hotswap`);
+    new EcsHotswapStack(app, `${stackPrefix}-ecs-hotswap`);
     new DockerStack(app, `${stackPrefix}-docker`);
     new DockerStackWithCustomFile(app, `${stackPrefix}-docker-with-custom-file`);
     const failed = new FailedStack(app, `${stackPrefix}-failed`)

diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts
@@ -1571,6 +1571,83 @@ integTest('hotswap deployment supports Fn::ImportValue intrinsic', withDefaultFi
   }
 }));
 
+integTest('hotswap deployment supports ecs service', withDefaultFixture(async (fixture) => {
+  // GIVEN
+  const stackArn = await fixture.cdkDeploy('ecs-hotswap', {
+    captureStderr: false,
+  });
+
+  // WHEN
+  const deployOutput = await fixture.cdkDeploy('ecs-hotswap', {
+    options: ['--hotswap'],
+    captureStderr: true,
+    onlyStderr: true,
+    modEnv: {
+      DYNAMIC_ECS_PROPERTY_VALUE: 'new value',
+    },
+  });
+
+  const response = await fixture.aws.cloudFormation('describeStacks', {
+    StackName: stackArn,
+  });
+  const serviceName = response.Stacks?.[0].Outputs?.find(output => output.OutputKey == 'ServiceName')?.OutputValue;
+
+  // THEN
+
+  // The deployment should not trigger a full deployment, thus the stack's status must remains
+  // "CREATE_COMPLETE"
+  expect(response.Stacks?.[0].StackStatus).toEqual('CREATE_COMPLETE');
+  expect(deployOutput).toContain(`ECS Service '${serviceName}' hotswapped!`);
+}));
+
+integTest('hotswap deployment for ecs service waits for deployment to complete', withDefaultFixture(async (fixture) => {
+  // GIVEN
+  const stackArn = await fixture.cdkDeploy('ecs-hotswap', {
+    captureStderr: false,
+  });
+
+  // WHEN
+  await fixture.cdkDeploy('ecs-hotswap', {
+    options: ['--hotswap'],
+    modEnv: {
+      DYNAMIC_ECS_PROPERTY_VALUE: 'new value',
+    },
+  });
+
+  const describeStacksResponse = await fixture.aws.cloudFormation('describeStacks', {
+    StackName: stackArn,
+  });
+  const clusterName = describeStacksResponse.Stacks?.[0].Outputs?.find(output => output.OutputKey == 'ClusterName')?.OutputValue!;
+  const serviceName = describeStacksResponse.Stacks?.[0].Outputs?.find(output => output.OutputKey == 'ServiceName')?.OutputValue!;
+
+  // THEN
+
+  const describeServicesResponse = await fixture.aws.ecs('describeServices', {
+    cluster: clusterName,
+    services: [serviceName],
+  });
+  expect(describeServicesResponse.services?.[0].deployments).toHaveLength(1); // only one deployment present
+
+}));
+
+integTest('hotswap deployment for ecs service detects failed deployment and errors', withDefaultFixture(async (fixture) => {
+  // GIVEN
+  await fixture.cdkDeploy('ecs-hotswap');
+
+  // WHEN
+  const deployOutput = await fixture.cdkDeploy('ecs-hotswap', {
+    options: ['--hotswap'],
+    modEnv: {
+      USE_INVALID_ECS_HOTSWAP_IMAGE: 'true',
+    },
+    allowErrExit: true,
+  });
+
+  // THEN
+  expect(deployOutput).toContain(`❌  ${fixture.stackNamePrefix}-ecs-hotswap failed: ResourceNotReady: Resource is not in the state deploymentCompleted`);
+  expect(deployOutput).not.toContain('hotswapped!');
+}));
+
 async function listChildren(parent: string, pred: (x: string) => Promise<boolean>) {
   const ret = new Array<string>();
   for (const child of await fs.readdir(parent, { encoding: 'utf-8' })) {

diff --git a/packages/aws-cdk/lib/api/hotswap/ecs-services.ts b/packages/aws-cdk/lib/api/hotswap/ecs-services.ts
@@ -119,11 +119,11 @@ export async function isHotswappableEcsServiceChange(
 
         // Step 3 - wait for the service deployments triggered in Step 2 to finish
         // configure a custom Waiter
-        (sdk.ecs() as any).api.waiters.deploymentToFinish = {
-          name: 'DeploymentToFinish',
+        (sdk.ecs() as any).api.waiters.deploymentCompleted = {
+          name: 'DeploymentCompleted',
           operation: 'describeServices',
-          delay: 10,
-          maxAttempts: 60,
+          delay: 6,
+          maxAttempts: 100,
           acceptors: [
             {
               matcher: 'pathAny',
@@ -143,16 +143,26 @@ export async function isHotswappableEcsServiceChange(
               expected: 'INACTIVE',
               state: 'failure',
             },
+
+            // failure if any services report a deployment with status FAILED
+            {
+              matcher: 'path',
+              argument: "length(services[].deployments[? rolloutState == 'FAILED'][]) > `0`",
+              expected: true,
+              state: 'failure',
+            },
+
+            // wait for all services to report only a single deployment
             {
               matcher: 'path',
-              argument: "length(services[].deployments[? status == 'PRIMARY' && runningCount < desiredCount][]) == `0`",
+              argument: 'length(services[? length(deployments) > `1`]) == `0`',
               expected: true,
               state: 'success',
             },
           ],
         };
-        // create a custom Waiter that uses the deploymentToFinish configuration added above
-        const deploymentWaiter = new (AWS as any).ResourceWaiter(sdk.ecs(), 'deploymentToFinish');
+        // create a custom Waiter that uses the deploymentCompleted configuration added above
+        const deploymentWaiter = new (AWS as any).ResourceWaiter(sdk.ecs(), 'deploymentCompleted');
         // wait for all of the waiters to finish
         await Promise.all(Object.entries(servicePerClusterUpdates).map(([clusterName, serviceUpdates]) => {
           return deploymentWaiter.wait({