feat(opensearchservice): nodeoptions for domain by sPredictorX1708 · Pull Request #32936 · aws/aws-cdk

sPredictorX1708 · 2025-01-15T13:05:13Z

Issue # (if applicable)

Reason for this change

Recently as a part of coordinator node project we introduced a new parameter called NodeOptions, currently its part of L1 construct but we wanted it to be part of L2 construct and that is why raising this PR.

Description of changes

The code is very much similar to - #28497 and follows all standard practices in the repository.

Describe any new or updated permissions being added

N/A

Description of how you validated changes

Unit tests -

> yarn test aws-opensearchservice
yarn run v1.22.22
$ jest aws-opensearchservice
 PASS  aws-opensearchservice/test/log-group-resource-policy.test.ts
 PASS  aws-opensearchservice/test/opensearch-access-policy.test.ts
 PASS  aws-opensearchservice/test/domain.test.ts (5.687 s)

=============================== Coverage summary ===============================
Statements   : 43.87% ( 9437/21508 )
Branches     : 23.09% ( 2037/8820 )
Functions    : 25.62% ( 1229/4796 )
Lines        : 44.6% ( 9285/20816 )
================================================================================
Jest: "global" coverage threshold for statements (55%) not met: 43.87%
Jest: "global" coverage threshold for branches (35%) not met: 23.09%

Test Suites: 3 passed, 3 total
Tests:       1326 passed, 1326 total
Snapshots:   0 total
Time:        7.538 s

Integration Tests -

    Running in parallel across regions: us-east-1, us-east-2, us-west-2
    Running test /Users/dubesar/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-opensearchservice/test/integ.opensearch.nodeoptions.js in us-east-1
      SUCCESS    aws-opensearchservice/test/integ.opensearch.nodeoptions-OpenSearchInteg/DefaultTest 658.125s
           NO ASSERTIONS
     
    Test Results: 
     
    Tests:    1 passed, 1 total
    ✨  Done in 659.62s.

Snapshots is also generated as a part of integration tests

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

YES

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

### Reason for this change Introduced new interfaces and validation for configuring coordinator nodes in OpenSearch domains, allowing users to specify node options such as instance type and count. ### Description of changes - Added `NodeOptions` and `NodeConfig` interfaces to define configurations for coordinator nodes. - Updated the `Domain` class to handle `nodeOptions` in the capacity configuration, including validation for instance type and count. - Enhanced unit tests to cover new configurations and validation scenarios for coordinator nodes. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

lpizzinidev

👍 Left some comments for adjustments

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

packages/aws-cdk-lib/aws-opensearchservice/README.md

GavinZZ

Thank you for contributing, just a few clarifying questions.

GavinZZ · 2025-02-26T23:12:22Z

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

+  /**
+   * Configuration for the node type
+   */
+  readonly nodeConfig: NodeConfig;


Does this need to be a required property when the inner properties are all optional?

Can we rely on the backend to respond to this one? Meaning right now we have a logic on what is required.

eg - if enabled = true, then we need to pass type and count, if enabled = false, type and count become optional.

Will keep this logic as it as is for now.

Not sure how this is related to the backend.

This is a pretty nit comment anyway: I'm mainly talking about the usage like below. Since the properties of NodeConfig are all optional, we can make nodeConfig an optional property as well so users are not limited to

{ nodeType: someType, nodeConfig: {} }

and can also simplify this to below

{ nodeType: someType, }

GavinZZ · 2025-02-26T23:15:25Z

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

