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

fix(batch): managed compute environment now properly works with compute resources and instanceRole has correct docstring and type definition #6549

Merged
merged 10 commits into from
Mar 6, 2020
Merged

fix(batch): managed compute environment now properly works with compute resources and instanceRole has correct docstring and type definition #6549

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f05b60e
fix(batch): fixing 'managed' compute environment and some other bugs;…
andrestone Mar 2, 2020
d9c7f42
Merge remote-tracking branch 'upstream/master' into patch-aws-batch
andrestone Mar 2, 2020
cea5eb1
fix(batch): fixing build / tests
andrestone Mar 3, 2020
2574965
Merge branch 'master' into patch-aws-batch
andrestone Mar 3, 2020
11667bf
fix(batch): changes required by reviewer
andrestone Mar 5, 2020
bb4fb53
Merge remote-tracking branch 'upstream/master' into patch-aws-batch
andrestone Mar 5, 2020
f9d5938
Merge remote-tracking branch 'origin/patch-aws-batch' into patch-aws-…
andrestone Mar 5, 2020
69b0d7f
fix(batch): adjusting tests because we're taking the launchtemplate s…
andrestone Mar 5, 2020
c8514ac
fix(batch): updating expected.json
andrestone Mar 5, 2020
472053d
Merge branch 'master' into patch-aws-batch
mergify[bot] Mar 6, 2020
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
93 changes: 55 additions & 38 deletions packages/@aws-cdk/aws-batch/lib/compute-environment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,33 @@ export enum AllocationStrategy {
*/
export interface ComputeResources {
/**
* The IAM role applied to EC2 resources in the compute environment.
* The allocation strategy to use for the compute resource in case not enough instances of the best
* fitting instance type can be allocated. This could be due to availability of the instance type in
* the region or Amazon EC2 service limits. If this is not specified, the default for the EC2
* ComputeResourceType is BEST_FIT, which will use only the best fitting instance type, waiting for
* additional capacity if it's not available. This allocation strategy keeps costs lower but can limit
* scaling. If you are using Spot Fleets with BEST_FIT then the Spot Fleet IAM Role must be specified.
* BEST_FIT_PROGRESSIVE will select an additional instance type that is large enough to meet the
* requirements of the jobs in the queue, with a preference for an instance type with a lower cost.
* The default value for the SPOT instance type is SPOT_CAPACITY_OPTIMIZED, which is only available for
* for this type of compute resources and will select an additional instance type that is large enough
* to meet the requirements of the jobs in the queue, with a preference for an instance type that is
* less likely to be interrupted.
*
* @default AllocationStrategy.BEST_FIT
*/
readonly allocationStrategy?: AllocationStrategy;
iliapolo marked this conversation as resolved.
Show resolved Hide resolved

/**
* The Amazon ECS instance profile applied to Amazon EC2 instances in a compute environment. You can specify
* the short name or full Amazon Resource Name (ARN) of an instance profile. For example, ecsInstanceRole or
* arn:aws:iam::<aws_account_id>:instance-profile/ecsInstanceRole . For more information, see Amazon ECS
* Instance Role in the AWS Batch User Guide.
*
* @default - a new role will be created.
* @link https://docs.aws.amazon.com/batch/latest/userguide/instance_IAM_role.html
*/
readonly instanceRole?: iam.IRole;
readonly instanceRole?: string;
iliapolo marked this conversation as resolved.
Show resolved Hide resolved

/**
* The types of EC2 instances that may be launched in the compute environment. You can specify instance
Expand All @@ -70,7 +92,7 @@ export interface ComputeResources {
/**
* The EC2 security group(s) associated with instances launched in the compute environment.
*
* @default AWS default security group.
* @default - AWS default security group.
*/
readonly securityGroups?: ec2.ISecurityGroup[];

Expand Down Expand Up @@ -133,7 +155,7 @@ export interface ComputeResources {
* The EC2 key pair that is used for instances launched in the compute environment.
* If no key is defined, then SSH access is not allowed to provisioned compute resources.
*
* @default - No SSH access will be possible.
* @default - no SSH access will be possible.
*/
readonly ec2KeyPair?: string;

Expand Down Expand Up @@ -169,40 +191,25 @@ export interface ComputeResources {
* Properties for creating a new Compute Environment
*/
export interface ComputeEnvironmentProps {
/**
* The allocation strategy to use for the compute resource in case not enough instances ofthe best
* fitting instance type can be allocated. This could be due to availability of the instance type in
* the region or Amazon EC2 service limits. If this is not specified, the default is BEST_FIT, which
* will use only the best fitting instance type, waiting for additional capacity if it's not available.
* This allocation strategy keeps costs lower but can limit scaling. If you are using Spot Fleets with
* BEST_FIT then the Spot Fleet IAM Role must be specified. BEST_FIT_PROGRESSIVE will select an additional
* instance type that is large enough to meet the requirements of the jobs in the queue, with a preference
* for an instance type with a lower cost. SPOT_CAPACITY_OPTIMIZED is only available for Spot Instance
* compute resources and will select an additional instance type that is large enough to meet the requirements
* of the jobs in the queue, with a preference for an instance type that is less likely to be interrupted.
*
* @default AllocationStrategy.BEST_FIT
*/
readonly allocationStrategy?: AllocationStrategy;
iliapolo marked this conversation as resolved.
Show resolved Hide resolved

/**
* A name for the compute environment.
*
* Up to 128 letters (uppercase and lowercase), numbers, hyphens, and underscores are allowed.
*
* @default Cloudformation-generated name
* @default - CloudFormation-generated name
*/
readonly computeEnvironmentName?: string;

/**
* The details of the compute resources managed by this environment.
* The details of the required compute resources for the managed compute environment.
iliapolo marked this conversation as resolved.
Show resolved Hide resolved
*
* If specified, and this is an managed compute environment, the property will be ignored.
* If specified, and this is an unmanaged compute environment, will throw an error.
*
* By default, AWS Batch managed compute environments use a recent, approved version of the
* Amazon ECS-optimized AMI for compute resources.
*
* @default - AWS-managed compute resources
* @default - CloudFormation defaults
* @link https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-batch-computeenvironment-computeresources.html
*/
readonly computeResources?: ComputeResources;

Expand Down Expand Up @@ -302,19 +309,29 @@ export class ComputeEnvironment extends Resource implements IComputeEnvironment
const spotFleetRole = this.getSpotFleetRole(props);
let computeResources: CfnComputeEnvironment.ComputeResourcesProperty | undefined;

// Only allow compute resources to be set when using UNMANAGED type
if (props.computeResources && !this.isManaged(props)) {
// Only allow compute resources to be set when using MANAGED type
if (props.computeResources && this.isManaged(props)) {
computeResources = {
allocationStrategy: props.allocationStrategy || AllocationStrategy.BEST_FIT,
allocationStrategy: props.computeResources.allocationStrategy
|| (
props.computeResources.type === ComputeResourceType.SPOT
? AllocationStrategy.SPOT_CAPACITY_OPTIMIZED
: AllocationStrategy.BEST_FIT
),
bidPercentage: props.computeResources.bidPercentage,
desiredvCpus: props.computeResources.desiredvCpus,
ec2KeyPair: props.computeResources.ec2KeyPair,
imageId: props.computeResources.image && props.computeResources.image.getImage(this).imageId,
instanceRole: props.computeResources.instanceRole
? props.computeResources.instanceRole.roleArn
: new iam.Role(this, 'Resource-Instance-Role', {
assumedBy: new iam.ServicePrincipal('batch.amazonaws.com'),
}).roleArn,
? props.computeResources.instanceRole
: new iam.CfnInstanceProfile(this, 'Instance-Profile', {
iliapolo marked this conversation as resolved.
Show resolved Hide resolved
roles: [ new iam.Role(this, 'Ecs-Instance-Role', {
assumedBy: new iam.ServicePrincipal('ec2.amazonaws.com'),
managedPolicies: [
iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AmazonEC2ContainerServiceforEC2Role')
]
}).roleName]
}).attrArn,
instanceTypes: this.buildInstanceTypes(props.computeResources.instanceTypes),
maxvCpus: props.computeResources.maxvCpus || 256,
minvCpus: props.computeResources.minvCpus || 0,
Expand All @@ -338,7 +355,7 @@ export class ComputeEnvironment extends Resource implements IComputeEnvironment
assumedBy: new iam.ServicePrincipal('batch.amazonaws.com'),
}).roleArn,
state: props.enabled === undefined ? 'ENABLED' : (props.enabled ? 'ENABLED' : 'DISABLED'),
type: this.isManaged(props) ? 'UNMANAGED' : 'MANAGED',
type: this.isManaged(props) ? 'MANAGED' : 'UNMANAGED',
});

if (props.computeResources && props.computeResources.vpc) {
Expand All @@ -365,12 +382,12 @@ export class ComputeEnvironment extends Resource implements IComputeEnvironment
return;
}

if (this.isManaged(props) && props.computeResources !== undefined) {
throw new Error('It is not allowed to set computeResources on an AWS managed compute environment');
if (!this.isManaged(props) && props.computeResources !== undefined) {
throw new Error('It is not allowed to set computeResources on an AWS unmanaged compute environment');
}

if (!this.isManaged(props) && props.computeResources === undefined) {
throw new Error('computeResources is missing but required on an unmanaged compute environment');
if (this.isManaged(props) && props.computeResources === undefined) {
throw new Error('computeResources is missing but required on a managed compute environment');
}

// Setting a bid percentage is only allowed on SPOT resources +
Expand All @@ -385,7 +402,7 @@ export class ComputeEnvironment extends Resource implements IComputeEnvironment
}

// SPOT_CAPACITY_OPTIMIZED allocation is not allowed
if (props.allocationStrategy && props.allocationStrategy === AllocationStrategy.SPOT_CAPACITY_OPTIMIZED) {
if (props.computeResources.allocationStrategy && props.computeResources.allocationStrategy === AllocationStrategy.SPOT_CAPACITY_OPTIMIZED) {
throw new Error('The SPOT_CAPACITY_OPTIMIZED allocation strategy is only allowed if the environment is a SPOT type compute environment');
}
} else {
Expand Down Expand Up @@ -440,7 +457,7 @@ export class ComputeEnvironment extends Resource implements IComputeEnvironment
* @param props - the compute environment construct properties
*/
private getSpotFleetRole(props: ComputeEnvironmentProps): iam.IRole | undefined {
if (props.allocationStrategy && props.allocationStrategy !== AllocationStrategy.BEST_FIT) {
if (props.computeResources?.allocationStrategy && props.computeResources.allocationStrategy !== AllocationStrategy.BEST_FIT) {
return undefined;
}

Expand Down
57 changes: 32 additions & 25 deletions packages/@aws-cdk/aws-batch/test/compute-environment.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { expect, haveResource, haveResourceLike, ResourcePart } from '@aws-cdk/assert';
import {expect, haveResource, haveResourceLike, ResourcePart} from '@aws-cdk/assert';
import '@aws-cdk/assert/jest';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as ecs from '@aws-cdk/aws-ecs';
import * as iam from '@aws-cdk/aws-iam';
import * as cdk from '@aws-cdk/core';
import { throws } from 'assert';
import * as batch from '../lib';
import {throws} from 'assert';
import * as batch from "../lib";

describe('Batch Compute Evironment', () => {
let expectedManagedDefaultComputeProps: any;
Expand Down Expand Up @@ -34,7 +34,7 @@ describe('Batch Compute Evironment', () => {
AllocationStrategy: batch.AllocationStrategy.BEST_FIT,
InstanceRole: {
'Fn::GetAtt': [
'testcomputeenvResourceInstanceRole7FD819B9',
'testcomputeenvInstanceProfileCBD87EAB',
'Arn'
]
},
Expand All @@ -59,37 +59,39 @@ describe('Batch Compute Evironment', () => {
});

describe('when validating props', () => {
test('should deny setting compute resources when using type managed', () => {
test('should deny setting compute resources when using type unmanaged', () => {
// THEN
throws(() => {
// WHEN
new batch.ComputeEnvironment(stack, 'test-compute-env', {
managed: false,
computeResources: {
vpc,
vpc
},
});
});
});

test('should deny if creating an unmanged environment with no provided compute resource props', () => {
test('should deny if creating a managed environment with no provided compute resource props', () => {
// THEN
throws(() => {
// WHEN
new batch.ComputeEnvironment(stack, 'test-compute-env', {
managed: false,
managed: true,
});
});
});
});

describe('using spot resources', () => {
test('should provide a spotfleet role if one is not given', () => {
test('should provide a spot fleet role if one is not given and allocationStrategy is BEST_FIT', () => {
// WHEN
new batch.ComputeEnvironment(stack, 'test-compute-env', {
managed: false,
managed: true,
computeResources: {
type: batch.ComputeResourceType.SPOT,
vpc,
allocationStrategy: batch.AllocationStrategy.BEST_FIT,
vpc
},
});

Expand Down Expand Up @@ -124,7 +126,7 @@ describe('Batch Compute Evironment', () => {
throws(() => {
// WHEN
new batch.ComputeEnvironment(stack, 'test-compute-env', {
managed: false,
managed: true,
computeResources: {
vpc,
type: batch.ComputeResourceType.SPOT,
Expand All @@ -139,7 +141,7 @@ describe('Batch Compute Evironment', () => {
throws(() => {
// WHEN
new batch.ComputeEnvironment(stack, 'test-compute-env', {
managed: false,
managed: true,
computeResources: {
vpc,
type: batch.ComputeResourceType.SPOT,
Expand All @@ -155,9 +157,9 @@ describe('Batch Compute Evironment', () => {
test('renders the correct cloudformation properties', () => {
// WHEN
const props = {
allocationStrategy: batch.AllocationStrategy.BEST_FIT,
computeEnvironmentName: 'my-test-compute-env',
computeResources: {
allocationStrategy: batch.AllocationStrategy.BEST_FIT,
vpc,
computeResourcesTags: new cdk.Tag('foo', 'bar'),
desiredvCpus: 1,
Expand All @@ -166,9 +168,14 @@ describe('Batch Compute Evironment', () => {
generation: ec2.AmazonLinuxGeneration.AMAZON_LINUX_2,
hardwareType: ecs.AmiHardwareType.STANDARD,
}),
instanceRole: new iam.Role(stack, 'test-compute-env-instance-role', {
assumedBy: new iam.ServicePrincipal('batch.amazonaws.com'),
}),
instanceRole: new iam.CfnInstanceProfile(stack, 'Instance-Profile', {
roles: [ new iam.Role(stack, 'Ecs-Instance-Role', {
assumedBy: new iam.ServicePrincipal('ec2.amazonaws.com'),
managedPolicies: [
iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AmazonEC2ContainerServiceforEC2Role')
]
}).roleName]
}).attrArn,
instanceTypes: [
ec2.InstanceType.of(ec2.InstanceClass.T2, ec2.InstanceSize.MICRO),
],
Expand All @@ -186,7 +193,7 @@ describe('Batch Compute Evironment', () => {
},
} as batch.ComputeResources,
enabled: false,
managed: false,
managed: true,
};

new batch.ComputeEnvironment(stack, 'test-compute-env', props);
Expand All @@ -211,7 +218,7 @@ describe('Batch Compute Evironment', () => {
},
InstanceRole: {
'Fn::GetAtt': [
props.computeResources.instanceRole ? `${props.computeResources.instanceRole.node.uniqueId}F3B86D94` : '',
props.computeResources.instanceRole ? "InstanceProfile" : '',
'Arn'
]
},
Expand Down Expand Up @@ -251,7 +258,7 @@ describe('Batch Compute Evironment', () => {
test('should default to a best_fit strategy', () => {
// WHEN
new batch.ComputeEnvironment(stack, 'test-compute-env', {
managed: false,
managed: true,
computeResources: {
vpc,
},
Expand Down Expand Up @@ -303,7 +310,7 @@ describe('Batch Compute Evironment', () => {
test('should default to 0', () => {
// WHEN
new batch.ComputeEnvironment(stack, 'test-compute-env', {
managed: false,
managed: true,
computeResources: {
vpc,
},
Expand All @@ -323,7 +330,7 @@ describe('Batch Compute Evironment', () => {
test('should default to 256', () => {
// WHEN
new batch.ComputeEnvironment(stack, 'test-compute-env', {
managed: false,
managed: true,
computeResources: {
vpc,
},
Expand All @@ -342,7 +349,7 @@ describe('Batch Compute Evironment', () => {
test('should generate a role for me', () => {
// WHEN
new batch.ComputeEnvironment(stack, 'test-compute-env', {
managed: false,
managed: true,
computeResources: {
vpc,
},
Expand All @@ -358,7 +365,7 @@ describe('Batch Compute Evironment', () => {
test('should default to optimal matching', () => {
// WHEN
new batch.ComputeEnvironment(stack, 'test-compute-env', {
managed: false,
managed: true,
computeResources: {
vpc,
},
Expand All @@ -377,7 +384,7 @@ describe('Batch Compute Evironment', () => {
test('should default to EC2', () => {
// WHEN
new batch.ComputeEnvironment(stack, 'test-compute-env', {
managed: false,
managed: true,
computeResources: {
vpc,
},
Expand Down
Loading