-
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(ebs): set default volumeType to gp3(under feature flag) #29934
chore(ebs): set default volumeType to gp3(under feature flag) #29934
Conversation
@@ -128,7 +127,8 @@ The following json shows the current recommended set of flags, as `cdk init` wou | |||
"@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse": true, | |||
"@aws-cdk/aws-codepipeline:defaultPipelineTypeToV2": true, | |||
"@aws-cdk/aws-kms:reduceCrossAccountRegionPolicyScope": true, | |||
"@aws-cdk/aws-eks:nodegroupNameAttribute": true | |||
"@aws-cdk/aws-eks:nodegroupNameAttribute": true, | |||
"@aws-cdk/aws-ec2:ebsDefaultGp3Volume": true |
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.
Any reason not to have something like "@aws-cdk/aws-ec2:ebsDefaultVolume": "gp3"
? The current setup makes future changes in the default volume type a little more awkward, we'll have to have multiple aws-cdk/aws-ec2:ebsDefaultXXXVolume
flags, make sure there are no conflicts, etc.
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.
Let's say if we introduce "@aws-cdk/aws-ec2:ebsDefaultVolume": "gp3"
in this PR and after a while AWS announces gp4
as a new default and we modify this feature flag to "@aws-cdk/aws-ec2:ebsDefaultVolume": "gp4"
, this would cause breaking changes for those opting in gp3
and we still need a check to see if they have explicitly enable this flag as gp3
. I am not sure if that would be easier to maintain. Another consideration is conventionally our feature flags are boolean except for target partitions and we generally don't update value of existing feature flags. Will check this with the maintainers.
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.
we modify this feature flag to "@aws-cdk/aws-ec2:ebsDefaultVolume": "gp4", this would cause breaking changes for those opting in gp3 and we still need a check to see if they have explicitly enable this flag as gp3
Wouldn't we only change the recommended value to gp4
? Users that opted in to gp3
would be in the same situation as those who enabled ebsDefaultGp3Volume
if we introduce ebsDefaultGp4Volume
. Arguably even worse off, since they would both need to disable ebsDefaultGp3Volume
and enable ebsDefaultGp4Volume
, vs just updating ebsDefaultVolume
to gp4
.
I'm leaving it up to you guys, I might not be seeing the whole picture, just throwing it out there 👍
If we make this switch without a feature flag, what is the behavior of this change? Obviously there will be a replacement of instances but will this cause downtime? If we can just update it, no reason not to. |
@TheRealAmazonKendra Let me check and update here. |
CloudFormation would trigger in-place volume update but this is what Amazon Q said:
So I guess we probably still need a feature flag for that. |
I was looking around to see what the EOL date is for these instance types (there is none) and found this https://aws.amazon.com/blogs/storage/migrate-your-amazon-ebs-volumes-from-gp2-to-gp3-and-save-up-to-20-on-costs/. It seems that there isn't actually downtime when a replacement happens but that there may be other considerations we need to take into account if someone is migrating. |
All that to say, I totally agree that we need to add a feature flag here but we also probably need to add a bit more than just that. Our documentation should make these limitations clear. I'm not sure if there's much else we can do to assist in a smooth upgrade. From the fact that gp3 was released in 2020 and we haven't had any new ones since then, I'm a bit torn about how much we need to future proof this. If we want to really future proof it, we could update the default here to be This approach would still require a feature flag but it would ensure that we aren't having to update that feature flag and/or overwrite it in the future. Thoughts on this approach? If it feels like overkill, I'm not opposed to how you've added the feature flag as is. Perhaps we should check with the EBS team to see if there's anything on the horizon for gp4 or similar and to see if there's anything thorny we need to know about updating via CloudFormation so we can warn users. |
I'm interested to know more about this. Are you referring latestamazonlinux2() which essentially lookup the value from ssm parameter through a context provider and cache in the context? Is there any existing context provider we can reuse for this use case? Adding a new context provider just for this seems a little bit overkill to me. I am not a big fan of RE |
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.
Let's also add the flag to https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/cx-api/README.md
@GavinZZ DONE |
Read through all the comments and I actually like the idea proposed by @nmussy. Haven't thought about the implementation difficulty but my first instinct is that we don't need anything else to support string type value in feature flag. When Note that I also prefer specifying explicit value instead of |
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.
LGTM, discussed with the team and decided to continue using boolean value for feature flags.
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). |
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). |
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). |
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)
As the EBS console is now having
gp3
as the default volumeType, this PR set the default volumeType to gp3 if undefined under feature flag.Closes #29931
Reason for this change
Description of changes
Description of how you validated changes
I have deployed the sample below and verified the volume type is
gp3
from console.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license