-
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(opensearchservice): configuring gp3 throughput #26172
feat(opensearchservice): configuring gp3 throughput #26172
Conversation
…an opensearch cluster
@@ -1575,6 +1583,7 @@ export class Domain extends DomainBase implements IDomain, ec2.IConnectable { | |||
volumeSize: ebsEnabled ? volumeSize : undefined, | |||
volumeType: ebsEnabled ? volumeType : undefined, | |||
iops: ebsEnabled ? props.ebs?.iops : undefined, | |||
throughput: ebsEnabled ? props.ebs?.throughput : undefined, |
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.
It doesn't look like we have any check on the volumeType. It says iops and
throughput are only supported for iops/gp3 types, but we only check if it's ebs
type?
Do you know what happens if throughput
is defined for gp2
volumes? Does it
just ignore that property or does it throw an error?
I think we need something like what we have in the volume
construct
aws-cdk/packages/aws-cdk-lib/aws-ec2/lib/volume.ts
Lines 704 to 771 in 53d24f1
// Enforce maximum ratio of IOPS/GiB: | |
// https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ebs-volume-types.html | |
const maximumRatios: { [key: string]: number } = {}; | |
maximumRatios[EbsDeviceVolumeType.GENERAL_PURPOSE_SSD_GP3] = 500; | |
maximumRatios[EbsDeviceVolumeType.PROVISIONED_IOPS_SSD] = 50; | |
maximumRatios[EbsDeviceVolumeType.PROVISIONED_IOPS_SSD_IO2] = 500; | |
const maximumRatio = maximumRatios[volumeType]; | |
if (props.size && (props.iops > maximumRatio * props.size.toGibibytes({ rounding: SizeRoundingBehavior.FAIL }))) { | |
throw new Error(`\`${volumeType}\` volumes iops has a maximum ratio of ${maximumRatio} IOPS/GiB.`); | |
} | |
const maximumThroughputRatios: { [key: string]: number } = {}; | |
maximumThroughputRatios[EbsDeviceVolumeType.GP3] = 0.25; | |
const maximumThroughputRatio = maximumThroughputRatios[volumeType]; | |
if (props.throughput && props.iops) { | |
const iopsRatio = (props.throughput / props.iops); | |
if (iopsRatio > maximumThroughputRatio) { | |
throw new Error(`Throughput (MiBps) to iops ratio of ${iopsRatio} is too high; maximum is ${maximumThroughputRatio} MiBps per iops`); | |
} | |
} | |
} | |
if (props.enableMultiAttach) { | |
const volumeType = props.volumeType ?? EbsDeviceVolumeType.GENERAL_PURPOSE_SSD; | |
if ( | |
![ | |
EbsDeviceVolumeType.PROVISIONED_IOPS_SSD, | |
EbsDeviceVolumeType.PROVISIONED_IOPS_SSD_IO2, | |
].includes(volumeType) | |
) { | |
throw new Error('multi-attach is supported exclusively on `PROVISIONED_IOPS_SSD` and `PROVISIONED_IOPS_SSD_IO2` volumes.'); | |
} | |
} | |
if (props.size) { | |
const size = props.size.toGibibytes({ rounding: SizeRoundingBehavior.FAIL }); | |
// Enforce minimum & maximum volume size: | |
// https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-ebs-volume.html | |
const sizeRanges: { [key: string]: { Min: number, Max: number } } = {}; | |
sizeRanges[EbsDeviceVolumeType.GENERAL_PURPOSE_SSD] = { Min: 1, Max: 16384 }; | |
sizeRanges[EbsDeviceVolumeType.GENERAL_PURPOSE_SSD_GP3] = { Min: 1, Max: 16384 }; | |
sizeRanges[EbsDeviceVolumeType.PROVISIONED_IOPS_SSD] = { Min: 4, Max: 16384 }; | |
sizeRanges[EbsDeviceVolumeType.PROVISIONED_IOPS_SSD_IO2] = { Min: 4, Max: 16384 }; | |
sizeRanges[EbsDeviceVolumeType.THROUGHPUT_OPTIMIZED_HDD] = { Min: 125, Max: 16384 }; | |
sizeRanges[EbsDeviceVolumeType.COLD_HDD] = { Min: 125, Max: 16384 }; | |
sizeRanges[EbsDeviceVolumeType.MAGNETIC] = { Min: 1, Max: 1024 }; | |
const volumeType = props.volumeType ?? EbsDeviceVolumeType.GENERAL_PURPOSE_SSD; | |
const { Min, Max } = sizeRanges[volumeType]; | |
if (size < Min || size > Max) { | |
throw new Error(`\`${volumeType}\` volumes must be between ${Min} GiB and ${Max} GiB in size.`); | |
} | |
} | |
if (props.throughput) { | |
const throughputRange = { Min: 125, Max: 1000 }; | |
const { Min, Max } = throughputRange; | |
if (props.volumeType != EbsDeviceVolumeType.GP3) { | |
throw new Error( | |
'throughput property requires volumeType: EbsDeviceVolumeType.GP3', | |
); | |
} | |
if (props.throughput < Min || props.throughput > Max) { | |
throw new Error( | |
`throughput property takes a minimum of ${Min} and a maximum of ${Max}`, | |
); | |
} | |
} |
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 did think about checks specification checks, the other option for iops
didn't any checks defined so I didn't it for the throughput
option`. I'll get that sorted in the next commit :)
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.
Added a bunch of tests and validation checks for you @corymhall , let me know if there's anything i've missed.
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
@sktan The last build failed. Do you need help? |
Hey, thanks for the offer @jumic. I haven't gotten around to fixing it just yet, it failed due to another PR making a change to the default parameters causing my integration test to provide a This will get sorted soon |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
3 similar comments
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
@corymhall we should be good to go again for this PR |
// https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-opensearchservice-domain-ebsoptions.html | ||
if (ebsEnabled) { | ||
// Check if iops or throughput if general purpose is configured | ||
if (volumeType == ec2.EbsDeviceVolumeType.GENERAL_PURPOSE_SSD || volumeType == ec2.EbsDeviceVolumeType.STANDARD) { |
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.
if (volumeType == ec2.EbsDeviceVolumeType.GENERAL_PURPOSE_SSD || volumeType == ec2.EbsDeviceVolumeType.STANDARD) { | |
if (volumeType === ec2.EbsDeviceVolumeType.GENERAL_PURPOSE_SSD || volumeType === ec2.EbsDeviceVolumeType.STANDARD) { |
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). |
The interface EbsOptions for the opensearchservices CDK construct is missing a provisioned throughput option for eg gp3 instance types.
iops is there, but not throughput
Closes #26137.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license