Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 4 commits into from
Dec 23, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
64555d2
add I4I and R7GD to list of OpenSearch nodes not requiring EBS volumes
mazyu36 Dec 19, 2024
1189a14
Merge branch 'main' into fix-opensearch
mazyu36 Dec 19, 2024
508f00a
refactor
mazyu36 Dec 20, 2024
7794ffe
Merge branch 'main' into fix-opensearch
mergify[bot] Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...ice/test/integ.opensearch.ebs.js.snapshot/cdk-integ-opensearch-instance-store.assets.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

62 changes: 60 additions & 2 deletions ...e/test/integ.opensearch.ebs.js.snapshot/cdk-integ-opensearch-instance-store.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"Resources": {
"Domain66AC69E0": {
"Domain19FCBCB91": {
"Type": "AWS::OpenSearchService::Domain",
"Properties": {
"ClusterConfig": {
Expand All @@ -20,7 +20,65 @@
"EncryptionAtRestOptions": {
"Enabled": false
},
"EngineVersion": "OpenSearch_2.5",
"EngineVersion": "OpenSearch_2.17",
"LogPublishingOptions": {},
"NodeToNodeEncryptionOptions": {
"Enabled": false
}
},
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
},
"Domain2644FE48C": {
"Type": "AWS::OpenSearchService::Domain",
"Properties": {
"ClusterConfig": {
"DedicatedMasterEnabled": false,
"InstanceCount": 1,
"InstanceType": "i4i.xlarge.search",
"MultiAZWithStandbyEnabled": false,
"ZoneAwarenessEnabled": false
},
"DomainEndpointOptions": {
"EnforceHTTPS": false,
"TLSSecurityPolicy": "Policy-Min-TLS-1-0-2019-07"
},
"EBSOptions": {
"EBSEnabled": false
},
"EncryptionAtRestOptions": {
"Enabled": false
},
"EngineVersion": "OpenSearch_2.17",
"LogPublishingOptions": {},
"NodeToNodeEncryptionOptions": {
"Enabled": false
}
},
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
},
"Domain3BE108C7A": {
"Type": "AWS::OpenSearchService::Domain",
"Properties": {
"ClusterConfig": {
"DedicatedMasterEnabled": false,
"InstanceCount": 1,
"InstanceType": "r7gd.xlarge.search",
"MultiAZWithStandbyEnabled": false,
"ZoneAwarenessEnabled": false
},
"DomainEndpointOptions": {
"EnforceHTTPS": false,
"TLSSecurityPolicy": "Policy-Min-TLS-1-0-2019-07"
},
"EBSOptions": {
"EBSEnabled": false
},
"EncryptionAtRestOptions": {
"Enabled": false
},
"EngineVersion": "OpenSearch_2.17",
"LogPublishingOptions": {},
"NodeToNodeEncryptionOptions": {
"Enabled": false
Expand Down
20 changes: 15 additions & 5 deletions ...work-integ/test/aws-opensearchservice/test/integ.opensearch.ebs.js.snapshot/manifest.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

100 changes: 95 additions & 5 deletions ...ramework-integ/test/aws-opensearchservice/test/integ.opensearch.ebs.js.snapshot/tree.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 19 additions & 16 deletions .../@aws-cdk-testing/framework-integ/test/aws-opensearchservice/test/integ.opensearch.ebs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,26 @@ class TestStack extends Stack {
constructor(scope: Construct, id: string, props?: StackProps) {
super(scope, id, props);

// deploy the latest opensearch domain with instance store only (no EBS)
const domainProps: opensearch.DomainProps = {
removalPolicy: RemovalPolicy.DESTROY,
version: opensearch.EngineVersion.OPENSEARCH_2_5,
// specify the instance type that supports instance store
capacity: {
multiAzWithStandbyEnabled: false,
dataNodeInstanceType: 'i4g.large.search',
dataNodes: 1,
},
// force ebs configuration to be disabled
ebs: {
enabled: false,
},
};
const instanceTypes = ['i4g.large.search', 'i4i.xlarge.search', 'r7gd.xlarge.search'];

new opensearch.Domain(this, 'Domain', domainProps);
instanceTypes.forEach((instanceType, index) => {
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);
Copy link
Contributor

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

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,
},
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

});
}
}

Expand Down
10 changes: 5 additions & 5 deletions packages/aws-cdk-lib/aws-opensearchservice/lib/domain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

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

Copy link
Contributor Author

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.

throw new Error('I3, R6GD, I4G, I4I, IM4GN and R7GD instance types do not support EBS storage volumes.');
}

if (isSomeInstanceType('m3', 'r3', 't2') && encryptionAtRestEnabled) {
Expand All @@ -1592,10 +1592,10 @@ export class Domain extends DomainBase implements IDomain, ec2.IConnectable {
throw new Error('T2 and T3 instance types do not support UltraWarm storage.');
}

// Only R3, I3, R6GD, I4G and IM4GN support instance storage, per
// Only R3, I3, R6GD, I4G, I4I, IM4GN and R7GD support instance storage, per
// https://aws.amazon.com/opensearch-service/pricing/
if (!ebsEnabled && !isEveryDatanodeInstanceType('r3', 'i3', 'r6gd', 'i4g', 'im4gn')) {
throw new Error('EBS volumes are required when using instance types other than R3, I3, R6GD, I4G, or IM4GN.');
if (!ebsEnabled && !isEveryDatanodeInstanceType('r3', 'i3', 'r6gd', 'i4g', 'i4i', 'im4gn', 'r7gd')) {
throw new Error('EBS volumes are required when using instance types other than R3, I3, R6GD, I4G, I4I, M4GN or R7GD.');
}

// Only for a valid ebs volume configuration, per
Expand Down
Loading
Loading