fix(opensearch): add I4I and R7GD to list of OpenSearch nodes not requiring EBS volumes #32592

mazyu36 · 2024-12-19T14:12:16Z

Issue # (if applicable)

Closes #32070.
Closes #32138.

Reason for this change

I4I and R7GD instances don't require ebs volumes.
But at the moment, a domain with I4I and R7GD instances cannot be deployed.

Description of changes

Added I4I and R7GD to 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

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

nmussy · 2024-12-19T15:26:34Z

packages/aws-cdk-lib/aws-opensearchservice/lib/domain.ts

@@ -1576,8 +1576,8 @@ export class Domain extends DomainBase implements IDomain, ec2.IConnectable {

    // Validate against instance type restrictions, per
    // 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) {


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) throw

You 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.`);

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.

nmussy · 2024-12-19T15:29:46Z

...ges/@aws-cdk-testing/framework-integ/test/aws-opensearchservice/test/integ.opensearch.ebs.ts

+      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);


The domainProps variable seems unnecessary, especially because it has to be typed to get IDE hints instead of inheriting the constructor's signature

Suggested change

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,

},

});

mazyu36 · 2024-12-20T03:55:50Z

packages/aws-cdk-lib/aws-opensearchservice/lib/domain.ts

+    ];
+
+    const supportInstanceStorageInstanceType = [
+      ec2.InstanceClass.R3,


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.

You should still be able to deploy previous gen instances to test this, but it probably doesn't matter either way

codecov · 2024-12-20T04:00:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.54%. Comparing base (cb985ba) to head (7794ffe).
Report is 1 commits behind head on main.

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

Flag	Coverage Δ
suite.unit	`80.54% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`80.54% <ø> (ø)`

nmussy

LGTM, thanks for the refactor

mergify · 2024-12-23T21:42:57Z

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-cdk-automation · 2024-12-23T22:14:21Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 7794ffe
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-12-23T22:14:46Z

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).

github-actions · 2024-12-23T22:15:12Z

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.

add I4I and R7GD to list of OpenSearch nodes not requiring EBS volumes

64555d2

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Dec 19, 2024

aws-cdk-automation requested a review from a team December 19, 2024 14:12

github-actions bot added the distinguished-contributor [Pilot] contributed 50+ PRs to the CDK label Dec 19, 2024

Merge branch 'main' into fix-opensearch

1189a14

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 19, 2024

nmussy reviewed Dec 19, 2024

View reviewed changes

refactor

508f00a

mazyu36 commented Dec 20, 2024

View reviewed changes

mazyu36 requested a review from nmussy December 20, 2024 04:38

nmussy approved these changes Dec 20, 2024

View reviewed changes

aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Dec 20, 2024

moelasmar approved these changes Dec 23, 2024

View reviewed changes

Merge branch 'main' into fix-opensearch

7794ffe

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 23, 2024

mergify bot merged commit e364d2b into aws:main Dec 23, 2024
20 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Dec 23, 2024

mazyu36 deleted the fix-opensearch branch December 23, 2024 22:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(opensearch): add I4I and R7GD to list of OpenSearch nodes not requiring EBS volumes #32592

fix(opensearch): add I4I and R7GD to list of OpenSearch nodes not requiring EBS volumes #32592

mazyu36 commented Dec 19, 2024

nmussy Dec 19, 2024

mazyu36 Dec 20, 2024

nmussy Dec 19, 2024

mazyu36 Dec 20, 2024

mazyu36 Dec 20, 2024

nmussy Dec 20, 2024

codecov bot commented Dec 20, 2024 •

edited

Loading

nmussy left a comment

mergify bot commented Dec 23, 2024

aws-cdk-automation commented Dec 23, 2024

mergify bot commented Dec 23, 2024

github-actions bot commented Dec 23, 2024

fix(opensearch): add I4I and R7GD to list of OpenSearch nodes not requiring EBS volumes #32592

fix(opensearch): add I4I and R7GD to list of OpenSearch nodes not requiring EBS volumes #32592

Conversation

mazyu36 commented Dec 19, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

nmussy Dec 19, 2024

Choose a reason for hiding this comment

mazyu36 Dec 20, 2024

Choose a reason for hiding this comment

nmussy Dec 19, 2024

Choose a reason for hiding this comment

mazyu36 Dec 20, 2024

Choose a reason for hiding this comment

mazyu36 Dec 20, 2024

Choose a reason for hiding this comment

nmussy Dec 20, 2024

Choose a reason for hiding this comment

codecov bot commented Dec 20, 2024 • edited Loading

Codecov Report

nmussy left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 23, 2024

aws-cdk-automation commented Dec 23, 2024

AWS CodeBuild CI Report

mergify bot commented Dec 23, 2024

github-actions bot commented Dec 23, 2024

codecov bot commented Dec 20, 2024 •

edited

Loading