feat(ecs-patterns): add ephemeral storage support #18106
Conversation
Fargate supports a new ephemeral storage option, to request up to 200 GiB of task storage. Support for it was added to aws-ecs but not to aws-ecs-patterns. This change adds support for ephemeral storage, so the following patterns can take advantage of it: * Application load balanced fargate service * Application multiple target groups fargate service * Network load balanced fargate service * Network multiple target groups fargate service * Queue processing fargate service * Scheduled fargate task Closes aws#18105 Signed-off-by: Eduardo Lopez <252504+tapichu@users.noreply.github.com>
|
|
github-actions
bot
added
the
@aws-cdk/aws-ecs-patterns
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| * | ||
| * This default is set in the underlying FargateTaskDefinition construct. | ||
| * | ||
| * @default 20 |
There was a problem hiding this comment.
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ecs-taskdefinition-ephemeralstorage.html suggests this number should be between 21 and 200:
The total amount, in GiB, of ephemeral storage to set for the task. The minimum supported value is 21 GiB and the maximum supported value is 200 GiB.
Would 20 not create a runtime error? And/or could there be some validation of ephemeralStorageGiB with an error thrown when it is not in these bounds? That is, produce a compile (synth) time error for earlier feedback.
There was a problem hiding this comment.
Would 20 not create a runtime error? And/or could there be some validation of ephemeralStorageGiB with an error thrown when it is not in these bounds?
Yes, entering a value below 21 or above 200 will throw a validation exception:
The default value of 20 GiB comes into play when ephemeralStorageGiB is not set. By default, Fargate provides 20 GiB of ephemeral storage. That is, users only need to add ephemeralStorageGiB to their task definition when their task needs more than 20 GiB:
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/fargate-task-storage.html
I documented the 20 GiB default value for ephemeral storage for consistency with aws-ecs:
You can see here that aws-ecs doesn't actually set the default value to 20 in the task definition, but rather sets ephemeralStorage to undefined when ephemeralStorageGiB is not set:
aws-cdk/packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts
Lines 470 to 472 in fa8f2e2
There was a problem hiding this comment.
👍🏽 thanks for the explanation and links!
Is there any reason the validation is only on FargateTaskDefinition and not TaskDefinition which it extends from?
The default value of 20 GiB comes into play when
ephemeralStorageGiBis not set. By default, Fargate provides 20 GiB of ephemeral storage. That is, users only need to addephemeralStorageGiBto their task definition when their task needs more than 20 GiB
I find this API quite confusing! You set this property if and only if you do not want the default value? If you do set it to the default of 20, a runtime error is seen? That's different from pretty much all other CloudFormation resources.
There was a problem hiding this comment.
Is there any reason the validation is only on FargateTaskDefinition and not TaskDefinition which it extends from?
I imagine that is because ephemeralStorage is a Fargate-specific option. It does not apply to non-Fargate tasks. So TaskDefinition defines the common properties for all ECS tasks and FargateTaskDefinition extends it to add the validation/options that are Fargate-specific.
I find this API quite confusing! You set this property if and only if you do not want the default value? If you do set it to the default of 20, a runtime error is seen?
I would agree this is confusing. It's probably this way to be backwards compatible. Before the ephemeralStorage option was added to the task definition, all Fargate tasks (on PV >= 1.4) would get 20 GiB of ephemeral storage by default. If ephemeralStorage had been added from day one, it would probably be a required option with no default value.
Maybe ECS can change their validation to not throw an error if ephemeralStorage is set to the default value (20), which would allow CloudFormation and the CDK to also set 20 as the default value and always set the property. The main downside would be having to maintain the default value in multiple places, and it being temporarily out of sync if ECS decides to change it.
There was a problem hiding this comment.
I think that having this say 20 as the the default input and then throwing an error on that value is not the way we want to go here. I realize that the aws-ecs module did it that way, but I tend to think that should be updated instead of this one following it's lead. Let's add a default of none, since we're not plugging the number 20 in anywhere on our end and add to this doc that the minimum value here is 21. If you'd like to make the same update to the aws-ecs docs, we'd welcome that but it's certainly not mandatory.
|
My team is hoping to take advantage of this feature, can we please update and re-review? |
rix0rrr
added
feature-request
packages/@aws-cdk/aws-ecs-patterns/lib/fargate/application-load-balanced-fargate-service.ts
Show resolved
Hide resolved
TheRealAmazonKendra
changed the title
github-actions
bot
added
the
effort/small
| @@ -76,6 +76,25 @@ If you need to encrypt the traffic between the load balancer and the ECS tasks, | |||
|
|
|||
| Additionally, if more than one application target group are needed, instantiate one of the following: | |||
|
|
|||
| On Fargate Platform Version 1.4.0 or later, you may specify up to 200GiB of [ephemeral storage](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/fargate-task-storage.html#fargate-task-storage-pv14): | |||
There was a problem hiding this comment.
Can you clarify here that the minimum is 21 because 20 is provided by default?
| @@ -125,6 +144,32 @@ const loadBalancedFargateService = new ecsPatterns.ApplicationMultipleTargetGrou | |||
| }); | |||
| ``` | |||
|
|
|||
| On Fargate Platform Version 1.4.0 or later, you may specify up to 200GiB of [ephemeral storage](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/fargate-task-storage.html#fargate-task-storage-pv14): | |||
There was a problem hiding this comment.
Same as above: Can you clarify here that the minimum is 21 because 20 is provided by default?
| @@ -169,6 +214,21 @@ If you specify the option `recordType` you can decide if you want the construct | |||
|
|
|||
| Additionally, if more than one network target group is needed, instantiate one of the following: | |||
|
|
|||
| On Fargate Platform Version 1.4.0 or later, you may specify up to 200GiB of [ephemeral storage](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/fargate-task-storage.html#fargate-task-storage-pv14): | |||
There was a problem hiding this comment.
Same as above: Can you clarify here that the minimum is 21 because 20 is provided by default?
| @@ -253,6 +313,49 @@ const loadBalancedFargateService = new ecsPatterns.NetworkMultipleTargetGroupsFa | |||
| }); | |||
| ``` | |||
|
|
|||
| On Fargate Platform Version 1.4.0 or later, you may specify up to 200GiB of [ephemeral storage](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/fargate-task-storage.html#fargate-task-storage-pv14): | |||
There was a problem hiding this comment.
Same as above: Can you clarify here that the minimum is 21 because 20 is provided by default?
| @@ -299,6 +402,27 @@ const queueProcessingFargateService = new ecsPatterns.QueueProcessingFargateServ | |||
|
|
|||
| when queue not provided by user, CDK will create a primary queue and a dead letter queue with default redrive policy and attach permission to the task to be able to access the primary queue. | |||
|
|
|||
| On Fargate Platform Version 1.4.0 or later, you may specify up to 200GiB of [ephemeral storage](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/fargate-task-storage.html#fargate-task-storage-pv14): | |||
There was a problem hiding this comment.
Same as above: Can you clarify here that the minimum is 21 because 20 is provided by default?
| * | ||
| * @default 20 | ||
| */ | ||
| readonly ephemeralStorageGiB?: number; |
There was a problem hiding this comment.
Same comment as above.
| @@ -115,6 +115,7 @@ describe('When Application Load Balancer', () => { | |||
| cpu: 256, | |||
| assignPublicIp: true, | |||
| memoryLimitMiB: 512, | |||
| ephemeralStorageGiB: 100, | |||
There was a problem hiding this comment.
Let's add in validation failure cases for below 21 and above 200.
| @@ -316,6 +316,40 @@ test('setting platform version', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| test('setting ephemeral storage on NLB Fargate Service', () => { | |||
There was a problem hiding this comment.
Same as above: Let's add in validation failure cases for below 21 and above 200.
| @@ -350,6 +350,7 @@ testDeprecated('test Fargate queue worker service construct - with optional prop | |||
| new ecsPatterns.QueueProcessingFargateService(stack, 'Service', { | |||
| cluster, | |||
| memoryLimitMiB: 512, | |||
| ephemeralStorageGiB: 100, | |||
There was a problem hiding this comment.
Same as above: Let's add in validation failure cases for below 21 and above 200.
| @@ -92,6 +92,7 @@ test('Can create a scheduled Fargate Task - with optional props', () => { | |||
| image: ecs.ContainerImage.fromRegistry('henk'), | |||
| memoryLimitMiB: 512, | |||
| cpu: 2, | |||
| ephemeralStorageGiB: 100, | |||
There was a problem hiding this comment.
Same as above: Let's add in validation failure cases for below 21 and above 200.
|
@tapichu thank you for kicking this off! Do you have time to push it over the line? Would you prefer someone else to take it on? |
|
@olizilla This will get closed as stale soon since changes were requested. Feel free to pick this up, either building off this PR or opening a new one. |
My laptop turned into a brick and it took some time to get that sorted. I should be able to work on it now. |
|
@tapichu Once you have made changes, feel free to assign this to me so that I can review it before our bots start threatening to close it. |
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
Fargate supports a new ephemeral storage option, to request up to 200
GiB of task storage. Support for it was added to aws-ecs but not to
aws-ecs-patterns.
This change adds support for ephemeral storage, so the following
patterns can take advantage of it:
Closes #18105
Signed-off-by: Eduardo Lopez 252504+tapichu@users.noreply.github.com
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license