From 1a9818e77db6c55634c6ef04c61ea27af12220b7 Mon Sep 17 00:00:00 2001 From: Lillie Dae Date: Tue, 25 Apr 2023 15:34:59 +0100 Subject: [PATCH 1/7] updated workflow request event process Signed-off-by: Lillie Dae --- .../Database/Interfaces/IWorkflowRepository.cs | 18 ++++++++++++++++++ .../Repositories/WorkflowRepository.cs | 18 +++++++++++++++++- .../Services/WorkflowExecuterService.cs | 9 ++------- 3 files changed, 37 insertions(+), 8 deletions(-) diff --git a/src/WorkflowManager/Database/Interfaces/IWorkflowRepository.cs b/src/WorkflowManager/Database/Interfaces/IWorkflowRepository.cs index 5cf6b7d52..41cb8abb7 100644 --- a/src/WorkflowManager/Database/Interfaces/IWorkflowRepository.cs +++ b/src/WorkflowManager/Database/Interfaces/IWorkflowRepository.cs @@ -17,6 +17,7 @@ using System; using System.Collections.Generic; using System.Threading.Tasks; +using Monai.Deploy.Messaging.Events; using Monai.Deploy.WorkflowManager.Contracts.Models; namespace Monai.Deploy.WorkflowManager.Database.Interfaces @@ -67,6 +68,23 @@ public interface IWorkflowRepository /// An aeTitle to retrieve workflows for. Task> GetWorkflowsByAeTitleAsync(List aeTitles); + /// + /// Retrieves a list of workflows based..
+ /// if clinical workflow has AET no data origin. => WorkflowRequestEvents received with CalledAET with that AET this workflow (regardless of what the CallingAET is)
+ /// if clinical workflow has AET and data_orgins => only WorkflowRequestEvents with CalledAET with that AET and CallingAET trigger this workflow.
+ /// + /// + /// If clinical workflow (workflow revision) exists with AET “MONAI” but no data_origins set + /// Any inbound WorkflowRequestEvents with CalledAET = “MONAI” trigger this workflow (regardless of what the CallingAET is) + /// + /// If clinical workflow (workflow revision) exists with AET “MONAI” and data_origins set as “PACS” + /// Only inbound WorkflowRequestEvents with CalledAET = “MONAI” and CallingAET = “PACS” trigger this workflow + /// + /// + /// + /// + Task> GetWorkflowsForWorkflowRequestAsync(string calledAeTitle, string callingAeTitle); + /// /// Creates a workflow object. /// diff --git a/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs b/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs index df0e1d0c8..10614f8df 100755 --- a/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs +++ b/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs @@ -199,9 +199,25 @@ public async Task> GetWorkflowsByAeTitleAsync(List> GetWorkflowsForWorkflowRequestAsync(string calledAeTitle, string callingAeTitle) + { + Guard.Against.NullOrEmpty(calledAeTitle); + Guard.Against.NullOrEmpty(callingAeTitle); + + var wfs = await _workflowCollection + .Find(x => + x.Deleted == null && + x.Workflow != null && + x.Workflow.InformaticsGateway != null && + (x.Workflow.InformaticsGateway.AeTitle == calledAeTitle && x.Workflow.InformaticsGateway.DataOrigins.Length == 0 || + x.Workflow.InformaticsGateway.AeTitle == calledAeTitle && x.Workflow.InformaticsGateway.DataOrigins.Contains(callingAeTitle))) + .ToListAsync(); + return wfs; + } + public async Task CreateAsync(Workflow workflow) { - Guard.Against.Null(workflow, nameof(workflow)); + Guard.Against.Null(workflow); var workflowRevision = new WorkflowRevision { diff --git a/src/WorkflowManager/WorkflowExecuter/Services/WorkflowExecuterService.cs b/src/WorkflowManager/WorkflowExecuter/Services/WorkflowExecuterService.cs index 31fd0b379..7a6c2ad0d 100755 --- a/src/WorkflowManager/WorkflowExecuter/Services/WorkflowExecuterService.cs +++ b/src/WorkflowManager/WorkflowExecuter/Services/WorkflowExecuterService.cs @@ -117,13 +117,8 @@ public async Task ProcessPayload(WorkflowRequestEvent message, Payload pay } else { - var aeTitles = new List - { - message.CalledAeTitle, - message.CallingAeTitle - }; - - workflows = await _workflowRepository.GetWorkflowsByAeTitleAsync(aeTitles) as List; + var result = await _workflowRepository.GetWorkflowsForWorkflowRequestAsync(message.CalledAeTitle, message.CallingAeTitle); + workflows = new List(result); } if (workflows is null || workflows.Any() is false) From dac3dc52859bfaf9b7e72f9eb42cf274c540a2cf Mon Sep 17 00:00:00 2001 From: Lillie Dae Date: Wed, 26 Apr 2023 11:36:22 +0100 Subject: [PATCH 2/7] update to interation tests Signed-off-by: Lillie Dae --- .../Repositories/WorkflowRepository.cs | 10 +++++++--- .../TestData/WorkflowRequestTestData.cs | 18 ++++++++--------- .../TestData/WorkflowRevisionTestData.cs | 20 +++++++++---------- .../Services/WorkflowExecuterServiceTests.cs | 1 + 4 files changed, 27 insertions(+), 22 deletions(-) diff --git a/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs b/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs index 10614f8df..c8fe0b574 100755 --- a/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs +++ b/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs @@ -206,11 +206,15 @@ public async Task> GetWorkflowsForWorkflowRequestAsync(s var wfs = await _workflowCollection .Find(x => - x.Deleted == null && x.Workflow != null && x.Workflow.InformaticsGateway != null && - (x.Workflow.InformaticsGateway.AeTitle == calledAeTitle && x.Workflow.InformaticsGateway.DataOrigins.Length == 0 || - x.Workflow.InformaticsGateway.AeTitle == calledAeTitle && x.Workflow.InformaticsGateway.DataOrigins.Contains(callingAeTitle))) + ((x.Workflow.InformaticsGateway.AeTitle == calledAeTitle && + (x.Workflow.InformaticsGateway.DataOrigins == null || + x.Workflow.InformaticsGateway.DataOrigins.Length == 0)) || + x.Workflow.InformaticsGateway.AeTitle == calledAeTitle && + x.Workflow.InformaticsGateway.DataOrigins != null && + x.Workflow.InformaticsGateway.DataOrigins.Any(d => d == callingAeTitle)) && + x.Deleted == null) .ToListAsync(); return wfs; } diff --git a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRequestTestData.cs b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRequestTestData.cs index 71b0b3771..153f860e4 100644 --- a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRequestTestData.cs +++ b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRequestTestData.cs @@ -39,8 +39,8 @@ public static class WorkflowRequestsTestData Workflows = new List() { }, CorrelationId = Guid.NewGuid().ToString(), Timestamp = DateTime.UtcNow, - CalledAeTitle = "Basic_AE", - CallingAeTitle = "Basic_AE_3", + CalledAeTitle = "MONAI", + CallingAeTitle = "PACS", } }, new WorkflowRequestTestData @@ -95,7 +95,7 @@ public static class WorkflowRequestsTestData Workflows = new List() { }, CorrelationId = Guid.NewGuid().ToString(), Timestamp = DateTime.UtcNow, - CalledAeTitle = "Multi_Revision", + CalledAeTitle = "MONAI_2", CallingAeTitle = "MWM", } }, @@ -258,8 +258,8 @@ public static class WorkflowRequestsTestData Workflows = new List() { }, CorrelationId = Guid.NewGuid().ToString(), Timestamp = DateTime.UtcNow, - CalledAeTitle = "MWM", - CallingAeTitle = "Basic_AE", + CalledAeTitle = "MONAI", + CallingAeTitle = "PACS", } }, new WorkflowRequestTestData @@ -272,8 +272,8 @@ public static class WorkflowRequestsTestData Workflows = new List() { }, CorrelationId = Guid.NewGuid().ToString(), Timestamp = DateTime.UtcNow, - CalledAeTitle = "Basic_AE", - CallingAeTitle = "MWM", + CalledAeTitle = "MONAI", + CallingAeTitle = "PACS", } }, new WorkflowRequestTestData @@ -286,8 +286,8 @@ public static class WorkflowRequestsTestData Workflows = new List() { }, CorrelationId = Guid.NewGuid().ToString(), Timestamp = DateTime.UtcNow, - CalledAeTitle = "Basic_AE", - CallingAeTitle = "MWM", + CalledAeTitle = "MONAI", + CallingAeTitle = "PACS", } }, new WorkflowRequestTestData diff --git a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRevisionTestData.cs b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRevisionTestData.cs index b6b001810..af87a77e9 100644 --- a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRevisionTestData.cs +++ b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRevisionTestData.cs @@ -54,7 +54,7 @@ public static class WorkflowRevisionsTestData }, InformaticsGateway = new InformaticsGateway() { - AeTitle = "Basic_AE" + AeTitle = "MONAI" } } } @@ -84,7 +84,7 @@ public static class WorkflowRevisionsTestData }, InformaticsGateway = new InformaticsGateway() { - AeTitle = "Basic_AE" + AeTitle = "MONAI" } } } @@ -114,7 +114,7 @@ public static class WorkflowRevisionsTestData }, InformaticsGateway = new InformaticsGateway() { - AeTitle = "Basic_AE_3" + AeTitle = "MONAI" } } } @@ -181,7 +181,7 @@ public static class WorkflowRevisionsTestData }, new WorkflowRevisionTestData() { - Name = "Basic_Workflow_Multiple_Revisions_1", + Name = "Basic_Workflow_Multiple_Revisions_1", //not to be confused with 'Basic_Workflow_multiple_revisions_1' (lower case) WorkflowRevision = new WorkflowRevision() { Id = Guid.NewGuid().ToString(), @@ -204,7 +204,7 @@ public static class WorkflowRevisionsTestData }, InformaticsGateway = new InformaticsGateway() { - AeTitle = "Multi_Revision" + AeTitle = "MONAI" } } } @@ -234,7 +234,7 @@ public static class WorkflowRevisionsTestData }, InformaticsGateway = new InformaticsGateway() { - AeTitle = "Multi_Revision" + AeTitle = "MONAI_2" } } } @@ -331,7 +331,7 @@ public static class WorkflowRevisionsTestData }, InformaticsGateway = new InformaticsGateway() { - AeTitle = "Multi_Created" + AeTitle = "Multi_Req" } } } @@ -675,7 +675,7 @@ public static class WorkflowRevisionsTestData }, new WorkflowRevisionTestData() { - Name = "Basic_Workflow_multiple_revisions_1", + Name = "Basic_Workflow_multiple_revisions_1", //not to be confused with 'Basic_Workflow_Multiple_Revisions_1' (upper case) WorkflowRevision = new WorkflowRevision() { Id = Guid.NewGuid().ToString(), @@ -698,7 +698,7 @@ public static class WorkflowRevisionsTestData }, InformaticsGateway = new InformaticsGateway() { - AeTitle = "Basic_AE" + AeTitle = "MONAI_2" } } } @@ -728,7 +728,7 @@ public static class WorkflowRevisionsTestData }, InformaticsGateway = new InformaticsGateway() { - AeTitle = "Basic_AE" + AeTitle = "MONAI_2" } } } diff --git a/tests/UnitTests/WorkflowExecuter.Tests/Services/WorkflowExecuterServiceTests.cs b/tests/UnitTests/WorkflowExecuter.Tests/Services/WorkflowExecuterServiceTests.cs index cb5d28d1f..52ec45dc7 100755 --- a/tests/UnitTests/WorkflowExecuter.Tests/Services/WorkflowExecuterServiceTests.cs +++ b/tests/UnitTests/WorkflowExecuter.Tests/Services/WorkflowExecuterServiceTests.cs @@ -143,6 +143,7 @@ public async Task ProcessPayload_ValidAeTitleWorkflowRequest_ReturnesTrue() }; _workflowRepository.Setup(w => w.GetWorkflowsByAeTitleAsync(It.IsAny>())).ReturnsAsync(workflows); + _workflowRepository.Setup(w => w.GetWorkflowsForWorkflowRequestAsync(It.IsAny(), It.IsAny())).ReturnsAsync(workflows); _workflowRepository.Setup(w => w.GetByWorkflowIdAsync(workflows[0].WorkflowId)).ReturnsAsync(workflows[0]); _workflowInstanceRepository.Setup(w => w.CreateAsync(It.IsAny>())).ReturnsAsync(true); _workflowInstanceRepository.Setup(w => w.GetByWorkflowsIdsAsync(It.IsAny>())).ReturnsAsync(new List()); From 1d56a22e3c00fbf5d2cd35212037b21874031afa Mon Sep 17 00:00:00 2001 From: Joe Batt Date: Tue, 9 May 2023 13:08:46 +0100 Subject: [PATCH 3/7] Integration tests for data_origins Signed-off-by: Joe Batt --- .../Features/WorkflowRequest.feature | 23 +++++ .../TestData/WorkflowRequestTestData.cs | 42 +++++++++ .../TestData/WorkflowRevisionTestData.cs | 92 +++++++++++++++++++ 3 files changed, 157 insertions(+) diff --git a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/Features/WorkflowRequest.feature b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/Features/WorkflowRequest.feature index 223127058..32d768db2 100644 --- a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/Features/WorkflowRequest.feature +++ b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/Features/WorkflowRequest.feature @@ -42,6 +42,29 @@ Scenario Outline: Publish a valid workflow request which creates multiple workfl | Basic_Workflow_1 | Basic_Workflow_2 | Basic_Multi_Id_WF_Request | | Basic_Workflow_1 | Basic_Workflow_3 | Basic_AeTitle_WF_Request | +@WorkflowRequest +Scenario Outline: Publish a workflow request which triggers a worflow based on called_aet and calling_aet + Given I have a clinical workflow + When I publish a Workflow Request Message with no artifacts + Then I can see 1 Workflow Instance is created + And 1 Task Dispatch event is published + Examples: + | workflow | workflowRequestMessage | + | Workflow_Called_AET | Called_AET_AIDE_Calling_AET_TEST | + | Workflow_Called_AET_Calling_AET | Called_AET_AIDE_Calling_AET_PACS1 | + | Workflow_Called_AET_Multi_Calling_AET | Called_AET_AIDE_Calling_AET_PACS1 | + | Workflow_Called_AET_Multi_Calling_AET | Called_AET_AIDE_Calling_AET_PACS2 | + +@WorkflowRequest +Scenario Outline: Publish a workflow request which doesnt trigger a worflow based calling_aet + Given I have a clinical workflow + When I publish a Workflow Request Message with no artifacts + Then I can see no Workflow Instances are created + Examples: + | workflow | workflowRequestMessage | + | Workflow_Called_AET_Calling_AET | Called_AET_AIDE_Calling_AET_TEST | + | Workflow_Called_AET_Multi_Calling_AET | Called_AET_AIDE_Calling_AET_TEST | + @WorkflowRequest Scenario: Publish a valid workflow request with mismatched AE title and workflow ID Given I have a clinical workflow Basic_Workflow_1 diff --git a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRequestTestData.cs b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRequestTestData.cs index 153f860e4..44d72c73b 100644 --- a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRequestTestData.cs +++ b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRequestTestData.cs @@ -346,6 +346,48 @@ public static class WorkflowRequestsTestData CallingAeTitle = "Non_Existent_Calling_AE", } }, + new WorkflowRequestTestData + { + Name = "Called_AET_AIDE_Calling_AET_TEST", + WorkflowRequestMessage = new WorkflowRequestMessage + { + Bucket = "bucket1", + PayloadId = Guid.NewGuid(), + Workflows = new List() { }, + CorrelationId = Guid.NewGuid().ToString(), + Timestamp = DateTime.UtcNow, + CalledAeTitle = "AIDE", + CallingAeTitle = "TEST", + } + }, + new WorkflowRequestTestData + { + Name = "Called_AET_AIDE_Calling_AET_PACS1", + WorkflowRequestMessage = new WorkflowRequestMessage + { + Bucket = "bucket1", + PayloadId = Guid.NewGuid(), + Workflows = new List() { }, + CorrelationId = Guid.NewGuid().ToString(), + Timestamp = DateTime.UtcNow, + CalledAeTitle = "AIDE", + CallingAeTitle = "PACS1", + } + }, + new WorkflowRequestTestData + { + Name = "Called_AET_AIDE_Calling_AET_PACS2", + WorkflowRequestMessage = new WorkflowRequestMessage + { + Bucket = "bucket1", + PayloadId = Guid.NewGuid(), + Workflows = new List() { }, + CorrelationId = Guid.NewGuid().ToString(), + Timestamp = DateTime.UtcNow, + CalledAeTitle = "AIDE", + CallingAeTitle = "PACS2", + } + }, }; } } diff --git a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRevisionTestData.cs b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRevisionTestData.cs index af87a77e9..b662979f4 100644 --- a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRevisionTestData.cs +++ b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/WorkflowRevisionTestData.cs @@ -3036,6 +3036,98 @@ public static class WorkflowRevisionsTestData } } }, + new WorkflowRevisionTestData() + { + Name = "Workflow_Called_AET", + WorkflowRevision = new WorkflowRevision() + { + Id = Guid.NewGuid().ToString(), + WorkflowId = Guid.NewGuid().ToString(), + Revision = 1, + Workflow = new Workflow() + { + Name = "Basic workflow 1", + Description = "Basic workflow 1", + Version = "1", + Tasks = new TaskObject[] + { + new TaskObject + { + Id = Guid.NewGuid().ToString(), + Type = "Basic_task", + Description = "Basic Workflow 1 Task 1", + Artifacts = new ArtifactMap(), + } + }, + InformaticsGateway = new InformaticsGateway() + { + AeTitle = "AIDE" + } + } + } + }, + new WorkflowRevisionTestData() + { + Name = "Workflow_Called_AET_Calling_AET", + WorkflowRevision = new WorkflowRevision() + { + Id = Guid.NewGuid().ToString(), + WorkflowId = Guid.NewGuid().ToString(), + Revision = 1, + Workflow = new Workflow() + { + Name = "Basic workflow 1", + Description = "Basic workflow 1", + Version = "1", + Tasks = new TaskObject[] + { + new TaskObject + { + Id = Guid.NewGuid().ToString(), + Type = "Basic_task", + Description = "Basic Workflow 1 Task 1", + Artifacts = new ArtifactMap(), + } + }, + InformaticsGateway = new InformaticsGateway() + { + AeTitle = "AIDE", + DataOrigins = new string[] { "PACS1" } + } + } + } + }, + new WorkflowRevisionTestData() + { + Name = "Workflow_Called_AET_Multi_Calling_AET", + WorkflowRevision = new WorkflowRevision() + { + Id = Guid.NewGuid().ToString(), + WorkflowId = Guid.NewGuid().ToString(), + Revision = 1, + Workflow = new Workflow() + { + Name = "Basic workflow 1", + Description = "Basic workflow 1", + Version = "1", + Tasks = new TaskObject[] + { + new TaskObject + { + Id = Guid.NewGuid().ToString(), + Type = "Basic_task", + Description = "Basic Workflow 1 Task 1", + Artifacts = new ArtifactMap(), + } + }, + InformaticsGateway = new InformaticsGateway() + { + AeTitle = "AIDE", + DataOrigins = new string[] { "PACS1", "PACS2" } + } + } + } + }, }; } } From ec289c49a5c1091d87435878484504e10405e323 Mon Sep 17 00:00:00 2001 From: samrooke <51709626+samrooke@users.noreply.github.com> Date: Tue, 9 May 2023 13:38:27 +0100 Subject: [PATCH 4/7] add api tests for payload delete (#788) Signed-off-by: Sam Rooke --- .../Features/PayloadApi.feature | 29 +++++++++++ .../TestData/PayloadTestData.cs | 48 +++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/Features/PayloadApi.feature b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/Features/PayloadApi.feature index 60545644c..aaa14069d 100644 --- a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/Features/PayloadApi.feature +++ b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/Features/PayloadApi.feature @@ -124,3 +124,32 @@ Scenario Outline: Get payload by Id returns 400 When I send a GET request Then I will get a 400 response And I will receive the error message Failed to validate id, not a valid guid + +@DeletePayloadById +Scenario Outline: Delete payload by Id returns 400 with invalid payload ID + Given I have an endpoint /payload/invalid-payload-id + When I send a DELETE request + Then I will get a 400 response + And I will receive the error message Failed to validate id, not a valid guid + +@DeletePayloadById +Scenario Outline: Delete payload by ID returns 404 when no payload exists + Given I have an endpoint /payload/c5c3635b-81dd-44a9-8c3b-71adec7d47c6 + When I send a DELETE request + Then I will get a 404 response + And I will receive the error message Payload with ID: c5c3635b-81dd-44a9-8c3b-71adec7d47c6 not found + +@DeletePayloadById +Scenario Outline: Delete payload by ID returns 400 when PayloadDeleted is already InProgress + Given I have an endpoint /payload/c5c3635b-81dd-44a9-8c3b-71adec7d47c6 + And I have a payload saved in mongo Payload_PayloadDeleted_InProgress + When I send a DELETE request + Then I will get a 400 response + And I will receive the error message Deletion of files for payload ID: c5c3635b-81dd-44a9-8c3b-71adec7d47c6 already in progress + +@DeletePayloadById +Scenario Outline: Delete payload by ID returns 202 + Given I have an endpoint /payload/d5c3633b-41de-44a9-8c3a-71adec3d47c1 + And I have a payload saved in mongo Payload_PayloadDeleted_No + When I send a DELETE request + Then I will get a 202 response diff --git a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/PayloadTestData.cs b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/PayloadTestData.cs index bd4183e53..35d3315b3 100644 --- a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/PayloadTestData.cs +++ b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/TestData/PayloadTestData.cs @@ -305,6 +305,54 @@ public static class PayloadsTestData } } }, + new PayloadTestData() + { + Name = "Payload_PayloadDeleted_InProgress", + Payload = new Payload() + { + Id = Guid.NewGuid().ToString(), + Timestamp = DateTime.UtcNow, + Bucket = "bucket_1", + CalledAeTitle = "MIG", + CallingAeTitle = "Basic_AE", + CorrelationId = Guid.NewGuid().ToString(), + PayloadId = "c5c3635b-81dd-44a9-8c3b-71adec7d47c6", + Workflows = new List { Guid.NewGuid().ToString() }, + FileCount = 50, + PayloadDeleted = PayloadDeleted.InProgress, + PatientDetails = new PatientDetails() + { + PatientDob = new DateTime(1996, 02, 05, 0, 0, 0, kind: DateTimeKind.Utc), + PatientId = Guid.NewGuid().ToString(), + PatientName = "Mike Mcgee", + PatientSex = "male" + } + } + }, + new PayloadTestData() + { + Name = "Payload_PayloadDeleted_No", + Payload = new Payload() + { + Id = Guid.NewGuid().ToString(), + Timestamp = DateTime.UtcNow, + Bucket = "bucket_1", + CalledAeTitle = "MIG", + CallingAeTitle = "Basic_AE", + CorrelationId = Guid.NewGuid().ToString(), + PayloadId = "d5c3633b-41de-44a9-8c3a-71adec3d47c1", + Workflows = new List { Guid.NewGuid().ToString() }, + FileCount = 50, + PayloadDeleted = PayloadDeleted.No, + PatientDetails = new PatientDetails() + { + PatientDob = new DateTime(1996, 02, 05, 0, 0, 0, kind: DateTimeKind.Utc), + PatientId = Guid.NewGuid().ToString(), + PatientName = "Mike Mcgee", + PatientSex = "male" + } + } + } }; } } From e80784412b6c0728a2d61b0788cac4f3f111056a Mon Sep 17 00:00:00 2001 From: Sam Rooke Date: Tue, 9 May 2023 14:00:13 +0100 Subject: [PATCH 5/7] fix file deletion from storage Signed-off-by: Sam Rooke --- .../Common/Services/PayloadService.cs | 20 +++++++++++++------ .../Logging/Log.300000.Payload.cs | 3 +++ .../Features/PayloadApi.feature | 2 +- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/WorkflowManager/Common/Services/PayloadService.cs b/src/WorkflowManager/Common/Services/PayloadService.cs index 558ba662c..c0a91670a 100644 --- a/src/WorkflowManager/Common/Services/PayloadService.cs +++ b/src/WorkflowManager/Common/Services/PayloadService.cs @@ -87,6 +87,7 @@ public PayloadService( Timestamp = eventPayload.Timestamp, PatientDetails = patientDetails, PayloadDeleted = PayloadDeleted.No, + Files = eventPayload.Payload.ToList() }; if (await _payloadRepository.CreateAsync(payload)) @@ -173,12 +174,12 @@ public async Task DeletePayloadFromStorageAsync(string payloadId) throw new MonaiNotFoundException($"Payload with ID: {payloadId} not found"); } - if (payload.PayloadDeleted == PayloadDeleted.InProgress) + if (payload.PayloadDeleted == PayloadDeleted.InProgress || payload.PayloadDeleted == PayloadDeleted.Yes) { - throw new MonaiBadRequestException($"Deletion of files for payload ID: {payloadId} already in progress"); + throw new MonaiBadRequestException($"Deletion of files for payload ID: {payloadId} already in progress or already deleted"); } - // update the payload to in progress before we request deletion form MinIO + // update the payload to in progress before we request deletion from storage payload.PayloadDeleted = PayloadDeleted.InProgress; await _payloadRepository.UpdateAsync(payload); @@ -188,12 +189,19 @@ public async Task DeletePayloadFromStorageAsync(string payloadId) { try { - await _storageService.RemoveObjectsAsync(payload.Bucket, payload.Files.Select(f => f.Path)); + // get all objects for the payload in storage to be deleted + var allPayloadObjects = await _storageService.ListObjectsAsync(payload.Bucket, payloadId, true); + + if (allPayloadObjects.Any()) + { + await _storageService.RemoveObjectsAsync(payload.Bucket, allPayloadObjects.Select(o => o.FilePath)); + } + payload.PayloadDeleted = PayloadDeleted.Yes; } - catch + catch (Exception ex) { - _logger.PayloadUpdateFailed(payloadId); + _logger.PayloadDeleteFailed(payloadId, ex); payload.PayloadDeleted = PayloadDeleted.Failed; } finally diff --git a/src/WorkflowManager/Logging/Log.300000.Payload.cs b/src/WorkflowManager/Logging/Log.300000.Payload.cs index d76144ce3..b7f23ef1b 100644 --- a/src/WorkflowManager/Logging/Log.300000.Payload.cs +++ b/src/WorkflowManager/Logging/Log.300000.Payload.cs @@ -37,5 +37,8 @@ public static partial class Log [LoggerMessage(EventId = 300005, Level = LogLevel.Error, Message = "Failed to update payload {payloadId} due to database error.")] public static partial void PayloadUpdateFailed(this ILogger logger, string payloadId); + + [LoggerMessage(EventId = 300006, Level = LogLevel.Error, Message = "Failed to delete payload {payloadId} from storage.")] + public static partial void PayloadDeleteFailed(this ILogger logger, string payloadId, Exception ex); } } diff --git a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/Features/PayloadApi.feature b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/Features/PayloadApi.feature index aaa14069d..86bc316a5 100644 --- a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/Features/PayloadApi.feature +++ b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/Features/PayloadApi.feature @@ -145,7 +145,7 @@ Scenario Outline: Delete payload by ID returns 400 when PayloadDeleted is alread And I have a payload saved in mongo Payload_PayloadDeleted_InProgress When I send a DELETE request Then I will get a 400 response - And I will receive the error message Deletion of files for payload ID: c5c3635b-81dd-44a9-8c3b-71adec7d47c6 already in progress + And I will receive the error message Deletion of files for payload ID: c5c3635b-81dd-44a9-8c3b-71adec7d47c6 already in progress or already deleted @DeletePayloadById Scenario Outline: Delete payload by ID returns 202 From 82d6bd2ceb28dbaa10a184dfb92329ababe18d9d Mon Sep 17 00:00:00 2001 From: Sam Rooke Date: Tue, 9 May 2023 14:11:39 +0100 Subject: [PATCH 6/7] revert saving files in db Signed-off-by: Sam Rooke --- src/WorkflowManager/Common/Services/PayloadService.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/WorkflowManager/Common/Services/PayloadService.cs b/src/WorkflowManager/Common/Services/PayloadService.cs index c0a91670a..1776a1479 100644 --- a/src/WorkflowManager/Common/Services/PayloadService.cs +++ b/src/WorkflowManager/Common/Services/PayloadService.cs @@ -86,8 +86,7 @@ public PayloadService( CallingAeTitle = eventPayload.CallingAeTitle, Timestamp = eventPayload.Timestamp, PatientDetails = patientDetails, - PayloadDeleted = PayloadDeleted.No, - Files = eventPayload.Payload.ToList() + PayloadDeleted = PayloadDeleted.No }; if (await _payloadRepository.CreateAsync(payload)) From e19c01e9f0a46eb112d758edb494267f7e0b0205 Mon Sep 17 00:00:00 2001 From: Sam Rooke Date: Tue, 9 May 2023 15:55:57 +0100 Subject: [PATCH 7/7] change memory_gb to memory Signed-off-by: Sam Rooke --- src/Shared/Shared/ValidationConstants.cs | 2 +- .../Plug-ins/Argo/StaticValues/Keys.cs | 15 --------------- .../Plug-ins/Argo/StaticValues/ResourcesKeys.cs | 8 +++++--- .../Validators/WorkflowValidator.cs | 15 ++++++--------- .../TaskManager.Argo.Tests/ArgoPluginTest.cs | 2 +- .../Validators/WorkflowValidatorTests.cs | 11 ++++------- 6 files changed, 17 insertions(+), 36 deletions(-) diff --git a/src/Shared/Shared/ValidationConstants.cs b/src/Shared/Shared/ValidationConstants.cs index 95d819478..973a7e556 100644 --- a/src/Shared/Shared/ValidationConstants.cs +++ b/src/Shared/Shared/ValidationConstants.cs @@ -61,7 +61,7 @@ public static class ValidationConstants /// /// Key for the memory. /// - public static readonly string Memory = "memory_gb"; + public static readonly string Memory = "memory"; /// /// Key for the GPU. diff --git a/src/TaskManager/Plug-ins/Argo/StaticValues/Keys.cs b/src/TaskManager/Plug-ins/Argo/StaticValues/Keys.cs index ead9718b8..5e78c2f15 100644 --- a/src/TaskManager/Plug-ins/Argo/StaticValues/Keys.cs +++ b/src/TaskManager/Plug-ins/Argo/StaticValues/Keys.cs @@ -88,21 +88,6 @@ internal static class Keys /// public static readonly string TaskPriorityClassName = "priority"; - /// - /// Key for CPU - /// - public static readonly string Cpu = "cpu"; - - /// - /// Key for memory allocation - /// - public static readonly string Memory = "memory_gb"; - - /// - /// Key for GPU - /// - public static readonly string Gpu = "number_gpu"; - /// /// Required arguments to run the Argo workflow. /// diff --git a/src/TaskManager/Plug-ins/Argo/StaticValues/ResourcesKeys.cs b/src/TaskManager/Plug-ins/Argo/StaticValues/ResourcesKeys.cs index 896c1233d..5d3fdb08b 100644 --- a/src/TaskManager/Plug-ins/Argo/StaticValues/ResourcesKeys.cs +++ b/src/TaskManager/Plug-ins/Argo/StaticValues/ResourcesKeys.cs @@ -14,14 +14,16 @@ * limitations under the License. */ +using static Monai.Deploy.WorkflowManager.Shared.ValidationConstants; + namespace Monai.Deploy.WorkflowManager.TaskManager.Argo.StaticValues { public static class ResourcesKeys { - public static readonly ResourcesKey GpuLimit = new() { TaskKey = "gpu_required", ArgoKey = "nvidia.com/gpu" }; + public static readonly ResourcesKey GpuLimit = new() { TaskKey = GpuRequired, ArgoKey = "nvidia.com/gpu" }; - public static readonly ResourcesKey MemoryLimit = new() { TaskKey = "memory_gb", ArgoKey = "memory" }; + public static readonly ResourcesKey MemoryLimit = new() { TaskKey = Memory, ArgoKey = "memory" }; - public static readonly ResourcesKey CpuLimit = new() { TaskKey = "cpu", ArgoKey = "cpu" }; + public static readonly ResourcesKey CpuLimit = new() { TaskKey = Cpu, ArgoKey = "cpu" }; } } diff --git a/src/WorkflowManager/WorkflowManager/Validators/WorkflowValidator.cs b/src/WorkflowManager/WorkflowManager/Validators/WorkflowValidator.cs index fd97eaaed..d9d355c39 100644 --- a/src/WorkflowManager/WorkflowManager/Validators/WorkflowValidator.cs +++ b/src/WorkflowManager/WorkflowManager/Validators/WorkflowValidator.cs @@ -375,16 +375,13 @@ private void ValidateArgoTask(TaskObject currentTask) } } - new List { Cpu, Memory }.ForEach(key => + if ( + currentTask.Args.TryGetValue(Cpu, out var val) && + (string.IsNullOrEmpty(val) || + (double.TryParse(val, out double parsedVal) && (parsedVal < 1 || Math.Truncate(parsedVal) != parsedVal)))) { - if ( - currentTask.Args.TryGetValue(key, out var val) && - (string.IsNullOrEmpty(val) || - (double.TryParse(val, out double parsedVal) && (parsedVal < 1 || Math.Truncate(parsedVal) != parsedVal)))) - { - Errors.Add($"Task: '{currentTask.Id}' value '{val}' provided for argument '{key}' is not valid. The value needs to be a whole number greater than 0."); - } - }); + Errors.Add($"Task: '{currentTask.Id}' value '{val}' provided for argument '{Cpu}' is not valid. The value needs to be a whole number greater than 0."); + } if ( currentTask.Args.TryGetValue(GpuRequired, out var gpuRequired) && diff --git a/tests/UnitTests/TaskManager.Argo.Tests/ArgoPluginTest.cs b/tests/UnitTests/TaskManager.Argo.Tests/ArgoPluginTest.cs index 82dbdb6ab..ec578a6ba 100755 --- a/tests/UnitTests/TaskManager.Argo.Tests/ArgoPluginTest.cs +++ b/tests/UnitTests/TaskManager.Argo.Tests/ArgoPluginTest.cs @@ -273,7 +273,7 @@ public async Task ArgoPlugin_ExecuteTask_WorkflowTemplates(string filename, int var message = GenerateTaskDispatchEventWithValidArguments(withoutDefaultArguments); message.TaskPluginArguments["gpu_required"] = "true"; - message.TaskPluginArguments["memory_gb"] = "1"; + message.TaskPluginArguments["memory"] = "1"; message.TaskPluginArguments["cpu"] = "1"; message.TaskPluginArguments["priority"] = "Helo"; Workflow? submittedArgoTemplate = null; diff --git a/tests/UnitTests/WorkflowManager.Tests/Validators/WorkflowValidatorTests.cs b/tests/UnitTests/WorkflowManager.Tests/Validators/WorkflowValidatorTests.cs index 0ab1439a3..d02f9acfc 100644 --- a/tests/UnitTests/WorkflowManager.Tests/Validators/WorkflowValidatorTests.cs +++ b/tests/UnitTests/WorkflowManager.Tests/Validators/WorkflowValidatorTests.cs @@ -209,7 +209,7 @@ public async Task ValidateWorkflow_ValidatesAWorkflow_ReturnsErrorsAndHasCorrect Description = "Test Argo Task", Args = { { "cpu", "0.1" }, - { "memory_gb", "0.1" }, + { "memory", "0.1" }, { "gpu_required", "2" } }, TaskDestinations = new TaskDestination[] @@ -343,7 +343,7 @@ public async Task ValidateWorkflow_ValidatesAWorkflow_ReturnsErrorsAndHasCorrect { "invalid_key", "value" }, { "workflow_template_name" ,"spot"}, { "cpu", "1" }, - { "memory_gb", "1" }, + { "memory", "1" }, { "gpu_required", "1" } } }, @@ -371,7 +371,7 @@ public async Task ValidateWorkflow_ValidatesAWorkflow_ReturnsErrorsAndHasCorrect Assert.True(errors.Count > 0); - Assert.Equal(46, errors.Count); + Assert.Equal(45, errors.Count); var convergingTasksDestinations = "Converging Tasks Destinations in tasks: (test-clinical-review-2, example-task) on task: example-task"; Assert.Contains(convergingTasksDestinations, errors); @@ -412,9 +412,6 @@ public async Task ValidateWorkflow_ValidatesAWorkflow_ReturnsErrorsAndHasCorrect var invalidArgoArg1 = "Task: 'test-argo-task' value '0.1' provided for argument 'cpu' is not valid. The value needs to be a whole number greater than 0."; Assert.Contains(invalidArgoArg1, errors); - var invalidArgoArg2 = "Task: 'test-argo-task' value '0.1' provided for argument 'memory_gb' is not valid. The value needs to be a whole number greater than 0."; - Assert.Contains(invalidArgoArg2, errors); - var invalidArgoArg3 = "Task: 'test-argo-task' value '2' provided for argument 'gpu_required' is not valid. The value needs to be 'true' or 'false'."; Assert.Contains(invalidArgoArg3, errors); @@ -436,7 +433,7 @@ public async Task ValidateWorkflow_ValidatesAWorkflow_ReturnsErrorsAndHasCorrect var invalidSourceName = "Data origin invalid_origin does not exists. Please review sources configuration management."; Assert.Contains(invalidSourceName, errors); - var invalidArgoKey = $"Task: 'invalid-key-argo-task' args has invalid keys: invalid_key. Please only specify keys from the following list: workflow_template_name, priority, cpu, memory_gb, gpu_required."; + var invalidArgoKey = $"Task: 'invalid-key-argo-task' args has invalid keys: invalid_key. Please only specify keys from the following list: workflow_template_name, priority, cpu, memory, gpu_required."; Assert.Contains(invalidArgoKey, errors); }