applying multi-tenancy to task apis, deploy, predict apis #3416

dhrubo-os · 2025-01-21T23:54:48Z

Description

[applying multi-tenancy to task apis, deploy, predict apis]

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 <dhrubo@amazon.com>

dbwiddis

Overall LGTM pending fix of the comments indicated.

dbwiddis · 2025-01-22T02:58:42Z

common/src/main/java/org/opensearch/ml/common/MLTask.java

@@ -305,6 +318,8 @@ public static MLTask parse(XContentParser parser) throws IOException {
                case REMOTE_JOB_FIELD:
                    remoteJob = parser.map();
                    break;
+                case TENANT_ID_FIELD:
+                    tenantId = parser.textOrNull();


missing a break; here.

dbwiddis · 2025-01-22T03:02:11Z

common/src/main/java/org/opensearch/ml/common/transport/deploy/MLDeployModelInput.java

        this.modelId = in.readString();
        this.taskId = in.readString();
+        this.tenantId = streamInputVersion.onOrAfter(VERSION_2_19_0) ? in.readOptionalString() : null;


(Style) I guess this works but I've nearly always seen the new fields added at the end of the stream.

(Same comment on the writeTo() and MLForwardInput later.)

Since they match it won't create a problem, but from a readability standpoint it requires some extra vigilance.

Agree. Addressing this.

dbwiddis · 2025-01-22T03:09:03Z

plugin/src/main/java/org/opensearch/ml/action/deploy/TransportDeployModelAction.java

                FunctionName functionName = mlModel.getAlgorithm();
                Boolean isHidden = mlModel.getIsHidden();
+                if (!TenantAwareHelper.validateTenantResource(mlFeatureEnabledSetting, tenantId, mlModel.getTenantId(), listener)) {


Less a comment for this PR and more one to think about for later. This returns 401 (Unauthorized) but we might want to consider returning 404 (Not Found) in many cases when trying to manipulate a tenant resource that you don't own.

Yeah that can be an option. We can decide later and address that in a separate PR.

dbwiddis · 2025-01-22T03:12:19Z

plugin/src/main/java/org/opensearch/ml/action/deploy/TransportDeployModelAction.java

                if (functionName == FunctionName.REMOTE && !mlFeatureEnabledSetting.isRemoteInferenceEnabled()) {
                    throw new IllegalStateException(REMOTE_INFERENCE_DISABLED_ERR_MSG);
                } else if (FunctionName.isDLModel(functionName) && !mlFeatureEnabledSetting.isLocalModelEnabled()) {
                    throw new IllegalStateException(LOCAL_MODEL_DISABLED_ERR_MSG);
                }
                if (!isUserInitiatedDeployRequest) {
-                    deployModel(deployModelRequest, mlModel, modelId, wrappedListener, listener);
+                    deployModel(deployModelRequest, mlModel, modelId, tenantId, wrappedListener, listener);
                } else if (isHidden != null && isHidden) {


Not your change, but this readability breaks my brain.

Suggested change

} else if (isHidden != null && isHidden) {

} else if (Boolean.TRUE.equals(isHidden)) {

dbwiddis · 2025-01-22T03:14:41Z

plugin/src/main/java/org/opensearch/ml/action/tasks/DeleteTaskTransportAction.java

+            ActionListener<DeleteResponse> wrappedListener = ActionListener.runBefore(actionListener, context::restore);
+
+            sdkClient
+                .getDataObjectAsync(getDataObjectRequest, client.threadPool().executor(GENERAL_THREAD_POOL))


I thought we were avoiding using the thread pool here?

missed that, addressing it.

dbwiddis · 2025-01-22T03:18:49Z

plugin/src/main/java/org/opensearch/ml/action/tasks/DeleteTaskTransportAction.java

+            sdkClient
+                .deleteDataObjectAsync(
+                    DeleteDataObjectRequest.builder().index(deleteRequest.index()).id(deleteRequest.id()).build(),
+                    client.threadPool().executor(GENERAL_THREAD_POOL)


same comment on thread pool

dbwiddis · 2025-01-22T03:20:36Z

plugin/src/main/java/org/opensearch/ml/autoredeploy/MLModelAutoReDeployer.java

            // allow custom deploy plan and not deploy to all case, we need to check if the added nodes in planning worker nodes.
            List<String> needRedeployPlanningWorkerNodes = Arrays
                .stream(planningWorkerNodes.toArray(new String[0]))
                .filter(addedNodes::contains)
-                .collect(Collectors.toList());
-            nodeIds = needRedeployPlanningWorkerNodes.size() > 0 ? planningWorkerNodes.toArray(new String[0]) : null;
+                .toList();


This works on JDK21 but since this will be backported you may want to keep the old version

good call out.

dbwiddis · 2025-01-22T03:24:21Z

plugin/src/test/java/org/opensearch/ml/action/deploy/TransportDeployModelActionTests.java

            listener.onResponse(mlModel);
            return null;
-        }).when(mlModelManager).getModel(anyString(), isNull(), any(String[].class), Mockito.isA(ActionListener.class));
+        }).when(mlModelManager).getModel(anyString(), any(), isNull(), any(String[].class), Mockito.isA(ActionListener.class));


