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.

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

[Backport 2.x] Handle Not Found deprovision exceptions as successful deletions #806

Merged
merged 1 commit into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
- Register system index descriptors through SystemIndexPlugin.getSystemIndexDescriptors ([#750](https://github.com/opensearch-project/flow-framework/pull/750))

### Bug Fixes
- Handle Not Found exceptions as successful deletions for agents and models ([#805](https://github.com/opensearch-project/flow-framework/pull/805))

### Infrastructure
### Documentation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ Compatible with OpenSearch 2.16.0
- Add allow_delete parameter to Deprovision API ([#763](https://github.com/opensearch-project/flow-framework/pull/763))
- Improve Template and WorkflowState builders ([#778](https://github.com/opensearch-project/flow-framework/pull/778))

### Bug Fixes
- Handle Not Found deprovision exceptions as successful deletions ([#805](https://github.com/opensearch-project/flow-framework/pull/805))

### Infrastructure
- Update dependency com.fasterxml.jackson.core:jackson-core to v2.17.2 ([#760](https://github.com/opensearch-project/flow-framework/pull/760))
- Update dependency gradle to v8.8 ([#725](https://github.com/opensearch-project/flow-framework/pull/725))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.opensearch.ExceptionsHelper;
import org.opensearch.OpenSearchStatusException;
import org.opensearch.action.delete.DeleteResponse;
import org.opensearch.action.support.PlainActionFuture;
import org.opensearch.core.action.ActionListener;
import org.opensearch.core.rest.RestStatus;
import org.opensearch.flowframework.exception.FlowFrameworkException;
import org.opensearch.flowframework.exception.WorkflowStepException;
import org.opensearch.flowframework.util.ParseUtils;
Expand Down Expand Up @@ -85,9 +87,21 @@ public void onResponse(DeleteResponse deleteResponse) {
@Override
public void onFailure(Exception ex) {
Exception e = getSafeException(ex);
String errorMessage = (e == null ? "Failed to delete agent " + agentId : e.getMessage());
logger.error(errorMessage, e);
deleteAgentFuture.onFailure(new WorkflowStepException(errorMessage, ExceptionsHelper.status(e)));
// NOT_FOUND exception should be treated as successful deletion
// https://github.com/opensearch-project/ml-commons/issues/2751
if (e instanceof OpenSearchStatusException && ((OpenSearchStatusException) e).status().equals(RestStatus.NOT_FOUND)) {
deleteAgentFuture.onResponse(
new WorkflowData(
Map.ofEntries(Map.entry(AGENT_ID, agentId)),
currentNodeInputs.getWorkflowId(),
currentNodeInputs.getNodeId()
)
);
} else {
String errorMessage = (e == null ? "Failed to delete agent " + agentId : e.getMessage());
logger.error(errorMessage, e);
deleteAgentFuture.onFailure(new WorkflowStepException(errorMessage, ExceptionsHelper.status(e)));
}
}
});
} catch (FlowFrameworkException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.opensearch.ExceptionsHelper;
import org.opensearch.OpenSearchStatusException;
import org.opensearch.action.delete.DeleteResponse;
import org.opensearch.action.support.PlainActionFuture;
import org.opensearch.core.action.ActionListener;
import org.opensearch.core.rest.RestStatus;
import org.opensearch.flowframework.exception.FlowFrameworkException;
import org.opensearch.flowframework.exception.WorkflowStepException;
import org.opensearch.flowframework.util.ParseUtils;
Expand Down Expand Up @@ -86,9 +88,21 @@ public void onResponse(DeleteResponse deleteResponse) {
@Override
public void onFailure(Exception ex) {
Exception e = getSafeException(ex);
String errorMessage = (e == null ? "Failed to delete model " + modelId : e.getMessage());
logger.error(errorMessage, e);
deleteModelFuture.onFailure(new WorkflowStepException(errorMessage, ExceptionsHelper.status(e)));
// NOT_FOUND exception should be treated as successful deletion
// https://github.com/opensearch-project/ml-commons/issues/2751
if (e instanceof OpenSearchStatusException && ((OpenSearchStatusException) e).status().equals(RestStatus.NOT_FOUND)) {
deleteModelFuture.onResponse(
new WorkflowData(
Map.ofEntries(Map.entry(MODEL_ID, modelId)),
currentNodeInputs.getWorkflowId(),
currentNodeInputs.getNodeId()
)
);
} else {
String errorMessage = (e == null ? "Failed to delete model " + modelId : e.getMessage());
logger.error(errorMessage, e);
deleteModelFuture.onFailure(new WorkflowStepException(errorMessage, ExceptionsHelper.status(e)));
}
}
});
} catch (FlowFrameworkException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/
package org.opensearch.flowframework.workflow;

import org.opensearch.OpenSearchStatusException;
import org.opensearch.action.delete.DeleteResponse;
import org.opensearch.action.support.PlainActionFuture;
import org.opensearch.core.action.ActionListener;
Expand Down Expand Up @@ -73,6 +74,30 @@ public void testDeleteAgent() throws IOException, ExecutionException, Interrupte
assertEquals(agentId, future.get().getContent().get(AGENT_ID));
}

public void testDeleteAgentNotFound() throws IOException, ExecutionException, InterruptedException {

String agentId = randomAlphaOfLength(5);
DeleteAgentStep deleteAgentStep = new DeleteAgentStep(machineLearningNodeClient);

doAnswer(invocation -> {
ActionListener<DeleteResponse> actionListener = invocation.getArgument(1);
actionListener.onFailure(new OpenSearchStatusException("No agent found with that id", RestStatus.NOT_FOUND));
return null;
}).when(machineLearningNodeClient).deleteAgent(any(String.class), any());

PlainActionFuture<WorkflowData> future = deleteAgentStep.execute(
inputData.getNodeId(),
inputData,
Map.of("step_1", new WorkflowData(Map.of(AGENT_ID, agentId), "workflowId", "nodeId")),
Map.of("step_1", AGENT_ID),
Collections.emptyMap()
);
verify(machineLearningNodeClient).deleteAgent(any(String.class), any());

assertTrue(future.isDone());
assertEquals(agentId, future.get().getContent().get(AGENT_ID));
}

public void testNoAgentIdInOutput() throws IOException {
DeleteAgentStep deleteAgentStep = new DeleteAgentStep(machineLearningNodeClient);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/
package org.opensearch.flowframework.workflow;

import org.opensearch.OpenSearchStatusException;
import org.opensearch.action.delete.DeleteResponse;
import org.opensearch.action.support.PlainActionFuture;
import org.opensearch.core.action.ActionListener;
Expand Down Expand Up @@ -73,6 +74,30 @@ public void testDeleteModel() throws IOException, ExecutionException, Interrupte
assertEquals(modelId, future.get().getContent().get(MODEL_ID));
}

public void testDeleteModelNotFound() throws IOException, ExecutionException, InterruptedException {

String modelId = randomAlphaOfLength(5);
DeleteModelStep deleteModelStep = new DeleteModelStep(machineLearningNodeClient);

doAnswer(invocation -> {
ActionListener<DeleteResponse> actionListener = invocation.getArgument(1);
actionListener.onFailure(new OpenSearchStatusException("No model found with that id", RestStatus.NOT_FOUND));
return null;
}).when(machineLearningNodeClient).deleteModel(any(String.class), any());

PlainActionFuture<WorkflowData> future = deleteModelStep.execute(
inputData.getNodeId(),
inputData,
Map.of("step_1", new WorkflowData(Map.of(MODEL_ID, modelId), "workflowId", "nodeId")),
Map.of("step_1", MODEL_ID),
Collections.emptyMap()
);
verify(machineLearningNodeClient).deleteModel(any(String.class), any());

assertTrue(future.isDone());
assertEquals(modelId, future.get().getContent().get(MODEL_ID));
}

public void testNoModelIdInOutput() throws IOException {
DeleteModelStep deleteModelStep = new DeleteModelStep(machineLearningNodeClient);

Expand Down
Loading