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

Jump to bottom

feat(ecs): ECS optimized Windows images #3376

Merged
merged 15 commits into from
Jul 25, 2019
Merged

feat(ecs): ECS optimized Windows images #3376

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
eb63ce4
feat(ecs): support windows images
Jul 22, 2019
48780fb
fix(ecs): throw if both windowsVersion and generation props are set
Jul 23, 2019
47e81a6
fix(ecs): remove linux mention in windows hardware error
Jul 23, 2019
e3e5739
fix(ecs): fix windowsVersion assignation, update test
Jul 23, 2019
5c01a39
fix(ecs): windowsVersion test deepEqual
Jul 23, 2019
b1f003b
fix(ecs): test linux ami ssm parameter
Jul 23, 2019
a95a33f
Merge branch 'master' into feat-windows-ecs
Jul 23, 2019
f2a8e1d
Merge branch 'master' into feat-windows-ecs
Jul 24, 2019
0309b87
fix(ecs): remove duplicate core import
Jul 24, 2019
9bc06bf
fix(ecs): tslint
Jul 24, 2019
12d99eb
feat(ecs): EcsOptimizedAmi refactor, use static constructors
Jul 24, 2019
450b507
fix(ecs): remove unused interfaces
Jul 24, 2019
42bfb0b
fix(ecs): remove duplicate import
Jul 25, 2019
3cbeb5f
fix(ecs): fix property ordering
Jul 25, 2019
19e874d
fix(ecs): refactor EcsOptimizedImage
Jul 25, 2019
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
26 changes: 22 additions & 4 deletions packages/@aws-cdk/aws-ecs/lib/cluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,11 @@ export class Cluster extends Resource implements ICluster {
}
}

export enum WindowsOptimizedVersion {
SERVER_2019 = '2019',
SERVER_2016 = '2016',
}

/**
* The properties that define which ECS-optimized AMI is used.
*/
Expand All @@ -242,6 +247,13 @@ export interface EcsOptimizedAmiProps {
*/
readonly generation?: ec2.AmazonLinuxGeneration;

/**
* The Windows Server version to use.
*
* @default none, uses Linux generation
*/
readonly windowsVersion?: WindowsOptimizedVersion;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, the generation and windowsVersion properties would be merged, but I didn't think it was worth the refactoring and breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can @deprecate generation in favor of amazonLinuxGeneration (still support generation for backwards compat and in the next MV we will remove)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts? Maybe in a subsequent PR? At least update #3398 with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I didn't see your previous comment. I can add it to this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't we just replace it with an image union construct, to have the definition enforce the mutual exclusion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes!

Copy link
Contributor

Choose a reason for hiding this comment

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

So what's up with this? Didn't we say this is gone?


/**
* The ECS-optimized AMI variant to use.
*
Expand All @@ -251,10 +263,11 @@ export interface EcsOptimizedAmiProps {
}

/**
* Construct a Linux machine image from the latest ECS Optimized AMI published in SSM
* Construct a Linux or Windows machine image from the latest ECS Optimized AMI published in SSM
*/
export class EcsOptimizedAmi implements ec2.IMachineImage {
private readonly generation: ec2.AmazonLinuxGeneration;
private readonly generation?: ec2.AmazonLinuxGeneration;
private readonly windowsVersion?: WindowsOptimizedVersion;
private readonly hwType: AmiHardwareType;

private readonly amiParameterName: string;
Expand All @@ -270,6 +283,10 @@ export class EcsOptimizedAmi implements ec2.IMachineImage {
} else {
this.generation = props.generation;
}
} else if (props && props.windowsVersion) {
if (this.hwType !== AmiHardwareType.STANDARD) {
throw new Error('Windows Server does not support special hardware type. Use Amazon Linux 2 instead');
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove "use Amazon Linux 2 instead". I am assuming that if a user explicitly specified they wanted Windows, they should probably just not specify the HW type and not use Linux instead... :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
} else { // generation not defined in props object
// always default to Amazon Linux v2 regardless of HW
this.generation = ec2.AmazonLinuxGeneration.AMAZON_LINUX_2;
Expand All @@ -279,6 +296,7 @@ export class EcsOptimizedAmi implements ec2.IMachineImage {
this.amiParameterName = "/aws/service/ecs/optimized-ami/"
+ ( this.generation === ec2.AmazonLinuxGeneration.AMAZON_LINUX ? "amazon-linux/" : "" )
+ ( this.generation === ec2.AmazonLinuxGeneration.AMAZON_LINUX_2 ? "amazon-linux-2/" : "" )
+ ( this.windowsVersion ? `windows_server/${this.windowsVersion}/english/full/` : "" )
+ ( this.hwType === AmiHardwareType.GPU ? "gpu/" : "" )
+ ( this.hwType === AmiHardwareType.ARM ? "arm64/" : "" )
+ "recommended/image_id";
Expand All @@ -291,7 +309,7 @@ export class EcsOptimizedAmi implements ec2.IMachineImage {
const ami = ssm.StringParameter.valueForStringParameter(scope, this.amiParameterName);
return {
imageId: ami,
osType: ec2.OperatingSystemType.LINUX
osType: this.windowsVersion ? ec2.OperatingSystemType.WINDOWS : ec2.OperatingSystemType.LINUX
};
}
}
Expand Down Expand Up @@ -510,7 +528,7 @@ export interface CloudMapNamespaceOptions {
export enum AmiHardwareType {

/**
* Use the Amazon ECS-optimized Amazon Linux 2 AMI.
* Use the standard Amazon ECS-optimized AMI.
*/
STANDARD = 'Standard',

Expand Down
44 changes: 44 additions & 0 deletions packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,8 +274,52 @@ export = {
hardwareType: ecs.AmiHardwareType.GPU,
}),
});
}, /Amazon Linux does not support special hardware type/);

