applying sdkclient changes to config index #3521

dhrubo-os · 2025-02-10T23:42:50Z

Description

applying sdkclient (wrapper of client) to config index during index related operations

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Dhrubo Saha <[email protected]>

codecov · 2025-02-11T01:03:05Z

Codecov Report

Attention: Patch coverage is 83.07692% with 22 lines in your changes missing coverage. Please review.

Project coverage is 80.27%. Comparing base (d7dec0f) to head (c218da4).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
.../opensearch/ml/engine/encryptor/EncryptorImpl.java	82.94%	20 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3521      +/-   ##
============================================
+ Coverage     80.25%   80.27%   +0.02%     
- Complexity     6906     6921      +15     
============================================
  Files           610      610              
  Lines         30077    30143      +66     
  Branches       3368     3374       +6     
============================================
+ Hits          24137    24196      +59     
- Misses         4487     4495       +8     
+ Partials       1453     1452       -1

Flag	Coverage Δ
ml-commons	`80.27% <83.07%> (+0.02%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jngz-es · 2025-02-11T18:41:26Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/encryptor/EncryptorImpl.java

+    private PutDataObjectRequest createPutDataObjectRequest(String tenantId, String masterKeyId, String generatedMasterKey) {
+        return PutDataObjectRequest
+            .builder()
+            .tenantId(tenantId)


I see the below map stores DEFAULT_TENANT_ID, but here we are using tenantId, not sure why we don't keep it consistent.

so DEFAULT_TENANT_ID is required for tenantMasterKeys as we can't have any null key in the dictionary. But for single tenancy we can have null tenant id. Here the logic is for single tenancy (when tenantId is null), we add master key in the DEFAULT_TENANT_ID key to overcome the null key issue in the hashmap.

For reference, I answered a similar review question on Flow Framework. TLDR:

old system had a single string variable for non-multitenancy

new system has a key value map for multitenancy with tenantID as the key

options to support both in same codebase are to keep a separate String (outside the map) and map (with tenantId keys).

for simplicity, putting everything in the map avoids lots of conditionals everywhere

but the map can't have a null key. We ended up using the empty string as our "default" key. It doesn't really matter what string it is

jngz-es · 2025-02-11T18:46:52Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/encryptor/EncryptorImpl.java

+        sdkClient
+            .putDataObjectAsync(createPutDataObjectRequest(tenantId, masterKeyId, generatedMasterKey))
+            .whenComplete(
+                (putDataObjectResponse, throwable1) -> handlePutDataObjectResponse(


Looks like we don't check the response putDataObjectResponse in the function handlePutDataObjectResponse, is it what we expect?

jngz-es · 2025-02-11T19:01:37Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/encryptor/EncryptorImpl.java

+            exceptionRef.set(new ResourceNotFoundException(MASTER_KEY_NOT_READY_ERROR));
+            latch.countDown();
+        } else {
+            handleGetDataObjectSuccess(response1, tenantId, masterKeyId, exceptionRef, latch, context);


I think there is potential dead loop here. In the function handleGetDataObjectSuccess, if still getting empty response, the function initializeNewMasterKey will be executed again, then handleVersionConflictResponse -> handleGetDataObjectSuccess -> initializeNewMasterKey.
Actually in 2.18, the logic is that if getting VersionConflictEngineException, only re-get index once, won't retry creating index.

Signed-off-by: Dhrubo Saha <[email protected]>

xinyual · 2025-02-12T04:04:47Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/encryptor/EncryptorImpl.java

+            )
+            .whenComplete((response, throwable) -> {
+                try {
+                    handleVersionConflictResponse(tenantId, masterKeyId, context, response, throwable, exceptionRef, latch);


one question when handling version conflict. We call getDataObjectAsync(getDataObjectRequest) -> and if empty -> initializeNewMasterKey -> putDataObjectAsync -> find VersionConflictEngineException -> handleVersionConflict -> getDataObjectAsync -> handleVersionConflictResponse, does the two getDataObjectAsync totally same? Looks like they send same request. If so, we will also get empty and looks we cannot handle version conflict. Do I miss something?

* applying sdkclient changes to config index Signed-off-by: Dhrubo Saha <[email protected]> * addressed comments Signed-off-by: Dhrubo Saha <[email protected]> --------- Signed-off-by: Dhrubo Saha <[email protected]> (cherry picked from commit eccfd44)

opensearch-trigger-bot · 2025-02-12T23:12:47Z

The backport to 2.19 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.19 2.19
# Navigate to the new working tree
cd .worktrees/backport-2.19
# Create a new branch
git switch --create backport/backport-3521-to-2.19
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 eccfd4484c79555868022c761091ce649e999ab9
# Push it to GitHub
git push --set-upstream origin backport/backport-3521-to-2.19
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.19

Then, create a pull request where the base branch is 2.19 and the compare/head branch is backport/backport-3521-to-2.19.

* applying sdkclient changes to config index Signed-off-by: Dhrubo Saha <[email protected]> * addressed comments Signed-off-by: Dhrubo Saha <[email protected]> --------- Signed-off-by: Dhrubo Saha <[email protected]> (cherry picked from commit eccfd44)

* applying sdkclient changes to config index Signed-off-by: Dhrubo Saha <[email protected]> * addressed comments Signed-off-by: Dhrubo Saha <[email protected]> --------- Signed-off-by: Dhrubo Saha <[email protected]> (cherry picked from commit eccfd44) Co-authored-by: Dhrubo Saha <[email protected]>

dhrubo-os requested review from b4sjoo, mingshl, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27 and xinyual as code owners February 10, 2025 23:42

dhrubo-os had a problem deploying to ml-commons-cicd-env February 10, 2025 23:44 — with GitHub Actions Failure

applying sdkclient changes to config index

f2e29c1

Signed-off-by: Dhrubo Saha <[email protected]>

dhrubo-os force-pushed the remaining_multi_tenancy branch from 0452cb5 to f2e29c1 Compare February 11, 2025 00:02

dhrubo-os temporarily deployed to ml-commons-cicd-env February 11, 2025 00:04 — with GitHub Actions Inactive

dhrubo-os had a problem deploying to ml-commons-cicd-env February 11, 2025 00:04 — with GitHub Actions Failure

dhrubo-os temporarily deployed to ml-commons-cicd-env February 11, 2025 01:02 — with GitHub Actions Inactive

dhrubo-os temporarily deployed to ml-commons-cicd-env February 11, 2025 03:24 — with GitHub Actions Inactive

dbwiddis approved these changes Feb 11, 2025

View reviewed changes

jngz-es reviewed Feb 11, 2025

View reviewed changes

dhrubo-os force-pushed the remaining_multi_tenancy branch from 9f011fa to 61671ab Compare February 11, 2025 21:38

dhrubo-os had a problem deploying to ml-commons-cicd-env February 11, 2025 21:39 — with GitHub Actions Failure

addressed comments

c218da4

Signed-off-by: Dhrubo Saha <[email protected]>

dhrubo-os force-pushed the remaining_multi_tenancy branch from 61671ab to c218da4 Compare February 11, 2025 21:43

dhrubo-os temporarily deployed to ml-commons-cicd-env February 11, 2025 21:45 — with GitHub Actions Inactive

dhrubo-os had a problem deploying to ml-commons-cicd-env February 11, 2025 21:45 — with GitHub Actions Failure

dhrubo-os temporarily deployed to ml-commons-cicd-env February 11, 2025 22:43 — with GitHub Actions Inactive

dhrubo-os temporarily deployed to ml-commons-cicd-env February 11, 2025 23:25 — with GitHub Actions Inactive

dbwiddis approved these changes Feb 12, 2025

View reviewed changes

jngz-es approved these changes Feb 12, 2025

View reviewed changes

xinyual reviewed Feb 12, 2025

View reviewed changes

b4sjoo approved these changes Feb 12, 2025

View reviewed changes

dhrubo-os merged commit eccfd44 into opensearch-project:main Feb 12, 2025
10 checks passed

dhrubo-os added backport 2.x backport 2.19 labels Feb 12, 2025

opensearch-trigger-bot bot mentioned this pull request Feb 12, 2025

[Backport 2.x] applying sdkclient changes to config index #3534

Merged

opensearch-trigger-bot bot mentioned this pull request Feb 12, 2025

[Backport 2.19] applying sdkclient changes to config index #3535

Closed

dhrubo-os added backport 2.19 and removed backport 2.19 labels Feb 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

applying sdkclient changes to config index #3521

applying sdkclient changes to config index #3521

dhrubo-os commented Feb 10, 2025

codecov bot commented Feb 11, 2025 •

edited

Loading

jngz-es Feb 11, 2025

dhrubo-os Feb 11, 2025

dbwiddis Feb 12, 2025

jngz-es Feb 11, 2025

dhrubo-os Feb 11, 2025

jngz-es Feb 11, 2025

dhrubo-os Feb 11, 2025

xinyual Feb 12, 2025 •

edited

Loading

opensearch-trigger-bot bot commented Feb 12, 2025

applying sdkclient changes to config index #3521

applying sdkclient changes to config index #3521

Conversation

dhrubo-os commented Feb 10, 2025

Description

Related Issues

Check List

codecov bot commented Feb 11, 2025 • edited Loading

Codecov Report

jngz-es Feb 11, 2025

Choose a reason for hiding this comment

dhrubo-os Feb 11, 2025

Choose a reason for hiding this comment

dbwiddis Feb 12, 2025

Choose a reason for hiding this comment

jngz-es Feb 11, 2025

Choose a reason for hiding this comment

dhrubo-os Feb 11, 2025

Choose a reason for hiding this comment

jngz-es Feb 11, 2025

Choose a reason for hiding this comment

dhrubo-os Feb 11, 2025

Choose a reason for hiding this comment

xinyual Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Feb 12, 2025

codecov bot commented Feb 11, 2025 •

edited

Loading

xinyual Feb 12, 2025 •

edited

Loading