+      const coordinatorConfig = props.capacity.nodeOptions.find(opt => opt.nodeType === NodeType.COORDINATOR)?.nodeConfig;
+      if (coordinatorConfig?.enabled) {
+        const coordinatorType = initializeInstanceType(defaultCoordinatorInstanceType, coordinatorConfig.type);
+        if (!cdk.Token.isUnresolved(coordinatorType) && !coordinatorType.endsWith('.search')) {


is the unresolved token check needed as initializeInstanceType has done the check.

Followed the same as it was for warm, data, master. Can you please check

GavinZZ · 2025-02-26T23:17:07Z

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


+    if (props.capacity?.nodeOptions) {
+      // Validate coordinator node configuration
+      const coordinatorConfig = props.capacity.nodeOptions.find(opt => opt.nodeType === NodeType.COORDINATOR)?.nodeConfig;


I think find only returns the first matching item, what about the rest, are we going to do validation?

We can't have more than one nodeConfig object for a single node type

GavinZZ · 2025-02-26T23:18:08Z

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

  expect(metric.dimensions).toHaveProperty('DomainName');
 }
+
+each(testedOpenSearchVersions).test('can configure coordinator nodes with nodeOptions', (engineVersion) => {


can you add a test case where we have two NodeOptions items and the second one is has invalid type that doesn't end with .search.

We can't have two nodeoptions of same node type. The backend have checks for that and will throw error.

mergify · 2025-03-03T22:32:44Z

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

mergify · 2025-03-03T22:40:40Z

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

codecov · 2025-03-03T22:42:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.21%. Comparing base (97744e7) to head (d44a542).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32936   +/-   ##
=======================================
  Coverage   82.21%   82.21%           
=======================================
  Files         119      119           
  Lines        6876     6876           
  Branches     1162     1162           
=======================================
  Hits         5653     5653           
  Misses       1120     1120           
  Partials      103      103

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

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

Components	Coverage Δ
packages/aws-cdk	`∅ <ø> (∅)`
packages/aws-cdk-lib/core	`82.21% <ø> (ø)`

Pull request has been modified.

mergify · 2025-03-03T23:17:06Z

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 · 2025-03-03T23:47:55Z

AWS CodeBuild CI Report

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

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

mergify · 2025-03-03T23:48:13Z

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 · 2025-03-03T23:48:29Z

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.

dubesar1708 added 2 commits January 15, 2025 15:29

added integration tests

09f5281

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jan 15, 2025

aws-cdk-automation requested a review from a team January 15, 2025 13:05

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Jan 15, 2025

sPredictorX1708 mentioned this pull request Jan 15, 2025

Amazon OpenSearch Service: High Level Constructs For OpenSearch Coordinator (NodeOptions) Feature #32553

Closed

2 tasks

aws-cdk-automation previously requested changes Jan 15, 2025

View reviewed changes

added readme for nodeoptions

4d52b74

sPredictorX1708 requested a review from aws-cdk-automation January 15, 2025 13:18

dubesar1708 and others added 2 commits January 16, 2025 00:52

fixing build issue because of nodeoptions

d2c9026

Merge branch 'main' into feature/opensearch-nodeoptions

224d1c4

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

lpizzinidev suggested changes Jan 24, 2025

View reviewed changes

github-actions bot mentioned this pull request Feb 1, 2025

Monthly PR metrics report #33266

Closed

sPredictorX1708 and others added 2 commits February 20, 2025 22:23

Merge branch 'aws:main' into feature/opensearch-nodeoptions

fe1f2d6

Added nodeType Type and having coordinator as a possible value

fafa903

sPredictorX1708 requested a review from lpizzinidev February 20, 2025 17:12

dubesar1708 and others added 7 commits February 20, 2025 17:28

Fix the trailing spaces

7c920ad

feat(opensearch): use enum for NodeType in node options

d42e03e

Merge branch 'main' into feature/opensearch-nodeoptions

098e7e8

docs(opensearch): update README example to use NodeType enum

345483d

docs(opensearch): add documentation link for coordinator nodes

dd22c08

docs(opensearch): update documentation link for coordinator nodes

ca50c34

docs(opensearch): add import statement to coordinator nodes example

c88f138

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 22, 2025

GavinZZ suggested changes Feb 26, 2025

View reviewed changes

GavinZZ self-assigned this Feb 26, 2025

sPredictorX1708 requested a review from GavinZZ March 3, 2025 08:26

GavinZZ previously approved these changes Mar 3, 2025

View reviewed changes

Merge branch 'main' into feature/opensearch-nodeoptions

d44a542

sPredictorX1708 requested a review from GavinZZ March 3, 2025 22:54

GavinZZ approved these changes Mar 3, 2025

View reviewed changes

Merge branch 'main' into feature/opensearch-nodeoptions

33499c7

mergify bot merged commit 1b6f0c3 into aws:main Mar 3, 2025
10 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2025

Conversation

sPredictorX1708 commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

YES

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GavinZZ left a comment

Choose a reason for hiding this comment

Uh oh!

GavinZZ Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

sPredictorX1708 Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

GavinZZ Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GavinZZ Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

sPredictorX1708 Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

GavinZZ Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

sPredictorX1708 Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

GavinZZ Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

sPredictorX1708 Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Mar 3, 2025

Uh oh!

mergify bot commented Mar 3, 2025

Uh oh!

codecov bot commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mergify bot commented Mar 3, 2025

Uh oh!

aws-cdk-automation commented Mar 3, 2025

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Mar 3, 2025

Uh oh!

Uh oh!

github-actions bot commented Mar 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sPredictorX1708 commented Jan 15, 2025 •

edited

Loading

GavinZZ Mar 3, 2025 •

edited

Loading

sPredictorX1708 Mar 3, 2025 •

edited

Loading

codecov bot commented Mar 3, 2025 •

edited

Loading