-
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
feat(ec2/ecs): cacheInContext
properties for machine images
#16021
Conversation
Most `MachineImage` implementations look up AMIs from SSM Parameters, and by default they will all look up the Parameters on each deployment. This leads to instance replacement. Since we already know the SSM Parameter Name and CDK already has a cached SSM context lookup, it should be simple to get a stable AMI ID. This is not ideal because the AMI will grow outdated over time, but users should have the option to pick non-updating images in a convenient way. Fixes #12484.
I'm not married to the property name, welcoming suggestions for improvement. |
Previously, Bottlerocket had to be explicitly (and only) selected via setting `machineImageType`, which would pick an appropriate `machineImage`. Setting `machineImage` to `new BottleRocketImage()` would not be sufficient, since the feature also requires configuring additional UserData commands which are only added if `machineImageType` was set. This method of configuration does not allow customization of the AMI, such as introduced in #16021. Instead, we reverse the logic: `machineImageType` may still be necessary to autoconfigure UserData if we can't know what the machineImage is (for example in case of a preconfigured AutoScalingGroup), but otherwise is derived from what `machineImage` is being used.
As introduced in #10097, Bottlerocket had to be explicitly (and only) selected via setting `machineImageType`, which would pick an appropriate `machineImage`. Setting `machineImage` to `new BottleRocketImage()` would not be sufficient, since the feature also requires configuring additional UserData commands which are only added if `machineImageType` was set. This method of configuration does not allow customization of the AMI, such as introduced in #16021. Instead, we reverse the logic: `machineImageType` may still be necessary to autoconfigure UserData if we can't know what the machineImage is (for example in case of a preconfigured AutoScalingGroup), but otherwise is derived from what `machineImage` is being used. We allow configuring both fields at the same time for the case when the autodetection fails. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
* | ||
* @default false | ||
*/ | ||
readonly cachedInContext?: boolean; |
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 think just "cache" would be fine
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 want people thinking about "context" though...
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.
Personally, this feels like an implementation detail that doesn't need to be surfaced in the API name. All the customer cares is that the AMI does not change automatically without cache eviction
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 with you in general. What concerns me is that users might not know where to look for said cache eviction--which is why I want that word in there.
As introduced in aws#10097, Bottlerocket had to be explicitly (and only) selected via setting `machineImageType`, which would pick an appropriate `machineImage`. Setting `machineImage` to `new BottleRocketImage()` would not be sufficient, since the feature also requires configuring additional UserData commands which are only added if `machineImageType` was set. This method of configuration does not allow customization of the AMI, such as introduced in aws#16021. Instead, we reverse the logic: `machineImageType` may still be necessary to autoconfigure UserData if we can't know what the machineImage is (for example in case of a preconfigured AutoScalingGroup), but otherwise is derived from what `machineImage` is being used. We allow configuring both fields at the same time for the case when the autodetection fails. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
As introduced in aws#10097, Bottlerocket had to be explicitly (and only) selected via setting `machineImageType`, which would pick an appropriate `machineImage`. Setting `machineImage` to `new BottleRocketImage()` would not be sufficient, since the feature also requires configuring additional UserData commands which are only added if `machineImageType` was set. This method of configuration does not allow customization of the AMI, such as introduced in aws#16021. Instead, we reverse the logic: `machineImageType` may still be necessary to autoconfigure UserData if we can't know what the machineImage is (for example in case of a preconfigured AutoScalingGroup), but otherwise is derived from what `machineImage` is being used. We allow configuring both fields at the same time for the case when the autodetection fails. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
As introduced in aws#10097, Bottlerocket had to be explicitly (and only) selected via setting `machineImageType`, which would pick an appropriate `machineImage`. Setting `machineImage` to `new BottleRocketImage()` would not be sufficient, since the feature also requires configuring additional UserData commands which are only added if `machineImageType` was set. This method of configuration does not allow customization of the AMI, such as introduced in aws#16021. Instead, we reverse the logic: `machineImageType` may still be necessary to autoconfigure UserData if we can't know what the machineImage is (for example in case of a preconfigured AutoScalingGroup), but otherwise is derived from what `machineImage` is being used. We allow configuring both fields at the same time for the case when the autodetection fails. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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.
See comment, feel free to merge if you disagree
* | ||
* @default false | ||
*/ | ||
readonly cachedInContext?: boolean; |
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.
Personally, this feels like an implementation detail that doesn't need to be surfaced in the API name. All the customer cares is that the AMI does not change automatically without cache eviction
Thank you for contributing! Your pull request will be updated from master 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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Most
MachineImage
implementations look up AMIs from SSM Parameters,and by default they will all look up the Parameters on each deployment.
This leads to instance replacement. Since we already know the SSM
Parameter Name and CDK already has a cached SSM context lookup, it
should be simple to get a stable AMI ID. This is not ideal because the
AMI will grow outdated over time, but users should have the option to
pick non-updating images in a convenient way.
Fixes #12484.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license