aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.5k stars 3.84k forks source link

AsgCapacityProvider: Cannot disable drainhooks after taskDrainTime got deprecated #24641

Closed tommydongaws closed 1 year ago

tommydongaws commented 1 year ago

Describe the bug

Previously in CDKv1 [1] there was a taskDrainTime property which could be used to disable drainhooks. After it got removed, there is no way to remove the drainhooks without enabling "enableManagedTerminationProtection".

From the code here: if (!options.taskDrainTime || options.taskDrainTime.toSeconds() !== 0) { new InstanceDrainHook(autoScalingGroup, 'DrainECSHook', { autoScalingGroup, cluster: this, drainTime: options.taskDrainTime, topicEncryptionKey: options.topicEncryptionKey, }); }

It look like v2 is still using taskDrainTime to decide whether or not to disable the drain hook which doesn't seem right. Could you add taskDrainTime or a diff option to disable the drainhook?

[1] https://docs.aws.amazon.com/cdk/api/v1/docs/@aws-cdk_aws-ecs.AsgCapacityProviderProps.html [2] https://github.com/aws/aws-cdk/blob/main/packages/@aws-cdk/aws-ecs/lib/cluster.ts#L559

Expected Behavior

For there to be no drainhook or an option to disable the drainhook.

Current Behavior

The code looks for the deprecated property "taskDrainTime" to decide make a drainhook or not. The only way to disable it is to enable enableManagedTerminationProtection which cause the value of taskDrainTime to be 0.

Reproduction Steps

When you run this it will create a drain hook.

import as cdk from 'aws-cdk-lib'; import { Construct } from 'constructs'; import as ec2 from 'aws-cdk-lib/aws-ec2'; import as autoscaling from 'aws-cdk-lib/aws-autoscaling'; import as ecs from 'aws-cdk-lib/aws-ecs';

export class CdktestStack extends cdk.Stack { constructor(scope: Construct, id: string, props?: cdk.StackProps) { super(scope, id, props);

// VPC
const vpc = new ec2.Vpc(this, 'TheVPC', {
  ipAddresses: ec2.IpAddresses.cidr('10.0.0.0/16'),
})

 const cluster = new ecs.Cluster(this, 'Cluster', {
  vpc,
});

const autoScalingGroup = new autoscaling.AutoScalingGroup(this, 'ASG', {
  vpc,
  instanceType: new ec2.InstanceType('t2.micro'),
  machineImage: ecs.EcsOptimizedImage.amazonLinux2(),
  minCapacity: 0,
  maxCapacity: 100,
});

const capacityProvider = new ecs.AsgCapacityProvider(this, 'AsgCapacityProvider', {
  autoScalingGroup: autoScalingGroup,
  enableManagedTerminationProtection: false,
  enableManagedScaling: false,

});

cluster.addAsgCapacityProvider(capacityProvider,{});

const taskDefinition = new ecs.Ec2TaskDefinition(this, 'TaskDef');

taskDefinition.addContainer('web', {
  image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
  memoryReservationMiB: 256,
});

new ecs.Ec2Service(this, 'EC2Service', {
  cluster,
  taskDefinition,
  capacityProviderStrategies: [
    {
      capacityProvider: capacityProvider.capacityProviderName,
      weight: 1,
    },
  ],
});

} }

Possible Solution

Add an option to disable the drainhook.

Additional Information/Context

No response

CDK CLI Version

2.69.0 (build 60a5b2a)

Framework Version

No response

Node.js Version

v18.15.0

OS

Windows 11

Language

Typescript

Language Version

No response

Other information

No response

pahud commented 1 year ago

Hi

Yes according to the doc, taskDrainTime has been deprecated and should enable enableManagedTerminationProtection instead. Are you trying to disable drain hooks with managedTerminationProtection disabled? Can you elaborate more on your use case?

tommydongaws commented 1 year ago

Hi, Yes, Im trying to disable drain hooks with managedTerminationProtection disabled. Then attach a custom drain hook. This was possible previously when taskDrainTime was exposed but not anymore.

pahud commented 1 year ago

Yeah I am afraid you will need to enable managedTerminationProtection now. Is there any reason you can't enable it?

github-actions[bot] commented 1 year ago

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.