-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(ec2): volume initialization rate for EBS volume #34570
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
Conversation
| const app = new cdk.App({ | ||
| postCliContext: { | ||
| '@aws-cdk/aws-lambda:useCdkManagedLogGroup': false, | ||
| }, | ||
| }); |
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.
why do we need this change?
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.
This is my mistake. I've reverted now.
...k-testing/framework-integ/test/aws-s3-deployment/test/integ.bucket-deployment-signcontent.js
Outdated
Show resolved
Hide resolved
…nt/test/integ.bucket-deployment-signcontent.js
| } | ||
|
|
||
| if (!props.volumeInitializationRate.isUnresolved()) { | ||
| const rateMiBs = props.volumeInitializationRate.toBytes() / (1024 * 1024); |
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.
| const rateMiBs = props.volumeInitializationRate.toBytes() / (1024 * 1024); | |
| const rateMiBs = props.volumeInitializationRate.toMebibytes(); |
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.
Same as other comment, I think this is not worked for the Size less than 1 MiB.
| volumeInitializationRate: rate, | ||
| snapshotId: 'snap-0123456789abcdefABCDEF', | ||
| }); | ||
| }).toThrow(`volumeInitializationRate must be between 100 and 300 MiB/s, got: ${rate.toBytes() / (1024 * 1024)} MiB/s`); |
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.
| }).toThrow(`volumeInitializationRate must be between 100 and 300 MiB/s, got: ${rate.toBytes() / (1024 * 1024)} MiB/s`); | |
| }).toThrow(`volumeInitializationRate must be between 100 and 300 MiB/s, got: ${rate.toMebibytes()} MiB/s`); |
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 this code is not worked for the Size less than 1 MiB.
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.
why wouldn't it? internally it does the same thing
public static readonly Mebibytes = new StorageUnit('mebibytes', 1024 * 1024);
function convert(amount: number, fromUnit: StorageUnit, toUnit: StorageUnit, options: SizeConversionOptions = {}) {
const rounding = options.rounding ?? SizeRoundingBehavior.FAIL;
if (fromUnit.inBytes === toUnit.inBytes) { return amount; }
if (Token.isUnresolved(amount)) {
throw new Error(`Size must be specified as 'Size.${toUnit}()' here since its value comes from a token and cannot be converted (got Size.${fromUnit})`);
}
const multiplier = fromUnit.inBytes / toUnit.inBytes;
const value = amount * multiplier;
switch (rounding) {
case SizeRoundingBehavior.NONE:
return value;
case SizeRoundingBehavior.FLOOR:
return Math.floor(value);
default:
case SizeRoundingBehavior.FAIL:
if (!Number.isInteger(value)) {
throw new Error(`'${amount} ${fromUnit}' cannot be converted into a whole number of ${toUnit}.`);
}
return value;
}
}
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 apologize for the lack of clarity.
When a value of 1MiB or more is input, an intuitive error message is returned, but otherwise only '${amount} ${fromUnit}' cannot be converted into a whole number of ${toUnit}. is returned, making it difficult for users to understand where the problem lies.
I think it is better to display
volumeInitializationRate must be between 100 and 300 MiB/s, got: 0.5 MiB/s
than
'${amount} ${fromUnit}' cannot be converted into a whole number of ${toUnit}..
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.
SizeRoundingBehavior should allow you to modify that, the default is fail.
toMebibytes({ rounding: SizeRoundingBehavior.NONE })
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.
Thank you for nice information. I've updated to use it.
|
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. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
None
Reason for this change
AWS EBS now supports for specifying volume initialization rate but AWS CDK cannot configure this parameter.
Description of changes
volumeInitializationRatetoVolumePropsDescribe any new or updated permissions being added
none
Description of how you validated changes
Add both unit and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license