fix(ecs): validation for task definition fails when task-level memory is defined but container-level memory and memoryReservation are not defined with EC2 compatibility #26027
Conversation
… is defined but container-level memory and memoryReservation are not defined with EC2 compatibility
|
|
github-actions
bot
added
beginning-contributor
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
aws-cdk-automation
added
the
pr/needs-community-review
| for (const container of this.containers) { | ||
| if (!container.memoryLimitSpecified) { | ||
| ret.push(`ECS Container ${container.containerName} must have at least one of 'memoryLimitMiB' or 'memoryReservationMiB' specified`); | ||
| if (!this._memory) { |
There was a problem hiding this comment.
We should also account for the scenario where both are provided.
If using the EC2 launch type, you must specify either a task-level memory value or a container-level memory value. If you specify both a container-level memory and memoryReservation value, memory must be greater than memoryReservation . If you specify memoryReservation , then that value is subtracted from the available memory resources for the container instance where the container is placed. Otherwise, the value of memory is used.
There was a problem hiding this comment.
Hi @corymhall, thank you for your review!
Please let me clarify your comment. What do you mean the scenario where both are provided? Is the scenario where both a task-level memory value and a container-level memory value are specified? Currently I don't understand what we should account for additionally in the scenario.
In my understanding,
pattern 1: only task-level memory value is specified.
--> In that case, we don't have to check whether container-level memory value is specified.
pattern 2: only container-level memory value is specified.
--> In that case, we should check whether either memoryLimitMiB or memoryReservationMiB is specified. This validation is already implemented.
pattern 3: both a task-level memory value and a container-level memory value are specified.
--> In that case, we don't have to check whether container-level memory value is specified.
There was a problem hiding this comment.
If you specify both a container-level memory and memoryReservation value, memory must be greater than memoryReservation
I think this should be validated.
There was a problem hiding this comment.
In my understanding, this is already validated here.
aws-cdk/packages/aws-cdk-lib/aws-ecs/lib/container-definition.ts
Lines 490 to 492 in f8a94d8
corymhall
removed
the
pr/needs-community-review
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
… is defined but container-level memory and memoryReservation are not defined with EC2 compatibility (aws#26027) Currently, validation for ECS task definition fails when task-level memory is defined but container-level memory and memoryReservation are not defined with EC2 compatibility. On the other hand, if we specify task-level memory, we can omit container-level memory and memoryReservation parameters from ECS API perspective. This PR solves the issue by skipping the validation when task-level memory is defined. Closes aws#25275 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
… is defined but container-level memory and memoryReservation are not defined with EC2 compatibility (aws#26027) Currently, validation for ECS task definition fails when task-level memory is defined but container-level memory and memoryReservation are not defined with EC2 compatibility. On the other hand, if we specify task-level memory, we can omit container-level memory and memoryReservation parameters from ECS API perspective. This PR solves the issue by skipping the validation when task-level memory is defined. Closes aws#25275 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Currently, validation for ECS task definition fails when task-level memory is defined but container-level memory and memoryReservation are not defined with EC2 compatibility. On the other hand, if we specify task-level memory, we can omit container-level memory and memoryReservation parameters from ECS API perspective. This PR solves the issue by skipping the validation when task-level memory is defined.
Closes #25275
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license