From d14b1ccfd2257cb61586b42eca712ad7991c2332 Mon Sep 17 00:00:00 2001 From: Nishtha Mehrotra Date: Tue, 7 Jan 2025 12:17:28 -0800 Subject: [PATCH 1/5] Delete workflow metadata on deleting workflow Signed-off-by: Nishtha Mehrotra --- .../transport/TransportDeleteWorkflowAction.kt | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportDeleteWorkflowAction.kt b/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportDeleteWorkflowAction.kt index 8fb2533fb..38af085cb 100644 --- a/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportDeleteWorkflowAction.kt +++ b/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportDeleteWorkflowAction.kt @@ -137,11 +137,12 @@ class TransportDeleteWorkflowAction @Inject constructor( if (canDelete) { val delegateMonitorIds = (workflow.inputs[0] as CompositeInput).getMonitorIds() var deletableMonitors = listOf() + val delegateMonitors = getDeletableDelegates(workflowId, delegateMonitorIds, user) // User can only delete the delegate monitors only in the case if all monitors can be deleted // if there are monitors in this workflow that are referenced in other workflows, we cannot delete the monitors. // We will not partially delete monitors. we delete them all or fail the request. if (deleteDelegateMonitors == true) { - deletableMonitors = getDeletableDelegates(workflowId, delegateMonitorIds, user) + deletableMonitors = delegateMonitors val monitorsDiff = delegateMonitorIds.toMutableList() monitorsDiff.removeAll(deletableMonitors.map { it.id }) @@ -168,10 +169,11 @@ class TransportDeleteWorkflowAction @Inject constructor( val failedMonitorIds = tryDeletingMonitors(deletableMonitors, RefreshPolicy.IMMEDIATE) // Update delete workflow response deleteWorkflowResponse.nonDeletedMonitors = failedMonitorIds - // Delete monitors workflow metadata - // Monitor metadata will be in workflowId-monitorId-metadata format - metadataIdsToDelete.addAll(deletableMonitors.map { MonitorMetadata.getId(it, workflowMetadataId) }) } + + // Delete monitors workflow metadata + // Monitor metadata will be in workflowId-monitorId-metadata format + metadataIdsToDelete.addAll(delegateMonitors.map { MonitorMetadata.getId(it, workflowMetadataId) }) try { // Delete the monitors workflow metadata val deleteMonitorWorkflowMetadataResponse: BulkByScrollResponse = client.suspendUntil { From 722de742aec5610ca9029c27efb615acbbfd602e Mon Sep 17 00:00:00 2001 From: Nishtha Mehrotra Date: Tue, 14 Jan 2025 17:12:09 -0800 Subject: [PATCH 2/5] Added UTs for ensuring monitor workflow metadata is deleted on deleting monitor workflow Signed-off-by: Nishtha Mehrotra --- .../alerting/MonitorDataSourcesIT.kt | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt b/alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt index ae5619879..8c92993cc 100644 --- a/alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt +++ b/alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt @@ -5053,6 +5053,9 @@ class MonitorDataSourcesIT : AlertingSingleNodeTestCase() { assertNotNull(getWorkflowResponse) assertEquals(workflowId, getWorkflowResponse.id) + // Verify that monitor workflow metadata exists + assertNotNull(searchMonitorMetadata("${workflowResponse.id}-metadata-${monitorResponse.id}-metadata")) + deleteWorkflow(workflowId, false) // Verify that the workflow is deleted try { @@ -5068,6 +5071,19 @@ class MonitorDataSourcesIT : AlertingSingleNodeTestCase() { // Verify that the monitor is not deleted val existingDelegate = getMonitorResponse(monitorResponse.id) assertNotNull(existingDelegate) + + // Verify that the monitor workflow metadata is deleted + try { + searchMonitorMetadata("${workflowResponse.id}-metadata-${monitorResponse.id}-metadata") + fail("expected searchMonitorMetadata method to throw exception") + } catch (e: Exception) { + e.message?.let { + assertTrue( + "Exception not returning GetMonitor Action error ", + it.contains("List is empty") + ) + } + } } fun `test delete workflow delegate monitor deleted`() { @@ -5093,6 +5109,9 @@ class MonitorDataSourcesIT : AlertingSingleNodeTestCase() { assertNotNull(getWorkflowResponse) assertEquals(workflowId, getWorkflowResponse.id) + // Verify that monitor workflow metadata exists + assertNotNull(searchMonitorMetadata("${workflowResponse.id}-metadata-${monitorResponse.id}-metadata")) + deleteWorkflow(workflowId, true) // Verify that the workflow is deleted try { @@ -5116,6 +5135,18 @@ class MonitorDataSourcesIT : AlertingSingleNodeTestCase() { ) } } + // Verify that the monitor workflow metadata is deleted + try { + searchMonitorMetadata("${workflowResponse.id}-metadata-${monitorResponse.id}-metadata") + fail("expected searchMonitorMetadata method to throw exception") + } catch (e: Exception) { + e.message?.let { + assertTrue( + "Exception not returning GetMonitor Action error ", + it.contains("List is empty") + ) + } + } } fun `test delete executed workflow with metadata deleted`() { From bfaa80d8753f5c4ad3b318f2bb447b99739aa252 Mon Sep 17 00:00:00 2001 From: Nishtha Mehrotra Date: Tue, 14 Jan 2025 17:18:45 -0800 Subject: [PATCH 3/5] Updated documentation notes Signed-off-by: Nishtha Mehrotra --- .../alerting/transport/TransportDeleteWorkflowAction.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportDeleteWorkflowAction.kt b/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportDeleteWorkflowAction.kt index 38af085cb..5a965689a 100644 --- a/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportDeleteWorkflowAction.kt +++ b/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportDeleteWorkflowAction.kt @@ -172,7 +172,7 @@ class TransportDeleteWorkflowAction @Inject constructor( } // Delete monitors workflow metadata - // Monitor metadata will be in workflowId-monitorId-metadata format + // Monitor metadata will be in workflowId-metadata-monitorId-metadata format metadataIdsToDelete.addAll(delegateMonitors.map { MonitorMetadata.getId(it, workflowMetadataId) }) try { // Delete the monitors workflow metadata From 6f5b955bff69d27ac7e007c875763a328a55ac93 Mon Sep 17 00:00:00 2001 From: Nishtha Mehrotra Date: Tue, 14 Jan 2025 17:27:35 -0800 Subject: [PATCH 4/5] Updated error logs Signed-off-by: Nishtha Mehrotra --- .../kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt b/alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt index 8c92993cc..d2360fc07 100644 --- a/alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt +++ b/alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt @@ -5079,7 +5079,7 @@ class MonitorDataSourcesIT : AlertingSingleNodeTestCase() { } catch (e: Exception) { e.message?.let { assertTrue( - "Exception not returning GetMonitor Action error ", + "Expected 0 hits for searchMonitorMetadata, got non-0 results.", it.contains("List is empty") ) } @@ -5119,7 +5119,7 @@ class MonitorDataSourcesIT : AlertingSingleNodeTestCase() { } catch (e: Exception) { e.message?.let { assertTrue( - "Exception not returning GetWorkflow Action error ", + "Expected 0 hits for searchMonitorMetadata, got non-0 results.", it.contains("Workflow not found.") ) } @@ -5142,7 +5142,7 @@ class MonitorDataSourcesIT : AlertingSingleNodeTestCase() { } catch (e: Exception) { e.message?.let { assertTrue( - "Exception not returning GetMonitor Action error ", + "Exception not returning searchMonitorMetadata error ", it.contains("List is empty") ) } From b4bfcd28530853258f72554daac737ad592a82c2 Mon Sep 17 00:00:00 2001 From: Nishtha Mehrotra Date: Tue, 14 Jan 2025 17:29:33 -0800 Subject: [PATCH 5/5] Updated assert Signed-off-by: Nishtha Mehrotra --- .../kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt b/alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt index d2360fc07..ad6e8a199 100644 --- a/alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt +++ b/alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt @@ -5119,7 +5119,7 @@ class MonitorDataSourcesIT : AlertingSingleNodeTestCase() { } catch (e: Exception) { e.message?.let { assertTrue( - "Expected 0 hits for searchMonitorMetadata, got non-0 results.", + "Exception not returning GetWorkflow Action error ", it.contains("Workflow not found.") ) } @@ -5142,7 +5142,7 @@ class MonitorDataSourcesIT : AlertingSingleNodeTestCase() { } catch (e: Exception) { e.message?.let { assertTrue( - "Exception not returning searchMonitorMetadata error ", + "Expected 0 hits for searchMonitorMetadata, got non-0 results.", it.contains("List is empty") ) }