Undeploy models with no WorkerNodes #3380

brianf-aws · 2025-01-11T03:36:30Z

This PR aims to undeploy modelIds that have no nodes associated to them so as to keep the intention of undeploy truthful.

Description

When performing undeploy if the model has no nodes associated to it then it will reset the index to UNDEPLOY status

Here is an example of why this code change is needed

This secnario is for the PARTIALLY_DEPLOYED issue.

Have nodes a,b,c,d in cluster associated with modelID:@ i.e. peform deploy on it
Bring a,b down while having the syncup job running
By Now sync up will make this PARTIALLY_UNDEPLOYED
stop sync up
Bring other 2 c,d down and bring 2 nodes (these now have new ids 1,2)
bring the other two nodes back which have different ids so now the cluster has (1,2,3,4)
But the model index says PARTIALLY_DEPLOYED and no nodes are servicing

This code fix says. If no nodes are servicing this model then I need to set the index to UNDEPLOYED no matter if its already UNDEPLOYED or not.

Related Issues

Resolves #3285

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.

This commit aims to undeploy modelIds that have no nodes associated to them so as to keep the intention of undeploy truthful. Signed-off-by: Brian Flores <[email protected]>

ylwu-amzn · 2025-01-11T03:46:01Z

plugin/src/main/java/org/opensearch/ml/action/undeploy/TransportUndeployModelsAction.java

