From 196ac092777d12f6572a1885f61db65f7a7dd12b Mon Sep 17 00:00:00 2001 From: Chen Date: Thu, 19 Sep 2024 18:14:37 -0700 Subject: [PATCH 01/11] initial working solution to bug --- .../lib/managed-compute-environment.ts | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts b/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts index c9f03e9f0dd73..3bfacfc79f6fb 100644 --- a/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts +++ b/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts @@ -5,7 +5,7 @@ import * as ec2 from '../../aws-ec2'; import * as eks from '../../aws-eks'; import * as iam from '../../aws-iam'; import { IRole } from '../../aws-iam'; -import { ArnFormat, Duration, ITaggable, Lazy, Resource, Stack, TagManager, TagType } from '../../core'; +import { Annotations, ArnFormat, Duration, ITaggable, Lazy, Resource, Stack, TagManager, TagType } from '../../core'; /** * Represents a Managed ComputeEnvironment. Batch will provision EC2 Instances to @@ -1063,10 +1063,12 @@ export class ManagedEc2EksComputeEnvironment extends ManagedComputeEnvironmentBa } public addInstanceType(instanceType: ec2.InstanceType): void { + // TODO: implement check if it is arm this.instanceTypes.push(instanceType); } public addInstanceClass(instanceClass: ec2.InstanceClass): void { + // TODO: implement check if it is arm this.instanceClasses.push(instanceClass); } } @@ -1133,13 +1135,37 @@ export class FargateComputeEnvironment extends ManagedComputeEnvironmentBase imp function renderInstances(types?: ec2.InstanceType[], classes?: ec2.InstanceClass[], useOptimalInstanceClasses?: boolean): string[] { const instances = []; - + let hasArmInstances = false; + let hasX86Instances = false; + for (const instanceType of types ?? []) { instances.push(instanceType.toString()); + if (instanceType.architecture === ec2.InstanceArchitecture.ARM_64) { + hasArmInstances = true; + } else { + hasX86Instances = true; + } } + for (const instanceClass of classes ?? []) { instances.push(instanceClass); + const instanceType = new ec2.InstanceType(`${instanceClass.toString()}.large`); + if (instanceType.architecture === ec2.InstanceArchitecture.ARM_64) { + hasArmInstances = true; + } else { + hasX86Instances = true; + } + } + + if (hasArmInstances && hasX86Instances) { + throw new Error('Cannot mix ARM and x86 instance types or classes'); + } + + if (hasArmInstances && (useOptimalInstanceClasses || useOptimalInstanceClasses === undefined)) { + console.warn('Warning: "optimal" instance types are not supported with ARM instance classes. Setting useOptimalInstanceClasses to false'); + useOptimalInstanceClasses = false; } + if (useOptimalInstanceClasses || useOptimalInstanceClasses === undefined) { instances.push('optimal'); } From c195f1b185792f6d8e81345d3bd89fe2a325ccb8 Mon Sep 17 00:00:00 2001 From: Chen Date: Thu, 19 Sep 2024 18:16:15 -0700 Subject: [PATCH 02/11] add warning from annotations library --- .../lib/managed-compute-environment.ts | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts b/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts index 3bfacfc79f6fb..5b670a969b6d6 100644 --- a/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts +++ b/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts @@ -685,7 +685,7 @@ export class ManagedEc2EcsComputeEnvironment extends ManagedComputeEnvironmentBa minvCpus: this.minvCpus, instanceRole: this.instanceProfile.attrArn, // this is not a typo; this property actually takes a profile, not a standard role instanceTypes: Lazy.list({ - produce: () => renderInstances(this.instanceTypes, this.instanceClasses, props.useOptimalInstanceClasses), + produce: () => renderInstances(this, this.instanceTypes, this.instanceClasses, props.useOptimalInstanceClasses), }), type: this.spot ? 'SPOT' : 'EC2', spotIamFleetRole: this.spotFleetRole?.roleArn, @@ -712,14 +712,18 @@ export class ManagedEc2EcsComputeEnvironment extends ManagedComputeEnvironmentBa resourceName: this.physicalName, }); - this.node.addValidation({ validate: () => validateInstances(this.instanceTypes, this.instanceClasses, props.useOptimalInstanceClasses) }); + this.node.addValidation({ + validate: () => validateInstances.call(this, this.instanceTypes, this.instanceClasses, props.useOptimalInstanceClasses), + }); } public addInstanceType(instanceType: ec2.InstanceType): void { + // TODO add check this.instanceTypes.push(instanceType); } public addInstanceClass(instanceClass: ec2.InstanceClass): void { + // TODO add check this.instanceClasses.push(instanceClass); } } @@ -1034,7 +1038,7 @@ export class ManagedEc2EksComputeEnvironment extends ManagedComputeEnvironmentBa ...baseManagedResourceProperties(this, subnetIds).computeResources as CfnComputeEnvironment.ComputeResourcesProperty, minvCpus: this.minvCpus, instanceRole: this.instanceProfile.attrArn, // this is not a typo; this property actually takes a profile, not a standard role - instanceTypes: Lazy.list({ produce: () => renderInstances(this.instanceTypes, this.instanceClasses, props.useOptimalInstanceClasses) }), + instanceTypes: Lazy.list({ produce: () => renderInstances(this, this.instanceTypes, this.instanceClasses, props.useOptimalInstanceClasses) }), type: this.spot ? 'SPOT' : 'EC2', allocationStrategy: this.allocationStrategy, bidPercentage: this.spotBidPercentage, @@ -1059,7 +1063,9 @@ export class ManagedEc2EksComputeEnvironment extends ManagedComputeEnvironmentBa resourceName: this.physicalName, }); - this.node.addValidation({ validate: () => validateInstances(this.instanceTypes, this.instanceClasses, props.useOptimalInstanceClasses) }); + this.node.addValidation({ + validate: () => validateInstances.call(this, this.instanceTypes, this.instanceClasses, props.useOptimalInstanceClasses), + }); } public addInstanceType(instanceType: ec2.InstanceType): void { @@ -1133,7 +1139,7 @@ export class FargateComputeEnvironment extends ManagedComputeEnvironmentBase imp } } -function renderInstances(types?: ec2.InstanceType[], classes?: ec2.InstanceClass[], useOptimalInstanceClasses?: boolean): string[] { +function renderInstances(scope: Construct, types?: ec2.InstanceType[], classes?: ec2.InstanceClass[], useOptimalInstanceClasses?: boolean): string[] { const instances = []; let hasArmInstances = false; let hasX86Instances = false; @@ -1162,7 +1168,7 @@ function renderInstances(types?: ec2.InstanceType[], classes?: ec2.InstanceClass } if (hasArmInstances && (useOptimalInstanceClasses || useOptimalInstanceClasses === undefined)) { - console.warn('Warning: "optimal" instance types are not supported with ARM instance classes. Setting useOptimalInstanceClasses to false'); + Annotations.of(scope).addWarningV2('@aws-cdk/aws-batch:optimalNotSupportedWithARM', '\'optimal\' instance types are not supported with ARM instance types or classes, setting useOptimalInstanceClasses to false'); useOptimalInstanceClasses = false; } @@ -1207,8 +1213,8 @@ function determineAllocationStrategy(id: string, allocationStrategy?: Allocation return result; } -function validateInstances(types?: ec2.InstanceType[], classes?: ec2.InstanceClass[], useOptimalInstanceClasses?: boolean): string[] { - if (renderInstances(types, classes, useOptimalInstanceClasses).length === 0) { +function validateInstances(this: Construct, types?: ec2.InstanceType[], classes?: ec2.InstanceClass[], useOptimalInstanceClasses?: boolean): string[] { + if (renderInstances(this, types, classes, useOptimalInstanceClasses).length === 0) { return ["Specifies 'useOptimalInstanceClasses: false' without specifying any instance types or classes"]; } From 114e8353839cf5c452f4e3adf7c67adff025e93d Mon Sep 17 00:00:00 2001 From: Chen Date: Fri, 20 Sep 2024 14:33:18 -0700 Subject: [PATCH 03/11] addInstanceType and addInstanceClass architecture compatibility check --- .../lib/managed-compute-environment.ts | 36 +++++++++++++++---- 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts b/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts index 5b670a969b6d6..bb37bca63550e 100644 --- a/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts +++ b/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts @@ -718,12 +718,13 @@ export class ManagedEc2EcsComputeEnvironment extends ManagedComputeEnvironmentBa } public addInstanceType(instanceType: ec2.InstanceType): void { - // TODO add check + checkArchitectureConsistency(instanceType, this.instanceTypes, this.instanceClasses); this.instanceTypes.push(instanceType); } public addInstanceClass(instanceClass: ec2.InstanceClass): void { - // TODO add check + const tempInstanceType = new ec2.InstanceType(`${instanceClass.toString()}.large`); + checkArchitectureConsistency(tempInstanceType, this.instanceTypes, this.instanceClasses); this.instanceClasses.push(instanceClass); } } @@ -1069,12 +1070,13 @@ export class ManagedEc2EksComputeEnvironment extends ManagedComputeEnvironmentBa } public addInstanceType(instanceType: ec2.InstanceType): void { - // TODO: implement check if it is arm + checkArchitectureConsistency(instanceType, this.instanceTypes, this.instanceClasses); this.instanceTypes.push(instanceType); } public addInstanceClass(instanceClass: ec2.InstanceClass): void { - // TODO: implement check if it is arm + const tempInstanceType = new ec2.InstanceType(`${instanceClass.toString()}.large`); + checkArchitectureConsistency(tempInstanceType, this.instanceTypes, this.instanceClasses); this.instanceClasses.push(instanceClass); } } @@ -1139,6 +1141,28 @@ export class FargateComputeEnvironment extends ManagedComputeEnvironmentBase imp } } +function checkArchitectureConsistency( + newInstanceType: ec2.InstanceType, + existingTypes: ec2.InstanceType[], + existingClasses: ec2.InstanceClass[] +): void { + const newArchitecture = newInstanceType.architecture; + + for (const existingType of existingTypes ?? []) { + if (existingType.architecture !== newArchitecture) { + throw new Error(`Cannot add instance type ${newInstanceType.toString()} with architecture ${newArchitecture}. It conflicts with existing instance type ${existingType.toString()} with architecture ${existingType.architecture}.`); + } + } + + for (const existingClass of existingClasses ?? []) { + const tempInstanceType = new ec2.InstanceType(`${existingClass.toString()}.large`); + const existingClassArchitecture = tempInstanceType.architecture; + if (existingClassArchitecture !== newArchitecture) { + throw new Error(`Cannot add instance type ${newInstanceType.toString()} with architecture ${newArchitecture}. It conflicts with existing instance class ${existingClass} with architecture ${existingClassArchitecture}.`); + } + } +} + function renderInstances(scope: Construct, types?: ec2.InstanceType[], classes?: ec2.InstanceClass[], useOptimalInstanceClasses?: boolean): string[] { const instances = []; let hasArmInstances = false; @@ -1155,8 +1179,8 @@ function renderInstances(scope: Construct, types?: ec2.InstanceType[], classes?: for (const instanceClass of classes ?? []) { instances.push(instanceClass); - const instanceType = new ec2.InstanceType(`${instanceClass.toString()}.large`); - if (instanceType.architecture === ec2.InstanceArchitecture.ARM_64) { + const tempInstanceType = new ec2.InstanceType(`${instanceClass.toString()}.large`); + if (tempInstanceType.architecture === ec2.InstanceArchitecture.ARM_64) { hasArmInstances = true; } else { hasX86Instances = true; From 7184db8cbf3cc97c3332ec1744dfd3d09894dde8 Mon Sep 17 00:00:00 2001 From: Chen Date: Fri, 20 Sep 2024 14:36:30 -0700 Subject: [PATCH 04/11] test cases for error and warning messages when there is mixed architecture --- .../test/managed-compute-environment.test.ts | 75 ++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts b/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts index b2d572fd8de37..f1953202f5ff9 100644 --- a/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts +++ b/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts @@ -1,5 +1,5 @@ import { capitalizePropertyNames } from './utils'; -import { Template } from '../../assertions'; +import { Annotations, Template } from '../../assertions'; import * as ec2 from '../../aws-ec2'; import * as eks from '../../aws-eks'; import { ArnPrincipal, Role, ServicePrincipal } from '../../aws-iam'; @@ -328,6 +328,79 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] }); }); + test('warning when optimal instance classes are used with ARM instances', () => { + // WHEN + const ce = new ComputeEnvironment(stack, 'MyCE', { + ...defaultProps, + vpc, + instanceClasses: [ec2.InstanceClass.A1], + instanceTypes: [ + ec2.InstanceType.of(ec2.InstanceClass.STANDARD6_GRAVITON, ec2.InstanceSize.LARGE) + ], + useOptimalInstanceClasses: true, + }); + + // THEN + Annotations.fromStack(stack).hasWarning('*', + '\'optimal\' instance types are not supported with ARM instance types or classes, setting useOptimalInstanceClasses to false [ack: @aws-cdk/aws-batch:optimalNotSupportedWithARM]' + ); + }); + + test('mixing ARM and x86 instance classes should throw an error', () => { + // THEN + const CE = new ComputeEnvironment(stack, 'MyCE', { + ...defaultProps, + vpc, + instanceClasses: [ec2.InstanceClass.M6G, ec2.InstanceClass.C4], + }); + + expect(() => { + Template.fromStack(stack).hasResourceProperties('AWS::Batch::ComputeEnvironment', { + ...expectedProps, + ComputeResources: { + ...defaultComputeResources, + }, + }); + }).toThrow(/Cannot mix ARM and x86 instance types or classes/) + }); + + test('mixing ARM and x86 instance types should throw an error', () => { + // THEN + const CE = new ComputeEnvironment(stack, 'MyCE', { + ...defaultProps, + vpc, + instanceTypes: [ec2.InstanceType.of(ec2.InstanceClass.STANDARD6_GRAVITON, ec2.InstanceSize.LARGE), ec2.InstanceType.of(ec2.InstanceClass.R4, ec2.InstanceSize.LARGE)], + }); + + expect(() => { + Template.fromStack(stack).hasResourceProperties('AWS::Batch::ComputeEnvironment', { + ...expectedProps, + ComputeResources: { + ...defaultComputeResources, + }, + }); + }).toThrow(/Cannot mix ARM and x86 instance types or classes/) + }); + + test('mixing ARM and x86 instance classes and types should throw an error', () => { + // THEN + const CE = new ComputeEnvironment(stack, 'MyCE', { + ...defaultProps, + vpc, + instanceClasses: [ec2.InstanceClass.A1], + instanceTypes: [ec2.InstanceType.of(ec2.InstanceClass.M3, ec2.InstanceSize.LARGE)], + }); + + expect(() => { + Template.fromStack(stack).hasResourceProperties('AWS::Batch::ComputeEnvironment', { + ...expectedProps, + ComputeResources: { + ...defaultComputeResources, + }, + }); + }).toThrow(/Cannot mix ARM and x86 instance types or classes/) + }); + test('creates and uses instanceProfile, even when instanceRole is specified', () => { // WHEN new ComputeEnvironment(stack, 'MyCE', { From fe6f2f5dac05e1781125b5cf8dbfe1c8310379aa Mon Sep 17 00:00:00 2001 From: Chen Date: Fri, 20 Sep 2024 14:37:28 -0700 Subject: [PATCH 05/11] optimal is not included in when there are arm instances --- .../test/managed-compute-environment.test.ts | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts b/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts index f1953202f5ff9..4352a4fa4b906 100644 --- a/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts +++ b/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts @@ -237,6 +237,26 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] }); }); + test('should not include optimal for ARM instance classes', () => { + // WHEN + new ComputeEnvironment(stack, 'MyCE', { + ...defaultProps, + vpc, + instanceClasses: [ec2.InstanceClass.M6G], + }); + + // THEN + Template.fromStack(stack).hasResourceProperties('AWS::Batch::ComputeEnvironment', { + ...expectedProps, + ComputeResources: { + ...defaultComputeResources, + InstanceTypes: [ + 'm6g', + ], + }, + }); + }); + test('instance types are correctly rendered', () => { // WHEN const ce = new ComputeEnvironment(stack, 'MyCE', { From 9f211f6d4b89875de926bca101a6ce4bdb5ac737 Mon Sep 17 00:00:00 2001 From: Chen Date: Fri, 20 Sep 2024 14:38:28 -0700 Subject: [PATCH 06/11] test cases for addInstanceType and addInstanceClass errors when mixed architecture --- .../test/managed-compute-environment.test.ts | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts b/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts index 4352a4fa4b906..ef81de88d7782 100644 --- a/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts +++ b/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts @@ -257,6 +257,38 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] }); }); + test('throws error when adding instance type with conflicting architecture', () => { + // WHEN + const ce = new ComputeEnvironment(stack, 'MyCE', { + ...defaultProps, + vpc, + instanceTypes: [ec2.InstanceType.of(ec2.InstanceClass.R4, ec2.InstanceSize.LARGE)], // x86 architecture + }); + + // THEN + expect(() => { + ce.addInstanceType(ec2.InstanceType.of(ec2.InstanceClass.ARM1, ec2.InstanceSize.LARGE)); // ARM architecture + }).toThrow( + new Error('Cannot add instance type a1.large with architecture arm64. It conflicts with existing instance type r4.large with architecture x86_64.') + ); + }); + + test('instance types throw error on conflicting architectures', () => { + // WHEN + const ce = new ComputeEnvironment(stack, 'MyCE', { + ...defaultProps, + vpc, + instanceTypes: [ec2.InstanceType.of(ec2.InstanceClass.R4, ec2.InstanceSize.LARGE)], + }); + + // THEN + expect(() => { + ce.addInstanceClass(ec2.InstanceClass.STANDARD6_GRAVITON); + }).toThrow( + new Error('Cannot add instance type standard6-graviton.large with architecture arm64. It conflicts with existing instance type r4.large with architecture x86_64.') + ); + }); + test('instance types are correctly rendered', () => { // WHEN const ce = new ComputeEnvironment(stack, 'MyCE', { From 52b1a06c8c9e0db58e38258f290cda2929ac129c Mon Sep 17 00:00:00 2001 From: Chen Date: Mon, 23 Sep 2024 13:03:24 -0700 Subject: [PATCH 07/11] linter changes --- .../lib/managed-compute-environment.ts | 13 ++++---- .../test/managed-compute-environment.test.ts | 31 ++++++++++--------- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts b/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts index bb37bca63550e..9ae9f13577c7f 100644 --- a/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts +++ b/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts @@ -1066,7 +1066,7 @@ export class ManagedEc2EksComputeEnvironment extends ManagedComputeEnvironmentBa this.node.addValidation({ validate: () => validateInstances.call(this, this.instanceTypes, this.instanceClasses, props.useOptimalInstanceClasses), - }); + }); } public addInstanceType(instanceType: ec2.InstanceType): void { @@ -1144,7 +1144,7 @@ export class FargateComputeEnvironment extends ManagedComputeEnvironmentBase imp function checkArchitectureConsistency( newInstanceType: ec2.InstanceType, existingTypes: ec2.InstanceType[], - existingClasses: ec2.InstanceClass[] + existingClasses: ec2.InstanceClass[], ): void { const newArchitecture = newInstanceType.architecture; @@ -1163,11 +1163,11 @@ function checkArchitectureConsistency( } } -function renderInstances(scope: Construct, types?: ec2.InstanceType[], classes?: ec2.InstanceClass[], useOptimalInstanceClasses?: boolean): string[] { +function renderInstances( + scope: Construct, types?: ec2.InstanceType[], classes?: ec2.InstanceClass[], useOptimalInstanceClasses?: boolean): string[] { const instances = []; let hasArmInstances = false; let hasX86Instances = false; - for (const instanceType of types ?? []) { instances.push(instanceType.toString()); if (instanceType.architecture === ec2.InstanceArchitecture.ARM_64) { @@ -1176,7 +1176,7 @@ function renderInstances(scope: Construct, types?: ec2.InstanceType[], classes?: hasX86Instances = true; } } - + for (const instanceClass of classes ?? []) { instances.push(instanceClass); const tempInstanceType = new ec2.InstanceType(`${instanceClass.toString()}.large`); @@ -1237,7 +1237,8 @@ function determineAllocationStrategy(id: string, allocationStrategy?: Allocation return result; } -function validateInstances(this: Construct, types?: ec2.InstanceType[], classes?: ec2.InstanceClass[], useOptimalInstanceClasses?: boolean): string[] { +function validateInstances( + this: Construct, types?: ec2.InstanceType[], classes?: ec2.InstanceClass[], useOptimalInstanceClasses?: boolean): string[] { if (renderInstances(this, types, classes, useOptimalInstanceClasses).length === 0) { return ["Specifies 'useOptimalInstanceClasses: false' without specifying any instance types or classes"]; } diff --git a/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts b/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts index ef81de88d7782..3f04b6d94f840 100644 --- a/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts +++ b/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts @@ -262,17 +262,17 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] const ce = new ComputeEnvironment(stack, 'MyCE', { ...defaultProps, vpc, - instanceTypes: [ec2.InstanceType.of(ec2.InstanceClass.R4, ec2.InstanceSize.LARGE)], // x86 architecture + instanceTypes: [ec2.InstanceType.of(ec2.InstanceClass.R4, ec2.InstanceSize.LARGE)], // x86 architecture }); - + // THEN expect(() => { - ce.addInstanceType(ec2.InstanceType.of(ec2.InstanceClass.ARM1, ec2.InstanceSize.LARGE)); // ARM architecture + ce.addInstanceType(ec2.InstanceType.of(ec2.InstanceClass.ARM1, ec2.InstanceSize.LARGE)); // ARM architecture }).toThrow( - new Error('Cannot add instance type a1.large with architecture arm64. It conflicts with existing instance type r4.large with architecture x86_64.') + new Error('Cannot add instance type a1.large with architecture arm64. It conflicts with existing instance type r4.large with architecture x86_64.'), ); }); - + test('instance types throw error on conflicting architectures', () => { // WHEN const ce = new ComputeEnvironment(stack, 'MyCE', { @@ -280,12 +280,12 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] vpc, instanceTypes: [ec2.InstanceType.of(ec2.InstanceClass.R4, ec2.InstanceSize.LARGE)], }); - + // THEN expect(() => { ce.addInstanceClass(ec2.InstanceClass.STANDARD6_GRAVITON); }).toThrow( - new Error('Cannot add instance type standard6-graviton.large with architecture arm64. It conflicts with existing instance type r4.large with architecture x86_64.') + new Error('Cannot add instance type standard6-graviton.large with architecture arm64. It conflicts with existing instance type r4.large with architecture x86_64.'), ); }); @@ -387,14 +387,14 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] vpc, instanceClasses: [ec2.InstanceClass.A1], instanceTypes: [ - ec2.InstanceType.of(ec2.InstanceClass.STANDARD6_GRAVITON, ec2.InstanceSize.LARGE) + ec2.InstanceType.of(ec2.InstanceClass.STANDARD6_GRAVITON, ec2.InstanceSize.LARGE), ], useOptimalInstanceClasses: true, }); // THEN - Annotations.fromStack(stack).hasWarning('*', - '\'optimal\' instance types are not supported with ARM instance types or classes, setting useOptimalInstanceClasses to false [ack: @aws-cdk/aws-batch:optimalNotSupportedWithARM]' + Annotations.fromStack(stack).hasWarning('*', + '\'optimal\' instance types are not supported with ARM instance types or classes, setting useOptimalInstanceClasses to false [ack: @aws-cdk/aws-batch:optimalNotSupportedWithARM]', ); }); @@ -413,7 +413,7 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] ...defaultComputeResources, }, }); - }).toThrow(/Cannot mix ARM and x86 instance types or classes/) + }).toThrow(/Cannot mix ARM and x86 instance types or classes/); }); test('mixing ARM and x86 instance types should throw an error', () => { @@ -421,7 +421,10 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] const CE = new ComputeEnvironment(stack, 'MyCE', { ...defaultProps, vpc, - instanceTypes: [ec2.InstanceType.of(ec2.InstanceClass.STANDARD6_GRAVITON, ec2.InstanceSize.LARGE), ec2.InstanceType.of(ec2.InstanceClass.R4, ec2.InstanceSize.LARGE)], + instanceTypes: [ + ec2.InstanceType.of(ec2.InstanceClass.STANDARD6_GRAVITON, ec2.InstanceSize.LARGE), + ec2.InstanceType.of(ec2.InstanceClass.R4, ec2.InstanceSize.LARGE), + ], }); expect(() => { @@ -431,7 +434,7 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] ...defaultComputeResources, }, }); - }).toThrow(/Cannot mix ARM and x86 instance types or classes/) + }).toThrow(/Cannot mix ARM and x86 instance types or classes/); }); test('mixing ARM and x86 instance classes and types should throw an error', () => { @@ -450,7 +453,7 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] ...defaultComputeResources, }, }); - }).toThrow(/Cannot mix ARM and x86 instance types or classes/) + }).toThrow(/Cannot mix ARM and x86 instance types or classes/); }); test('creates and uses instanceProfile, even when instanceRole is specified', () => { From 8d6233d8a1ad87826469e47260e6e90e4c0a2c47 Mon Sep 17 00:00:00 2001 From: Chen Date: Mon, 23 Sep 2024 13:07:13 -0700 Subject: [PATCH 08/11] update integration tests --- ...efaultTestDeployAssertD4528F80.assets.json | 2 +- .../batch-stack.assets.json | 6 +- .../batch-stack.template.json | 106 +++++++++++ .../cdk.out | 2 +- .../integ.json | 2 +- .../manifest.json | 34 +++- .../tree.json | 176 ++++++++++++++++++ .../test/integ.managed-compute-environment.ts | 9 + 8 files changed, 329 insertions(+), 8 deletions(-) diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/BatchManagedComputeEnvironmentTestDefaultTestDeployAssertD4528F80.assets.json b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/BatchManagedComputeEnvironmentTestDefaultTestDeployAssertD4528F80.assets.json index de953f1e34aad..7f02cd17ec799 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/BatchManagedComputeEnvironmentTestDefaultTestDeployAssertD4528F80.assets.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/BatchManagedComputeEnvironmentTestDefaultTestDeployAssertD4528F80.assets.json @@ -1,5 +1,5 @@ { - "version": "36.0.0", + "version": "36.0.24", "files": { "21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": { "source": { diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/batch-stack.assets.json b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/batch-stack.assets.json index 7fbccde788f96..9430f59388b84 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/batch-stack.assets.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/batch-stack.assets.json @@ -1,7 +1,7 @@ { - "version": "36.0.0", + "version": "36.0.24", "files": { - "29532266ce5c96c372f99765edc76d90da88dd7316798a6e86946bc0ffa1802d": { + "430386354edc9ab4d2ea248849aff4ff559a8b7c4d92f62491ad75c8c3746edb": { "source": { "path": "batch-stack.template.json", "packaging": "file" @@ -9,7 +9,7 @@ "destinations": { "current_account-current_region": { "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}", - "objectKey": "29532266ce5c96c372f99765edc76d90da88dd7316798a6e86946bc0ffa1802d.json", + "objectKey": "430386354edc9ab4d2ea248849aff4ff559a8b7c4d92f62491ad75c8c3746edb.json", "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}" } } diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/batch-stack.template.json b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/batch-stack.template.json index c99047b447f4b..59f2b9f6177dd 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/batch-stack.template.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/batch-stack.template.json @@ -986,6 +986,112 @@ "UpdatePolicy": {} } }, + "noOptimalInstancesForARMSecurityGroup7157B016": { + "Type": "AWS::EC2::SecurityGroup", + "Properties": { + "GroupDescription": "batch-stack/noOptimalInstancesForARM/SecurityGroup", + "SecurityGroupEgress": [ + { + "CidrIp": "0.0.0.0/0", + "Description": "Allow all outbound traffic by default", + "IpProtocol": "-1" + } + ], + "VpcId": { + "Ref": "vpcA2121C38" + } + } + }, + "noOptimalInstancesForARMInstanceProfileRole926AEEC5": { + "Type": "AWS::IAM::Role", + "Properties": { + "AssumeRolePolicyDocument": { + "Statement": [ + { + "Action": "sts:AssumeRole", + "Effect": "Allow", + "Principal": { + "Service": "ec2.amazonaws.com" + } + } + ], + "Version": "2012-10-17" + }, + "ManagedPolicyArns": [ + { + "Fn::Join": [ + "", + [ + "arn:", + { + "Ref": "AWS::Partition" + }, + ":iam::aws:policy/service-role/AmazonEC2ContainerServiceforEC2Role" + ] + ] + } + ] + } + }, + "noOptimalInstancesForARMInstanceProfile37BE9D32": { + "Type": "AWS::IAM::InstanceProfile", + "Properties": { + "Roles": [ + { + "Ref": "noOptimalInstancesForARMInstanceProfileRole926AEEC5" + } + ] + } + }, + "noOptimalInstancesForARMF146AA4D": { + "Type": "AWS::Batch::ComputeEnvironment", + "Properties": { + "ComputeResources": { + "AllocationStrategy": "BEST_FIT_PROGRESSIVE", + "Ec2Configuration": [ + { + "ImageIdOverride": { + "Ref": "SsmParameterValueawsserviceamiamazonlinuxlatestamznamihvmx8664gp2C96584B6F00A464EAD1953AFF4B05118Parameter" + }, + "ImageType": "ECS_AL2" + } + ], + "InstanceRole": { + "Fn::GetAtt": [ + "noOptimalInstancesForARMInstanceProfile37BE9D32", + "Arn" + ] + }, + "InstanceTypes": [ + "a1.large", + "m6g" + ], + "MaxvCpus": 256, + "MinvCpus": 0, + "SecurityGroupIds": [ + { + "Fn::GetAtt": [ + "noOptimalInstancesForARMSecurityGroup7157B016", + "GroupId" + ] + } + ], + "Subnets": [ + { + "Ref": "vpcPrivateSubnet1Subnet934893E8" + }, + { + "Ref": "vpcPrivateSubnet2Subnet7031C2BA" + } + ], + "Type": "EC2" + }, + "ReplaceComputeEnvironment": false, + "State": "ENABLED", + "Type": "managed", + "UpdatePolicy": {} + } + }, "taggedCESecurityGroup82CCF59F": { "Type": "AWS::EC2::SecurityGroup", "Properties": { diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/cdk.out b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/cdk.out index 1f0068d32659a..4efaa16f29af9 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/cdk.out +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/cdk.out @@ -1 +1 @@ -{"version":"36.0.0"} \ No newline at end of file +{"version":"36.0.24"} \ No newline at end of file diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/integ.json b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/integ.json index 1d97ef0a4308e..43bc3c948c0f6 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/integ.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/integ.json @@ -1,5 +1,5 @@ { - "version": "36.0.0", + "version": "36.0.24", "testCases": { "BatchManagedComputeEnvironmentTest/DefaultTest": { "stacks": [ diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/manifest.json b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/manifest.json index 9ec827b896504..b0bbd05ccad33 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/manifest.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/manifest.json @@ -1,5 +1,5 @@ { - "version": "36.0.0", + "version": "36.0.24", "artifacts": { "batch-stack.assets": { "type": "cdk:asset-manifest", @@ -18,7 +18,7 @@ "validateOnSynth": false, "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}", "cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}", - "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/29532266ce5c96c372f99765edc76d90da88dd7316798a6e86946bc0ffa1802d.json", + "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/430386354edc9ab4d2ea248849aff4ff559a8b7c4d92f62491ad75c8c3746edb.json", "requiresBootstrapStackVersion": 6, "bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version", "additionalDependencies": [ @@ -316,6 +316,36 @@ "data": "AllocationStrategySPOTCAPACITYEE4582C5" } ], + "/batch-stack/noOptimalInstancesForARM": [ + { + "type": "aws:cdk:warning", + "data": "'optimal' instance types are not supported with ARM instance types or classes, setting useOptimalInstanceClasses to false [ack: @aws-cdk/aws-batch:optimalNotSupportedWithARM]" + } + ], + "/batch-stack/noOptimalInstancesForARM/SecurityGroup/Resource": [ + { + "type": "aws:cdk:logicalId", + "data": "noOptimalInstancesForARMSecurityGroup7157B016" + } + ], + "/batch-stack/noOptimalInstancesForARM/InstanceProfileRole/Resource": [ + { + "type": "aws:cdk:logicalId", + "data": "noOptimalInstancesForARMInstanceProfileRole926AEEC5" + } + ], + "/batch-stack/noOptimalInstancesForARM/InstanceProfile": [ + { + "type": "aws:cdk:logicalId", + "data": "noOptimalInstancesForARMInstanceProfile37BE9D32" + } + ], + "/batch-stack/noOptimalInstancesForARM/Resource": [ + { + "type": "aws:cdk:logicalId", + "data": "noOptimalInstancesForARMF146AA4D" + } + ], "/batch-stack/taggedCE/SecurityGroup/Resource": [ { "type": "aws:cdk:logicalId", diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/tree.json b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/tree.json index 7d665f06bcdc1..82758d1f53a4e 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/tree.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.js.snapshot/tree.json @@ -1677,6 +1677,182 @@ "version": "0.0.0" } }, + "noOptimalInstancesForARM": { + "id": "noOptimalInstancesForARM", + "path": "batch-stack/noOptimalInstancesForARM", + "children": { + "SecurityGroup": { + "id": "SecurityGroup", + "path": "batch-stack/noOptimalInstancesForARM/SecurityGroup", + "children": { + "Resource": { + "id": "Resource", + "path": "batch-stack/noOptimalInstancesForARM/SecurityGroup/Resource", + "attributes": { + "aws:cdk:cloudformation:type": "AWS::EC2::SecurityGroup", + "aws:cdk:cloudformation:props": { + "groupDescription": "batch-stack/noOptimalInstancesForARM/SecurityGroup", + "securityGroupEgress": [ + { + "cidrIp": "0.0.0.0/0", + "description": "Allow all outbound traffic by default", + "ipProtocol": "-1" + } + ], + "vpcId": { + "Ref": "vpcA2121C38" + } + } + }, + "constructInfo": { + "fqn": "aws-cdk-lib.aws_ec2.CfnSecurityGroup", + "version": "0.0.0" + } + } + }, + "constructInfo": { + "fqn": "aws-cdk-lib.aws_ec2.SecurityGroup", + "version": "0.0.0" + } + }, + "InstanceProfileRole": { + "id": "InstanceProfileRole", + "path": "batch-stack/noOptimalInstancesForARM/InstanceProfileRole", + "children": { + "ImportInstanceProfileRole": { + "id": "ImportInstanceProfileRole", + "path": "batch-stack/noOptimalInstancesForARM/InstanceProfileRole/ImportInstanceProfileRole", + "constructInfo": { + "fqn": "aws-cdk-lib.Resource", + "version": "0.0.0" + } + }, + "Resource": { + "id": "Resource", + "path": "batch-stack/noOptimalInstancesForARM/InstanceProfileRole/Resource", + "attributes": { + "aws:cdk:cloudformation:type": "AWS::IAM::Role", + "aws:cdk:cloudformation:props": { + "assumeRolePolicyDocument": { + "Statement": [ + { + "Action": "sts:AssumeRole", + "Effect": "Allow", + "Principal": { + "Service": "ec2.amazonaws.com" + } + } + ], + "Version": "2012-10-17" + }, + "managedPolicyArns": [ + { + "Fn::Join": [ + "", + [ + "arn:", + { + "Ref": "AWS::Partition" + }, + ":iam::aws:policy/service-role/AmazonEC2ContainerServiceforEC2Role" + ] + ] + } + ] + } + }, + "constructInfo": { + "fqn": "aws-cdk-lib.aws_iam.CfnRole", + "version": "0.0.0" + } + } + }, + "constructInfo": { + "fqn": "aws-cdk-lib.aws_iam.Role", + "version": "0.0.0" + } + }, + "InstanceProfile": { + "id": "InstanceProfile", + "path": "batch-stack/noOptimalInstancesForARM/InstanceProfile", + "attributes": { + "aws:cdk:cloudformation:type": "AWS::IAM::InstanceProfile", + "aws:cdk:cloudformation:props": { + "roles": [ + { + "Ref": "noOptimalInstancesForARMInstanceProfileRole926AEEC5" + } + ] + } + }, + "constructInfo": { + "fqn": "aws-cdk-lib.aws_iam.CfnInstanceProfile", + "version": "0.0.0" + } + }, + "Resource": { + "id": "Resource", + "path": "batch-stack/noOptimalInstancesForARM/Resource", + "attributes": { + "aws:cdk:cloudformation:type": "AWS::Batch::ComputeEnvironment", + "aws:cdk:cloudformation:props": { + "computeResources": { + "maxvCpus": 256, + "type": "EC2", + "securityGroupIds": [ + { + "Fn::GetAtt": [ + "noOptimalInstancesForARMSecurityGroup7157B016", + "GroupId" + ] + } + ], + "subnets": [ + { + "Ref": "vpcPrivateSubnet1Subnet934893E8" + }, + { + "Ref": "vpcPrivateSubnet2Subnet7031C2BA" + } + ], + "minvCpus": 0, + "instanceRole": { + "Fn::GetAtt": [ + "noOptimalInstancesForARMInstanceProfile37BE9D32", + "Arn" + ] + }, + "instanceTypes": [ + "a1.large", + "m6g" + ], + "allocationStrategy": "BEST_FIT_PROGRESSIVE", + "ec2Configuration": [ + { + "imageIdOverride": { + "Ref": "SsmParameterValueawsserviceamiamazonlinuxlatestamznamihvmx8664gp2C96584B6F00A464EAD1953AFF4B05118Parameter" + }, + "imageType": "ECS_AL2" + } + ] + }, + "replaceComputeEnvironment": false, + "state": "ENABLED", + "type": "managed", + "updatePolicy": {} + } + }, + "constructInfo": { + "fqn": "aws-cdk-lib.aws_batch.CfnComputeEnvironment", + "version": "0.0.0" + } + } + }, + "constructInfo": { + "fqn": "aws-cdk-lib.aws_batch.ManagedEc2EcsComputeEnvironment", + "version": "0.0.0" + } + }, "taggedCE": { "id": "taggedCE", "path": "batch-stack/taggedCE", diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.ts b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.ts index 0d71b9cfbff4e..239972e6bd0ad 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.ts +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-batch/test/integ.managed-compute-environment.ts @@ -69,6 +69,15 @@ new ManagedEc2EcsComputeEnvironment(stack, 'AllocationStrategySPOT_CAPACITY', { allocationStrategy: AllocationStrategy.SPOT_CAPACITY_OPTIMIZED, }); +new ManagedEc2EcsComputeEnvironment(stack, 'noOptimalInstancesForARM', { + vpc, + images: [{ + image: new ec2.AmazonLinuxImage(), + }], + instanceClasses: [ec2.InstanceClass.M6G], + instanceTypes: [ec2.InstanceType.of(ec2.InstanceClass.A1, ec2.InstanceSize.LARGE)], +}); + const taggedEc2Ecs = new ManagedEc2EcsComputeEnvironment(stack, 'taggedCE', { vpc, images: [{ From e4b286430863f31516705eaa22b1632ec3e8e5e9 Mon Sep 17 00:00:00 2001 From: Chen Date: Fri, 27 Sep 2024 10:33:09 -0700 Subject: [PATCH 09/11] remove changing value useOptimalInstanceClasses & update test cases --- .../lib/managed-compute-environment.ts | 7 ++- .../test/managed-compute-environment.test.ts | 45 ++++++++++++++++++- 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts b/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts index 9ae9f13577c7f..c5f839ed7f7c1 100644 --- a/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts +++ b/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts @@ -1191,12 +1191,11 @@ function renderInstances( throw new Error('Cannot mix ARM and x86 instance types or classes'); } - if (hasArmInstances && (useOptimalInstanceClasses || useOptimalInstanceClasses === undefined)) { - Annotations.of(scope).addWarningV2('@aws-cdk/aws-batch:optimalNotSupportedWithARM', '\'optimal\' instance types are not supported with ARM instance types or classes, setting useOptimalInstanceClasses to false'); - useOptimalInstanceClasses = false; + if (hasArmInstances && useOptimalInstanceClasses) { + Annotations.of(scope).addWarningV2('@aws-cdk/aws-batch:optimalNotSupportedWithARM', '\'optimal\' instance types are not supported with ARM instance types or classes. Deploying will cause an error, please set useOptimalInstanceClasses to false'); } - if (useOptimalInstanceClasses || useOptimalInstanceClasses === undefined) { + if (useOptimalInstanceClasses || (!hasArmInstances && useOptimalInstanceClasses === undefined)) { instances.push('optimal'); } diff --git a/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts b/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts index 3f04b6d94f840..20e6530236298 100644 --- a/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts +++ b/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts @@ -336,6 +336,28 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] }); }); + test('respects useOptimalInstanceClasses: false for ARM instances', () => { + // WHEN + const myCE = new ComputeEnvironment(stack, 'MyCE', { + ...defaultProps, + vpc, + useOptimalInstanceClasses: false, + }); + + myCE.addInstanceType(ec2.InstanceType.of(ec2.InstanceClass.A1, ec2.InstanceSize.LARGE)); + + // THEN + Template.fromStack(stack).hasResourceProperties('AWS::Batch::ComputeEnvironment', { + ...expectedProps, + ComputeResources: { + ...defaultComputeResources, + InstanceTypes: [ + 'a1.large', + ], + }, + }); + }); + test('does not throw with useOptimalInstanceClasses: false and a call to addInstanceClass()', () => { // WHEN const myCE = new ComputeEnvironment(stack, 'MyCE', { @@ -394,10 +416,31 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] // THEN Annotations.fromStack(stack).hasWarning('*', - '\'optimal\' instance types are not supported with ARM instance types or classes, setting useOptimalInstanceClasses to false [ack: @aws-cdk/aws-batch:optimalNotSupportedWithARM]', + '\'optimal\' instance types are not supported with ARM instance types or classes. Deploying will cause an error, please set useOptimalInstanceClasses to false [ack: @aws-cdk/aws-batch:optimalNotSupportedWithARM]', ); }); + test('no warning when optimal instance classes is undefined with ARM instances', () => { + // WHEN + const myCE = new ComputeEnvironment(stack, 'MyCE', { + ...defaultProps, + vpc, + }); + + myCE.addInstanceType(ec2.InstanceType.of(ec2.InstanceClass.ARM1, ec2.InstanceSize.LARGE)); + + // THEN + Template.fromStack(stack).hasResourceProperties('AWS::Batch::ComputeEnvironment', { + ...expectedProps, + ComputeResources: { + ...defaultComputeResources, + InstanceTypes: [ + 'a1.large', + ], + }, + }); + }); + test('mixing ARM and x86 instance classes should throw an error', () => { // THEN const CE = new ComputeEnvironment(stack, 'MyCE', { From 016a89a18ec4efba9f8d2d2d2a2f219419a913d1 Mon Sep 17 00:00:00 2001 From: Chen Date: Fri, 27 Sep 2024 11:35:38 -0700 Subject: [PATCH 10/11] change error to warning for mixed instances & modify test cases --- .../lib/managed-compute-environment.ts | 2 +- .../test/managed-compute-environment.test.ts | 33 +++++-------------- 2 files changed, 10 insertions(+), 25 deletions(-) diff --git a/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts b/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts index c5f839ed7f7c1..86a415bc5254f 100644 --- a/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts +++ b/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts @@ -1188,7 +1188,7 @@ function renderInstances( } if (hasArmInstances && hasX86Instances) { - throw new Error('Cannot mix ARM and x86 instance types or classes'); + Annotations.of(scope).addWarningV2('@aws-cdk/aws-batch:mixingARMAndx86InstancesNotSupported', 'Cannot mix ARM and x86 instance types or classes, deploying will cause an error'); } if (hasArmInstances && useOptimalInstanceClasses) { diff --git a/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts b/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts index 20e6530236298..e6864122b9225 100644 --- a/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts +++ b/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts @@ -449,14 +449,9 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] instanceClasses: [ec2.InstanceClass.M6G, ec2.InstanceClass.C4], }); - expect(() => { - Template.fromStack(stack).hasResourceProperties('AWS::Batch::ComputeEnvironment', { - ...expectedProps, - ComputeResources: { - ...defaultComputeResources, - }, - }); - }).toThrow(/Cannot mix ARM and x86 instance types or classes/); + Annotations.fromStack(stack).hasWarning('*', + 'Cannot mix ARM and x86 instance types or classes, deploying will cause an error [ack: @aws-cdk/aws-batch:mixingARMAndx86InstancesNotSupported]', + ); }); test('mixing ARM and x86 instance types should throw an error', () => { @@ -470,14 +465,9 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] ], }); - expect(() => { - Template.fromStack(stack).hasResourceProperties('AWS::Batch::ComputeEnvironment', { - ...expectedProps, - ComputeResources: { - ...defaultComputeResources, - }, - }); - }).toThrow(/Cannot mix ARM and x86 instance types or classes/); + Annotations.fromStack(stack).hasWarning('*', + 'Cannot mix ARM and x86 instance types or classes, deploying will cause an error [ack: @aws-cdk/aws-batch:mixingARMAndx86InstancesNotSupported]', + ); }); test('mixing ARM and x86 instance classes and types should throw an error', () => { @@ -489,14 +479,9 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] instanceTypes: [ec2.InstanceType.of(ec2.InstanceClass.M3, ec2.InstanceSize.LARGE)], }); - expect(() => { - Template.fromStack(stack).hasResourceProperties('AWS::Batch::ComputeEnvironment', { - ...expectedProps, - ComputeResources: { - ...defaultComputeResources, - }, - }); - }).toThrow(/Cannot mix ARM and x86 instance types or classes/); + Annotations.fromStack(stack).hasWarning('*', + 'Cannot mix ARM and x86 instance types or classes, deploying will cause an error [ack: @aws-cdk/aws-batch:mixingARMAndx86InstancesNotSupported]', + ); }); test('creates and uses instanceProfile, even when instanceRole is specified', () => { From 3d5b53f3397b81456bd5f4f801c50b61f3944d76 Mon Sep 17 00:00:00 2001 From: Chen Date: Tue, 1 Oct 2024 10:56:13 -0700 Subject: [PATCH 11/11] remove error message when adding types or classes in case of regressions --- .../lib/managed-compute-environment.ts | 28 ---------------- .../test/managed-compute-environment.test.ts | 32 ------------------- 2 files changed, 60 deletions(-) diff --git a/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts b/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts index 86a415bc5254f..ff80865d56813 100644 --- a/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts +++ b/packages/aws-cdk-lib/aws-batch/lib/managed-compute-environment.ts @@ -718,13 +718,10 @@ export class ManagedEc2EcsComputeEnvironment extends ManagedComputeEnvironmentBa } public addInstanceType(instanceType: ec2.InstanceType): void { - checkArchitectureConsistency(instanceType, this.instanceTypes, this.instanceClasses); this.instanceTypes.push(instanceType); } public addInstanceClass(instanceClass: ec2.InstanceClass): void { - const tempInstanceType = new ec2.InstanceType(`${instanceClass.toString()}.large`); - checkArchitectureConsistency(tempInstanceType, this.instanceTypes, this.instanceClasses); this.instanceClasses.push(instanceClass); } } @@ -1070,13 +1067,10 @@ export class ManagedEc2EksComputeEnvironment extends ManagedComputeEnvironmentBa } public addInstanceType(instanceType: ec2.InstanceType): void { - checkArchitectureConsistency(instanceType, this.instanceTypes, this.instanceClasses); this.instanceTypes.push(instanceType); } public addInstanceClass(instanceClass: ec2.InstanceClass): void { - const tempInstanceType = new ec2.InstanceType(`${instanceClass.toString()}.large`); - checkArchitectureConsistency(tempInstanceType, this.instanceTypes, this.instanceClasses); this.instanceClasses.push(instanceClass); } } @@ -1141,28 +1135,6 @@ export class FargateComputeEnvironment extends ManagedComputeEnvironmentBase imp } } -function checkArchitectureConsistency( - newInstanceType: ec2.InstanceType, - existingTypes: ec2.InstanceType[], - existingClasses: ec2.InstanceClass[], -): void { - const newArchitecture = newInstanceType.architecture; - - for (const existingType of existingTypes ?? []) { - if (existingType.architecture !== newArchitecture) { - throw new Error(`Cannot add instance type ${newInstanceType.toString()} with architecture ${newArchitecture}. It conflicts with existing instance type ${existingType.toString()} with architecture ${existingType.architecture}.`); - } - } - - for (const existingClass of existingClasses ?? []) { - const tempInstanceType = new ec2.InstanceType(`${existingClass.toString()}.large`); - const existingClassArchitecture = tempInstanceType.architecture; - if (existingClassArchitecture !== newArchitecture) { - throw new Error(`Cannot add instance type ${newInstanceType.toString()} with architecture ${newArchitecture}. It conflicts with existing instance class ${existingClass} with architecture ${existingClassArchitecture}.`); - } - } -} - function renderInstances( scope: Construct, types?: ec2.InstanceType[], classes?: ec2.InstanceClass[], useOptimalInstanceClasses?: boolean): string[] { const instances = []; diff --git a/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts b/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts index e6864122b9225..77511cc051449 100644 --- a/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts +++ b/packages/aws-cdk-lib/aws-batch/test/managed-compute-environment.test.ts @@ -257,38 +257,6 @@ describe.each([ManagedEc2EcsComputeEnvironment, ManagedEc2EksComputeEnvironment] }); }); - test('throws error when adding instance type with conflicting architecture', () => { - // WHEN - const ce = new ComputeEnvironment(stack, 'MyCE', { - ...defaultProps, - vpc, - instanceTypes: [ec2.InstanceType.of(ec2.InstanceClass.R4, ec2.InstanceSize.LARGE)], // x86 architecture - }); - - // THEN - expect(() => { - ce.addInstanceType(ec2.InstanceType.of(ec2.InstanceClass.ARM1, ec2.InstanceSize.LARGE)); // ARM architecture - }).toThrow( - new Error('Cannot add instance type a1.large with architecture arm64. It conflicts with existing instance type r4.large with architecture x86_64.'), - ); - }); - - test('instance types throw error on conflicting architectures', () => { - // WHEN - const ce = new ComputeEnvironment(stack, 'MyCE', { - ...defaultProps, - vpc, - instanceTypes: [ec2.InstanceType.of(ec2.InstanceClass.R4, ec2.InstanceSize.LARGE)], - }); - - // THEN - expect(() => { - ce.addInstanceClass(ec2.InstanceClass.STANDARD6_GRAVITON); - }).toThrow( - new Error('Cannot add instance type standard6-graviton.large with architecture arm64. It conflicts with existing instance type r4.large with architecture x86_64.'), - ); - }); - test('instance types are correctly rendered', () => { // WHEN const ce = new ComputeEnvironment(stack, 'MyCE', {