picked a random one of these to comment on. I've been using nullable(String.class) for the tenant ID, but whatever you do, be consistent. any() should work unless you get an argument conflict.

Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

ylwu-amzn

See Dan already approved. Approve directly as maintainer to unblock the merge.

opensearch-trigger-bot · 2025-01-22T22:17:42Z

The backport to 2.x 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.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-3416-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1659a60d8c7e74471a57fa9c2c243aca214c6005
# Push it to GitHub
git push --set-upstream origin backport/backport-3416-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3416-to-2.x.

…-project#3416) * applying multi-tenancy to task, deploy, predict Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressed comments Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> --------- Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

) * applying multi-tenancy to task, deploy, predict * addressed comments --------- Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

applying multi-tenancy to task, deploy, predict

2dd953a

Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

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

dhrubo-os temporarily deployed to ml-commons-cicd-env January 21, 2025 23:56 — with GitHub Actions Inactive

dhrubo-os had a problem deploying to ml-commons-cicd-env January 21, 2025 23:56 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env January 22, 2025 00:30 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env January 22, 2025 02:44 — with GitHub Actions Failure

dbwiddis approved these changes Jan 22, 2025

View reviewed changes

dhrubo-os had a problem deploying to ml-commons-cicd-env January 22, 2025 17:56 — with GitHub Actions Failure

dhrubo-os force-pushed the main branch from 097ed4a to 9de0582 Compare January 22, 2025 18:01

dhrubo-os had a problem deploying to ml-commons-cicd-env January 22, 2025 18:02 — with GitHub Actions Failure

addressed comments

24317cb

Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

dhrubo-os force-pushed the main branch from 9de0582 to 24317cb Compare January 22, 2025 18:36

dhrubo-os had a problem deploying to ml-commons-cicd-env January 22, 2025 18:38 — with GitHub Actions Failure

dhrubo-os temporarily deployed to ml-commons-cicd-env January 22, 2025 18:38 — with GitHub Actions Inactive

dhrubo-os temporarily deployed to ml-commons-cicd-env January 22, 2025 19:37 — with GitHub Actions Inactive

dhrubo-os temporarily deployed to ml-commons-cicd-env January 22, 2025 20:18 — with GitHub Actions Inactive

ylwu-amzn approved these changes Jan 22, 2025

View reviewed changes

rbhavna approved these changes Jan 22, 2025

View reviewed changes

dhrubo-os merged commit 1659a60 into opensearch-project:main Jan 22, 2025
8 checks passed

dhrubo-os added the backport 2.x label Jan 22, 2025

dhrubo-os mentioned this pull request Jan 22, 2025

[Backport to 2.x] applying multi-tenancy to task apis, deploy, predict apis (#3416) #3420

Merged

5 tasks

mingshl pushed a commit that referenced this pull request Jan 23, 2025

applying multi-tenancy to task apis, deploy, predict apis (#3416) (#3420

ed36d6e

) * applying multi-tenancy to task, deploy, predict * addressed comments --------- Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

Hailong-am mentioned this pull request Jan 24, 2025

add multi tenancy support opensearch-project/skills#489

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

applying multi-tenancy to task apis, deploy, predict apis #3416

applying multi-tenancy to task apis, deploy, predict apis #3416

dhrubo-os commented Jan 21, 2025

dbwiddis left a comment

dbwiddis Jan 22, 2025

dbwiddis Jan 22, 2025

dhrubo-os Jan 22, 2025

dbwiddis Jan 22, 2025

dhrubo-os Jan 22, 2025

dbwiddis Jan 22, 2025

dbwiddis Jan 22, 2025

dhrubo-os Jan 22, 2025

dbwiddis Jan 22, 2025

dbwiddis Jan 22, 2025

dhrubo-os Jan 22, 2025

dbwiddis Jan 22, 2025

ylwu-amzn left a comment

opensearch-trigger-bot bot commented Jan 22, 2025

	} else if (isHidden != null && isHidden) {
	} else if (Boolean.TRUE.equals(isHidden)) {

applying multi-tenancy to task apis, deploy, predict apis #3416

applying multi-tenancy to task apis, deploy, predict apis #3416

Conversation

dhrubo-os commented Jan 21, 2025

Description

Related Issues

Check List

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylwu-amzn left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Jan 22, 2025