+        bulkUpdateRequest.setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE);
+        log.info("No models service: {}", modelIds.toString());
+        client.bulk(bulkUpdateRequest, ActionListener.wrap(br -> { log.debug("Successfully set modelIds to UNDEPLOY in index"); }, e -> {
+            log.error("Failed to set modelIds to UNDEPLOY in index", e);


Should we send this exception back to client side ? If yes, we should pass listener to this method and add this line here

listener.onFailure(e);

~~I'm not sure the user is concerned with the failure? The only way I see this as a problem if the model index does not exist and user does undeploy, this might cause a write issue.~~

Added your sugesstion to report the failure if it cant write back to the index.

mingshl · 2025-01-11T03:46:24Z

plugin/src/main/java/org/opensearch/ml/action/undeploy/TransportUndeployModelsAction.java

@@ -157,10 +163,36 @@ private void undeployModels(String[] targetNodeIds, String[] modelIds, ActionLis
        MLUndeployModelNodesRequest mlUndeployModelNodesRequest = new MLUndeployModelNodesRequest(targetNodeIds, modelIds);

        client.execute(MLUndeployModelAction.INSTANCE, mlUndeployModelNodesRequest, ActionListener.wrap(r -> {
+            if (r.getNodes().isEmpty()) {


it make sense that when execute undeploy model, the response return no worder nodes, then set the model to undeploy.

but this doesn't fix the partially undeployed issue, right?

PARTIALLY_UNDEPLOYED is a bit of a mixture of different scenarios one of them like so

Have nodes a,b,c,d in cluster associated with modelID:@ i.e. peform deploy on it

Bring a,b down while having the syncup job running

By Now sync up will make this PARTIALLY_UNDEPLOYED

stop sync up

Bring other 2 c,d down and bring 2 nodes (these now have new ids 1,2)

bring the other two nodes back which have different ids so now the cluster has (1,2,3,4)
But the model index says PARTIALLY_DEPLOYED and no nodes are servicing

This code fix says. If no nodes are servicing this model then I need to set the index to UNDEPLOYED no matter if its already UNDEPLOYED or not.

dhrubo-os · 2025-01-11T03:49:06Z

Can we add unit test?

Zhangxunmt · 2025-01-11T03:58:52Z

plugin/src/main/java/org/opensearch/ml/action/undeploy/TransportUndeployModelsAction.java

+        }
+        bulkUpdateRequest.setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE);
+        log.info("No models service: {}", modelIds.toString());
+        client.bulk(bulkUpdateRequest, ActionListener.wrap(br -> { log.debug("Successfully set modelIds to UNDEPLOY in index"); }, e -> {


I think you can return MLUndeployModelsResponse including the original nodes rather than empty. You can find some examples in the tests how to create a new MLUndeployModelsResponse.

If we don't return in br -> { log.debug("Successfully set modelIds to UNDEPLOY in index"); }, it's possible that when client side receive the undeploy response, the model still on DEPLOYED state.

Im not sure how I feel about creating a new one based on the failures,I think it will be misleading. I will pass the original response r instead.

The failures should just return a message. But for the success case we should return something rather than {}

if success case shows (to keep consistency with current output for partially deployed case):

{ "node id 1" : Not Found, "node id 2" : Not Found }

I don't think this will add much value from customer's POV.

But may be we can send a Success message to customer?

This will still give empty response which is not accurate. Since we cannot send nodes as response in this case, lets send something to show model/models undeployed successfully. Something like

{ <model_id_1>: "UNDEPLOYED SUCCESSFULLY", <model_id_2>: "UNDEPLOYED SUCCESSFULLY" }

We discussed internally that we would not want to send back this information as we don't want to break bwc if we send back a updated response.

Also I'm thinking if we write UNDEPLOYED Successfully, this may sound like it performed undeployement but the reality is that it is just updating the index and not performing any update on the nodes carrying the "model".

plugin/src/main/java/org/opensearch/ml/action/undeploy/TransportUndeployModelsAction.java

Now when entering this method its guaranteed to write to index first before sending back the MLUndeploy response. And will also send back a exception if the write back fails Signed-off-by: Brian Flores <[email protected]>

plugin/src/main/java/org/opensearch/ml/action/undeploy/TransportUndeployModelsAction.java

Added UTs for the 2 scenarios 1. Check that the bulk operation occured when no nodes are returned from the Undeploy response is , 2. Check that the bulk operation did not occur when there are nodes that have found the model within their cache. Signed-off-by: Brian Flores <[email protected]>

brianf-aws · 2025-01-12T04:35:29Z

Can we add unit test?

Added 2 UTs for code fix

Check that the bulk write occurred when undeploy returned {}. This is a sign that the stale model index is UNDEPLOYED
Check that bulk write did not occur when some nodes have a response to the model, undeploy occured and changed index.

ml-commons/plugin/src/main/java/org/opensearch/ml/action/undeploy/TransportUndeployModelAction.java

Lines 192 to 195 in 22b558d

    
           client.bulk(bulkRequest, ActionListener.runAfter(actionListener, () -> { 
        
               syncUpUndeployedModels(syncUpRequest); 
        
               listener.onResponse(undeployModelNodesResponse); 
        
           }));

plugin/src/main/java/org/opensearch/ml/action/undeploy/TransportUndeployModelsAction.java

Signed-off-by: Brian Flores <[email protected]>

ylwu-amzn · 2025-01-14T00:59:52Z

plugin/src/main/java/org/opensearch/ml/action/undeploy/TransportUndeployModelsAction.java

+            ActionListener<MLUndeployModelsResponse> listenerWithContextRestoration = ActionListener
+                .runBefore(listener, () -> threadContext.restore());
+            ActionListener<BulkResponse> bulkResponseListener = ActionListener.wrap(br -> {
+                log.debug("Successfully set modelIds to UNDEPLOY in index");


Add model ids to log?

ylwu-amzn · 2025-01-14T01:01:58Z

plugin/src/main/java/org/opensearch/ml/action/undeploy/TransportUndeployModelsAction.java

+        }
+
+        bulkUpdateRequest.setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE);
+        log.info("No nodes service: {}", Arrays.toString(modelIds));


How about clarifying more ?

Suggested change

log.info("No nodes service: {}", Arrays.toString(modelIds));

log.info("No nodes running these models: {}", Arrays.toString(modelIds));

Valid feedback added to commit 77f6e5b

Signed-off-by: Brian Flores <[email protected]>

undeploy models with no WorkerNodes

12e6aa7

This commit aims to undeploy modelIds that have no nodes associated to them so as to keep the intention of undeploy truthful. Signed-off-by: Brian Flores <[email protected]>

brianf-aws requested review from b4sjoo, dhrubo-os, mingshl, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27 and xinyual as code owners January 11, 2025 03:36

brianf-aws temporarily deployed to ml-commons-cicd-env-require-approval January 11, 2025 03:36 — with GitHub Actions Inactive

ylwu-amzn reviewed Jan 11, 2025

View reviewed changes

mingshl reviewed Jan 11, 2025

View reviewed changes

Zhangxunmt reviewed Jan 11, 2025

View reviewed changes

ylwu-amzn reviewed Jan 11, 2025

View reviewed changes

plugin/src/main/java/org/opensearch/ml/action/undeploy/TransportUndeployModelsAction.java Outdated Show resolved Hide resolved

brianf-aws requested a deployment to ml-commons-cicd-env-require-approval January 11, 2025 04:36 — with GitHub Actions Waiting

Exit early when no nodes service the model

9104cb8

Now when entering this method its guaranteed to write to index first before sending back the MLUndeploy response. And will also send back a exception if the write back fails Signed-off-by: Brian Flores <[email protected]>

brianf-aws had a problem deploying to ml-commons-cicd-env-require-approval January 11, 2025 04:51 — with GitHub Actions Failure

ylwu-amzn reviewed Jan 11, 2025

View reviewed changes

plugin/src/main/java/org/opensearch/ml/action/undeploy/TransportUndeployModelsAction.java Outdated Show resolved Hide resolved

brianf-aws temporarily deployed to ml-commons-cicd-env-require-approval January 12, 2025 04:30 — with GitHub Actions Inactive

dhrubo-os reviewed Jan 12, 2025

View reviewed changes

plugin/src/main/java/org/opensearch/ml/action/undeploy/TransportUndeployModelsAction.java Show resolved Hide resolved

brianf-aws had a problem deploying to ml-commons-cicd-env-require-approval January 12, 2025 07:39 — with GitHub Actions Failure

pyek-bot reviewed Jan 13, 2025

View reviewed changes

plugin/src/main/java/org/opensearch/ml/action/undeploy/TransportUndeployModelsAction.java Outdated Show resolved Hide resolved

update code change with comment explaining the change

fc8ded6

Signed-off-by: Brian Flores <[email protected]>

brianf-aws temporarily deployed to ml-commons-cicd-env-require-approval January 13, 2025 19:55 — with GitHub Actions Inactive

brianf-aws had a problem deploying to ml-commons-cicd-env-require-approval January 13, 2025 19:55 — with GitHub Actions Failure

brianf-aws requested a deployment to ml-commons-cicd-env-require-approval January 13, 2025 20:57 — with GitHub Actions Waiting

add context stash/restore to write operation

6323086

Signed-off-by: Brian Flores <[email protected]>

brianf-aws had a problem deploying to ml-commons-cicd-env-require-approval January 14, 2025 00:44 — with GitHub Actions Failure

Apply spotless

46363ef

Signed-off-by: Brian Flores <[email protected]>

brianf-aws temporarily deployed to ml-commons-cicd-env-require-approval January 14, 2025 00:45 — with GitHub Actions Inactive

ylwu-amzn reviewed Jan 14, 2025

View reviewed changes

Add better logging to write request

77f6e5b

Signed-off-by: Brian Flores <[email protected]>

brianf-aws temporarily deployed to ml-commons-cicd-env-require-approval January 14, 2025 01:09 — with GitHub Actions Inactive

brianf-aws requested a deployment to ml-commons-cicd-env-require-approval January 14, 2025 03:30 — with GitHub Actions Waiting

wrap exception into 5xx

24896bf

Signed-off-by: Brian Flores <[email protected]>

brianf-aws temporarily deployed to ml-commons-cicd-env-require-approval January 14, 2025 05:13 — with GitHub Actions Inactive

brianf-aws deployed to ml-commons-cicd-env-require-approval January 14, 2025 18:21 — with GitHub Actions Active

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Undeploy models with no WorkerNodes #3380

Undeploy models with no WorkerNodes #3380

brianf-aws commented Jan 11, 2025 •

edited

Loading

ylwu-amzn Jan 11, 2025

dhrubo-os Jan 11, 2025

brianf-aws Jan 11, 2025 •

edited

Loading

mingshl Jan 11, 2025

brianf-aws Jan 11, 2025

dhrubo-os commented Jan 11, 2025

Zhangxunmt Jan 11, 2025

ylwu-amzn Jan 11, 2025

brianf-aws Jan 11, 2025

Zhangxunmt Jan 11, 2025

dhrubo-os Jan 11, 2025 •

edited

Loading

rbhavna Jan 13, 2025

brianf-aws Jan 14, 2025

brianf-aws commented Jan 12, 2025

ylwu-amzn Jan 14, 2025

ylwu-amzn Jan 14, 2025

brianf-aws Jan 14, 2025

	log.info("No nodes service: {}", Arrays.toString(modelIds));
	log.info("No nodes running these models: {}", Arrays.toString(modelIds));

Undeploy models with no WorkerNodes #3380

Are you sure you want to change the base?

Undeploy models with no WorkerNodes #3380

Conversation

brianf-aws commented Jan 11, 2025 • edited Loading

Description

Related Issues

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianf-aws Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhrubo-os commented Jan 11, 2025

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

dhrubo-os Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianf-aws commented Jan 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianf-aws commented Jan 11, 2025 •

edited

Loading

brianf-aws Jan 11, 2025 •

edited

Loading

dhrubo-os Jan 11, 2025 •

edited

Loading