-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix(opensearch): add I4I and R7GD to list of OpenSearch nodes not requiring EBS volumes #32592
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
| // https://docs.aws.amazon.com/opensearch-service/latest/developerguide/supported-instance-types.html | ||
| if (isSomeInstanceType('i3', 'r6gd', 'i4g', 'im4gn') && ebsEnabled) { | ||
| throw new Error('I3, R6GD, I4G, and IM4GN instance types do not support EBS storage volumes.'); | ||
| if (isSomeInstanceType('i3', 'r6gd', 'i4g', 'i4i', 'im4gn', 'r7gd') && ebsEnabled) { |
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 would be nice to refactor these instance type lists and store them in a named constant, to improve readability and maintenance. You could also use their human-friendlier InstanceClass names, something like:
private static readonly UNSUPPORTED_EBS = [
ec2.InstanceClass.IO3,
ec2.InstanceClass.MEMORY6_GRAVITON2_NVME_DRIVE,
// ...
];
// ...
if(isSomeInstanceType(...UNSUPPORTED_EBS) && ebsEnabled) throwYou could also generate the error message using this list, without having to update them both manually:
function formatInstanceTypesList(instanceTypes: string[]): string {
return instanceTypes.map((type) => type.toUpperCase()).join(', ').replace(/, ([^,]*)$/, ' and $1');
}
throw new Error(`${formatInstanceTypesList(UNSUPPORTED_EBS)} instance types do not support EBS storage volumes.`);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. I took your suggestions into account and tried refactoring the relevant parts for this fix.
As for unrelated parts, I plan to refactor them in a separate PR.
| const domainProps: opensearch.DomainProps = { | ||
| removalPolicy: RemovalPolicy.DESTROY, | ||
| version: opensearch.EngineVersion.OPENSEARCH_2_17, | ||
| // specify the instance type that supports instance store | ||
| capacity: { | ||
| multiAzWithStandbyEnabled: false, | ||
| dataNodeInstanceType: instanceType, | ||
| dataNodes: 1, | ||
| }, | ||
| // force ebs configuration to be disabled | ||
| ebs: { | ||
| enabled: false, | ||
| }, | ||
| }; | ||
|
|
||
| new opensearch.Domain(this, `Domain${index + 1}`, domainProps); |
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.
The domainProps variable seems unnecessary, especially because it has to be typed to get IDE hints instead of inheriting the constructor's signature
| const domainProps: opensearch.DomainProps = { | |
| removalPolicy: RemovalPolicy.DESTROY, | |
| version: opensearch.EngineVersion.OPENSEARCH_2_17, | |
| // specify the instance type that supports instance store | |
| capacity: { | |
| multiAzWithStandbyEnabled: false, | |
| dataNodeInstanceType: instanceType, | |
| dataNodes: 1, | |
| }, | |
| // force ebs configuration to be disabled | |
| ebs: { | |
| enabled: false, | |
| }, | |
| }; | |
| new opensearch.Domain(this, `Domain${index + 1}`, domainProps); | |
| new opensearch.Domain(this, `Domain${index + 1}`, { | |
| removalPolicy: RemovalPolicy.DESTROY, | |
| version: opensearch.EngineVersion.OPENSEARCH_2_17, | |
| // specify the instance type that supports instance store | |
| capacity: { | |
| multiAzWithStandbyEnabled: false, | |
| dataNodeInstanceType: instanceType, | |
| dataNodes: 1, | |
| }, | |
| // force ebs configuration to be disabled | |
| ebs: { | |
| enabled: 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.
fixed.
| ]; | ||
|
|
||
| const supportInstanceStorageInstanceType = [ | ||
| ec2.InstanceClass.R3, |
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'm not sure whether R3 should be included in unSupportEbsInstanceType because I couldn't find any documentation, and it has already been deprecated.
To avoid breaking changes, I decided to separate unSupportEbsInstanceType and supportInstanceStorageInstanceType in line with the current implementation, just in case.
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.
You should still be able to deploy previous gen instances to test this, but it probably doesn't matter either way
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32592 +/- ##
=======================================
Coverage 80.54% 80.54%
=======================================
Files 106 106
Lines 6954 6954
Branches 1287 1287
=======================================
Hits 5601 5601
Misses 1175 1175
Partials 178 178
Flags with carried forward coverage won't be shown. Click here to find out more.
|
nmussy
left a comment
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, thanks for the refactor
|
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. |
Issue # (if applicable)
Closes #32070.
Closes #32138.
Reason for this change
I4IandR7GDinstances don't require ebs volumes.But at the moment, a domain with
I4IandR7GDinstances cannot be deployed.Description of changes
Added
I4IandR7GDto not requiring EBS volumes instances list.Describe any new or updated permissions being added
Add unit tests and integ tests.
<!— What new or updated IAM permissions are needed to support the changes being introduced ? -->
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license