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-patterns): Recent update breaks FargateService L3 constructs which have added additional containers to the task definition #21484

Closed
peterwoodworth opened this issue Aug 5, 2022 · 2 comments · Fixed by #21530
Assignees
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library bug This issue is a bug. p1

Comments

@peterwoodworth
Copy link
Contributor

peterwoodworth commented Aug 5, 2022

Describe the bug

A recent change was made to set memory at the container level, not just at the task definition level.

This was accomplished by setting the cpu and memory values of the container to be the same as the task definition, which isn't necessarily going to be the desired case.

This causes deployment to break for existing users of this construct who have added additional containers to their task definition.

Expected Behavior

Deployment not to break

Current Behavior

Deployment breaks because the total cpu and memory used in the containers exceeds the values allocated to the task definition.

Reproduction Steps

All three of these constructs fail deployment where they would previously succeed before this change. QueueProcessingFargateService will always automatically allocate the same value to both the task definition and default container, so passing in a cpu/memory value for this construct in particular isn't necessary to not be able to add additional containers.

Note: If these values are not specified on the additional containers, they will be allocated 0 cpu, which can be problematic if any cpu is desired

    const qserv = new QueueProcessingFargateService(this, 'QServ', {
      image: ContainerImage.fromRegistry('amazon/amazon-ecs-sample')
    });
    qserv.taskDefinition.addContainer('container', {
      image: ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
      cpu: 8
    })

    const nlbserv = new NetworkLoadBalancedFargateService(this, 'NLBServ', {
      cpu: 256,
      taskImageOptions: {
        image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
      },
    });
    nlbserv.taskDefinition.addContainer('container', {
      image: ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
      cpu: 8
    });

    const appserv = new ApplicationLoadBalancedFargateService(this, 'AppServ', {
      cpu: 256,
      taskImageOptions: {
        image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
      },
    });
    appserv.taskDefinition.addContainer('container', {
      image: ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
      cpu: 8
    });

Possible Solution

The documentation states:

The number of cpu units used by the task.

No mention of container cpu units is specified. If we want to be able to allocate the value, there should be a separate property for this so as not to break existing users, and to also allow for the addition of other containers if desired.

Additional Information/Context

No response

CDK CLI Version

2.34.1

Framework Version

No response

Node.js Version

.

OS

mac

Language

Typescript

Language Version

No response

Other information

No response

@peterwoodworth peterwoodworth added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 5, 2022
@github-actions github-actions bot added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Aug 5, 2022
@peterwoodworth peterwoodworth added p1 and removed needs-triage This issue or PR still needs to be triaged. labels Aug 5, 2022
@peterwoodworth
Copy link
Contributor Author

should be reverted by next release

@mergify mergify bot closed this as completed in #21530 Aug 10, 2022
mergify bot pushed a commit that referenced this issue Aug 10, 2022
…evel" (#21530)

This reverts #21201 due to #21484: Recent update breaks FargateService L3 constructs which have added additional containers to the task definition

Closes #21484
Re-opens #13127 

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@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.

josephedward pushed a commit to josephedward/aws-cdk that referenced this issue Aug 30, 2022
…evel" (aws#21530)

This reverts aws#21201 due to aws#21484: Recent update breaks FargateService L3 constructs which have added additional containers to the task definition

Closes aws#21484
Re-opens aws#13127 

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library bug This issue is a bug. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants