-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
chore(ec2): choose NAT instance v2 machineImage cpuType automatically from instanceType #30558
Conversation
… from instanceType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is something we should have thought of on the intial implementation. But updating it now would potentially be a breaking change due to x86 and arm compatibilities issues. Correct me if wrong but existing users who specified arm_64 instance type are actually using x86 type. Now if we allow this change, their instance is going to be updated with arm_64 instance type and may be breaking change.
If this is correct, I would recommend you putting this fix under feature flag.
@GavinZZ Fortunately, no, it is not a breaking change, because we cannot deploy arm instance with x86 AMIs due to the mismatch of architecture. We get the following error during a deployment:
|
Thanks for clarification,I'm good to approve in this case. |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
… from instanceType (aws#30558) ### Issue # (if applicable) n/a ### Reason for this change When configuring NAT instance v2, currently we have to set machineImage manually when we want to use a graviton instance. Like this: ```ts const vpc = new Vpc(this, 'Vpc', { natGatewayProvider: NatProvider.instanceV2({ instanceType: InstanceType.of(InstanceClass.T4G, InstanceSize.NANO), // we should be able to omit this line! machineImage: MachineImage.latestAmazonLinux2023({ cpuType: AmazonLinuxCpuType.ARM_64 }), }), }); ``` This can be easily avoided if Nat instance v2 construct decides which cpu type to use for the given instance type. ### Description of changes Use `instanceType.architecture` to choose cpu type of a machine image. Now we can remove the redundant code: ```ts const vpc = new Vpc(this, 'Vpc', { natGatewayProvider: NatProvider.instanceV2({ instanceType: InstanceType.of(InstanceClass.T4G, InstanceSize.NANO), }), }); ``` ### Description of how you validated changes Added an integ test. ### Checklist - [X] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
… from instanceType (aws#30558) ### Issue # (if applicable) n/a ### Reason for this change When configuring NAT instance v2, currently we have to set machineImage manually when we want to use a graviton instance. Like this: ```ts const vpc = new Vpc(this, 'Vpc', { natGatewayProvider: NatProvider.instanceV2({ instanceType: InstanceType.of(InstanceClass.T4G, InstanceSize.NANO), // we should be able to omit this line! machineImage: MachineImage.latestAmazonLinux2023({ cpuType: AmazonLinuxCpuType.ARM_64 }), }), }); ``` This can be easily avoided if Nat instance v2 construct decides which cpu type to use for the given instance type. ### Description of changes Use `instanceType.architecture` to choose cpu type of a machine image. Now we can remove the redundant code: ```ts const vpc = new Vpc(this, 'Vpc', { natGatewayProvider: NatProvider.instanceV2({ instanceType: InstanceType.of(InstanceClass.T4G, InstanceSize.NANO), }), }); ``` ### Description of how you validated changes Added an integ test. ### Checklist - [X] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one. |
Issue # (if applicable)
n/a
Reason for this change
When configuring NAT instance v2, currently we have to set machineImage manually when we want to use a graviton instance.
Like this:
This can be easily avoided if Nat instance v2 construct decides which cpu type to use for the given instance type.
Description of changes
Use
instanceType.architecture
to choose cpu type of a machine image.Now we can remove the redundant code:
Description of how you validated changes
Added an integ test.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license