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.

Sign up for GitHub

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

feat(ecs): add support for Fargate PV1.4 ephemeral storage #15440

Merged
merged 2 commits into from
Aug 27, 2021
Merged

feat(ecs): add support for Fargate PV1.4 ephemeral storage #15440

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
63dd00c
feat(ecs): add support for Fargate PV1.4 ephemeral storage
otterley Jul 7, 2021
0c6ba5f
Merge branch 'master' into fargate-ecs-ephemeral-storage
mergify[bot] Aug 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/@aws-cdk/aws-ecs/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,17 @@ const fargateTaskDefinition = new ecs.FargateTaskDefinition(this, 'TaskDef', {
});
```

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):

```ts
const fargateTaskDefinition = new ecs.FargateTaskDefinition(this, 'TaskDef', {
memoryLimitMiB: 512,
cpu: 256,
ephemeralStorageGiB: 100
});
```

To add containers to a task definition, call `addContainer()`:

```ts
Expand Down
21 changes: 21 additions & 0 deletions packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,15 @@ export interface TaskDefinitionProps extends CommonTaskDefinitionProps {
* @default - No inference accelerators.
*/
readonly inferenceAccelerators?: InferenceAccelerator[];

/**
* The amount (in GiB) of ephemeral storage to be allocated to the task.
*
* Only supported in Fargate platform version 1.4.0 or later.
*
* @default - Undefined, in which case, the task will receive 20GiB ephemeral storage.
*/
readonly ephemeralStorageGiB?: number;
}

/**
Expand Down Expand Up @@ -329,6 +338,13 @@ export class TaskDefinition extends TaskDefinitionBase {
*/
public readonly compatibility: Compatibility;

/**
* The amount (in GiB) of ephemeral storage to be allocated to the task.
*
* Only supported in Fargate platform version 1.4.0 or later.
*/
public readonly ephemeralStorageGiB?: number;

Comment on lines +341 to +347
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually an input property would be added as a class attribute if it can possibly be referred to later in the code. I'm curious if there would be such a use case for ephemeral storage for which we need to add it here.

Copy link
Contributor Author

@otterley otterley Aug 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've run into a number of issues in the past with multiple L2 constructs where their input values were swallowed by the constructor and made inaccessible after that. This poses significant challenges to classes and methods that consume the resulting construct and need to examine it. So I am now making these attributes readable by default, unless there's some really good reason to hide them.

We simply cannot predict the future with certainty as to when these attributes might someday be useful to consumers. It's always easier to make them accessible from the beginning, just in case, rather than hide them by default and then have to undo our mistake later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @otterley, CDK team member weighing in here. In this case, I don't think it is necessary or helpful to make this a public class attribute, since the ephemeralStorageGiB value will be undefined if the user did not provide it in the input props. If the user provided this value to the constructor, then they should be able to design their CDK app in such a way that they have access to that value wherever they need it. In the case where the user does not provide a value here, they may be frustrated that ephemeralStorageGiB is undefined, and not informing them what the default value is.

I am willing to disagree and commit here. Since this is an optional attribute, it is ultimately a 2-way door decision. So, the decision is ultimately up to you.

/**
* The container definitions.
*/
Expand Down Expand Up @@ -399,6 +415,8 @@ export class TaskDefinition extends TaskDefinitionBase {
props.inferenceAccelerators.forEach(ia => this.addInferenceAccelerator(ia));
}

this.ephemeralStorageGiB = props.ephemeralStorageGiB;

const taskDef = new CfnTaskDefinition(this, 'Resource', {
containerDefinitions: Lazy.any({ produce: () => this.renderContainers() }, { omitEmptyArray: true }),
volumes: Lazy.any({ produce: () => this.renderVolumes() }, { omitEmptyArray: true }),
Expand All @@ -424,6 +442,9 @@ export class TaskDefinition extends TaskDefinitionBase {
produce: () =>
!isFargateCompatible(this.compatibility) ? this.renderInferenceAccelerators() : undefined,
}, { omitEmptyArray: true }),
ephemeralStorage: this.ephemeralStorageGiB ? {
sizeInGiB: this.ephemeralStorageGiB,
} : undefined,
});

if (props.placementConstraints) {
Expand Down
20 changes: 20 additions & 0 deletions packages/@aws-cdk/aws-ecs/lib/fargate/fargate-task-definition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,15 @@ export interface FargateTaskDefinitionProps extends CommonTaskDefinitionProps {
* @default 512
*/
readonly memoryLimitMiB?: number;

/**
* The amount (in GiB) of ephemeral storage to be allocated to the task. The maximum supported value is 200 GiB.
*
* NOTE: This parameter is only supported for tasks hosted on AWS Fargate using platform version 1.4.0 or later.
*
* @default 20
*/
readonly ephemeralStorageGiB?: number;
}

/**
Expand Down Expand Up @@ -104,6 +113,11 @@ export class FargateTaskDefinition extends TaskDefinition implements IFargateTas
// we need to explicitly write the type here, as type deduction for enums won't lead to
// the import being generated in the .d.ts file.

/**
* The amount (in GiB) of ephemeral storage to be allocated to the task.
*/
public readonly ephemeralStorageGiB?: number;

Comment on lines +116 to +120
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same suggestion as before, we can possibly remove adding it as a class attribute here as well.

/**
* Constructs a new instance of the FargateTaskDefinition class.
*/
Expand All @@ -115,5 +129,11 @@ export class FargateTaskDefinition extends TaskDefinition implements IFargateTas
compatibility: Compatibility.FARGATE,
networkMode: NetworkMode.AWS_VPC,
});

if (props.ephemeralStorageGiB && (props.ephemeralStorageGiB < 21 || props.ephemeralStorageGiB > 200)) {
throw new Error('Ephemeral storage size must be between 21GiB and 200GiB');
}

this.ephemeralStorageGiB = props.ephemeralStorageGiB;
}
}
30 changes: 30 additions & 0 deletions packages/@aws-cdk/aws-ecs/test/fargate/fargate-task-definition.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ nodeunitShim({
taskRole: new iam.Role(stack, 'TaskRole', {
assumedBy: new iam.ServicePrincipal('ecs-tasks.amazonaws.com'),
}),
ephemeralStorageGiB: 21,
});

taskDefinition.addVolume({
Expand All @@ -76,6 +77,9 @@ nodeunitShim({
'Arn',
],
},
EphemeralStorage: {
SizeInGiB: 21,
},
Family: 'myApp',
Memory: '1024',
NetworkMode: 'awsvpc',
Expand Down Expand Up @@ -131,6 +135,32 @@ nodeunitShim({

test.done();
},

'throws when ephemeral storage request is too high'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
test.throws(() => {
new ecs.FargateTaskDefinition(stack, 'FargateTaskDef', {
ephemeralStorageGiB: 201,
});
}, /Ephemeral storage size must be between 21GiB and 200GiB/);

// THEN
test.done();
},

'throws when ephemeral storage request is too low'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
test.throws(() => {
new ecs.FargateTaskDefinition(stack, 'FargateTaskDef', {
ephemeralStorageGiB: 20,
});
}, /Ephemeral storage size must be between 21GiB and 200GiB/);

// THEN
test.done();
},
},

'When importing from an existing Fargate TaskDefinition': {
Expand Down