close #3536 - add batch pattern constructs #4732
Conversation
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
stephnr
changed the title
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| /** | ||
| * The IAM role applied to EC2 resources in the compute environment. | ||
| */ | ||
| readonly instanceRole: iam.IRole; |
There was a problem hiding this comment.
We always create Roles by default if a customer doesn't pass one. This should be optional (readonly instanceRole?: iam.IRole).
There was a problem hiding this comment.
Perfect, I will go ahead and fix this.
| * | ||
| * @default optimal | ||
| */ | ||
| readonly instanceTypes?: ec2.InstanceType[]; |
There was a problem hiding this comment.
Can you clarify why does this need to be an array? (This is an honest question, maybe I'm missing something)
There was a problem hiding this comment.
A very good question, its because of the instance types property type here in the Cloudformation documentation for the AWS::Batch::ComputeEnvironment resource.
|
|
||
| /** | ||
| * The minimum number of EC2 vCPUs that an environment should maintain (even if the compute environment state is DISABLED). | ||
| * Each vCPU is equivalent to 1,024 CPU shares. You must specify at least one vCPU. |
There was a problem hiding this comment.
This comment ("You must specify at least one vCPU.") is in direction contradiction to the default of 0.
There was a problem hiding this comment.
Good eye 👍 I will adjust it.
| /** | ||
| * The VPC subnets into which the compute resources are launched. | ||
| */ | ||
| readonly subnets: ec2.ISubnet[]; |
There was a problem hiding this comment.
To be consistent with the rest of the Construct Library, this should be instead a pair of properties:
readonly vpc: ec2.IVpc;
readonly vpcSubnets?: ec2.SubnetSelection;There was a problem hiding this comment.
Perfect, I didn't see this earlier but this makes a lot of sense.
| /** | ||
| * The desired number of EC2 vCPUS in the compute environment. Each vCPU | ||
| * is equivalent to 1,024 CPU shares. You must specify at least one vCPU. | ||
| */ |
There was a problem hiding this comment.
Default description missing.
| }); | ||
|
|
||
| const jobQueue = new CfnJobQueue(this, 'Resource', { | ||
| computeEnvironmentOrder: props.computeEnvironmentOrder ? props.computeEnvironmentOrder.map(cp => { |
There was a problem hiding this comment.
Indentation is off here:
computeEnvironmentOrder: props.computeEnvironmentOrder
? props.computeEnvironmentOrder.map(cp => ({
computeEnvironment: cp.computeEnvironment.computeEnvironmentArn,
order: cp.order,
} as CfnJobQueue.ComputeEnvironmentOrderProperty)
)
: [
{
computeEnvironment: new ComputeEnvironment(this, 'Resource-Batch-Compute-Environment').computeEnvironmentArn,
order: 1,
},
],
| computeEnvironment: cp.computeEnvironment.computeEnvironmentArn, | ||
| order: cp.order, | ||
| } as CfnJobQueue.ComputeEnvironmentOrderProperty; | ||
| }) : [ |
There was a problem hiding this comment.
Same here (indent of the ?: expression.
There was a problem hiding this comment.
Got it. I copied out your recommendation from another conversation here but lets review it after you take a look again.
| "@aws-cdk/aws-ec2": "1.14.0", | ||
| "@aws-cdk/aws-iam": "1.14.0", | ||
| "@aws-cdk/core": "1.14.0", | ||
| "@aws-cdk/aws-ecs": "1.14.0" |
There was a problem hiding this comment.
Try to have dependencies and peerDependencies in the same, alphabetical, order.
| }; | ||
|
|
||
| const app = new cdk.App(); | ||
| const stack = new cdk.Stack(app, 'test-batch-job-queue', { env }); |
There was a problem hiding this comment.
I think neither env nor app are needed for this test.
| }, | ||
| }, | ||
| } | ||
| }; No newline at end of file |
There was a problem hiding this comment.
Actually, since this is adding the first L2 code, can you use Jest instead of NodeUnit? You have examples in the repo of other packages using Jest (like IAM).
There was a problem hiding this comment.
Jest would be perfect! I am much more comfortable testing using that.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
@skinny85 the updates based on your comments have been published now |
|
Thanks @stephnr - I'll take a look this week! |
| * the jobs in the queue, with a preference for instance types that are less | ||
| * likely to be interrupted | ||
| */ | ||
| SPOT_CAPACITY_OPTIMIZED = 'SPOT_CAPACITY_OPTIMIZED', |
There was a problem hiding this comment.
Empty lines here:
export enum AllocationStrategy {
/**
* Batch will use the best fitting instance type will be used
* when assigning a batch job in this compute environment
*/
BEST_FIT = 'BEST_FIT',
/**
* Batch will select additional instance types that are large enough to
* meet the requirements of the jobs in the queue, with a preference for
* instance types with a lower cost per unit vCPU
*/
BEST_FIT_PROGRESSIVE = 'BEST_FIT_PROGRESSIVE',
/**
* This is only available for Spot Instance compute resources and will select
* additional instance types that are large enough to meet the requirements of
* the jobs in the queue, with a preference for instance types that are less
* likely to be interrupted
*/
SPOT_CAPACITY_OPTIMIZED = 'SPOT_CAPACITY_OPTIMIZED',
}Same comment for all enums above.
| /** | ||
| * AWS Batch manages your compute resources | ||
| */ | ||
| MANAGED = 'MANAGED', |
There was a problem hiding this comment.
I wouldn't align these. It's just not worth it (adding a third value that's longer than the previous ones would make for a horrible diff). Just have MANAGED = 'MANAGED'.
There was a problem hiding this comment.
Yep, I have an alignment tool but good point.
| /** | ||
| * Resources will be EC2 On-Demand resources | ||
| */ | ||
| EC2 = 'EC2', |
There was a problem hiding this comment.
Wouldn't ON_DEMAND be a better name for this then?
There was a problem hiding this comment.
It woulddddddd. Lets enforce some naming practices :P haha. I will swap this out.
| * Property to determine if AWS Batch | ||
| * should manage your compute resources | ||
| */ | ||
| export enum ComputeEnvironmentType { |
There was a problem hiding this comment.
I'm wondering whether this enum is worth it. Wouldn't it be simpler to just have managed?: boolean as a creation property of ComputeEnvironment? I think we're trying to mirror the API too closely, instead of aiming for simplicity.
There was a problem hiding this comment.
Hm, that is a better idea. I agree. I will swap this out.
| /** | ||
| * Property to specify how compute resources will be provisioned based | ||
| */ | ||
| export enum ComputeEnvironmentStatus { |
There was a problem hiding this comment.
Same exact comment. To me, this is simply an enabled?: boolean creation property.
| }); | ||
|
|
||
| // THEN | ||
| const expectedProps = expectedUnmanagedDefaultComputeProps({ |
There was a problem hiding this comment.
Same here (inline expectedProps).
| test.ok(true, 'No tests are specified for this package.'); | ||
| test.done(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
We need at least one integ test as well.
There was a problem hiding this comment.
I will write one up. Just a basic one for SPOT & ON_DEMAND. Will we need more?
| import { ResourcePart } from '@aws-cdk/assert/lib/assertions/have-resource'; | ||
| import { InstanceClass, InstanceSize, InstanceType } from '@aws-cdk/aws-ec2'; | ||
| import { Repository } from '@aws-cdk/aws-ecr'; | ||
| import { EcrImage, LinuxParameters, MountPoint, Ulimit, Volume } from '@aws-cdk/aws-ecs'; |
There was a problem hiding this comment.
Please, don't import any classes (other than from the @aws-cdk/core package) directly. Always refer to them with a prefix.
| // THEN | ||
| expect(job.jobDefinitionArn).toEqual(existingJob.jobDefinitionArn); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
General comment: I'm not a fan of helper methods in tests. I think it's better to use before to set things up:
describe('Batch Job Definition', () => {
let stack: cdk.Stack;
let jobDefProps: batch.JobDefinitionProps;
beforeEach(() => {
stack = new cdk.Stack();
const jobRepo = new Repository(stack, 'job-repo');
const role = new iam.Role(stack, 'job-role', {
assumedBy: new iam.ServicePrincipal('batch.amazonaws.com'),
});
const linuxParams = new LinuxParameters(stack, 'job-linux-params', {
initProcessEnabled: true,
sharedMemorySize: 1,
});
jobDefProps = {
jobDefinitionName: 'test-job',
containerProps: {
command: [ 'echo "Hello World"' ],
environment: {
foo: 'bar',
},
jobRole: role,
gpuCount: 1,
image: EcrImage.fromEcrRepository(jobRepo),
instanceType: InstanceType.of(InstanceClass.T2, InstanceSize.MICRO),
linuxParams,
memoryLimitMiB: 1,
mountPoints: new Array<MountPoint>(),
privileged: true,
readOnly: true,
ulimits: new Array<Ulimit>(),
user: 'root',
vcpus: 2,
volumes: new Array<Volume>(),
},
nodeProps: {
count: 2,
mainNode: 1,
rangeProps: new Array<batch.INodeRangeProps>(),
},
parameters: {
foo: 'bar',
},
retryAttempts: 2,
timeout: Duration.seconds(30),
};
});
test('renders the correct CloudFormation resource', () => {
new batch.JobDefinition(stack, 'JobDefinition', jobDefProps);
expect(stack).toHaveResourceLike('AWS::Batch::JobDefinition', {
JobDefinitionName: jobDefProps.jobDefinitionName,
ContainerProperties: jobDefProps.containerProps ? {
Command: jobDefProps.containerProps.command,
Environment: [
{
Name: 'foo',
Value: 'bar',
},
],
InstanceType: jobDefProps.containerProps.instanceType.toString(),
LinuxParameters: {},
Memory: jobDefProps.containerProps.memoryLimitMiB,
MountPoints: [],
Privileged: jobDefProps.containerProps.privileged,
ReadonlyRootFilesystem: jobDefProps.containerProps.readOnly,
Ulimits: [],
User: jobDefProps.containerProps.user,
Vcpus: jobDefProps.containerProps.vcpus,
Volumes: [],
} : undefined,
NodeProperties: jobDefProps.nodeProps ? {
MainNode: jobDefProps.nodeProps.mainNode,
NodeRangeProperties: [],
NumNodes: jobDefProps.nodeProps.count,
} : undefined,
Parameters: {
foo: 'bar',
},
RetryStrategy: {
Attempts: jobDefProps.retryAttempts,
},
Timeout: {
AttemptDurationSeconds: jobDefProps.timeout ? jobDefProps.timeout.toSeconds() : -1,
},
Type: 'container',
});
});
test('can be imported from an ARN', () => {
const importedJob = batch.JobDefinition.fromJobDefinitionArn(stack, 'job-def-clone',
'arn:aws:batch:us-east-1:123456789012:job-definition/job-def-name:1');
// THEN
expect(importedJob.jobDefinitionName).toEqual('job-def-name');
expect(importedJob.jobDefinitionArn).toEqual('arn:aws:batch:us-east-1:123456789012:job-definition/job-def-name:1');
});
});
|
Aha, perfect! I found this out as well while doing the integration tests.
It's a bit misleading because the property name is `ServiceRole` but it's
not an IAM service role. Instead it is a role that is used by the spawned
compute units in the corresponding ECS cluster.
That being said, I am working on fixing this to just allow the user to
provide a role or to have us define a default role for them.
…On Thu, 28 Nov 2019, 08:48 Benjamin Weigel, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In ***@***.***/aws-batch/lib/compute-environment.ts
<#4732 (comment)>:
> + launchTemplate: props.computeResources.launchTemplate,
+ maxvCpus: props.computeResources.maxvCpus || 256,
+ minvCpus: props.computeResources.minvCpus || 0,
+ placementGroup: props.computeResources.placementGroup ? props.computeResources.placementGroup.ref : undefined,
+ securityGroupIds: this.buildSecurityGroupIds(props.computeResources.securityGroupIds),
+ spotIamFleetRole: props.computeResources.spotIamFleetRole ? props.computeResources.spotIamFleetRole.roleArn : undefined,
+ subnets: props.computeResources.vpc.selectSubnets(props.computeResources.vpcSubnets).subnetIds,
+ tags: props.computeResources.computeResourcesTags ? props.computeResources.computeResourcesTags : undefined,
+ type: props.computeResources.type || ComputeResourceType.EC2,
+ };
+ }
+
+ const computeEnvironment = new CfnComputeEnvironment(this, 'Resource', {
+ computeEnvironmentName: this.physicalName,
+ computeResources,
+ serviceRole: new iam.CfnServiceLinkedRole(this, 'Resource-Service-Linked-Role', {
Thanks for the great work @stephnr <https://github.com/stephnr> . I have
been trying it out and noticed, that it currently doesn't work, due to the
fact that service linked roles (and therefore CfnServiceLinkedRole /
AWS::IAM::ServiceLinkedRole) are currently not supported by AWS Batch
(see that
https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_aws-services-that-work-with-iam.html
).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4732?email_source=notifications&email_token=ABHBLZ2KQ2OO4G24GXFMABDQV5ZVFA5CNFSM4JGBKJG2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCNIWPRA#discussion_r351629737>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABHBLZ7D46P6OHISL77AZPLQV5ZVFANCNFSM4JGBKJGQ>
.
|
| instanceRole: props.computeResources.instanceRole ? | ||
| props.computeResources.instanceRole.roleArn : | ||
| new iam.Role(this, 'Resource-Role', { | ||
| assumedBy: new iam.ServicePrincipal('batch.amazonaws.com'), | ||
| }).roleArn, |
There was a problem hiding this comment.
Hey. Ran into another hickup. This will fail, because Cloudformation expects an Instance Profile, not a role:
5/7 | 1:08:40 PM | CREATE_FAILED | AWS::Batch::ComputeEnvironment | compute-env (computeenv5B70C65C)
Operation failed, ComputeEnvironment went INVALID with error:
CLIENT_ERROR - Invalid IamInstanceProfile: arn:aws:iam::0123456789:role/batch-compute-env-stack-computeenvResourceRole92B0-Q4UIZBETE6B1
You will need to create a new iam.CfnInstanceProfile.
There was a problem hiding this comment.
Good eye, I fixed this up now. We needed to make sure to use the service-role/AWSBatchServiceRole.
I didn't need to end up using that L0 construct.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
mergify
bot
dismissed
skinny85’s stale review
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
@skinny85 next round of updates is ready w/ updated integration tests 👍 I made it work this time around with using Container images from any registry, ECR, or even a local asset image. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
2 similar comments
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
@skinny85 are the build failures above due to other peoples work? I don't see what is wrong 🤔 |
|
@stephnr I think you need to rebase/merge once more, now that I would do it myself, but GitHub shows some conflicts. (The problem is that the newest version is now |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thanks @stephnr ! I addressed a few cosmetic comments from the PR that you missed, and resolved the conflicts with latest. I have no further comments, so I'm merging the PR. Thanks so much for your perseverance with sticking with this PR for so long! |
|
@stephnr actually, there's one more thing. Would you open another PR, updating the ReadMe of the module, explaining how to use the new beautiful constructs? |
|
Sorry for the delay. I've been sick all week. But I'll try and take a look
at this first thing in the morning ✌️
…On Tue, Feb 25, 2020, 00:14 Adam Ruka ***@***.***> wrote:
@stephnr <https://github.com/stephnr> actually, there's one more thing.
Would you open another PR, updating the ReadMe of the module, explaining
how to use the new beautiful constructs?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4732?email_source=notifications&email_token=ABHBLZ2ERKV5QYZ7CXOJ7SDRERIGVA5CNFSM4JGBKJG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMZ4WXQ#issuecomment-590596958>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABHBLZ6JKSEYYSUHMFCSG4LRERIGVANCNFSM4JGBKJGQ>
.
|
|
This is a great addition to the CDK and something I'll be using in the near term. Quick question - is there a reason that specifying a Launch Template for a Compute Environment is missing? It is a capability of AWS Batch that I prefer to use over custom AMIs. |
|
@wleepang good question! I believe the reason why we didn't add this support yet is because that feature is not yet implemented in Cloudformation. Once it has been added to the Cloudformation spec for Batch, we can then update these constructs to match. |
|
Can you clarify? I have created CloudFormation templates for Batch Compute Environments with Launch Templates for a while now. |
|
Thanks for the PR. This implementation needs further review. The concept of "Managed Compute Environments" is misinterpreted and there are many silent unexpected behaviours happening because of that. I'll be working on a patch to fix these things and an implementation for the Launch Template. Is there any API design rule that prevents requiring dev interaction with L0 constructs? It would be for the launch template. I can't think of any useful abstraction for it since it's already super simple. The usage would be something like this: const myLaunchTemplate = new ec2.CfnLaunchTemplate(this, 'LaunchTemplate', {
launchTemplateName: 'extra-storage-template',
launchTemplateData: {
blockDeviceMappings: [
{
deviceName: '/dev/xvdcz',
ebs: {
encrypted: true,
volumeSize: 100,
volumeType: 'gp2'
}
}
]
}
});and then const myComputeEnv = new batch.ComputeEnvironment(this, 'ComputeEnv', {
computeResources: {
launchTemplate: {
launchTemplateName: myLaunchTemplate.launchTemplateName as string,
},
vpc,
},
computeEnvironmentName: 'MyStorageCapableComputeEnvironment',
}); |
|
Aha I see. Forgive me then. But the more help the better. I say let's move
forward with getting this supported and of course doing anything bug fixes.
I still need to write up the README but perhaps we can collaborate a little
and get working on multiple fronts for this.
…On Mon, Mar 2, 2020, 17:12 andrestone ***@***.***> wrote:
Thanks for the PR.
This implementation needs further review.
The concept of "Managed Compute Environments" is misinterpreted and there
are many silent unexpected behaviours happening because of that.
I'll be working on a patch to fix these things and an implementation for
the Launch Template. Is there any API design rule that prevents requiring
dev interaction with L0 constructs? It would be for the launch template. I
can't think of any useful abstraction for it since it's already super
simple.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4732?email_source=notifications&email_token=ABHBLZZJ374RCB2KPB43ZCDRFPLIFA5CNFSM4JGBKJG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENP47VI#issuecomment-593481685>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABHBLZ4O4TTOBJ3Q6QAIVJLRFPLIFANCNFSM4JGBKJGQ>
.
|
This looks promising! Would you be interested in whipping together a PR to get this supported? I can help you review and develop it further as well. |
It's done. Thanks! |
6 participants
WORK IN PROGRESS - Submitted for early review
This PR includes 3 pattern constructs under the
@aws-cdk/batchpackage:Each of them expanding upon the existing underlying Cfn generated classes to help construct AWS Batch resources. A few caveats worth noticing:
☝️ These points were introduced to mimic the functionality of the
ec2.Vpc(...)construct.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license