-
Notifications
You must be signed in to change notification settings - Fork 36
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
Delete workflow state when template is deleted and no resources exist #689
Conversation
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #689 +/- ##
============================================
+ Coverage 72.56% 73.78% +1.21%
- Complexity 697 722 +25
============================================
Files 82 82
Lines 3605 3662 +57
Branches 309 312 +3
============================================
+ Hits 2616 2702 +86
+ Misses 845 818 -27
+ Partials 144 142 -2 ☔ View full report in Codecov by Sentry. |
Provisioned a no-op template: POST localhost:9200/_plugins/_flow_framework/workflow
{
"name": "test",
"description": "Flow template",
"use_case": "TESTING",
"version": {
"template": "1.0.0",
"compatibility": ["2.12.0", "3.0.0"]
},
"workflows": {
"provision": {
"user_params": {},
"nodes": [
{
"id": "no op",
"type": "noop"
}
]
}
}
} {
"workflow_id": "vZAyLY8BaPsQZP-UDULE"
} State exists: GET localhost:9200/_plugins/_flow_framework/workflow/vZAyLY8BaPsQZP-UDULE/_status?all=true {
"workflow_id": "vZAyLY8BaPsQZP-UDULE",
"state": "NOT_STARTED",
"provisioning_progress": "NOT_STARTED"
} Deleted template: DELETE localhost:9200/_plugins/_flow_framework/workflow/vZAyLY8BaPsQZP-UDULE {
"_index": ".plugins-flow-framework-templates",
"_id": "vZAyLY8BaPsQZP-UDULE",
"_version": 2,
"result": "deleted",
"_shards": {
"total": 1,
"successful": 1,
"failed": 0
},
"_seq_no": 1,
"_primary_term": 1
} State entry is gone! GET localhost:9200/_plugins/_flow_framework/workflow/vZAyLY8BaPsQZP-UDULE/_status?all=true {
"error": "Fail to find workflow vZAyLY8BaPsQZP-UDULE"
} Provisioned a template with resources (full template omitted):
State exists with resources localhost:9200/_plugins/_flow_framework/workflow/vpA5LY8BaPsQZP-UJkIb/_status?all=true {
"workflow_id": "vpA5LY8BaPsQZP-UJkIb",
"state": "COMPLETED",
"provisioning_progress": "DONE",
"provision_start_time": 1714450671376,
"provision_end_time": 1714450677712,
"resources_created": [
{
"workflow_step_name": "create_connector",
"resource_id": "v5A5LY8BaPsQZP-UK0Ia",
"resource_type": "connector_id",
"workflow_step_id": "create_connector_1"
},
{
"workflow_step_name": "register_remote_model",
"resource_id": "wpA5LY8BaPsQZP-UNUIO",
"resource_type": "model_id",
"workflow_step_id": "register_model_2"
},
{
"workflow_step_name": "deploy_model",
"resource_id": "wpA5LY8BaPsQZP-UNUIO",
"resource_type": "model_id",
"workflow_step_id": "register_model_2"
},
{
"workflow_step_name": "register_agent",
"resource_id": "xJA5LY8BaPsQZP-UPULJ",
"resource_type": "agent_id",
"workflow_step_id": "sub_agent"
},
{
"workflow_step_name": "register_agent",
"resource_id": "xZA5LY8BaPsQZP-UP0Ij",
"resource_type": "agent_id",
"workflow_step_id": "root_agent"
}
]
} Deleting the template: localhost:9200/_plugins/_flow_framework/workflow/vpA5LY8BaPsQZP-UJkIb {
"_index": ".plugins-flow-framework-templates",
"_id": "vpA5LY8BaPsQZP-UJkIb",
"_version": 3,
"result": "deleted",
"_shards": {
"total": 1,
"successful": 1,
"failed": 0
},
"_seq_no": 4,
"_primary_term": 1
} State is not deleted! localhost:9200/_plugins/_flow_framework/workflow/vpA5LY8BaPsQZP-UJkIb/_status?all=true {
"workflow_id": "vpA5LY8BaPsQZP-UJkIb",
"state": "COMPLETED",
"provisioning_progress": "DONE",
"provision_start_time": 1714450671376,
"provision_end_time": 1714450677712,
"resources_created": [
{
"workflow_step_name": "create_connector",
"resource_id": "v5A5LY8BaPsQZP-UK0Ia",
"resource_type": "connector_id",
"workflow_step_id": "create_connector_1"
},
{
"workflow_step_name": "register_remote_model",
"resource_id": "wpA5LY8BaPsQZP-UNUIO",
"resource_type": "model_id",
"workflow_step_id": "register_model_2"
},
{
"workflow_step_name": "deploy_model",
"resource_id": "wpA5LY8BaPsQZP-UNUIO",
"resource_type": "model_id",
"workflow_step_id": "register_model_2"
},
{
"workflow_step_name": "register_agent",
"resource_id": "xJA5LY8BaPsQZP-UPULJ",
"resource_type": "agent_id",
"workflow_step_id": "sub_agent"
},
{
"workflow_step_name": "register_agent",
"resource_id": "xZA5LY8BaPsQZP-UP0Ij",
"resource_type": "agent_id",
"workflow_step_id": "root_agent"
}
]
} Deprovisioning: localhost:9200/_plugins/_flow_framework/workflow/vpA5LY8BaPsQZP-UJkIb/_deprovision
State document deleted! localhost:9200/_plugins/_flow_framework/workflow/vpA5LY8BaPsQZP-UJkIb/_status?all=true
|
src/main/java/org/opensearch/flowframework/transport/DeprovisionWorkflowTransportAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/DeprovisionWorkflowTransportAction.java
Show resolved
Hide resolved
Can we add one integration test here also (we might already have one with delete/deprovision but we should now check this new logic is applied) |
src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java
Outdated
Show resolved
Hide resolved
Overall the fix looks good. IMO, we should also include a |
Can we make that a suggestion for 2.15? I'm keeping this PR to the context of bug fix, and a new API parameter is a new feature. |
Signed-off-by: Daniel Widdis <[email protected]>
src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/DeleteWorkflowTransportAction.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
…#689) * Add test for updating state index Signed-off-by: Daniel Widdis <[email protected]> * Add method to delete state document to FlowFrameworkIndidesHandler Signed-off-by: Daniel Widdis <[email protected]> * Delete state document on deprovision if no template exists Signed-off-by: Daniel Widdis <[email protected]> * Add check whether state can be deleted to FlowFrameworkIndicesHandler Signed-off-by: Daniel Widdis <[email protected]> * Conditionally delete state when deleting template Signed-off-by: Daniel Widdis <[email protected]> * Add IT to test workflow state deletion on deprovision/no-resources Signed-off-by: Daniel Widdis <[email protected]> * Fix typo Signed-off-by: Daniel Widdis <[email protected]> * Properly test ResponseException on Workflow State 404 Signed-off-by: Daniel Widdis <[email protected]> * Fetch workflowId of newly created template Signed-off-by: Daniel Widdis <[email protected]> * Stabilize IT Signed-off-by: Daniel Widdis <[email protected]> --------- Signed-off-by: Daniel Widdis <[email protected]> (cherry picked from commit ff072de) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…resources exist (#694) Delete workflow state when template is deleted and no resources exist (#689) * Add test for updating state index * Add method to delete state document to FlowFrameworkIndidesHandler * Delete state document on deprovision if no template exists * Add check whether state can be deleted to FlowFrameworkIndicesHandler * Conditionally delete state when deleting template * Add IT to test workflow state deletion on deprovision/no-resources * Fix typo * Properly test ResponseException on Workflow State 404 * Fetch workflowId of newly created template * Stabilize IT --------- (cherry picked from commit ff072de) Signed-off-by: Daniel Widdis <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
Deletes workflow state when:
Issues Resolved
Fixes #688
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.