aws-cdk icon indicating copy to clipboard operation
aws-cdk copied to clipboard

(aws_ecs): Add support for `DefaultCapacityProviderStrategy` to the `Cluster` L2 construct

Open vobornij opened this issue 3 years ago • 9 comments

Step Function state language does not support provider parameter for running ECS task https://docs.aws.amazon.com/step-functions/latest/dg/connect-ecs.html. I think default provider is the only option there. To use it conveniently from CDK, there is an open ticket with a described workaround https://github.com/aws/aws-cdk/issues/7967#issuecomment-651583630.

Proposed Solution

The L2 construct a2s_ecs.Cluster currently doesn't support a default provider, could you please add it? https://docs.aws.amazon.com/cdk/api/latest/python/aws_cdk.aws_ecs/Cluster.html#aws_cdk.aws_ecs.Cluster.add_asg_capacity_provider

I tried to use L1 constructs and ran into several issues. CfnCluster.default_capacity_provider_strategy (via CapacityProviderStrategyItemProperty) and CfnClusterCapacityProviderAssociations expect provider as a string instead of specific type. This is probably not a problem of CDK as the AWS::ECS::ClusterCapacityProviderAssociations in CloudFormation docs have a similar problem. Examples in the docs are fine. Could you please forward the problem to the correct place? https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-ecs.CfnCluster.CapacityProviderStrategyItemProperty.html https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-ecs.CfnClusterCapacityProviderAssociations.html https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ecs-clustercapacityproviderassociations.html

I tried to create the association using a generic CfnResource, i came up with this:

ecs_cluster = ecs.Cluster(...)
provider = ecs.AsgCapacityProvider(...)

association = CfnResource(self, "capacity_association2",
                type="AWS::ECS::ClusterCapacityProviderAssociations",
                properties= {
                    "CapacityProviders": [
                        provider.node.children[0].ref
                ],
                    "Cluster": ecs_cluster.node.default_child.ref,
                    "DefaultCapacityProviderStrategy": [
                        {
                            "CapacityProvider": provider.node.children[0].ref
                        }
                    ]
                }
            )

In Python CDK, ecs.AsgCapacityProvider did not have a default_child, so to access its reference, I had to use provider.node.children[0].ref

Environment

  • CDK CLI Version : 1.108.1
  • **Framework Version: ** 1.108.1
  • Node.js Version: v10.19.0
  • OS : Ubuntu
  • Language (Version): 3.8.5

Other

To run an ECS task from step function using a custom provider from CDK, there is currently an open ticket with a described workaround https://github.com/aws/aws-cdk/issues/7967#issuecomment-651583630.


This is a 🚀 Feature Request

vobornij avatar Jun 21 '21 18:06 vobornij

Hey @vobornij :wave:

Thanks for opening this. If I understand correctly this issue is a request to add support for DefaultCapacityProviderStrategy in the CDK's aws-ecs Cluster construct.

I tried to create the association using a generic CfnResource, i came up with this: ...

Did this workaround work for you?

I am going to relabel this as a feature-request since this is requesting that a new parameter be supported. Let me know if you think this should really be a bug though and why.

I am marking this issue as p2 which means that we are unable to work on this immediately. We use +1s to help us prioritize our work, and as always we are happy to take contributions if anyone is interested to pick this up and submit a PR (please make sure to follow our contribution guidelines.) 🙏

ryparker avatar Jun 22 '21 22:06 ryparker

Hi @ryparker,

thanks. I was confused about the use of L1 constructs and I thought it was not possible to use the specific Cfn class because of the string references. Now I found that this works, is it the right way to do it?

ecs.CfnClusterCapacityProviderAssociations(
	self, 
	"test_association", 
	capacity_providers=[provider.node.children[0].ref], 
	cluster=ecs_cluster.node.default_child.ref, 
	default_capacity_provider_strategy[
		ecs.CfnClusterCapacityProviderAssociations.CapacityProviderStrategyProperty(
			capacity_provider=provider.node.children[0].ref
		)
	]
)

I still can't access default_child in provider, is that OK?

You can relable it to feature request. Adding the param to L2 cluster should solve it.

vobornij avatar Jun 23 '21 09:06 vobornij

After looking into the implementation it looks like the capacity provider ref is assigned to the capactiyProviderName property.

Also in docs

Have you tried using provider.capacityProviderName instead of provider.node.children[0].ref?

ryparker avatar Jun 28 '21 21:06 ryparker

Thank you, that works. I was looking for a generic way to reference resources, but this is fine. I'm sorry for my ignorance, It was confusing to me because during synthetization, the capacity_provider_name property string is some intermediate token of the CDK, and in the generated CloudFormation template it becomes an object with a Ref field. The documented CloudFormation-generated name refers to another level of abstraction/stage of deployment process, right? From the other side, CfnClusterCapacityProviderAssociations doesn't specify what strings it accepts. It could be just my poor understanding of CloudFormation and CDK, but I think that better description of L1 constructs in the official docs would be helpful. https://docs.aws.amazon.com/cdk/latest/guide/constructs.html#constructs_l1_using

