feat(batch): fargate support for jobs #13591
Conversation
|
|
kokachev
force-pushed
the
feature/batch-fargate-support
branch
from
166c998
to
3c4fd44
Compare
kokachev
force-pushed
the
feature/batch-fargate-support
branch
from
bef6f46
to
f3eadba
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
@iliapolo review? |
|
@iliapolo should I close this PR? |
|
@kokachev No - I still have this on my radar. Would be good if you resolved the conflicts that arose in the meantime though :) |
| ```ts | ||
| const vpc = new ec2.Vpc(this, 'VPC'); | ||
|
|
||
| const spotEnvironment = new batch.ComputeEnvironment(stack, 'MyFargateEnvironment', { |
There was a problem hiding this comment.
| const spotEnvironment = new batch.ComputeEnvironment(stack, 'MyFargateEnvironment', { | |
| const fargateSpotEnvironment = new batch.ComputeEnvironment(stack, 'MyFargateEnvironment', { |
| /** | ||
| * Resources will be Fargate resources. | ||
| */ | ||
| FARGATE_SPOT = 'FARGATE_SPOT', |
There was a problem hiding this comment.
What does spot mean in the context of fargate? would be nice to have a sentence or two about it.
| @@ -82,6 +82,164 @@ describe('Batch Compute Evironment', () => { | |||
| }); | |||
| }); | |||
| }); | |||
| describe('using fargate resources', () => { | |||
There was a problem hiding this comment.
We also need an integ test. Add a compute environment to https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/aws-batch/test/integ.batch.ts#L30
iliapolo
added
the
response-requested
|
This PR has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled. |
github-actions
bot
added
the
closing-soon
|
will update the PR. |
|
Looking forward to being able to play with this :) |
| @@ -458,6 +523,9 @@ export class JobDefinition extends Resource implements IJobDefinition { | |||
| user: container.user, | |||
| vcpus: container.vcpus || 1, | |||
| volumes: container.volumes, | |||
| fargatePlatformConfiguration: container.fargatePlatformConfiguration ? { | |||
There was a problem hiding this comment.
Sorry, my github-fu is weak. Not sure how to put a comment on code that isn't a part of the PR.
But above, when setting the resourceRequirements, don't we need a way to pass those in for fargate?
resourceRequirements: container.gpuCount
? [{ type: 'GPU', value: String(container.gpuCount) }]
: undefined
Also, pretty sure that vcpus needs to be not set
peterwoodworth
removed
closing-soon
iliapolo
added
the
response-requested
iliapolo
changed the title
iliapolo
changed the title
|
Looking forward to this PR being merged in 👍 ... just in time for what I'm working on. 😄 |
|
@iliapolo @brendonparker since it has been a little while since the author has responded, I've opened PR #15848 that responds to all of the feedback left on this MR in hopes that this will be brought to completion soon. A review as soon as you get a chance would be greatly appreciated. |
|
@DDynamic sweet 👍😊 |
|
Closing in favor of #15848 |
Added Fargate support for Batch jobs. Note: this is not entirely my work - most of it was done by @kokachev. It is an updated version of Fargate support for batch jobs based on the feedback left in #13591. - Doc fixes - Integration test addition - Network configuration for Fargate - Support `ResourceRequirements` for Fargate jobs - Other minor fixes revealed by integration test closes: #13590, #13591 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Added Fargate support for Batch jobs.
closes: #13590
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license