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

feat(batch): ephemeralStorage property on job definitions #25399

Merged
merged 31 commits into from
Jun 20, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
fe9d5ce
add ephemeralStorage property and unit test
sumupitchayan May 2, 2023
3854e10
add ephemeralStorage in README
sumupitchayan May 2, 2023
81efde2
add new prop to integ test
sumupitchayan May 2, 2023
0ec9174
Merge branch 'main' into sumughan/batch-add-ephemeralstorage-prop
sumupitchayan May 3, 2023
2af1f44
add new prop to IEcsContainerDefinition
sumupitchayan May 3, 2023
22e06ab
move ephemeral storage to Fargate class
sumupitchayan May 4, 2023
5d51fb2
add ephemeralStorage to CloudFormation and fix test format
sumupitchayan May 4, 2023
3d07bd1
update integ test snapshots
sumupitchayan May 4, 2023
1699e71
remove README typo
sumupitchayan May 4, 2023
c023e0e
remove test comments
sumupitchayan May 4, 2023
b71a742
Merge branch 'main' into sumughan/batch-add-ephemeralstorage-prop
sumupitchayan May 4, 2023
f95a4c8
Merge branch 'main' into sumughan/batch-add-ephemeralstorage-prop
sumupitchayan May 5, 2023
84c5040
Update packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts
sumupitchayan May 5, 2023
2719d0c
Update packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts
sumupitchayan May 5, 2023
cdc7006
Update packages/@aws-cdk/aws-batch-alpha/test/ecs-container-definitio…
sumupitchayan May 5, 2023
bdfb89a
rename to ephemeralStorageSize
sumupitchayan May 8, 2023
03b220c
finish renaming to ephemeralStorageSize
sumupitchayan May 8, 2023
8218d4c
add validation and unit test
sumupitchayan May 8, 2023
58b2ce5
make prop undefined by default
sumupitchayan May 8, 2023
c8023c3
refactor code for breaking change, moving executionRole property to E…
sumupitchayan May 9, 2023
c2e5e1d
Merge branch 'main' into sumughan/batch-add-ephemeralstorage-prop
sumupitchayan May 9, 2023
d341a1f
Kaizen suggestion - writing 20 GiB for default
sumupitchayan May 9, 2023
06e4dab
kaizen suggestion: simplify test case and use throw error('') instead…
sumupitchayan May 9, 2023
d36b2b3
add test adding ephemeralStorage as a Token
sumupitchayan May 9, 2023
06529b6
Merge branch 'main' into sumughan/batch-add-ephemeralstorage-prop
sumupitchayan May 9, 2023
72ace6e
fix calvin formatting suggestions
sumupitchayan Jun 8, 2023
62728cd
Update packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts
sumupitchayan Jun 8, 2023
b0c672b
Update packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts
sumupitchayan Jun 8, 2023
ef9727d
fix build failure by adding quotes within error expected error msg
sumupitchayan Jun 20, 2023
b025fbd
kaizen suggestion, remove creating new app in unit test
sumupitchayan Jun 20, 2023
97fded2
Merge branch 'main' into sumughan/batch-add-ephemeralstorage-prop
mergify[bot] Jun 20, 2023
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
33 changes: 33 additions & 0 deletions packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,13 @@ export interface IEcsFargateContainerDefinition extends IEcsContainerDefinition
* @default LATEST
*/
readonly fargatePlatformVersion?: ecs.FargatePlatformVersion;

/**
* The size for ephemeral storage. Service default is 20 GiB.
*
* @default - undefined
sumupitchayan marked this conversation as resolved.
Show resolved Hide resolved
*/
readonly ephemeralStorageSize?: Size;
}

/**
Expand Down Expand Up @@ -928,6 +935,13 @@ export interface EcsFargateContainerDefinitionProps extends EcsContainerDefiniti
* @default - a Role will be created
*/
readonly executionRole?: iam.IRole;