vobornij avatar Jun 29 '21 10:06 vobornij

@vobornij what exactly would you like to see explained better in the docs? I agree that L1 constructs could be explained better in our developer guide, I'd like to have your feedback here

peterwoodworth avatar Jul 06 '21 20:07 peterwoodworth

you cannot use ClusterCapacityProviderAssociations if you already used cluster.addAsgCapacityProvider.

failed to deploy: UPDATE_ROLLBACK_COMPLETE: Resource handler returned message: "The cluster already contains capacity provider associations" (RequestToken: 6a8c3....2cca, HandlerErrorCode: AlreadyExists)

I am trying to run a step functions task on a cluster with a capacity provider and:

  • the step function task does not allow for a capacity provider strategy
  • the ecs cluster construct does not expose any method to set a default capacity provider strategy
  • this cloudformation low level construct does not seem to work

Did anyone find a solution here? Are there any alternatives worth following?

trobert2 avatar Jun 27 '22 12:06 trobert2

Hi,

I have the same issue as @trobert2, but trying to run a ScheduledEc2Task.

If I create the association using CfnClusterCapacityProviderAssociations without the cluster.addAsgCapacityProvider, I get this error during the synth: Cluster for this service needs Ec2 capacity. Call addXxxCapacity() on the cluster.

Am I doing something wrong? The workaround provided by @vobornij is not working from me. Is there a plan to add DefaultCapacityProviderStrategy to the Cluster L2 construct?

SimoneBrigante avatar Aug 04 '22 11:08 SimoneBrigante

Was able to get around this issue using Aspects

class SetDefaultCapacityProviderStrategy implements cdk.IAspect {
  constructor(
    private readonly defaultCapacityProvider: ecs.AsgCapacityProvider
  ) {}

  visit(node: IConstruct): void {
    if (node instanceof ecs.CfnClusterCapacityProviderAssociations) {
      node.defaultCapacityProviderStrategy = [
        {
          capacityProvider: this.defaultCapacityProvider.capacityProviderName,
        },
      ];
    }
  }
}

And then in the construct that's creating the cluster

    cdk.Aspects.of(this).add(
      new SetDefaultCapacityProviderStrategy(defaultCapacityProvider)
    );
    ```

piotrromanowski avatar Aug 04 '22 15:08 piotrromanowski

Solution provided by @piotrromanowski works well however it'd be nice to have this supported out-of-the-box on the L2 level construct directly.

razakj avatar Aug 11 '22 01:08 razakj

Agreed. Capacity providers in CDK seem more like an afterthought in multiple places.

piotrromanowski avatar Aug 11 '22 13:08 piotrromanowski

I've got a branch that I'll create a PR for once I've added tests as suggested by docs https://github.com/aws/aws-cdk/compare/main...piotrromanowski:aws-cdk:Add_cluster_service_capacity_provider_support

Edit: FWIW we've been using this in production for a while now and it's been working out well

piotrromanowski avatar Aug 11 '22 13:08 piotrromanowski

This issue got solved and merged ( https://github.com/aws/aws-cdk/pull/23955 )already we can close this one.

homakk avatar Mar 06 '23 20:03 homakk

Thanks @homakk for letting us know to close this.

If you create a PR, next time you can put "closes #" or "fixes #" to automatically close the related issue on PR merge 🙂

peterwoodworth avatar Mar 08 '23 20:03 peterwoodworth

⚠️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.

github-actions[bot] avatar Mar 08 '23 20:03 github-actions[bot]

I encountered a problem migrating from CfnClusterCapacityProviderAssociations to the new L2 construct. When I tried to deploy the change to an existing cluster, I encountered "The cluster already contains capacity provider associations". The solution was:

  1. Create a new visitor for making changes to the generated CfnClusterCapacityProviderAssociations. This has to be a visitor because the ECS cluster construct uses a visitor to create the CfnClusterCapacityProviderAssociations, so only a later visitor is able to find the node.
  2. Use overrideLogicalId to change the id to match the original id. This prevents CloudFormation from trying to create a new physical resource instead.
  3. Change the association's cluster to use the cluster arn instead of a ref. This may only be necessary because our old Cfn version was using an arn, but without this CloudFormation was creating a new physical resource and unable to associate it to the cluster.

This is the visitor:

class MatchCapacityProviderAssociatonsToCdk1 implements IAspect {

    private id: string;

    constructor(id: string) {
        this.id = id;
    }

    public visit(construct: IConstruct) {
        let clusterId: string = "";
        for (let child of construct.node.children) {
            if (child instanceof CfnCluster) {
                clusterId = child.logicalId;
            }
        }
        for (let child of construct.node.children) {
            if (child instanceof CfnClusterCapacityProviderAssociations) {
                child.cluster = cdk.Fn.getAtt(clusterId, 'Arn').toString();
                child.overrideLogicalId(id)
            }
        }
    }
}

jhchuah-amazon avatar Oct 31 '23 17:10 jhchuah-amazon