-
Notifications
You must be signed in to change notification settings - Fork 4k
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(ecs): add validation checks to memory cpu combinations of FARGATE compatible task definitions #30166
Merged
Merged
feat(ecs): add validation checks to memory cpu combinations of FARGATE compatible task definitions #30166
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
a6055a2
ecs fargate cpu-mem validation
mrlikl f8abbd8
fix example in readme
mrlikl 9bad34e
integ tests
mrlikl 39ea694
Merge branch 'main' into ecs-mem
mrlikl 7f67769
Merge branch 'main' into ecs-mem
mrlikl 3ef0622
remove typecasting, combine checks
mrlikl c067408
Merge branch 'main' into ecs-mem
mrlikl 928efc2
support lazy loading
mrlikl 8ef2fb1
Merge branch 'main' into ecs-mem
mrlikl e0978bd
spelling+add integ tests
mrlikl 78f66a0
support lazy values
mrlikl 62d09ae
revert windows outside of fargate checks
mrlikl b4f36b2
Merge branch 'main' into ecs-mem
mergify[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -473,6 +473,12 @@ export class TaskDefinition extends TaskDefinitionBase { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.checkFargateWindowsBasedTasksSize(props.cpu!, props.memoryMiB!, props.runtimePlatform!); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Validate CPU and memory combinations if fargate compatible | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (isFargateCompatible(props.compatibility)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Check the combination as per doc https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-cpu-memory-error.html | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.validateFargateTaskDefinitionMemoryCpu(props.cpu!, props.memoryMiB!); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.runtimePlatform = props.runtimePlatform; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this._cpu = props.cpu; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this._memory = props.memoryMiB; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -894,6 +900,35 @@ export class TaskDefinition extends TaskDefinitionBase { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
throw new Error(`If operatingSystemFamily is ${runtimePlatform.operatingSystemFamily!._operatingSystemFamily}, then cpu must be in 1024 (1 vCPU), 2048 (2 vCPU), or 4096 (4 vCPU). Provided value was: ${cpu}`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private validateFargateTaskDefinitionMemoryCpu(cpu: string, memory: string) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const validCpuMemoryCombinations = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ cpu: 256, memory: [512, 1024, 2048] }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ cpu: 512, memory: this.range(1024, 4096, 1024) }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ cpu: 1024, memory: this.range(2048, 8192, 1024) }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ cpu: 2048, memory: this.range(4096, 16384, 1024) }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ cpu: 4096, memory: this.range(8192, 30720, 1024) }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ cpu: 8192, memory: this.range(16384, 61440, 4096) }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ cpu: 16384, memory: this.range(32768, 122880, 8192) }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const isValidCombination = validCpuMemoryCombinations.some((combo) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return combo.cpu === Number(cpu) && combo.memory.includes(Number(memory)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!isValidCombination) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
throw new Error('Invalid CPU and memory combinations for FARGATE compatible task definition - https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-cpu-memory-error.html'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private range(start: number, end: number, step: number): number[] { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
What about avoiding |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const result = []; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
for (let i = start; i <= end; i += step) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
result.push(i); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return result; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,28 +21,24 @@ describe('fargate task definition', () => { | |
|
||
}); | ||
|
||
test('support lazy cpu and memory values', () => { | ||
test('does not support lazy cpu and memory values', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be able to support this by adding validation on the this.node.addValidation({ validate: () => this. validateFargateTaskDefinitionMemoryCpu(...); |
||
// GIVEN | ||
const stack = new cdk.Stack(); | ||
|
||
new ecs.FargateTaskDefinition(stack, 'FargateTaskDef', { | ||
cpu: cdk.Lazy.number({ produce: () => 128 }), | ||
memoryLimitMiB: cdk.Lazy.number({ produce: () => 1024 }), | ||
}); | ||
|
||
// THEN | ||
Template.fromStack(stack).hasResourceProperties('AWS::ECS::TaskDefinition', { | ||
Cpu: '128', | ||
Memory: '1024', | ||
}); | ||
expect(() => { | ||
new ecs.FargateTaskDefinition(stack, 'FargateTaskDef', { | ||
cpu: cdk.Lazy.number({ produce: () => 256 }), | ||
memoryLimitMiB: cdk.Lazy.number({ produce: () => 1024 }), | ||
}); | ||
}).toThrow(/Invalid CPU and memory combinations for FARGATE compatible task definition/); | ||
|
||
}); | ||
|
||
test('with all properties set', () => { | ||
// GIVEN | ||
const stack = new cdk.Stack(); | ||
const taskDefinition = new ecs.FargateTaskDefinition(stack, 'FargateTaskDef', { | ||
cpu: 128, | ||
cpu: 256, | ||
executionRole: new iam.Role(stack, 'ExecutionRole', { | ||
path: '/', | ||
assumedBy: new iam.CompositePrincipal( | ||
|
@@ -72,7 +68,7 @@ describe('fargate task definition', () => { | |
|
||
// THEN | ||
Template.fromStack(stack).hasResourceProperties('AWS::ECS::TaskDefinition', { | ||
Cpu: '128', | ||
Cpu: '256', | ||
ExecutionRoleArn: { | ||
'Fn::GetAtt': [ | ||
'ExecutionRole605A040B', | ||
|
@@ -195,6 +191,30 @@ describe('fargate task definition', () => { | |
}); | ||
}).toThrow(/'pidMode' can only be set to 'host' for Fargate containers, got: 'task'./); | ||
}); | ||
|
||
test('throws error when invalid CPU and memory combination is provided', () => { | ||
const stack = new cdk.Stack(); | ||
|
||
expect(() => { | ||
new ecs.FargateTaskDefinition(stack, 'FargateTaskDef', { | ||
cpu: 256, | ||
memoryLimitMiB: 125, | ||
}); | ||
}).toThrow(/Invalid CPU and memory combinations for FARGATE compatible task definition/); | ||
}); | ||
|
||
test('succesfull when valid CPU and memory combination is provided', () => { | ||
Leo10Gama marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const stack = new cdk.Stack(); | ||
new ecs.FargateTaskDefinition(stack, 'FargateTaskDef', { | ||
cpu: 256, | ||
memoryLimitMiB: 512, | ||
}); | ||
|
||
Template.fromStack(stack).hasResourceProperties('AWS::ECS::TaskDefinition', { | ||
Cpu: '256', | ||
Memory: '512', | ||
}); | ||
}); | ||
}); | ||
describe('When configuredAtLaunch in the Volume', ()=> { | ||
test('do not throw when configuredAtLaunch is false', () => { | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we take the opportunity to remove the
!
s? It's a bit annoying to see them.