/**
* The size for ephemeral storage. Service default is 20 GiB.
*
* @default - undefined
sumupitchayan marked this conversation as resolved.
Show resolved Hide resolved
*/
readonly ephemeralStorageSize?: Size;
}

/**
Expand All @@ -936,6 +950,7 @@ export interface EcsFargateContainerDefinitionProps extends EcsContainerDefiniti
export class EcsFargateContainerDefinition extends EcsContainerDefinitionBase implements IEcsFargateContainerDefinition {
public readonly fargatePlatformVersion?: ecs.FargatePlatformVersion;
public readonly assignPublicIp?: boolean;
public readonly ephemeralStorageSize?: Size;

/**
* The role used by Amazon ECS container and AWS Fargate agents to make AWS API calls on your behalf.
Expand All @@ -950,7 +965,22 @@ export class EcsFargateContainerDefinition extends EcsContainerDefinitionBase im
super(scope, id, props);
this.assignPublicIp = props.assignPublicIp;
this.fargatePlatformVersion = props.fargatePlatformVersion;
this.ephemeralStorageSize = props.ephemeralStorageSize;
this.executionRole = props.executionRole ?? createExecutionRole(this, 'ExecutionRole');

// validates ephemeralStorageSize is within limits
if (props.ephemeralStorageSize) {
this.node.addValidation({
sumupitchayan marked this conversation as resolved.
Show resolved Hide resolved
validate() {
if (props.ephemeralStorageSize!.toGibibytes() > 200) {
return ['Ephemeral Storage must be at most 200 GiB.'];
} else if (props.ephemeralStorageSize!.toGibibytes() < 21) {
return ['Ephemeral Storage must be minimum 21 GiB.'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Take a look at how we structure our other error messages and see if you can come up with the best version here.

Copy link

@gael-ft gael-ft May 17, 2023

Choose a reason for hiding this comment

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

Hello @sumupitchayan

Do you need some help to complete the PR ?
Looking at the review, this is the only comment that was not fixed.

Looking at ComputeEnvironments here an example of message could be:

ECS Fargate container '${id}' has 'ephemeralStorage' = ${props.ephemeralStorageSize} > 200 GiB; 'ephemeralStorage' cannot be greater than 200 GiB

Sorry in advance for direct ping, but it is about a week without any change and we're looking actively on this fix as it would avoid us stepping back from alpha package ...

}
return [];
},
});
}
}

/**
Expand All @@ -959,6 +989,9 @@ export class EcsFargateContainerDefinition extends EcsContainerDefinitionBase im
public _renderContainerDefinition(): CfnJobDefinition.ContainerPropertiesProperty {
return {
...super._renderContainerDefinition(),
ephemeralStorage: this.ephemeralStorageSize? {
sizeInGiB: this.ephemeralStorageSize?.toGibibytes(),
} : undefined,
fargatePlatformConfiguration: {
platformVersion: this.fargatePlatformVersion?.toString(),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import * as efs from 'aws-cdk-lib/aws-efs';
import { ArnPrincipal, Role } from 'aws-cdk-lib/aws-iam';
import * as logs from 'aws-cdk-lib/aws-logs';
import { Secret } from 'aws-cdk-lib/aws-secretsmanager';
import { Size, Stack } from 'aws-cdk-lib';
import { Size, Stack, App } from 'aws-cdk-lib';
import { EcsContainerDefinitionProps, EcsEc2ContainerDefinition, EcsFargateContainerDefinition, EcsJobDefinition, EcsVolume, IEcsEc2ContainerDefinition, LinuxParameters, UlimitName } from '../lib';
import { CfnJobDefinitionProps } from 'aws-cdk-lib/aws-batch';
import { capitalizePropertyNames } from './utils';
Expand Down Expand Up @@ -36,6 +36,7 @@ const defaultExpectedProps: CfnJobDefinitionProps = {
},
};

let app: App;
sumupitchayan marked this conversation as resolved.
Show resolved Hide resolved
let stack: Stack;
let pascalCaseExpectedProps: any;

Expand Down Expand Up @@ -677,7 +678,8 @@ describe('EC2 containers', () => {
describe('Fargate containers', () => {
// GIVEN
beforeEach(() => {
stack = new Stack();
app = new App();
stack = new Stack(app);
pascalCaseExpectedProps = capitalizePropertyNames(stack, defaultExpectedProps);
});

Expand Down Expand Up @@ -711,4 +713,50 @@ describe('Fargate containers', () => {
},
});
});

test('can set ephemeralStorage', () => {
// WHEN
new EcsJobDefinition(stack, 'ECSJobDefn', {
container: new EcsFargateContainerDefinition(stack, 'EcsFargateContainer', {
...defaultContainerProps,
fargatePlatformVersion: ecs.FargatePlatformVersion.LATEST,
ephemeralStorageSize: Size.gibibytes(100),
}),
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Batch::JobDefinition', {
...pascalCaseExpectedProps,
ContainerProperties: {
...pascalCaseExpectedProps.ContainerProperties,
ExecutionRoleArn: {
'Fn::GetAtt': ['EcsFargateContainerExecutionRole3286EAFE', 'Arn'],
},
EphemeralStorage: {
SizeInGiB: Size.gibibytes(100).toGibibytes(),
},
},
});
});

test('ephemeralStorage validation', () => {
sumupitchayan marked this conversation as resolved.
Show resolved Hide resolved
// WHEN
new EcsJobDefinition(stack, 'ECSJobDefn', {
container: new EcsFargateContainerDefinition(stack, 'EcsFargateContainer', {
...defaultContainerProps,
fargatePlatformVersion: ecs.FargatePlatformVersion.LATEST,
ephemeralStorageSize: Size.gibibytes(19),
}),
});

new EcsJobDefinition(stack, 'ECSJobDefn2', {
container: new EcsFargateContainerDefinition(stack, 'EcsFargateContainer2', {
...defaultContainerProps,
fargatePlatformVersion: ecs.FargatePlatformVersion.LATEST,
ephemeralStorageSize: Size.gibibytes(201),
}),
});

expect(() => app.synth()).toThrow('Validation failed with the following errors:\n [Default/EcsFargateContainer] Ephemeral Storage must be minimum 21 GiB.\n [Default/EcsFargateContainer2] Ephemeral Storage must be at most 200 GiB.');
});
});
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "30.1.0",
"version": "31.0.0",
"files": {
"21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": {
"source": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"30.1.0"}
{"version":"31.0.0"}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "30.1.0",
"version": "31.0.0",
"testCases": {
"BatchEcsJobDefinitionTest/DefaultTest": {
"stacks": [
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "30.1.0",
"version": "31.0.0",
"artifacts": {
"stack.assets": {
"type": "cdk:asset-manifest",
Expand All @@ -17,7 +17,7 @@
"validateOnSynth": false,
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}",
"cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}",
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/c292a4e3a2a62cd3f3134971757eb866dc5224bc69f7109ec27d12ab882f2345.json",
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/7eabaa659955f076359ed72f88d929cfe7651a904b6038ae0f3b3215ab36ac6c.json",
"requiresBootstrapStackVersion": 6,
"bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version",
"additionalDependencies": [
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
{
"version": "30.1.0",
"version": "31.0.0",
"files": {
"c292a4e3a2a62cd3f3134971757eb866dc5224bc69f7109ec27d12ab882f2345": {
"7eabaa659955f076359ed72f88d929cfe7651a904b6038ae0f3b3215ab36ac6c": {
"source": {
"path": "stack.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "c292a4e3a2a62cd3f3134971757eb866dc5224bc69f7109ec27d12ab882f2345.json",
"objectKey": "7eabaa659955f076359ed72f88d929cfe7651a904b6038ae0f3b3215ab36ac6c.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,9 @@
"Type": "container",
"ContainerProperties": {
"Environment": [],
"EphemeralStorage": {
"SizeInGiB": 100
},
"ExecutionRoleArn": {
"Fn::GetAtt": [
"myFargateContainerExecutionRoleB9EB79EA",
Expand Down
Loading