Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ECS Cluster.addCapacity does not set autoscalingGroup property #5893

Closed
nathanpeck opened this issue Jan 21, 2020 · 10 comments
Closed

ECS Cluster.addCapacity does not set autoscalingGroup property #5893

nathanpeck opened this issue Jan 21, 2020 · 10 comments
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/medium Medium work item – several days of effort needs-reproduction This issue needs reproduction. p2

Comments

@nathanpeck
Copy link
Member

nathanpeck commented Jan 21, 2020

When calling the ECS Cluster.addCapacity() method it does not actually set the "autoscalingGroup" property as expected:

Reproduction Steps

    // Create an ECS cluster
    this.cluster = new ecs.Cluster(this, 'Cluster', {
      vpc: this.vpc
    });

    // Add capacity to it
    this.cluster.addCapacity('greeter-capacity', {
      instanceType: new ec2.InstanceType('t3.xlarge'),
      minCapacity: 3,
      maxCapacity: 3
    });

    console.log(this.cluster.autoscalingGroup);

Expectation

According to the docs the expected output from the above code sample would be the construct for the autoscaling group that was created by addCapacity(): https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-ecs.Cluster.html#autoscalinggroup

However instead you just see "undefined". This is because the getter for autoscalingGroup property uses an internal property which is only set in one place in the constructor: https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-ecs/lib/cluster.ts#L116

If you do not specify capacity options at constructor time and instead choose to subsequently use the addCapacity() method then this property is never set within the addAutoscalingGroup() method, therefore the reference to the autoscaling group is not retrievable via the getter method as expected

Workarounds

You can specify capacity options as part of the constructor to get around this issue, instead of using addCapacity(). However, we should fix this so that addCapacity() sets the property correctly, as expected.

Environment

  • CLI Version : 1.21.0
  • Framework Version: 1.21.0

This is 🐛 Bug Report

@nathanpeck nathanpeck added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 21, 2020
@nathanpeck nathanpeck changed the title ECS Cluster.addCapacity does not keep autoscalingGroup property consistent ECS Cluster.addCapacity does not set autoscalingGroup property Jan 21, 2020
@piradeepk piradeepk added @aws-cdk/aws-ecs Related to Amazon Elastic Container needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Jan 21, 2020
@piradeepk piradeepk added the p2 label Jan 22, 2020
@nathanpeck
Copy link
Member Author

Looking into the code for this I think there is a slightly deeper problem. The addCapacity() function can be called multiple times, to add multiple ASG's to the cluster. The autoscalingGroup property is not an array, so it can only ever point to a single ASG.

I think design wise we need to create a new property which is a list of the different capacities in the cluster, ideally at the same time that we add support for multiple capacity providers.

@bf-sodle
Copy link

Is it currently possible at all to create an auto-scaling cluster using the CDK or CloudFormation? The following code, in TypeScript CDK 1.23.0, fails to attach the ASG to the cluster. When trying to attach it manually through the UI, I get an error because the ASG doesn't have "instance protection from scale in" enabled. That's required because the default (and desired) behavior is to enable "managed termination protection" on the capacity provider. Neither of these parameters appear to be customizable through CDK or Cfn.

    const cluster = new ecs.Cluster(this, `ec2-cluster-${clusterName}`, {
      clusterName,
      vpc: this.vpc,
      containerInsights: enableInsights,
      capacity: {
        instanceType: ec2.InstanceType.of(instanceClass, instanceSize),
        machineImage,
        minCapacity,
        maxCapacity,
        allowAllOutbound: false,
        associatePublicIpAddress: false
      }
    });
    cluster.connections.addSecurityGroup(securityGroup);
    if (cluster.autoscalingGroup) {
      cluster.autoscalingGroup.scaleOnMetric(`${clusterName}-scaler`, {
        metric: cluster.metricCpuReservation(),
        adjustmentType: asg.AdjustmentType.CHANGE_IN_CAPACITY,
        scalingSteps: [
          { change: 1, lower: scaleOutCpuThreshold },
          { change: -1, upper: scaleInCpuThreshold }
        ]
      });
    }

@bf-sodle
Copy link

Nevermind, I see how this is working under the hood, and it's not using capacity providers at all. My problem is that my instance wasn't using the ECS-optimized AMI, and therefore couldn't add itself to the cluster.

@nathanpeck
Copy link
Member Author

@bf-sodle Correct, we are still working on adding full support for ECS capacity providers in CloudFormation. Once that support is in CloudFormation we can add it to CDK as well. You can watch this issue for updates on the CFN work: aws/containers-roadmap#631

@gandroz
Copy link

gandroz commented Jul 20, 2020

The addCapacity method allows you to add several auto scaling groups, so this getter is irrelevant in this case. It is only fed if you provide the asg to the construct. But I agree it is confusing and useless here. However, you could always access to the asg create with the addCapacity method as it is returned to you https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-ecs.Cluster.html#add-wbr-capacityid-options

This method adds compute capacity to a cluster by creating an AutoScalingGroup with the specified options.

Returns the AutoScalingGroup so you can add autoscaling settings to it.

@jgtvares
Copy link

jgtvares commented Sep 15, 2020

My question is not really about the issue, but is kinda' related.
Is there any difference between using addCapacity or addAutoscalingGroup?
I know that using addAutoscalingGroup I'll need to create an ASG first.
To me it seems that both methods do the same thing.
Am I wrong?

@gandroz
Copy link

gandroz commented Sep 15, 2020

My question is not really about the issue, but is kinda' related.
Is there any difference between using addCapacity or addAutoscalingGroup?
I know that using addAutoscalingGroup I'll need to create an ASG first.
To me it seems that both methods do the same thing.
Am I wrong?

As far as I understand it, addAutoscalingGroup adds an ASG already created to your cluster, whereas addCapacity creates an ASG under the hood, attaches it to the cluster and returns it so you can edit it. You have the same behavior when you use the console, you can choose to use an existing ASG or let ECS build one for you.

@jgtvares
Copy link

My question is not really about the issue, but is kinda' related.
Is there any difference between using addCapacity or addAutoscalingGroup?
I know that using addAutoscalingGroup I'll need to create an ASG first.
To me it seems that both methods do the same thing.
Am I wrong?

As far as I understand it, addAutoscalingGroup adds an ASG already created to your cluster, whereas addCapacity creates an ASG under the hood, attaches it to the cluster and returns it so you can edit it. You have the same behavior when you use the console, you can choose to use an existing ASG or let ECS build one for you.

Yeah!
Thanks for answering!

@SomayaB SomayaB assigned MrArnoldPalmer and unassigned piradeepk Oct 21, 2020
@SomayaB SomayaB assigned uttarasridhar and SoManyHs and unassigned PettitWesley Nov 3, 2020
@MrArnoldPalmer MrArnoldPalmer added the effort/medium Medium work item – several days of effort label Nov 10, 2020
@SoManyHs
Copy link
Contributor

With ASG Capacity Provider support now merged, these methods are now deprecated. Closing.

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/medium Medium work item – several days of effort needs-reproduction This issue needs reproduction. p2
Projects
None yet
Development

No branches or pull requests

9 participants