test.done();
},

"allows specifying windows image"(test: Test) {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'MyVpc', {});

const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
cluster.addCapacity('WindowsAutoScalingGroup', {
instanceType: new ec2.InstanceType('t2.micro'),
machineImage: new ecs.EcsOptimizedAmi({
windowsVersion: ecs.WindowsOptimizedVersion.SERVER_2019,
}),
});

// THEN
expect(stack).to(haveResource("AWS::AutoScaling::LaunchConfiguration", {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add an expectation on the parameter with the desired SSM path...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't understand your comment. I reused the existing HW AMI Type test:

aws-cdk/packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts

Lines 252 to 256 in 47e81a6

expect(stack).to(haveResource("AWS::AutoScaling::LaunchConfiguration", {
ImageId: {
Ref: "SsmParameterValueawsserviceecsoptimizedamiamazonlinux2gpurecommendedimageidC96584B6F00A464EAD1953AFF4B05118Parameter"
}
}));

Copy link
Contributor

Choose a reason for hiding this comment

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

This just shows the ImageId is a reference to a synthesized CloudFormation parameter that is added to your template. You should write your expectation against the default value of the parameter which will contain the SSM parameter name that you want.

What I usually do is expect(stack).toMatch({}) and then I can see which parts of the stack to assert against.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I was missing an assignation, thanks.

I have an issue with the matching now. I tried changing the MatchStyle, but it doesn't seem to detect when I purposefully change the Default matched value.

Should I use an integration test instead, to match the whole stack?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think there's a bug in the matcher (raise an issue?). What you could do in the meantime is as follows;

// you'll need an "App"
const app = new App();
const stack = new Stack(app, 'test');

// ...

// then
const assembly = app.synth();
const template = assembly.getStack(stack.stackName).template;
test.deepEqual(template.Parameters, { /* expectation here */ });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, thanks a lot. Should I also update the existing Linux AMI test?

aws-cdk/packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts

Lines 253 to 257 in 5c01a39

expect(stack).to(haveResource("AWS::AutoScaling::LaunchConfiguration", {
ImageId: {
Ref: "SsmParameterValueawsserviceecsoptimizedamiamazonlinux2gpurecommendedimageidC96584B6F00A464EAD1953AFF4B05118Parameter"
}
}));

If so, should I do it in another PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to add to this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks again for the help!

ImageId: {
Ref: "SsmParameterValueawsserviceecsoptimizedamirecommendedimageidC96584B6F00A464EAD1953AFF4B05118Parameter"
}
}));

test.done();
},

"errors if windows given with special HW type"(test: Test) {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'MyVpc', {});

const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });

// THEN
test.throws(() => {
cluster.addCapacity('WindowsGpuAutoScalingGroup', {
instanceType: new ec2.InstanceType('t2.micro'),
machineImage: new ecs.EcsOptimizedAmi({
windowsVersion: ecs.WindowsOptimizedVersion.SERVER_2019,
hardwareType: ecs.AmiHardwareType.GPU,
}),
});
}, /Windows Server does not support special hardware type/);

test.done();
},

Expand Down