From 8974dda4324f18722582e866e444a7ba1f4cd06c Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Wed, 11 Dec 2024 12:59:22 -0500 Subject: [PATCH 01/28] wip --- service/build.gradle | 1 + .../internal/NotificationConfiguration.java | 6 ++ .../pipelines/db/entities/PipelineRun.java | 7 +- .../notifications/NotificationService.java | 94 +++++++++++++++++++ .../notifications/PubsubService.java | 34 +++++++ .../TeaspoonsJobFailedNotification.java | 37 ++++++++ .../TeaspoonsJobSucceededNotification.java | 34 +++++++ .../service/PipelineRunsService.java | 6 +- .../steps/common/CompletePipelineRunStep.java | 10 +- service/src/main/resources/application.yml | 7 ++ .../common/CompletePipelineRunStepTest.java | 8 +- 11 files changed, 237 insertions(+), 7 deletions(-) create mode 100644 service/src/main/java/bio/terra/pipelines/app/configuration/internal/NotificationConfiguration.java create mode 100644 service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java create mode 100644 service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java create mode 100644 service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotification.java create mode 100644 service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotification.java diff --git a/service/build.gradle b/service/build.gradle index 47ad118c..ab1df168 100644 --- a/service/build.gradle +++ b/service/build.gradle @@ -89,6 +89,7 @@ dependencies { // gcs implementation platform('com.google.cloud:libraries-bom:26.44.0') implementation 'com.google.cloud:google-cloud-storage' + implementation 'com.google.cloud:google-cloud-pubsub' liquibaseRuntime 'info.picocli:picocli:4.6.1' liquibaseRuntime 'org.postgresql:postgresql:42.6.1' diff --git a/service/src/main/java/bio/terra/pipelines/app/configuration/internal/NotificationConfiguration.java b/service/src/main/java/bio/terra/pipelines/app/configuration/internal/NotificationConfiguration.java new file mode 100644 index 00000000..32da2ce4 --- /dev/null +++ b/service/src/main/java/bio/terra/pipelines/app/configuration/internal/NotificationConfiguration.java @@ -0,0 +1,6 @@ +package bio.terra.pipelines.app.configuration.internal; + +import org.springframework.boot.context.properties.ConfigurationProperties; + +@ConfigurationProperties("pipelines.notifications") +public record NotificationConfiguration(String projectId, String topicId) {} diff --git a/service/src/main/java/bio/terra/pipelines/db/entities/PipelineRun.java b/service/src/main/java/bio/terra/pipelines/db/entities/PipelineRun.java index c5513bf6..5aa49472 100644 --- a/service/src/main/java/bio/terra/pipelines/db/entities/PipelineRun.java +++ b/service/src/main/java/bio/terra/pipelines/db/entities/PipelineRun.java @@ -69,6 +69,9 @@ public class PipelineRun { @Column(name = "description") private String description; + @Column(name = "quota_consumed") + private Integer quotaConsumed; + /** Constructor for in progress or complete PipelineRun. */ public PipelineRun( UUID jobId, @@ -83,7 +86,8 @@ public PipelineRun( Instant created, Instant updated, CommonPipelineRunStatusEnum status, - String description) { + String description, + Integer quotaConsumed) { this.jobId = jobId; this.userId = userId; this.pipelineId = pipelineId; @@ -97,6 +101,7 @@ public PipelineRun( this.updated = updated; this.status = status; this.description = description; + this.quotaConsumed = quotaConsumed; } /** Constructor for creating a new GCP pipeline run. Timestamps are auto-generated. */ diff --git a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java new file mode 100644 index 00000000..41a53bd3 --- /dev/null +++ b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java @@ -0,0 +1,94 @@ +package bio.terra.pipelines.notifications; + +import bio.terra.pipelines.app.configuration.internal.NotificationConfiguration; +import bio.terra.pipelines.db.entities.PipelineRun; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.annotations.VisibleForTesting; +import jakarta.annotation.PostConstruct; +import java.io.IOException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.stereotype.Service; + +/** + * Service to encapsulate the logic for sending email notifications to users. Works with the Terra + * Thurloe service via PubSub messages. + */ +@Service +public class NotificationService { + private static final Logger logger = LoggerFactory.getLogger(NotificationService.class); + + private final PubsubService pubsubService; + private final NotificationConfiguration notificationConfiguration; + private final ObjectMapper objectMapper; + + public NotificationService( + PubsubService pubsubService, + NotificationConfiguration notificationConfiguration, + ObjectMapper objectMapper) { + this.pubsubService = pubsubService; + this.notificationConfiguration = notificationConfiguration; + this.objectMapper = objectMapper; + } + + @VisibleForTesting + @PostConstruct + protected void createTopic() { + try { + pubsubService.createTopic( + notificationConfiguration.projectId(), notificationConfiguration.topicId()); + } catch (IOException e) { + logger.warn("Error creating notification topic", e); + } + } + + public void pipelineRunSucceeded( + PipelineRun pipelineRun, + String pipelineDisplayName, + String quotaConsumedByJob, + String quotaRemaining) { + try { + pubsubService.publishMessage( + notificationConfiguration.projectId(), + notificationConfiguration.topicId(), + objectMapper.writeValueAsString( + new TeaspoonsJobSucceededNotification( + pipelineRun.getUserId(), + pipelineDisplayName, + pipelineRun.getJobId().toString(), + pipelineRun.getCreated().toString(), + pipelineRun.getUpdated().toString(), + quotaConsumedByJob, + quotaRemaining, + pipelineRun.getDescription()))); + } catch (IOException e) { + logger.error("Error sending pipelineRunSucceeded notification", e); + } + } + + public void pipelineRunFailed( + PipelineRun pipelineRun, + String pipelineDisplayName, + String quotaConsumedByJob, + String quotaRemaining, + String errorMessage) { + try { + pubsubService.publishMessage( + notificationConfiguration.projectId(), + notificationConfiguration.topicId(), + objectMapper.writeValueAsString( + new TeaspoonsJobFailedNotification( + pipelineRun.getUserId(), + pipelineDisplayName, + pipelineRun.getJobId().toString(), + errorMessage, + pipelineRun.getCreated().toString(), + pipelineRun.getUpdated().toString(), + quotaConsumedByJob, + quotaRemaining, + pipelineRun.getDescription()))); + } catch (IOException e) { + logger.error("Error sending pipelineRunFailed notification", e); + } + } +} diff --git a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java new file mode 100644 index 00000000..c46375b4 --- /dev/null +++ b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java @@ -0,0 +1,34 @@ +package bio.terra.pipelines.notifications; + +import com.google.cloud.pubsub.v1.Publisher; +import com.google.cloud.pubsub.v1.TopicAdminClient; +import com.google.protobuf.ByteString; +import com.google.pubsub.v1.PubsubMessage; +import com.google.pubsub.v1.Topic; +import com.google.pubsub.v1.TopicName; +import java.io.IOException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.stereotype.Service; + +/** Service to interact with Pubsub. Used by NotificationService. */ +@Service +public class PubsubService { + private static final Logger logger = LoggerFactory.getLogger(PubsubService.class); + + public void createTopic(String projectId, String topicId) throws IOException { + try (TopicAdminClient topicAdminClient = TopicAdminClient.create()) { + TopicName topicName = TopicName.of(projectId, topicId); + if (topicAdminClient.getTopic(topicName) == null) { + Topic topic = topicAdminClient.createTopic(topicName); + logger.info("Created topic: {}", topic.getName()); + } + } + } + + public void publishMessage(String projectId, String topicId, String message) throws IOException { + TopicName topicName = TopicName.of(projectId, topicId); + var publisher = Publisher.newBuilder(topicName).build(); + publisher.publish(PubsubMessage.newBuilder().setData(ByteString.copyFromUtf8(message)).build()); + } +} diff --git a/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotification.java b/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotification.java new file mode 100644 index 00000000..3c16bb09 --- /dev/null +++ b/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotification.java @@ -0,0 +1,37 @@ +package bio.terra.pipelines.notifications; + +public record TeaspoonsJobFailedNotification( + String notificationType, + String recipientUserId, + String pipelineDisplayName, + String jobId, + String errorMessage, + String timeSubmitted, + String timeCompleted, + String quotaConsumedByJob, + String quotaRemaining, + String userDescription) { + + public TeaspoonsJobFailedNotification( + String recipientUserId, + String pipelineDisplayName, + String jobId, + String errorMessage, + String timeSubmitted, + String timeCompleted, + String quotaConsumedByJob, + String quotaRemaining, + String userDescription) { + this( + "TeaspoonsJobFailedNotification", + recipientUserId, + pipelineDisplayName, + jobId, + errorMessage, + timeSubmitted, + timeCompleted, + quotaConsumedByJob, + quotaRemaining, + userDescription); + } +} diff --git a/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotification.java b/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotification.java new file mode 100644 index 00000000..2c8f3baf --- /dev/null +++ b/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotification.java @@ -0,0 +1,34 @@ +package bio.terra.pipelines.notifications; + +public record TeaspoonsJobSucceededNotification( + String notificationType, + String recipientUserId, + String pipelineDisplayName, + String jobId, + String timeSubmitted, + String timeCompleted, + String quotaConsumedByJob, + String quotaRemaining, + String userDescription) { + + public TeaspoonsJobSucceededNotification( + String recipientUserId, + String pipelineDisplayName, + String jobId, + String timeSubmitted, + String timeCompleted, + String quotaConsumedByJob, + String quotaRemaining, + String userDescription) { + this( + "TeaspoonsJobSucceededNotification", + recipientUserId, + pipelineDisplayName, + jobId, + timeSubmitted, + timeCompleted, + quotaConsumedByJob, + quotaRemaining, + userDescription); + } +} diff --git a/service/src/main/java/bio/terra/pipelines/service/PipelineRunsService.java b/service/src/main/java/bio/terra/pipelines/service/PipelineRunsService.java index 606a8baa..3c13ed23 100644 --- a/service/src/main/java/bio/terra/pipelines/service/PipelineRunsService.java +++ b/service/src/main/java/bio/terra/pipelines/service/PipelineRunsService.java @@ -287,7 +287,8 @@ public PipelineRun startPipelineRunInDb(UUID jobId, String userId) { } /** - * Mark a pipeline run as successful (status = SUCCEEDED) in our database. + * Mark a pipeline run as successful (status = SUCCEEDED) in our database and store the quota + * consumed by the job. * *

We expect this method to be called by the final step of a flight, at which point we assume * that the pipeline_run has completed successfully. Therefore, we do not do any checks on the @@ -296,12 +297,13 @@ public PipelineRun startPipelineRunInDb(UUID jobId, String userId) { */ @WriteTransaction public PipelineRun markPipelineRunSuccessAndWriteOutputs( - UUID jobId, String userId, Map outputs) { + UUID jobId, String userId, Map outputs, int quotaConsumed) { PipelineRun pipelineRun = getPipelineRun(jobId, userId); pipelineInputsOutputsService.savePipelineOutputs(pipelineRun.getId(), outputs); pipelineRun.setStatus(CommonPipelineRunStatusEnum.SUCCEEDED); + pipelineRun.setQuotaConsumed(quotaConsumed); return pipelineRunsRepository.save(pipelineRun); } diff --git a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java index 42c85730..87333051 100644 --- a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java +++ b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java @@ -12,6 +12,9 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +/** + * Step to mark a pipeline run as a success and write the outputs and quota consumed to the database + */ public class CompletePipelineRunStep implements Step { private final PipelineRunsService pipelineRunsService; private final Logger logger = LoggerFactory.getLogger(CompletePipelineRunStep.class); @@ -31,11 +34,14 @@ public StepResult doStep(FlightContext flightContext) { // validate and extract parameters from working map var workingMap = flightContext.getWorkingMap(); - FlightUtils.validateRequiredEntries(workingMap, ImputationJobMapKeys.PIPELINE_RUN_OUTPUTS); + FlightUtils.validateRequiredEntries( + workingMap, ImputationJobMapKeys.PIPELINE_RUN_OUTPUTS, ImputationJobMapKeys.QUOTA_CONSUMED); Map outputsMap = workingMap.get(ImputationJobMapKeys.PIPELINE_RUN_OUTPUTS, Map.class); + int quotaConsumed = workingMap.get(ImputationJobMapKeys.QUOTA_CONSUMED, Integer.class); - pipelineRunsService.markPipelineRunSuccessAndWriteOutputs(jobId, userId, outputsMap); + pipelineRunsService.markPipelineRunSuccessAndWriteOutputs( + jobId, userId, outputsMap, quotaConsumed); logger.info("Marked run {} as a success and wrote outputs to the db", jobId); diff --git a/service/src/main/resources/application.yml b/service/src/main/resources/application.yml index 26f682fc..043a4952 100644 --- a/service/src/main/resources/application.yml +++ b/service/src/main/resources/application.yml @@ -34,6 +34,9 @@ env: imputation: # the default currently points to the GCP dev workspace teaspoons-imputation-dev/teaspoons_imputation_dev_storage_workspace_20240726 storageWorkspaceStorageUrl: ${IMPUTATION_STORAGE_WORKSPACE_STORAGE_URL:gs://fc-secure-10efd4d7-392a-4e9e-89ea-d6629fbb06cc} + notifications: + projectId: ${NOTIFICATIONS_PROJECT_ID:broad-dsde-dev} + topicId: ${NOTIFICATIONS_TOPIC_ID:workbench-notifications-dev} # Below here is non-deployment-specific @@ -155,6 +158,10 @@ pipelines: quotaConsumedPollingIntervalSeconds: 60 quotaConsumedUseCallCaching: false + notifications: + projectId: ${env.pipelines.notifications.projectId} + topicId: ${env.pipelines.notifications.topicId} + terra.common: kubernetes: in-kubernetes: ${env.kubernetes.in-kubernetes} # whether to use a pubsub queue for Stairway; if false, use a local queue diff --git a/service/src/test/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStepTest.java b/service/src/test/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStepTest.java index 41fa464a..52527ba5 100644 --- a/service/src/test/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStepTest.java +++ b/service/src/test/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStepTest.java @@ -34,6 +34,7 @@ class CompletePipelineRunStepTest extends BaseEmbeddedDbTest { @Mock private FlightContext flightContext; private final UUID testJobId = TestUtils.TEST_NEW_UUID; + private final Integer quotaConsumed = 50; @BeforeEach void setup() { @@ -41,6 +42,7 @@ void setup() { var workingMap = new FlightMap(); workingMap.put(ImputationJobMapKeys.PIPELINE_RUN_OUTPUTS, TestUtils.TEST_PIPELINE_OUTPUTS); + workingMap.put(ImputationJobMapKeys.QUOTA_CONSUMED, quotaConsumed); when(flightContext.getInputParameters()).thenReturn(inputParameters); when(flightContext.getWorkingMap()).thenReturn(workingMap); @@ -68,7 +70,8 @@ void doStepSuccess() throws JsonProcessingException { null, null, CommonPipelineRunStatusEnum.SUCCEEDED, - TestUtils.TEST_PIPELINE_DESCRIPTION_1)); + TestUtils.TEST_PIPELINE_DESCRIPTION_1, + null)); // do the step var writeJobStep = new CompletePipelineRunStep(pipelineRunsService); @@ -79,12 +82,13 @@ void doStepSuccess() throws JsonProcessingException { assertEquals(StepStatus.STEP_RESULT_SUCCESS, result.getStepStatus()); - // make sure the run was updated with isSuccess + // make sure the run was updated with isSuccess and quotaConsumed PipelineRun writtenJob = pipelineRunsRepository .findByJobIdAndUserId(testJobId, inputParams.get(JobMapKeys.USER_ID, String.class)) .orElseThrow(); assertEquals(CommonPipelineRunStatusEnum.SUCCEEDED, writtenJob.getStatus()); + assertEquals(quotaConsumed, writtenJob.getQuotaConsumed()); assertTrue(writtenJob.getStatus().isSuccess()); // make sure outputs were written to db From d35f5fe4e0f8e21e9c8246e4bd564e968d12ae92 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Thu, 19 Dec 2024 13:21:12 -0500 Subject: [PATCH 02/28] resolve merge conflicts --- .../pipelines/common/utils/FlightBeanBag.java | 4 + .../notifications/NotificationService.java | 14 ++-- .../imputation/RunImputationGcpJobFlight.java | 9 +++ .../SendJobSucceededNotificationStep.java | 73 +++++++++++++++++++ ...taConsumed_update_pipelineDisplayName.yaml | 22 ++++++ .../common/utils/FlightBeanBagTest.java | 4 + .../PipelineRunsApiControllerTest.java | 32 +++++--- .../service/PipelineRunsServiceTest.java | 4 +- .../RunImputationGcpFlightTest.java | 3 +- 9 files changed, 145 insertions(+), 20 deletions(-) create mode 100644 service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java create mode 100644 service/src/main/resources/db/changesets/20241212_add_quotaConsumed_update_pipelineDisplayName.yaml diff --git a/service/src/main/java/bio/terra/pipelines/common/utils/FlightBeanBag.java b/service/src/main/java/bio/terra/pipelines/common/utils/FlightBeanBag.java index 163a88a2..d6d68c9a 100644 --- a/service/src/main/java/bio/terra/pipelines/common/utils/FlightBeanBag.java +++ b/service/src/main/java/bio/terra/pipelines/common/utils/FlightBeanBag.java @@ -9,6 +9,7 @@ import bio.terra.pipelines.dependencies.sam.SamService; import bio.terra.pipelines.dependencies.wds.WdsService; import bio.terra.pipelines.dependencies.workspacemanager.WorkspaceManagerService; +import bio.terra.pipelines.notifications.NotificationService; import bio.terra.pipelines.service.PipelineInputsOutputsService; import bio.terra.pipelines.service.PipelineRunsService; import bio.terra.pipelines.service.PipelinesService; @@ -38,6 +39,7 @@ public class FlightBeanBag { private final WorkspaceManagerService workspaceManagerService; private final RawlsService rawlsService; private final QuotasService quotasService; + private final NotificationService notificationService; private final ImputationConfiguration imputationConfiguration; private final CbasConfiguration cbasConfiguration; private final WdlPipelineConfiguration wdlPipelineConfiguration; @@ -54,6 +56,7 @@ public FlightBeanBag( CbasService cbasService, RawlsService rawlsService, QuotasService quotasService, + NotificationService notificationService, WorkspaceManagerService workspaceManagerService, ImputationConfiguration imputationConfiguration, CbasConfiguration cbasConfiguration, @@ -68,6 +71,7 @@ public FlightBeanBag( this.workspaceManagerService = workspaceManagerService; this.rawlsService = rawlsService; this.quotasService = quotasService; + this.notificationService = notificationService; this.imputationConfiguration = imputationConfiguration; this.cbasConfiguration = cbasConfiguration; this.wdlPipelineConfiguration = wdlPipelineConfiguration; diff --git a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java index 41a53bd3..52e04580 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java @@ -42,11 +42,8 @@ protected void createTopic() { } } - public void pipelineRunSucceeded( - PipelineRun pipelineRun, - String pipelineDisplayName, - String quotaConsumedByJob, - String quotaRemaining) { + public void sendPipelineRunSucceededNotification( + PipelineRun pipelineRun, String pipelineDisplayName, String quotaRemaining) { try { pubsubService.publishMessage( notificationConfiguration.projectId(), @@ -58,7 +55,7 @@ public void pipelineRunSucceeded( pipelineRun.getJobId().toString(), pipelineRun.getCreated().toString(), pipelineRun.getUpdated().toString(), - quotaConsumedByJob, + pipelineRun.getQuotaConsumed().toString(), quotaRemaining, pipelineRun.getDescription()))); } catch (IOException e) { @@ -66,10 +63,9 @@ public void pipelineRunSucceeded( } } - public void pipelineRunFailed( + public void sendPipelineRunFailedNotification( PipelineRun pipelineRun, String pipelineDisplayName, - String quotaConsumedByJob, String quotaRemaining, String errorMessage) { try { @@ -84,7 +80,7 @@ public void pipelineRunFailed( errorMessage, pipelineRun.getCreated().toString(), pipelineRun.getUpdated().toString(), - quotaConsumedByJob, + "0", quotaRemaining, pipelineRun.getDescription()))); } catch (IOException e) { diff --git a/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpJobFlight.java b/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpJobFlight.java index ccd4db7d..1c1d10a2 100644 --- a/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpJobFlight.java +++ b/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpJobFlight.java @@ -9,6 +9,7 @@ import bio.terra.pipelines.stairway.steps.common.FetchQuotaConsumedFromDataTableStep; import bio.terra.pipelines.stairway.steps.common.PollQuotaConsumedSubmissionStatusStep; import bio.terra.pipelines.stairway.steps.common.QuotaConsumedValidationStep; +import bio.terra.pipelines.stairway.steps.common.SendJobSucceededNotificationStep; import bio.terra.pipelines.stairway.steps.common.SubmitQuotaConsumedSubmissionStep; import bio.terra.pipelines.stairway.steps.imputation.PrepareImputationInputsStep; import bio.terra.pipelines.stairway.steps.imputation.gcp.AddDataTableRowStep; @@ -122,5 +123,13 @@ public RunImputationGcpJobFlight(FlightMap inputParameters, Object beanBag) { externalServiceRetryRule); addStep(new CompletePipelineRunStep(flightBeanBag.getPipelineRunsService()), dbRetryRule); + + addStep( + new SendJobSucceededNotificationStep( + flightBeanBag.getPipelineRunsService(), + flightBeanBag.getNotificationService(), + flightBeanBag.getPipelinesService(), + flightBeanBag.getQuotasService()), + dbRetryRule); } } diff --git a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java new file mode 100644 index 00000000..12cf6823 --- /dev/null +++ b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java @@ -0,0 +1,73 @@ +package bio.terra.pipelines.stairway.steps.common; + +import bio.terra.pipelines.common.utils.FlightUtils; +import bio.terra.pipelines.db.entities.Pipeline; +import bio.terra.pipelines.db.entities.PipelineRun; +import bio.terra.pipelines.db.entities.UserQuota; +import bio.terra.pipelines.dependencies.stairway.JobMapKeys; +import bio.terra.pipelines.notifications.NotificationService; +import bio.terra.pipelines.service.PipelineRunsService; +import bio.terra.pipelines.service.PipelinesService; +import bio.terra.pipelines.service.QuotasService; +import bio.terra.stairway.FlightContext; +import bio.terra.stairway.Step; +import bio.terra.stairway.StepResult; +import java.util.UUID; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Step to send an email notification that a job has succeeded. This step cannot fail (we catch all + * exceptions). + */ +public class SendJobSucceededNotificationStep implements Step { + private final PipelineRunsService pipelineRunsService; + private final PipelinesService pipelinesService; + private final QuotasService quotasService; + private final NotificationService notificationService; + private final Logger logger = LoggerFactory.getLogger(CompletePipelineRunStep.class); + + public SendJobSucceededNotificationStep( + PipelineRunsService pipelineRunsService, + NotificationService notificationService, + PipelinesService pipelinesService, + QuotasService quotasService) { + this.pipelineRunsService = pipelineRunsService; + this.notificationService = notificationService; + this.pipelinesService = pipelinesService; + this.quotasService = quotasService; + } + + @Override + public StepResult doStep(FlightContext flightContext) { + // we place the entire logic of this step in a try-catch so that it cannot fail + try { + // validate and extract parameters from input map + var inputParameters = flightContext.getInputParameters(); + FlightUtils.validateRequiredEntries(inputParameters, JobMapKeys.USER_ID); + + UUID jobId = UUID.fromString(flightContext.getFlightId()); + String userId = inputParameters.get(JobMapKeys.USER_ID, String.class); + + PipelineRun pipelineRun = pipelineRunsService.getPipelineRun(jobId, userId); + Pipeline pipeline = pipelinesService.getPipelineById(pipelineRun.getPipelineId()); + String pipelineDisplayName = pipeline.getDisplayName(); + UserQuota userQuota = + quotasService.getQuotaForUserAndPipeline(userId, pipeline.getName()).get(); + String quotaRemaining = String.valueOf(userQuota.getQuota() - userQuota.getQuotaConsumed()); + + // send email notification + notificationService.sendPipelineRunSucceededNotification( + pipelineRun, pipelineDisplayName, quotaRemaining); + } catch (Exception e) { + logger.error("Failed to send email notification", e); + } + return StepResult.getStepResultSuccess(); + } + + @Override + public StepResult undoStep(FlightContext flightContext) { + // nothing to undo + return StepResult.getStepResultSuccess(); + } +} diff --git a/service/src/main/resources/db/changesets/20241212_add_quotaConsumed_update_pipelineDisplayName.yaml b/service/src/main/resources/db/changesets/20241212_add_quotaConsumed_update_pipelineDisplayName.yaml new file mode 100644 index 00000000..8b4ea01b --- /dev/null +++ b/service/src/main/resources/db/changesets/20241212_add_quotaConsumed_update_pipelineDisplayName.yaml @@ -0,0 +1,22 @@ +# add quota_consumed to pipeline_runs table and update display_name for array_imputation in pipelines table + +databaseChangeLog: + - changeSet: + id: add quota_consumed to pipeline_runs table and update display_name for array_imputation in pipelines table + author: mma + changes: + - addColumn: + columns: + - column: + name: quota_consumed + type: int + constraints: + nullable: true + tableName: pipeline_runs + - update: + tableName: pipelines + columns: + - column: + name: display_name + value: "All of Us/AnVIL Array Imputation" + where: name='array_imputation' AND version=0 diff --git a/service/src/test/java/bio/terra/pipelines/common/utils/FlightBeanBagTest.java b/service/src/test/java/bio/terra/pipelines/common/utils/FlightBeanBagTest.java index 8348d68e..fd6ec0c4 100644 --- a/service/src/test/java/bio/terra/pipelines/common/utils/FlightBeanBagTest.java +++ b/service/src/test/java/bio/terra/pipelines/common/utils/FlightBeanBagTest.java @@ -11,6 +11,7 @@ import bio.terra.pipelines.dependencies.sam.SamService; import bio.terra.pipelines.dependencies.wds.WdsService; import bio.terra.pipelines.dependencies.workspacemanager.WorkspaceManagerService; +import bio.terra.pipelines.notifications.NotificationService; import bio.terra.pipelines.service.PipelineInputsOutputsService; import bio.terra.pipelines.service.PipelineRunsService; import bio.terra.pipelines.service.PipelinesService; @@ -31,6 +32,7 @@ class FlightBeanBagTest extends BaseEmbeddedDbTest { @Autowired private WorkspaceManagerService workspaceManagerService; @Autowired private RawlsService rawlsService; @Autowired private QuotasService quotasService; + @Autowired private NotificationService notificationService; @Autowired private ImputationConfiguration imputationConfiguration; @Autowired private CbasConfiguration cbasConfiguration; @Autowired private WdlPipelineConfiguration wdlPipelineConfiguration; @@ -48,6 +50,7 @@ void testFlightBeanBag() { cbasService, rawlsService, quotasService, + notificationService, workspaceManagerService, imputationConfiguration, cbasConfiguration, @@ -62,6 +65,7 @@ void testFlightBeanBag() { assertEquals(workspaceManagerService, flightBeanBag.getWorkspaceManagerService()); assertEquals(rawlsService, flightBeanBag.getRawlsService()); assertEquals(quotasService, flightBeanBag.getQuotasService()); + assertEquals(notificationService, flightBeanBag.getNotificationService()); assertEquals(imputationConfiguration, flightBeanBag.getImputationConfiguration()); assertEquals(cbasConfiguration, flightBeanBag.getCbasConfiguration()); assertEquals(wdlPipelineConfiguration, flightBeanBag.getWdlPipelineConfiguration()); diff --git a/service/src/test/java/bio/terra/pipelines/controller/PipelineRunsApiControllerTest.java b/service/src/test/java/bio/terra/pipelines/controller/PipelineRunsApiControllerTest.java index d952eed6..b3c8f87a 100644 --- a/service/src/test/java/bio/terra/pipelines/controller/PipelineRunsApiControllerTest.java +++ b/service/src/test/java/bio/terra/pipelines/controller/PipelineRunsApiControllerTest.java @@ -87,6 +87,8 @@ class PipelineRunsApiControllerTest { private final Instant updatedTime = Instant.now(); private final Map testOutputs = TestUtils.TEST_PIPELINE_OUTPUTS; + private final Integer testQuotaConsumed = 10; + @BeforeEach void beforeEach() { when(samConfiguration.baseUri()).thenReturn("baseSamUri"); @@ -441,7 +443,9 @@ void startImputationRunStairwayError() throws Exception { void getPipelineRunResultDoneSuccess() throws Exception { String pipelineName = PipelinesEnum.ARRAY_IMPUTATION.getValue(); String jobIdString = newJobId.toString(); - PipelineRun pipelineRun = getPipelineRunWithStatus(CommonPipelineRunStatusEnum.SUCCEEDED); + PipelineRun pipelineRun = + getPipelineRunWithStatusAndQuotaConsumed( + CommonPipelineRunStatusEnum.SUCCEEDED, testQuotaConsumed); ApiPipelineRunOutputs apiPipelineRunOutputs = new ApiPipelineRunOutputs(); apiPipelineRunOutputs.putAll(testOutputs); @@ -483,7 +487,8 @@ void getPipelineRunResultDoneFailed() throws Exception { String jobIdString = newJobId.toString(); String errorMessage = "test exception message"; Integer statusCode = 500; - PipelineRun pipelineRun = getPipelineRunWithStatus(CommonPipelineRunStatusEnum.FAILED); + PipelineRun pipelineRun = + getPipelineRunWithStatusAndQuotaConsumed(CommonPipelineRunStatusEnum.FAILED, null); ApiErrorReport errorReport = new ApiErrorReport().message(errorMessage).statusCode(statusCode); @@ -612,8 +617,10 @@ void getAllPipelineRunsWithNoPageToken() throws Exception { PipelineRun pipelineRunPreparing = getPipelineRunPreparing(preparingDescription); PipelineRun pipelineRunPreparingNoDescription = getPipelineRunPreparing(null); PipelineRun pipelineRunSucceeded = - getPipelineRunWithStatus(CommonPipelineRunStatusEnum.SUCCEEDED); - PipelineRun pipelineRunFailed = getPipelineRunWithStatus(CommonPipelineRunStatusEnum.FAILED); + getPipelineRunWithStatusAndQuotaConsumed( + CommonPipelineRunStatusEnum.SUCCEEDED, testQuotaConsumed); + PipelineRun pipelineRunFailed = + getPipelineRunWithStatusAndQuotaConsumed(CommonPipelineRunStatusEnum.FAILED, null); PageResponse> pageResponse = new PageResponse<>( List.of( @@ -757,7 +764,8 @@ private PipelineRun getPipelineRunPreparing(String description) { createdTime, updatedTime, CommonPipelineRunStatusEnum.PREPARING, - description); + description, + null); } /** helper method to create a PipelineRun object for a running job */ @@ -775,11 +783,16 @@ private PipelineRun getPipelineRunRunning() { createdTime, updatedTime, CommonPipelineRunStatusEnum.RUNNING, - TestUtils.TEST_PIPELINE_DESCRIPTION_1); + TestUtils.TEST_PIPELINE_DESCRIPTION_1, + null); } - /** helper method to create a PipelineRun object for a completed job. */ - private PipelineRun getPipelineRunWithStatus(CommonPipelineRunStatusEnum status) { + /** + * helper method to create a PipelineRun object for a completed job, specifying the status and + * quotaConsumed. + */ + private PipelineRun getPipelineRunWithStatusAndQuotaConsumed( + CommonPipelineRunStatusEnum status, Integer quotaConsumed) { return new PipelineRun( newJobId, testUser.getSubjectId(), @@ -793,6 +806,7 @@ private PipelineRun getPipelineRunWithStatus(CommonPipelineRunStatusEnum status) createdTime, updatedTime, status, - TestUtils.TEST_PIPELINE_DESCRIPTION_1); + TestUtils.TEST_PIPELINE_DESCRIPTION_1, + quotaConsumed); } } diff --git a/service/src/test/java/bio/terra/pipelines/service/PipelineRunsServiceTest.java b/service/src/test/java/bio/terra/pipelines/service/PipelineRunsServiceTest.java index 648eaa66..ffd60213 100644 --- a/service/src/test/java/bio/terra/pipelines/service/PipelineRunsServiceTest.java +++ b/service/src/test/java/bio/terra/pipelines/service/PipelineRunsServiceTest.java @@ -71,6 +71,7 @@ class PipelineRunsServiceTest extends BaseEmbeddedDbTest { private final String testControlWorkspaceGoogleProject = TestUtils.CONTROL_WORKSPACE_GOOGLE_PROJECT; private final String testUserDescription = TestUtils.TEST_USER_PROVIDED_DESCRIPTION; + private final Integer testQuotaConsumed = 10; private SimpleMeterRegistry meterRegistry; @@ -538,9 +539,10 @@ void markPipelineRunSuccess() { PipelineRun updatedPipelineRun = pipelineRunsService.markPipelineRunSuccessAndWriteOutputs( - testJobId, testUserId, TestUtils.TEST_PIPELINE_OUTPUTS); + testJobId, testUserId, TestUtils.TEST_PIPELINE_OUTPUTS, testQuotaConsumed); assertTrue(updatedPipelineRun.getStatus().isSuccess()); assertEquals(CommonPipelineRunStatusEnum.SUCCEEDED, updatedPipelineRun.getStatus()); + assertEquals(testQuotaConsumed, updatedPipelineRun.getQuotaConsumed()); PipelineOutput pipelineOutput = pipelineOutputsRepository.findPipelineOutputsByJobId(pipelineRun.getId()); diff --git a/service/src/test/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpFlightTest.java b/service/src/test/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpFlightTest.java index 75b13e72..bc96913c 100644 --- a/service/src/test/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpFlightTest.java +++ b/service/src/test/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpFlightTest.java @@ -39,7 +39,8 @@ class RunImputationGcpFlightTest extends BaseEmbeddedDbTest { "SubmitCromwellSubmissionStep", "PollCromwellSubmissionStatusStep", "CompletePipelineRunStep", - "FetchOutputsFromDataTableStep"); + "FetchOutputsFromDataTableStep", + "SendJobSucceededNotificationStep"); @Autowired FlightBeanBag flightBeanBag; private SimpleMeterRegistry meterRegistry; From 8ee74b767823fd1d9bebb6c228029db31b4ee56e Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Thu, 19 Dec 2024 13:22:05 -0500 Subject: [PATCH 03/28] resolve merge conflicts --- service/src/main/resources/db/changelog.xml | 1 + ...2_add_quota_consumed_update_pipeline_display_name.yaml} | 0 .../terra/pipelines/common/utils/FlightBeanBagTest.java | 7 ++++++- 3 files changed, 7 insertions(+), 1 deletion(-) rename service/src/main/resources/db/changesets/{20241212_add_quotaConsumed_update_pipelineDisplayName.yaml => 20241212_add_quota_consumed_update_pipeline_display_name.yaml} (100%) diff --git a/service/src/main/resources/db/changelog.xml b/service/src/main/resources/db/changelog.xml index 0d3316ec..0e0a32e7 100644 --- a/service/src/main/resources/db/changelog.xml +++ b/service/src/main/resources/db/changelog.xml @@ -8,6 +8,7 @@ + diff --git a/service/src/main/resources/db/changesets/20241212_add_quotaConsumed_update_pipelineDisplayName.yaml b/service/src/main/resources/db/changesets/20241212_add_quota_consumed_update_pipeline_display_name.yaml similarity index 100% rename from service/src/main/resources/db/changesets/20241212_add_quotaConsumed_update_pipelineDisplayName.yaml rename to service/src/main/resources/db/changesets/20241212_add_quota_consumed_update_pipeline_display_name.yaml diff --git a/service/src/test/java/bio/terra/pipelines/common/utils/FlightBeanBagTest.java b/service/src/test/java/bio/terra/pipelines/common/utils/FlightBeanBagTest.java index fd6ec0c4..49bb5bf9 100644 --- a/service/src/test/java/bio/terra/pipelines/common/utils/FlightBeanBagTest.java +++ b/service/src/test/java/bio/terra/pipelines/common/utils/FlightBeanBagTest.java @@ -18,6 +18,7 @@ import bio.terra.pipelines.service.QuotasService; import bio.terra.pipelines.testutils.BaseEmbeddedDbTest; import org.junit.jupiter.api.Test; +import org.mockito.Mock; import org.springframework.beans.factory.annotation.Autowired; class FlightBeanBagTest extends BaseEmbeddedDbTest { @@ -32,7 +33,11 @@ class FlightBeanBagTest extends BaseEmbeddedDbTest { @Autowired private WorkspaceManagerService workspaceManagerService; @Autowired private RawlsService rawlsService; @Autowired private QuotasService quotasService; - @Autowired private NotificationService notificationService; + + @Mock + private NotificationService + notificationService; // mock because at startup tries to auto-create a topic + @Autowired private ImputationConfiguration imputationConfiguration; @Autowired private CbasConfiguration cbasConfiguration; @Autowired private WdlPipelineConfiguration wdlPipelineConfiguration; From a72e8e14063caa37a84f34c2af473e072151fab9 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Mon, 16 Dec 2024 12:33:21 -0500 Subject: [PATCH 04/28] add failed notification hook --- ...StairwaySendFailedJobNotificationHook.java | 101 +++++++++++++++ .../dependencies/stairway/JobMapKeys.java | 2 + .../dependencies/stairway/JobService.java | 7 + .../service/PipelineRunsService.java | 1 + .../RunImputationAzureJobFlight.java | 1 + .../imputation/RunImputationGcpJobFlight.java | 1 + ...rwaySendFailedJobNotificationHookTest.java | 122 ++++++++++++++++++ .../testutils/StairwayTestUtils.java | 1 + 8 files changed, 236 insertions(+) create mode 100644 service/src/main/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHook.java create mode 100644 service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java diff --git a/service/src/main/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHook.java b/service/src/main/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHook.java new file mode 100644 index 00000000..1cc45a8a --- /dev/null +++ b/service/src/main/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHook.java @@ -0,0 +1,101 @@ +package bio.terra.pipelines.common.utils; + +import static bio.terra.pipelines.app.controller.JobApiUtils.buildApiErrorReport; +import static bio.terra.pipelines.common.utils.FlightUtils.flightMapKeyIsTrue; + +import bio.terra.pipelines.db.entities.Pipeline; +import bio.terra.pipelines.db.entities.PipelineRun; +import bio.terra.pipelines.db.entities.UserQuota; +import bio.terra.pipelines.dependencies.stairway.JobMapKeys; +import bio.terra.pipelines.generated.model.ApiErrorReport; +import bio.terra.pipelines.notifications.NotificationService; +import bio.terra.pipelines.service.PipelineRunsService; +import bio.terra.pipelines.service.PipelinesService; +import bio.terra.pipelines.service.QuotasService; +import bio.terra.stairway.FlightContext; +import bio.terra.stairway.FlightMap; +import bio.terra.stairway.FlightStatus; +import bio.terra.stairway.HookAction; +import bio.terra.stairway.StairwayHook; +import java.util.Optional; +import java.util.UUID; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.stereotype.Component; + +/** + * A {@link StairwayHook} that sends a Job Failed Notification email via pubsub/Thurloe upon flight + * failure. + * + *

This hook action will only run if the flight's input parameters contain the JobMapKeys key for + * DO_SEND_JOB_FAILURE_NOTIFICATION_HOOK and the flight's status is not SUCCESS. + * + *

The JobMapKeys key for PIPELINE_NAME is required to send the notification. + */ +@Component +public class StairwaySendFailedJobNotificationHook implements StairwayHook { + private final PipelineRunsService pipelineRunsService; + private final PipelinesService pipelinesService; + private final QuotasService quotasService; + private final NotificationService notificationService; + private static final Logger logger = + LoggerFactory.getLogger(StairwaySendFailedJobNotificationHook.class); + + public StairwaySendFailedJobNotificationHook( + PipelineRunsService pipelineRunsService, + NotificationService notificationService, + PipelinesService pipelinesService, + QuotasService quotasService) { + this.pipelineRunsService = pipelineRunsService; + this.notificationService = notificationService; + this.pipelinesService = pipelinesService; + this.quotasService = quotasService; + } + + @Override + public HookAction endFlight(FlightContext context) { + + FlightMap inputParameters = context.getInputParameters(); + + if (flightMapKeyIsTrue(inputParameters, JobMapKeys.DO_SEND_JOB_FAILURE_NOTIFICATION_HOOK) + && context.getFlightStatus() != FlightStatus.SUCCESS) { + logger.info( + "Flight has status {}, sending failed job notification email", context.getFlightStatus()); + + FlightUtils.validateRequiredEntries(inputParameters, JobMapKeys.USER_ID); + + UUID jobId = UUID.fromString(context.getFlightId()); + String userId = inputParameters.get(JobMapKeys.USER_ID, String.class); + + PipelineRun pipelineRun = pipelineRunsService.getPipelineRun(jobId, userId); + Pipeline pipeline = pipelinesService.getPipelineById(pipelineRun.getPipelineId()); + String pipelineDisplayName = pipeline.getDisplayName(); + // if flight fails before quota steps on user's first run, there won't be a row for them yet + // in the quotas table + UserQuota userQuota = + quotasService.getOrCreateQuotaForUserAndPipeline(userId, pipeline.getName()); + String quotaRemaining = String.valueOf(userQuota.getQuota() - userQuota.getQuotaConsumed()); + + // get exception + Optional exception = context.getResult().getException(); + String errorMessage; + if (exception.isPresent()) { + ApiErrorReport errorReport = + buildApiErrorReport(exception.get()); // use same logic that the status endpoint uses + errorMessage = errorReport.getMessage(); + } else { + // should this just fail? + logger.warn( + "No exception found in flight result for flight {} with status {}", + context.getFlightId(), + context.getFlightStatus()); + errorMessage = "Unknown error"; + } + + // send email notification + notificationService.sendPipelineRunFailedNotification( + pipelineRun, pipelineDisplayName, quotaRemaining, errorMessage); + } + return HookAction.CONTINUE; + } +} diff --git a/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobMapKeys.java b/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobMapKeys.java index 7ff217c6..7ee108b1 100644 --- a/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobMapKeys.java +++ b/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobMapKeys.java @@ -19,6 +19,8 @@ public class JobMapKeys { "do_set_pipeline_run_status_failed_hook"; public static final String DO_INCREMENT_METRICS_FAILED_COUNTER_HOOK = "do_increment_metrics_failed_counter_hook"; + public static final String DO_SEND_JOB_FAILURE_NOTIFICATION_HOOK = + "do_send_job_failure_notification_hook"; JobMapKeys() { throw new IllegalStateException("Attempted to instantiate utility class JobMapKeys"); diff --git a/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobService.java b/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobService.java index 76ca289a..9a9f5178 100644 --- a/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobService.java +++ b/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobService.java @@ -13,6 +13,7 @@ import bio.terra.pipelines.common.utils.FlightBeanBag; import bio.terra.pipelines.common.utils.PipelinesEnum; import bio.terra.pipelines.common.utils.StairwayFailedMetricsCounterHook; +import bio.terra.pipelines.common.utils.StairwaySendFailedJobNotificationHook; import bio.terra.pipelines.common.utils.StairwaySetPipelineRunStatusHook; import bio.terra.pipelines.dependencies.stairway.exception.*; import bio.terra.pipelines.dependencies.stairway.model.EnumeratedJob; @@ -112,6 +113,12 @@ public void initialize() { .addHook(new StairwayLoggingHook()) .addHook(new MonitoringHook(openTelemetry)) .addHook(new StairwayFailedMetricsCounterHook()) + .addHook( + new StairwaySendFailedJobNotificationHook( + flightBeanBag.getPipelineRunsService(), + flightBeanBag.getNotificationService(), + flightBeanBag.getPipelinesService(), + flightBeanBag.getQuotasService())) .addHook(new StairwaySetPipelineRunStatusHook(flightBeanBag.getPipelineRunsService())) .exceptionSerializer(new StairwayExceptionSerializer(objectMapper))); } diff --git a/service/src/main/java/bio/terra/pipelines/service/PipelineRunsService.java b/service/src/main/java/bio/terra/pipelines/service/PipelineRunsService.java index 3c13ed23..9073257a 100644 --- a/service/src/main/java/bio/terra/pipelines/service/PipelineRunsService.java +++ b/service/src/main/java/bio/terra/pipelines/service/PipelineRunsService.java @@ -163,6 +163,7 @@ public PipelineRun startPipelineRun(Pipeline pipeline, UUID jobId, String userId .addParameter(JobMapKeys.PIPELINE_ID, pipeline.getId()) .addParameter(JobMapKeys.DOMAIN_NAME, ingressConfiguration.getDomainName()) .addParameter(JobMapKeys.DO_SET_PIPELINE_RUN_STATUS_FAILED_HOOK, true) + .addParameter(JobMapKeys.DO_SEND_JOB_FAILURE_NOTIFICATION_HOOK, true) .addParameter(JobMapKeys.DO_INCREMENT_METRICS_FAILED_COUNTER_HOOK, true) .addParameter( ImputationJobMapKeys.PIPELINE_INPUT_DEFINITIONS, diff --git a/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationAzureJobFlight.java b/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationAzureJobFlight.java index 321c3982..5727163b 100644 --- a/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationAzureJobFlight.java +++ b/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationAzureJobFlight.java @@ -37,6 +37,7 @@ public RunImputationAzureJobFlight(FlightMap inputParameters, Object beanBag) { JobMapKeys.PIPELINE_ID, JobMapKeys.DOMAIN_NAME, JobMapKeys.DO_SET_PIPELINE_RUN_STATUS_FAILED_HOOK, + JobMapKeys.DO_SEND_JOB_FAILURE_NOTIFICATION_HOOK, JobMapKeys.DO_INCREMENT_METRICS_FAILED_COUNTER_HOOK, ImputationJobMapKeys.PIPELINE_INPUT_DEFINITIONS, ImputationJobMapKeys.PIPELINE_OUTPUT_DEFINITIONS, diff --git a/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpJobFlight.java b/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpJobFlight.java index 1c1d10a2..ff851c55 100644 --- a/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpJobFlight.java +++ b/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpJobFlight.java @@ -56,6 +56,7 @@ public RunImputationGcpJobFlight(FlightMap inputParameters, Object beanBag) { JobMapKeys.PIPELINE_ID, JobMapKeys.DOMAIN_NAME, JobMapKeys.DO_SET_PIPELINE_RUN_STATUS_FAILED_HOOK, + JobMapKeys.DO_SEND_JOB_FAILURE_NOTIFICATION_HOOK, JobMapKeys.DO_INCREMENT_METRICS_FAILED_COUNTER_HOOK, ImputationJobMapKeys.PIPELINE_INPUT_DEFINITIONS, ImputationJobMapKeys.PIPELINE_OUTPUT_DEFINITIONS, diff --git a/service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java b/service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java new file mode 100644 index 00000000..76e2b297 --- /dev/null +++ b/service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java @@ -0,0 +1,122 @@ +package bio.terra.pipelines.common.utils; + +import static bio.terra.pipelines.testutils.TestUtils.createNewPipelineRunWithJobId; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.params.provider.Arguments.arguments; +import static org.mockito.Mockito.when; + +import bio.terra.pipelines.db.entities.PipelineRun; +import bio.terra.pipelines.dependencies.stairway.JobMapKeys; +import bio.terra.pipelines.notifications.NotificationService; +import bio.terra.pipelines.service.PipelineRunsService; +import bio.terra.pipelines.service.PipelinesService; +import bio.terra.pipelines.service.QuotasService; +import bio.terra.pipelines.testutils.BaseEmbeddedDbTest; +import bio.terra.pipelines.testutils.StairwayTestUtils; +import bio.terra.pipelines.testutils.TestFlightContext; +import bio.terra.pipelines.testutils.TestUtils; +import bio.terra.stairway.FlightStatus; +import java.util.UUID; +import java.util.stream.Stream; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.mockito.Mock; +import org.springframework.beans.factory.annotation.Autowired; + +class StairwaySendFailedJobNotificationHookTest extends BaseEmbeddedDbTest { + StairwaySendFailedJobNotificationHook stairwaySendFailedJobNotificationHook; + @Autowired PipelineRunsService pipelineRunsService; + @Mock NotificationService notificationService; + @Autowired PipelinesService pipelinesService; + @Autowired QuotasService quotasService; + + private final UUID testJobId = TestUtils.TEST_NEW_UUID; + + @BeforeEach + void setup() { + when(notificationService).stairwaySendFailedJobNotificationHook = + new StairwaySendFailedJobNotificationHook( + pipelineRunsService, notificationService, pipelinesService, quotasService); + } + + private static Stream flightContexts() { + + return Stream.of( + // arguments: whether to include hook key in input Params, + // hook key value, flight status at endFlight time, whether failed job notification should + // be sent + + arguments( + true, + true, + FlightStatus.SUCCESS, + false), // flight was successful, so the pipelineRun status should not be set to FAILED + arguments( + true, + true, + FlightStatus.ERROR, + true), // flight failed, so the pipelineRun status should be set to FAILED + arguments( + true, + true, + FlightStatus.FATAL, + true), // flight failed dismally, so the pipelineRun status should be set to FAILED + arguments( + true, + true, + FlightStatus.QUEUED, + true), // flight in an unexpected status for end of flight, so the pipelineRun status + // should be set to FAILED + arguments( + false, + false, // doesn't matter + FlightStatus.ERROR, + false), // flight failed, but the hook key was not included in the input parameters + arguments( + true, + false, + FlightStatus.ERROR, + false)); // flight failed, but the hook key value is false + } + + @ParameterizedTest + @MethodSource("flightContexts") + void endFlight( + boolean includeHookKey, + boolean hookKeyValue, + FlightStatus endFlightStatus, + boolean shouldSendFailedJobNotification) + throws InterruptedException { + var context = new TestFlightContext(); + + if (includeHookKey) { + // this includes setting the DO_SET_PIPELINE_RUN_STATUS_FAILED_HOOK key to true + StairwayTestUtils.constructCreateJobInputs(context.getInputParameters()); + context + .getInputParameters() + .put(JobMapKeys.DO_SEND_JOB_FAILURE_NOTIFICATION_HOOK, hookKeyValue); + } + + // write a new pipelineRun to the db - this includes status set to PREPARING + PipelineRun pipelineRun = createNewPipelineRunWithJobId(UUID.fromString(context.getFlightId())); + + stairwaySendFailedJobNotificationHook.startFlight(context); + + // set the end flight status + context.flightStatus(endFlightStatus); + + stairwaySendFailedJobNotificationHook.endFlight(context); + + // the flight did not fail, so the pipelineRun status should not have been updated to FAILED + PipelineRun writtenPipelineRun = + pipelineRunsRepository.findByJobIdAndUserId(testJobId, TestUtils.TEST_USER_ID_1).get(); + if (shouldSendFailedJobNotification) { + assertEquals(CommonPipelineRunStatusEnum.FAILED, writtenPipelineRun.getStatus()); + } else { + assertNotEquals(CommonPipelineRunStatusEnum.FAILED, writtenPipelineRun.getStatus()); + } + } +} diff --git a/service/src/test/java/bio/terra/pipelines/testutils/StairwayTestUtils.java b/service/src/test/java/bio/terra/pipelines/testutils/StairwayTestUtils.java index 6820f361..95543ebf 100644 --- a/service/src/test/java/bio/terra/pipelines/testutils/StairwayTestUtils.java +++ b/service/src/test/java/bio/terra/pipelines/testutils/StairwayTestUtils.java @@ -189,6 +189,7 @@ public static FlightMap constructCreateJobInputs( inputParameters.put(JobMapKeys.DOMAIN_NAME, domainName); inputParameters.put(JobMapKeys.PIPELINE_ID, pipelineId); inputParameters.put(JobMapKeys.DO_INCREMENT_METRICS_FAILED_COUNTER_HOOK, true); + inputParameters.put(JobMapKeys.DO_SEND_JOB_FAILURE_NOTIFICATION_HOOK, true); inputParameters.put(JobMapKeys.DO_SET_PIPELINE_RUN_STATUS_FAILED_HOOK, true); inputParameters.put(ImputationJobMapKeys.PIPELINE_INPUT_DEFINITIONS, pipelineInputDefinitions); inputParameters.put( From 6a8751cb98f7e058390c136194d7c3a15fbd83f1 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Mon, 16 Dec 2024 12:57:23 -0500 Subject: [PATCH 05/28] add log message for topic notification --- .../terra/pipelines/notifications/NotificationService.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java index 52e04580..9266766d 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java @@ -35,6 +35,10 @@ public NotificationService( @PostConstruct protected void createTopic() { try { + logger.info( + "Creating notification topic in project id {} and topic id {}", + notificationConfiguration.projectId(), + notificationConfiguration.topicId()); pubsubService.createTopic( notificationConfiguration.projectId(), notificationConfiguration.topicId()); } catch (IOException e) { From 11ad24cd3d38bf69c1774e48769af4b9cc252cbe Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Mon, 16 Dec 2024 14:24:24 -0500 Subject: [PATCH 06/28] add catch for error in PubsubService --- .../java/bio/terra/pipelines/notifications/PubsubService.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java index c46375b4..7e5509a2 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java @@ -23,6 +23,8 @@ public void createTopic(String projectId, String topicId) throws IOException { Topic topic = topicAdminClient.createTopic(topicName); logger.info("Created topic: {}", topic.getName()); } + } catch (IOException e) { + logger.error("Error creating topic", e); } } From b40ce697413233eb5183cd9357dd155dcb25ac4a Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Mon, 16 Dec 2024 15:06:23 -0500 Subject: [PATCH 07/28] add another log --- .../bio/terra/pipelines/notifications/NotificationService.java | 1 + 1 file changed, 1 insertion(+) diff --git a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java index 9266766d..f863752a 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java @@ -34,6 +34,7 @@ public NotificationService( @VisibleForTesting @PostConstruct protected void createTopic() { + logger.info("POST CONSTRUCT"); try { logger.info( "Creating notification topic in project id {} and topic id {}", From 104e73d1c5e4372a8af06006e27b618c23d41318 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Mon, 16 Dec 2024 15:12:59 -0500 Subject: [PATCH 08/28] don't try to create topic --- .../bio/terra/pipelines/notifications/NotificationService.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java index f863752a..affa2477 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java @@ -4,7 +4,6 @@ import bio.terra.pipelines.db.entities.PipelineRun; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.annotations.VisibleForTesting; -import jakarta.annotation.PostConstruct; import java.io.IOException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -32,7 +31,7 @@ public NotificationService( } @VisibleForTesting - @PostConstruct + // @PostConstruct protected void createTopic() { logger.info("POST CONSTRUCT"); try { From 8c708bcea41359d9912afb0a33c91ee017522ce3 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Mon, 16 Dec 2024 15:14:37 -0500 Subject: [PATCH 09/28] add back @PostConstruct --- .../bio/terra/pipelines/notifications/NotificationService.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java index affa2477..f863752a 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java @@ -4,6 +4,7 @@ import bio.terra.pipelines.db.entities.PipelineRun; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.annotations.VisibleForTesting; +import jakarta.annotation.PostConstruct; import java.io.IOException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -31,7 +32,7 @@ public NotificationService( } @VisibleForTesting - // @PostConstruct + @PostConstruct protected void createTopic() { logger.info("POST CONSTRUCT"); try { From 3a24d4b36ebe0d73683245b8480d8f7f9a0f4031 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Mon, 16 Dec 2024 15:26:15 -0500 Subject: [PATCH 10/28] remove createTopic PostConstruct --- .../notifications/NotificationService.java | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java index f863752a..c9cda32b 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java @@ -3,8 +3,6 @@ import bio.terra.pipelines.app.configuration.internal.NotificationConfiguration; import bio.terra.pipelines.db.entities.PipelineRun; import com.fasterxml.jackson.databind.ObjectMapper; -import com.google.common.annotations.VisibleForTesting; -import jakarta.annotation.PostConstruct; import java.io.IOException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -31,21 +29,21 @@ public NotificationService( this.objectMapper = objectMapper; } - @VisibleForTesting - @PostConstruct - protected void createTopic() { - logger.info("POST CONSTRUCT"); - try { - logger.info( - "Creating notification topic in project id {} and topic id {}", - notificationConfiguration.projectId(), - notificationConfiguration.topicId()); - pubsubService.createTopic( - notificationConfiguration.projectId(), notificationConfiguration.topicId()); - } catch (IOException e) { - logger.warn("Error creating notification topic", e); - } - } + // @VisibleForTesting + // @PostConstruct + // protected void createTopic() { + // logger.info("POST CONSTRUCT"); + // try { + // logger.info( + // "Creating notification topic in project id {} and topic id {}", + // notificationConfiguration.projectId(), + // notificationConfiguration.topicId()); + // pubsubService.createTopic( + // notificationConfiguration.projectId(), notificationConfiguration.topicId()); + // } catch (IOException e) { + // logger.warn("Error creating notification topic", e); + // } + // } public void sendPipelineRunSucceededNotification( PipelineRun pipelineRun, String pipelineDisplayName, String quotaRemaining) { From ae3ea1b954dd3ad85d5f3682884d372681d84355 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Tue, 17 Dec 2024 09:42:55 -0500 Subject: [PATCH 11/28] shut down pubsub publisher after sending message --- .../notifications/NotificationService.java | 4 +- .../notifications/PubsubService.java | 62 ++++++++++++++++++- ...rwaySendFailedJobNotificationHookTest.java | 22 +++---- 3 files changed, 72 insertions(+), 16 deletions(-) diff --git a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java index c9cda32b..f62f1070 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java @@ -61,7 +61,7 @@ public void sendPipelineRunSucceededNotification( pipelineRun.getQuotaConsumed().toString(), quotaRemaining, pipelineRun.getDescription()))); - } catch (IOException e) { + } catch (IOException | InterruptedException e) { logger.error("Error sending pipelineRunSucceeded notification", e); } } @@ -86,7 +86,7 @@ public void sendPipelineRunFailedNotification( "0", quotaRemaining, pipelineRun.getDescription()))); - } catch (IOException e) { + } catch (IOException | InterruptedException e) { logger.error("Error sending pipelineRunFailed notification", e); } } diff --git a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java index 7e5509a2..8d715af0 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java @@ -1,12 +1,18 @@ package bio.terra.pipelines.notifications; +import com.google.api.core.ApiFuture; +import com.google.api.core.ApiFutureCallback; +import com.google.api.core.ApiFutures; +import com.google.api.gax.rpc.ApiException; import com.google.cloud.pubsub.v1.Publisher; import com.google.cloud.pubsub.v1.TopicAdminClient; +import com.google.common.util.concurrent.MoreExecutors; import com.google.protobuf.ByteString; import com.google.pubsub.v1.PubsubMessage; import com.google.pubsub.v1.Topic; import com.google.pubsub.v1.TopicName; import java.io.IOException; +import java.util.concurrent.TimeUnit; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.stereotype.Service; @@ -28,9 +34,59 @@ public void createTopic(String projectId, String topicId) throws IOException { } } - public void publishMessage(String projectId, String topicId, String message) throws IOException { + public void publishMessage(String projectId, String topicId, String message) + throws IOException, InterruptedException { + // TopicName topicName = TopicName.of(projectId, topicId); + // var publisher = Publisher.newBuilder(topicName).build(); + // + // publisher.publish(PubsubMessage.newBuilder().setData(ByteString.copyFromUtf8(message)).build()); + // publisher.awaitTermination() + TopicName topicName = TopicName.of(projectId, topicId); - var publisher = Publisher.newBuilder(topicName).build(); - publisher.publish(PubsubMessage.newBuilder().setData(ByteString.copyFromUtf8(message)).build()); + Publisher publisher = null; + + try { + // Create a publisher instance with default settings bound to the topic + publisher = Publisher.newBuilder(topicName).build(); + + ByteString data = ByteString.copyFromUtf8(message); + PubsubMessage pubsubMessage = PubsubMessage.newBuilder().setData(data).build(); + + // Once published, returns a server-assigned message id (unique within the topic) + ApiFuture future = publisher.publish(pubsubMessage); + + // Add an asynchronous callback to handle success / failure + ApiFutures.addCallback( + future, + new ApiFutureCallback() { + + @Override + public void onFailure(Throwable throwable) { + if (throwable instanceof ApiException) { + ApiException apiException = ((ApiException) throwable); + // details on the API exception + logger.warn( + "Google API exception! status code: {}, is retryable: {}", + apiException.getStatusCode().getCode(), + apiException.isRetryable()); + } + logger.error("Error publishing message to Google PubSub: {}", message); + } + + @Override + public void onSuccess(String messageId) { + // Once published, returns server-assigned message ids (unique within the topic) + logger.info("Published message ID: {}", messageId); + } + }, + MoreExecutors.directExecutor()); + + } finally { + if (publisher != null) { + // When finished with the publisher, shutdown to free up resources. + publisher.shutdown(); + publisher.awaitTermination(1, TimeUnit.MINUTES); + } + } } } diff --git a/service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java b/service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java index 76e2b297..610ce070 100644 --- a/service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java +++ b/service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java @@ -1,10 +1,7 @@ package bio.terra.pipelines.common.utils; import static bio.terra.pipelines.testutils.TestUtils.createNewPipelineRunWithJobId; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.params.provider.Arguments.arguments; -import static org.mockito.Mockito.when; import bio.terra.pipelines.db.entities.PipelineRun; import bio.terra.pipelines.dependencies.stairway.JobMapKeys; @@ -37,7 +34,9 @@ class StairwaySendFailedJobNotificationHookTest extends BaseEmbeddedDbTest { @BeforeEach void setup() { - when(notificationService).stairwaySendFailedJobNotificationHook = + // when(notificationService). + + stairwaySendFailedJobNotificationHook = new StairwaySendFailedJobNotificationHook( pipelineRunsService, notificationService, pipelinesService, quotasService); } @@ -111,12 +110,13 @@ void endFlight( stairwaySendFailedJobNotificationHook.endFlight(context); // the flight did not fail, so the pipelineRun status should not have been updated to FAILED - PipelineRun writtenPipelineRun = - pipelineRunsRepository.findByJobIdAndUserId(testJobId, TestUtils.TEST_USER_ID_1).get(); - if (shouldSendFailedJobNotification) { - assertEquals(CommonPipelineRunStatusEnum.FAILED, writtenPipelineRun.getStatus()); - } else { - assertNotEquals(CommonPipelineRunStatusEnum.FAILED, writtenPipelineRun.getStatus()); - } + // PipelineRun writtenPipelineRun = + // pipelineRunsRepository.findByJobIdAndUserId(testJobId, + // TestUtils.TEST_USER_ID_1).get(); + // if (shouldSendFailedJobNotification) { + // assertEquals(CommonPipelineRunStatusEnum.FAILED, writtenPipelineRun.getStatus()); + // } else { + // assertNotEquals(CommonPipelineRunStatusEnum.FAILED, writtenPipelineRun.getStatus()); + // } } } From c6b9cba006d79a769807136662b28eb05e7b2da3 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Tue, 17 Dec 2024 11:30:17 -0500 Subject: [PATCH 12/28] add log for projectId and topicId --- .../bio/terra/pipelines/notifications/PubsubService.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java index 8d715af0..b5cfc8bb 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java @@ -44,6 +44,7 @@ public void publishMessage(String projectId, String topicId, String message) TopicName topicName = TopicName.of(projectId, topicId); Publisher publisher = null; + logger.info("Publishing message to Google PubSub projectId {}, topicId {}", projectId, topicId); try { // Create a publisher instance with default settings bound to the topic @@ -62,11 +63,10 @@ public void publishMessage(String projectId, String topicId, String message) @Override public void onFailure(Throwable throwable) { - if (throwable instanceof ApiException) { - ApiException apiException = ((ApiException) throwable); + if (throwable instanceof ApiException apiException) { // details on the API exception logger.warn( - "Google API exception! status code: {}, is retryable: {}", + "Google API exception: status code {}, is retryable: {}", apiException.getStatusCode().getCode(), apiException.isRetryable()); } From 8a086587b14902328e62a821bdc28a151654e069 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Tue, 17 Dec 2024 11:42:52 -0500 Subject: [PATCH 13/28] use the correct env var :facepalm: --- service/src/main/resources/application.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/service/src/main/resources/application.yml b/service/src/main/resources/application.yml index 043a4952..2bd58604 100644 --- a/service/src/main/resources/application.yml +++ b/service/src/main/resources/application.yml @@ -35,8 +35,8 @@ env: # the default currently points to the GCP dev workspace teaspoons-imputation-dev/teaspoons_imputation_dev_storage_workspace_20240726 storageWorkspaceStorageUrl: ${IMPUTATION_STORAGE_WORKSPACE_STORAGE_URL:gs://fc-secure-10efd4d7-392a-4e9e-89ea-d6629fbb06cc} notifications: - projectId: ${NOTIFICATIONS_PROJECT_ID:broad-dsde-dev} - topicId: ${NOTIFICATIONS_TOPIC_ID:workbench-notifications-dev} + projectId: ${NOTIFICATION_PROJECT_ID:broad-dsde-dev} + topicId: ${NOTIFICATION_TOPIC_ID:workbench-notifications-dev} # Below here is non-deployment-specific From 5ffb6e49d1a7aa59ed2b15366d06ee26947bb579 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Wed, 18 Dec 2024 09:50:06 -0500 Subject: [PATCH 14/28] update gha versions, some misc cleanup --- .github/workflows/tag-publish.yml | 4 ++-- .../notifications/NotificationService.java | 18 +----------------- 2 files changed, 3 insertions(+), 19 deletions(-) diff --git a/.github/workflows/tag-publish.yml b/.github/workflows/tag-publish.yml index 52650edb..1a0deddf 100644 --- a/.github/workflows/tag-publish.yml +++ b/.github/workflows/tag-publish.yml @@ -176,7 +176,7 @@ jobs: - name: Auth to GCP id: 'auth' - uses: google-github-actions/auth@v0 + uses: google-github-actions/auth@v2 with: token_format: 'access_token' workload_identity_provider: 'projects/1038484894585/locations/global/workloadIdentityPools/github-wi-pool/providers/github-wi-provider' @@ -184,7 +184,7 @@ jobs: # Install gcloud, `setup-gcloud` automatically picks up authentication from `auth`. - name: 'Set up Cloud SDK' - uses: 'google-github-actions/setup-gcloud@v0' + uses: google-github-actions/setup-gcloud@v2 - name: Explicitly auth Docker for Artifact Registry run: gcloud auth configure-docker $GOOGLE_DOCKER_REPOSITORY --quiet diff --git a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java index f62f1070..10d3c687 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java @@ -29,22 +29,6 @@ public NotificationService( this.objectMapper = objectMapper; } - // @VisibleForTesting - // @PostConstruct - // protected void createTopic() { - // logger.info("POST CONSTRUCT"); - // try { - // logger.info( - // "Creating notification topic in project id {} and topic id {}", - // notificationConfiguration.projectId(), - // notificationConfiguration.topicId()); - // pubsubService.createTopic( - // notificationConfiguration.projectId(), notificationConfiguration.topicId()); - // } catch (IOException e) { - // logger.warn("Error creating notification topic", e); - // } - // } - public void sendPipelineRunSucceededNotification( PipelineRun pipelineRun, String pipelineDisplayName, String quotaRemaining) { try { @@ -56,7 +40,7 @@ public void sendPipelineRunSucceededNotification( pipelineRun.getUserId(), pipelineDisplayName, pipelineRun.getJobId().toString(), - pipelineRun.getCreated().toString(), + pipelineRun.getCreated().toString(), // TODO format this nicely pipelineRun.getUpdated().toString(), pipelineRun.getQuotaConsumed().toString(), quotaRemaining, From 3b6f61780d53bd514218de96470793a91075ac52 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Wed, 18 Dec 2024 15:11:02 -0500 Subject: [PATCH 15/28] things working, some tests --- ...StairwaySendFailedJobNotificationHook.java | 49 +-- .../dependencies/stairway/JobService.java | 6 +- .../BaseTeaspoonsJobNotification.java | 31 ++ .../notifications/NotificationService.java | 156 ++++++-- .../TeaspoonsJobFailedNotification.java | 44 ++- .../TeaspoonsJobSucceededNotification.java | 37 +- .../imputation/RunImputationGcpJobFlight.java | 7 +- .../SendJobSucceededNotificationStep.java | 28 +- ...rwaySendFailedJobNotificationHookTest.java | 3 +- .../NotificationServiceTest.java | 364 ++++++++++++++++++ .../notifications/PubsubServiceTest.java | 8 + .../TeaspoonsJobFailedNotificationTest.java | 69 ++++ ...TeaspoonsJobSucceededNotificationTest.java | 67 ++++ 13 files changed, 725 insertions(+), 144 deletions(-) create mode 100644 service/src/main/java/bio/terra/pipelines/notifications/BaseTeaspoonsJobNotification.java create mode 100644 service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java create mode 100644 service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java create mode 100644 service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotificationTest.java create mode 100644 service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotificationTest.java diff --git a/service/src/main/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHook.java b/service/src/main/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHook.java index 1cc45a8a..14ccf7c6 100644 --- a/service/src/main/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHook.java +++ b/service/src/main/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHook.java @@ -1,23 +1,14 @@ package bio.terra.pipelines.common.utils; -import static bio.terra.pipelines.app.controller.JobApiUtils.buildApiErrorReport; import static bio.terra.pipelines.common.utils.FlightUtils.flightMapKeyIsTrue; -import bio.terra.pipelines.db.entities.Pipeline; -import bio.terra.pipelines.db.entities.PipelineRun; -import bio.terra.pipelines.db.entities.UserQuota; import bio.terra.pipelines.dependencies.stairway.JobMapKeys; -import bio.terra.pipelines.generated.model.ApiErrorReport; import bio.terra.pipelines.notifications.NotificationService; -import bio.terra.pipelines.service.PipelineRunsService; -import bio.terra.pipelines.service.PipelinesService; -import bio.terra.pipelines.service.QuotasService; import bio.terra.stairway.FlightContext; import bio.terra.stairway.FlightMap; import bio.terra.stairway.FlightStatus; import bio.terra.stairway.HookAction; import bio.terra.stairway.StairwayHook; -import java.util.Optional; import java.util.UUID; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -34,22 +25,12 @@ */ @Component public class StairwaySendFailedJobNotificationHook implements StairwayHook { - private final PipelineRunsService pipelineRunsService; - private final PipelinesService pipelinesService; - private final QuotasService quotasService; private final NotificationService notificationService; private static final Logger logger = LoggerFactory.getLogger(StairwaySendFailedJobNotificationHook.class); - public StairwaySendFailedJobNotificationHook( - PipelineRunsService pipelineRunsService, - NotificationService notificationService, - PipelinesService pipelinesService, - QuotasService quotasService) { - this.pipelineRunsService = pipelineRunsService; + public StairwaySendFailedJobNotificationHook(NotificationService notificationService) { this.notificationService = notificationService; - this.pipelinesService = pipelinesService; - this.quotasService = quotasService; } @Override @@ -67,34 +48,8 @@ public HookAction endFlight(FlightContext context) { UUID jobId = UUID.fromString(context.getFlightId()); String userId = inputParameters.get(JobMapKeys.USER_ID, String.class); - PipelineRun pipelineRun = pipelineRunsService.getPipelineRun(jobId, userId); - Pipeline pipeline = pipelinesService.getPipelineById(pipelineRun.getPipelineId()); - String pipelineDisplayName = pipeline.getDisplayName(); - // if flight fails before quota steps on user's first run, there won't be a row for them yet - // in the quotas table - UserQuota userQuota = - quotasService.getOrCreateQuotaForUserAndPipeline(userId, pipeline.getName()); - String quotaRemaining = String.valueOf(userQuota.getQuota() - userQuota.getQuotaConsumed()); - - // get exception - Optional exception = context.getResult().getException(); - String errorMessage; - if (exception.isPresent()) { - ApiErrorReport errorReport = - buildApiErrorReport(exception.get()); // use same logic that the status endpoint uses - errorMessage = errorReport.getMessage(); - } else { - // should this just fail? - logger.warn( - "No exception found in flight result for flight {} with status {}", - context.getFlightId(), - context.getFlightStatus()); - errorMessage = "Unknown error"; - } - // send email notification - notificationService.sendPipelineRunFailedNotification( - pipelineRun, pipelineDisplayName, quotaRemaining, errorMessage); + notificationService.configureAndSendPipelineRunFailedNotification(jobId, userId, context); } return HookAction.CONTINUE; } diff --git a/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobService.java b/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobService.java index 9a9f5178..36a0d628 100644 --- a/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobService.java +++ b/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobService.java @@ -114,11 +114,7 @@ public void initialize() { .addHook(new MonitoringHook(openTelemetry)) .addHook(new StairwayFailedMetricsCounterHook()) .addHook( - new StairwaySendFailedJobNotificationHook( - flightBeanBag.getPipelineRunsService(), - flightBeanBag.getNotificationService(), - flightBeanBag.getPipelinesService(), - flightBeanBag.getQuotasService())) + new StairwaySendFailedJobNotificationHook(flightBeanBag.getNotificationService())) .addHook(new StairwaySetPipelineRunStatusHook(flightBeanBag.getPipelineRunsService())) .exceptionSerializer(new StairwayExceptionSerializer(objectMapper))); } diff --git a/service/src/main/java/bio/terra/pipelines/notifications/BaseTeaspoonsJobNotification.java b/service/src/main/java/bio/terra/pipelines/notifications/BaseTeaspoonsJobNotification.java new file mode 100644 index 00000000..9eefcf2e --- /dev/null +++ b/service/src/main/java/bio/terra/pipelines/notifications/BaseTeaspoonsJobNotification.java @@ -0,0 +1,31 @@ +package bio.terra.pipelines.notifications; + +import lombok.Getter; + +@Getter +public class BaseTeaspoonsJobNotification { + public final String recipientUserId; + public final String pipelineDisplayName; + public final String jobId; + public final String timeSubmitted; + public final String timeCompleted; + public final String quotaRemaining; + public final String userDescription; + + public BaseTeaspoonsJobNotification( + String recipientUserId, + String pipelineDisplayName, + String jobId, + String timeSubmitted, + String timeCompleted, + String quotaRemaining, + String userDescription) { + this.recipientUserId = recipientUserId; + this.pipelineDisplayName = pipelineDisplayName; + this.jobId = jobId; + this.timeSubmitted = timeSubmitted; + this.timeCompleted = timeCompleted; + this.quotaRemaining = quotaRemaining; + this.userDescription = userDescription; + } +} diff --git a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java index 10d3c687..5c44236c 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java @@ -1,75 +1,173 @@ package bio.terra.pipelines.notifications; +import static bio.terra.pipelines.app.controller.JobApiUtils.buildApiErrorReport; + import bio.terra.pipelines.app.configuration.internal.NotificationConfiguration; +import bio.terra.pipelines.db.entities.Pipeline; import bio.terra.pipelines.db.entities.PipelineRun; +import bio.terra.pipelines.db.entities.UserQuota; +import bio.terra.pipelines.generated.model.ApiErrorReport; +import bio.terra.pipelines.service.PipelineRunsService; +import bio.terra.pipelines.service.PipelinesService; +import bio.terra.pipelines.service.QuotasService; +import bio.terra.stairway.FlightContext; import com.fasterxml.jackson.databind.ObjectMapper; import java.io.IOException; +import java.time.Instant; +import java.time.ZoneId; +import java.time.format.DateTimeFormatter; +import java.util.Optional; +import java.util.UUID; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.stereotype.Service; /** - * Service to encapsulate the logic for sending email notifications to users. Works with the Terra - * Thurloe service via PubSub messages. + * Service to encapsulate the logic for composing and sending email notifications to users about + * completed pipeline runs. Works with the Terra Thurloe service via PubSub messages. */ @Service public class NotificationService { private static final Logger logger = LoggerFactory.getLogger(NotificationService.class); + private final PipelineRunsService pipelineRunsService; + private final PipelinesService pipelinesService; + private final QuotasService quotasService; private final PubsubService pubsubService; private final NotificationConfiguration notificationConfiguration; private final ObjectMapper objectMapper; public NotificationService( + PipelineRunsService pipelineRunsService, + PipelinesService pipelinesService, + QuotasService quotasService, PubsubService pubsubService, NotificationConfiguration notificationConfiguration, ObjectMapper objectMapper) { + this.pipelineRunsService = pipelineRunsService; + this.pipelinesService = pipelinesService; + this.quotasService = quotasService; this.pubsubService = pubsubService; this.notificationConfiguration = notificationConfiguration; this.objectMapper = objectMapper; } - public void sendPipelineRunSucceededNotification( - PipelineRun pipelineRun, String pipelineDisplayName, String quotaRemaining) { + /** + * Pull together the common fields for a notification. + * + * @param jobId the job id + * @param userId the user id + * @return the base notification object + */ + public BaseTeaspoonsJobNotification createBaseTeaspoonsJobNotification( + UUID jobId, String userId) { + PipelineRun pipelineRun = pipelineRunsService.getPipelineRun(jobId, userId); + Pipeline pipeline = pipelinesService.getPipelineById(pipelineRun.getPipelineId()); + String pipelineDisplayName = pipeline.getDisplayName(); + + // if flight fails before quota steps on user's first run, there won't be a row for them yet + // in the quotas table + UserQuota userQuota = + quotasService.getOrCreateQuotaForUserAndPipeline(userId, pipeline.getName()); + String quotaRemaining = String.valueOf(userQuota.getQuota() - userQuota.getQuotaConsumed()); + + return new BaseTeaspoonsJobNotification( + pipelineRun.getUserId(), + pipelineDisplayName, + pipelineRun.getJobId().toString(), + formatInstantToReadableString(pipelineRun.getCreated()), + formatInstantToReadableString(pipelineRun.getUpdated()), + quotaRemaining, + pipelineRun.getDescription()); + } + + /** + * Format an Instant as a date time string in UTC using the RFC-1123 date-time formatter, such as + * 'Tue, 3 Jun 2008 11:05:30 GMT'. + * + * @param dateTime the Instant to format + * @return the formatted date time string + */ + public String formatInstantToReadableString(Instant dateTime) { + return dateTime.atZone(ZoneId.of("UTC")).format(DateTimeFormatter.RFC_1123_DATE_TIME); + } + + /** + * Create a notification object for a failed job. + * + * @param jobId the job id + * @param userId the user id + * @param context the flight context + * @return the notification object + */ + public TeaspoonsJobFailedNotification createTeaspoonsJobFailedNotification( + UUID jobId, String userId, FlightContext context) { + // get exception + Optional exception = context.getResult().getException(); + String errorMessage; + if (exception.isPresent()) { + ApiErrorReport errorReport = + buildApiErrorReport(exception.get()); // use same logic that the status endpoint uses + errorMessage = errorReport.getMessage(); + } else { + logger.error( + "No exception found in flight result for flight {} with status {}", + context.getFlightId(), + context.getFlightStatus()); + errorMessage = "Unknown error"; + } + + return new TeaspoonsJobFailedNotification( + createBaseTeaspoonsJobNotification(jobId, userId), errorMessage); + } + + /** + * Create a notification object for a successful job. + * + * @param jobId the job id + * @param userId the user id + * @return the notification object + */ + public TeaspoonsJobSucceededNotification createTeaspoonsJobSucceededNotification( + UUID jobId, String userId) { + String quotaConsumedByJob = + pipelineRunsService.getPipelineRun(jobId, userId).getQuotaConsumed().toString(); + return new TeaspoonsJobSucceededNotification( + createBaseTeaspoonsJobNotification(jobId, userId), quotaConsumedByJob); + } + + /** + * Configure and send a notification that a job has succeeded. + * + * @param jobId the job id + * @param userId the user id + */ + public void configureAndSendPipelineRunSucceededNotification(UUID jobId, String userId) { try { pubsubService.publishMessage( notificationConfiguration.projectId(), notificationConfiguration.topicId(), - objectMapper.writeValueAsString( - new TeaspoonsJobSucceededNotification( - pipelineRun.getUserId(), - pipelineDisplayName, - pipelineRun.getJobId().toString(), - pipelineRun.getCreated().toString(), // TODO format this nicely - pipelineRun.getUpdated().toString(), - pipelineRun.getQuotaConsumed().toString(), - quotaRemaining, - pipelineRun.getDescription()))); + objectMapper.writeValueAsString(createTeaspoonsJobSucceededNotification(jobId, userId))); } catch (IOException | InterruptedException e) { logger.error("Error sending pipelineRunSucceeded notification", e); } } - public void sendPipelineRunFailedNotification( - PipelineRun pipelineRun, - String pipelineDisplayName, - String quotaRemaining, - String errorMessage) { + /** + * Configure and send a notification that a job has failed. + * + * @param jobId the job id + * @param userId the user id + * @param context the flight context + */ + public void configureAndSendPipelineRunFailedNotification( + UUID jobId, String userId, FlightContext context) { try { pubsubService.publishMessage( notificationConfiguration.projectId(), notificationConfiguration.topicId(), objectMapper.writeValueAsString( - new TeaspoonsJobFailedNotification( - pipelineRun.getUserId(), - pipelineDisplayName, - pipelineRun.getJobId().toString(), - errorMessage, - pipelineRun.getCreated().toString(), - pipelineRun.getUpdated().toString(), - "0", - quotaRemaining, - pipelineRun.getDescription()))); + createTeaspoonsJobFailedNotification(jobId, userId, context))); } catch (IOException | InterruptedException e) { logger.error("Error sending pipelineRunFailed notification", e); } diff --git a/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotification.java b/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotification.java index 3c16bb09..e6867859 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotification.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotification.java @@ -1,16 +1,29 @@ package bio.terra.pipelines.notifications; -public record TeaspoonsJobFailedNotification( - String notificationType, - String recipientUserId, - String pipelineDisplayName, - String jobId, - String errorMessage, - String timeSubmitted, - String timeCompleted, - String quotaConsumedByJob, - String quotaRemaining, - String userDescription) { +import lombok.Getter; + +@Getter +public class TeaspoonsJobFailedNotification extends BaseTeaspoonsJobNotification { + private static final String NOTIFICATION_TYPE = "TeaspoonsJobFailedNotification"; + private static final String QUOTA_CONSUMED_BY_FAILED_JOB = "0"; + public final String notificationType; + public final String errorMessage; + public final String quotaConsumedByJob; + + public TeaspoonsJobFailedNotification( + BaseTeaspoonsJobNotification baseTeaspoonsJobNotification, String errorMessage) { + super( + baseTeaspoonsJobNotification.recipientUserId, + baseTeaspoonsJobNotification.pipelineDisplayName, + baseTeaspoonsJobNotification.jobId, + baseTeaspoonsJobNotification.timeSubmitted, + baseTeaspoonsJobNotification.timeCompleted, + baseTeaspoonsJobNotification.quotaRemaining, + baseTeaspoonsJobNotification.userDescription); + this.notificationType = NOTIFICATION_TYPE; + this.errorMessage = errorMessage; + this.quotaConsumedByJob = QUOTA_CONSUMED_BY_FAILED_JOB; + } public TeaspoonsJobFailedNotification( String recipientUserId, @@ -19,19 +32,18 @@ public TeaspoonsJobFailedNotification( String errorMessage, String timeSubmitted, String timeCompleted, - String quotaConsumedByJob, String quotaRemaining, String userDescription) { - this( - "TeaspoonsJobFailedNotification", + super( recipientUserId, pipelineDisplayName, jobId, - errorMessage, timeSubmitted, timeCompleted, - quotaConsumedByJob, quotaRemaining, userDescription); + this.notificationType = NOTIFICATION_TYPE; + this.errorMessage = errorMessage; + this.quotaConsumedByJob = QUOTA_CONSUMED_BY_FAILED_JOB; } } diff --git a/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotification.java b/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotification.java index 2c8f3baf..85ba9c05 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotification.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotification.java @@ -1,15 +1,26 @@ package bio.terra.pipelines.notifications; -public record TeaspoonsJobSucceededNotification( - String notificationType, - String recipientUserId, - String pipelineDisplayName, - String jobId, - String timeSubmitted, - String timeCompleted, - String quotaConsumedByJob, - String quotaRemaining, - String userDescription) { +import lombok.Getter; + +@Getter +public class TeaspoonsJobSucceededNotification extends BaseTeaspoonsJobNotification { + private static final String NOTIFICATION_TYPE = "TeaspoonsJobSucceededNotification"; + public final String notificationType; + public final String quotaConsumedByJob; + + public TeaspoonsJobSucceededNotification( + BaseTeaspoonsJobNotification baseTeaspoonsJobNotification, String quotaConsumedByJob) { + super( + baseTeaspoonsJobNotification.recipientUserId, + baseTeaspoonsJobNotification.pipelineDisplayName, + baseTeaspoonsJobNotification.jobId, + baseTeaspoonsJobNotification.timeSubmitted, + baseTeaspoonsJobNotification.timeCompleted, + baseTeaspoonsJobNotification.quotaRemaining, + baseTeaspoonsJobNotification.userDescription); + this.notificationType = NOTIFICATION_TYPE; + this.quotaConsumedByJob = quotaConsumedByJob; + } public TeaspoonsJobSucceededNotification( String recipientUserId, @@ -20,15 +31,15 @@ public TeaspoonsJobSucceededNotification( String quotaConsumedByJob, String quotaRemaining, String userDescription) { - this( - "TeaspoonsJobSucceededNotification", + super( recipientUserId, pipelineDisplayName, jobId, timeSubmitted, timeCompleted, - quotaConsumedByJob, quotaRemaining, userDescription); + this.notificationType = NOTIFICATION_TYPE; + this.quotaConsumedByJob = quotaConsumedByJob; } } diff --git a/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpJobFlight.java b/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpJobFlight.java index ff851c55..bf03c925 100644 --- a/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpJobFlight.java +++ b/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationGcpJobFlight.java @@ -126,11 +126,6 @@ public RunImputationGcpJobFlight(FlightMap inputParameters, Object beanBag) { addStep(new CompletePipelineRunStep(flightBeanBag.getPipelineRunsService()), dbRetryRule); addStep( - new SendJobSucceededNotificationStep( - flightBeanBag.getPipelineRunsService(), - flightBeanBag.getNotificationService(), - flightBeanBag.getPipelinesService(), - flightBeanBag.getQuotasService()), - dbRetryRule); + new SendJobSucceededNotificationStep(flightBeanBag.getNotificationService()), dbRetryRule); } } diff --git a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java index 12cf6823..12b34778 100644 --- a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java +++ b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java @@ -1,14 +1,8 @@ package bio.terra.pipelines.stairway.steps.common; import bio.terra.pipelines.common.utils.FlightUtils; -import bio.terra.pipelines.db.entities.Pipeline; -import bio.terra.pipelines.db.entities.PipelineRun; -import bio.terra.pipelines.db.entities.UserQuota; import bio.terra.pipelines.dependencies.stairway.JobMapKeys; import bio.terra.pipelines.notifications.NotificationService; -import bio.terra.pipelines.service.PipelineRunsService; -import bio.terra.pipelines.service.PipelinesService; -import bio.terra.pipelines.service.QuotasService; import bio.terra.stairway.FlightContext; import bio.terra.stairway.Step; import bio.terra.stairway.StepResult; @@ -21,21 +15,11 @@ * exceptions). */ public class SendJobSucceededNotificationStep implements Step { - private final PipelineRunsService pipelineRunsService; - private final PipelinesService pipelinesService; - private final QuotasService quotasService; private final NotificationService notificationService; private final Logger logger = LoggerFactory.getLogger(CompletePipelineRunStep.class); - public SendJobSucceededNotificationStep( - PipelineRunsService pipelineRunsService, - NotificationService notificationService, - PipelinesService pipelinesService, - QuotasService quotasService) { - this.pipelineRunsService = pipelineRunsService; + public SendJobSucceededNotificationStep(NotificationService notificationService) { this.notificationService = notificationService; - this.pipelinesService = pipelinesService; - this.quotasService = quotasService; } @Override @@ -49,16 +33,8 @@ public StepResult doStep(FlightContext flightContext) { UUID jobId = UUID.fromString(flightContext.getFlightId()); String userId = inputParameters.get(JobMapKeys.USER_ID, String.class); - PipelineRun pipelineRun = pipelineRunsService.getPipelineRun(jobId, userId); - Pipeline pipeline = pipelinesService.getPipelineById(pipelineRun.getPipelineId()); - String pipelineDisplayName = pipeline.getDisplayName(); - UserQuota userQuota = - quotasService.getQuotaForUserAndPipeline(userId, pipeline.getName()).get(); - String quotaRemaining = String.valueOf(userQuota.getQuota() - userQuota.getQuotaConsumed()); - // send email notification - notificationService.sendPipelineRunSucceededNotification( - pipelineRun, pipelineDisplayName, quotaRemaining); + notificationService.configureAndSendPipelineRunSucceededNotification(jobId, userId); } catch (Exception e) { logger.error("Failed to send email notification", e); } diff --git a/service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java b/service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java index 610ce070..4e38a2da 100644 --- a/service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java +++ b/service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java @@ -37,8 +37,7 @@ void setup() { // when(notificationService). stairwaySendFailedJobNotificationHook = - new StairwaySendFailedJobNotificationHook( - pipelineRunsService, notificationService, pipelinesService, quotasService); + new StairwaySendFailedJobNotificationHook(notificationService); } private static Stream flightContexts() { diff --git a/service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java b/service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java new file mode 100644 index 00000000..0a9bb3ad --- /dev/null +++ b/service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java @@ -0,0 +1,364 @@ +package bio.terra.pipelines.notifications; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import bio.terra.pipelines.app.configuration.internal.NotificationConfiguration; +import bio.terra.pipelines.common.utils.CommonPipelineRunStatusEnum; +import bio.terra.pipelines.db.entities.Pipeline; +import bio.terra.pipelines.db.entities.PipelineRun; +import bio.terra.pipelines.db.entities.UserQuota; +import bio.terra.pipelines.db.repositories.PipelineRunsRepository; +import bio.terra.pipelines.dependencies.rawls.RawlsServiceApiException; +import bio.terra.pipelines.service.PipelineRunsService; +import bio.terra.pipelines.service.PipelinesService; +import bio.terra.pipelines.service.QuotasService; +import bio.terra.pipelines.testutils.BaseEmbeddedDbTest; +import bio.terra.pipelines.testutils.TestUtils; +import bio.terra.stairway.FlightContext; +import bio.terra.stairway.StepResult; +import bio.terra.stairway.StepStatus; +import com.fasterxml.jackson.databind.ObjectMapper; +import java.io.IOException; +import java.time.Instant; +import java.util.UUID; +import org.junit.jupiter.api.Test; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.mock.mockito.MockBean; + +class NotificationServiceTest extends BaseEmbeddedDbTest { + @InjectMocks @Autowired NotificationService notificationService; + @Autowired PipelineRunsService pipelineRunsService; + @Autowired PipelineRunsRepository pipelineRunsRepository; + @Autowired PipelinesService pipelinesService; + @Autowired QuotasService quotasService; + @Autowired NotificationConfiguration notificationConfiguration; + @Autowired ObjectMapper objectMapper; + @MockBean PubsubService pubsubService; + @Mock private FlightContext flightContext; + + UUID testJobId = TestUtils.TEST_NEW_UUID; + String testUserId = TestUtils.TEST_USER_ID_1; + Integer testQuotaConsumedByJob = 1000; + String testUserDescription = TestUtils.TEST_USER_PROVIDED_DESCRIPTION; + String testErrorMessage = "test error message"; + + @Test + void createBaseTeaspoonsJobNotification() { + Pipeline pipeline = pipelinesService.getPipelineById(1L); + PipelineRun writtenPipelineRun = + createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.SUCCEEDED); + + // initialize and set user quota + UserQuota userQuota = + quotasService.getOrCreateQuotaForUserAndPipeline(testUserId, pipeline.getName()); + UserQuota updatedUserQuota = + quotasService.updateQuotaConsumed(userQuota, testQuotaConsumedByJob); + int expectedQuotaRemaining = updatedUserQuota.getQuota() - updatedUserQuota.getQuotaConsumed(); + + BaseTeaspoonsJobNotification baseTeaspoonsJobNotification = + notificationService.createBaseTeaspoonsJobNotification(testJobId, testUserId); + + assertEquals(testUserId, baseTeaspoonsJobNotification.getRecipientUserId()); + assertEquals(pipeline.getDisplayName(), baseTeaspoonsJobNotification.getPipelineDisplayName()); + assertEquals(testJobId.toString(), baseTeaspoonsJobNotification.getJobId()); + assertEquals( + notificationService.formatInstantToReadableString(writtenPipelineRun.getCreated()), + baseTeaspoonsJobNotification.getTimeSubmitted()); + assertEquals( + notificationService.formatInstantToReadableString(writtenPipelineRun.getUpdated()), + baseTeaspoonsJobNotification.getTimeCompleted()); + assertEquals( + String.valueOf(expectedQuotaRemaining), baseTeaspoonsJobNotification.getQuotaRemaining()); + assertEquals(testUserDescription, baseTeaspoonsJobNotification.getUserDescription()); + } + + @Test + void createBaseTeaspoonsJobNotificationNoQuota() { + Pipeline pipeline = pipelinesService.getPipelineById(1L); + createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.SUCCEEDED); + + // don't initialize user in user_quota table + int expectedQuotaRemaining = + quotasService.getPipelineQuota(pipeline.getName()).getDefaultQuota(); + + BaseTeaspoonsJobNotification baseTeaspoonsJobNotification = + notificationService.createBaseTeaspoonsJobNotification(testJobId, testUserId); + + assertEquals(testUserId, baseTeaspoonsJobNotification.getRecipientUserId()); + assertEquals( + String.valueOf(expectedQuotaRemaining), baseTeaspoonsJobNotification.getQuotaRemaining()); + } + + @Test + void formatDateTime() { + Instant instant = Instant.parse("2021-08-25T12:34:56.789Z"); + String formattedDateTime = notificationService.formatInstantToReadableString(instant); + assertEquals("Wed, 25 Aug 2021 12:34:56 GMT", formattedDateTime); + } + + @Test + void createTeaspoonsJobFailedNotification() { + Pipeline pipeline = pipelinesService.getPipelineById(1L); + PipelineRun writtenPipelineRun = + createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.FAILED); + + // don't initialize user in user_quota table + int expectedQuotaRemaining = + quotasService.getPipelineQuota(pipeline.getName()).getDefaultQuota(); + + when(flightContext.getFlightId()).thenReturn(testJobId.toString()); + RawlsServiceApiException rawlsServiceApiException = + new RawlsServiceApiException(testErrorMessage); + StepResult stepResultFailedWithException = + new StepResult(StepStatus.STEP_RESULT_FAILURE_FATAL, rawlsServiceApiException); + when(flightContext.getResult()).thenReturn(stepResultFailedWithException); + + TeaspoonsJobFailedNotification teaspoonsJobFailedNotification = + notificationService.createTeaspoonsJobFailedNotification( + testJobId, testUserId, flightContext); + + assertEquals(testUserId, teaspoonsJobFailedNotification.getRecipientUserId()); + assertEquals( + pipeline.getDisplayName(), teaspoonsJobFailedNotification.getPipelineDisplayName()); + assertEquals(testJobId.toString(), teaspoonsJobFailedNotification.getJobId()); + assertEquals( + notificationService.formatInstantToReadableString(writtenPipelineRun.getCreated()), + teaspoonsJobFailedNotification.getTimeSubmitted()); + assertEquals( + notificationService.formatInstantToReadableString(writtenPipelineRun.getUpdated()), + teaspoonsJobFailedNotification.getTimeCompleted()); + assertEquals( + String.valueOf(expectedQuotaRemaining), teaspoonsJobFailedNotification.getQuotaRemaining()); + assertEquals(testUserDescription, teaspoonsJobFailedNotification.getUserDescription()); + + // fields specific to failed job notification + assertEquals( + "TeaspoonsJobFailedNotification", teaspoonsJobFailedNotification.getNotificationType()); + assertEquals(testErrorMessage, teaspoonsJobFailedNotification.getErrorMessage()); + assertEquals("0", teaspoonsJobFailedNotification.getQuotaConsumedByJob()); + } + + @Test + void createTeaspoonsJobFailedNotificationNoMessage() { + Pipeline pipeline = pipelinesService.getPipelineById(1L); + createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.FAILED); + + when(flightContext.getFlightId()).thenReturn(testJobId.toString()); + StepResult stepResultFailedNoException = new StepResult(StepStatus.STEP_RESULT_FAILURE_FATAL); + when(flightContext.getResult()).thenReturn(stepResultFailedNoException); + + TeaspoonsJobFailedNotification teaspoonsJobFailedNotification = + notificationService.createTeaspoonsJobFailedNotification( + testJobId, testUserId, flightContext); + + assertEquals(testUserId, teaspoonsJobFailedNotification.getRecipientUserId()); + + // test for message when no exception is present + assertEquals( + "TeaspoonsJobFailedNotification", teaspoonsJobFailedNotification.getNotificationType()); + assertEquals("Unknown error", teaspoonsJobFailedNotification.getErrorMessage()); + } + + @Test + void createTeaspoonsJobSucceededNotification() { + Pipeline pipeline = pipelinesService.getPipelineById(1L); + PipelineRun writtenPipelineRun = + createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.SUCCEEDED); + + // initialize and set user quota + UserQuota userQuota = + quotasService.getOrCreateQuotaForUserAndPipeline(testUserId, pipeline.getName()); + UserQuota updatedUserQuota = + quotasService.updateQuotaConsumed(userQuota, testQuotaConsumedByJob); + int expectedQuotaRemaining = updatedUserQuota.getQuota() - updatedUserQuota.getQuotaConsumed(); + + TeaspoonsJobSucceededNotification teaspoonsJobSucceededNotification = + notificationService.createTeaspoonsJobSucceededNotification(testJobId, testUserId); + + assertEquals(testUserId, teaspoonsJobSucceededNotification.getRecipientUserId()); + assertEquals( + pipeline.getDisplayName(), teaspoonsJobSucceededNotification.getPipelineDisplayName()); + assertEquals(testJobId.toString(), teaspoonsJobSucceededNotification.getJobId()); + assertEquals( + notificationService.formatInstantToReadableString(writtenPipelineRun.getCreated()), + teaspoonsJobSucceededNotification.getTimeSubmitted()); + assertEquals( + notificationService.formatInstantToReadableString(writtenPipelineRun.getUpdated()), + teaspoonsJobSucceededNotification.getTimeCompleted()); + assertEquals( + String.valueOf(expectedQuotaRemaining), + teaspoonsJobSucceededNotification.getQuotaRemaining()); + assertEquals(testUserDescription, teaspoonsJobSucceededNotification.getUserDescription()); + + // fields specific to succeeded job notification + assertEquals( + "TeaspoonsJobSucceededNotification", + teaspoonsJobSucceededNotification.getNotificationType()); + assertEquals( + testQuotaConsumedByJob.toString(), + teaspoonsJobSucceededNotification.getQuotaConsumedByJob()); + } + + @Test + void configureAndSendPipelineRunSucceededNotification() throws IOException, InterruptedException { + Pipeline pipeline = pipelinesService.getPipelineById(1L); + createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.SUCCEEDED); + + // success is a void method + doNothing().when(pubsubService).publishMessage(any(), any(), any()); + + notificationService.configureAndSendPipelineRunSucceededNotification(testJobId, testUserId); + + String stringifiedJobSucceededNotification = + objectMapper.writeValueAsString( + notificationService.createTeaspoonsJobSucceededNotification(testJobId, testUserId)); + // verify that the pubsub method was called + verify(pubsubService, times(1)) + .publishMessage( + notificationConfiguration.projectId(), + notificationConfiguration.topicId(), + stringifiedJobSucceededNotification); + } + + @Test + void configureAndSendPipelineRunSucceededNotificationIOException() + throws IOException, InterruptedException { + Pipeline pipeline = pipelinesService.getPipelineById(1L); + createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.SUCCEEDED); + + doThrow(new IOException()).when(pubsubService).publishMessage(any(), any(), any()); + + // exception should be caught + assertDoesNotThrow( + () -> + notificationService.configureAndSendPipelineRunSucceededNotification( + testJobId, testUserId)); + } + + @Test + void configureAndSendPipelineRunSucceededNotificationInterruptedException() + throws IOException, InterruptedException { + Pipeline pipeline = pipelinesService.getPipelineById(1L); + createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.SUCCEEDED); + + doThrow(new InterruptedException()).when(pubsubService).publishMessage(any(), any(), any()); + + // exception should be caught + assertDoesNotThrow( + () -> + notificationService.configureAndSendPipelineRunSucceededNotification( + testJobId, testUserId)); + } + + @Test + void configureAndSendPipelineRunFailedNotification() throws IOException, InterruptedException { + Pipeline pipeline = pipelinesService.getPipelineById(1L); + createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.FAILED); + + when(flightContext.getFlightId()).thenReturn(testJobId.toString()); + RawlsServiceApiException rawlsServiceApiException = + new RawlsServiceApiException(testErrorMessage); + StepResult stepResultFailedWithException = + new StepResult(StepStatus.STEP_RESULT_FAILURE_FATAL, rawlsServiceApiException); + when(flightContext.getResult()).thenReturn(stepResultFailedWithException); + + // success is a void method + doNothing().when(pubsubService).publishMessage(any(), any(), any()); + + notificationService.configureAndSendPipelineRunFailedNotification( + testJobId, testUserId, flightContext); + + String stringifiedJobFailedNotification = + objectMapper.writeValueAsString( + notificationService.createTeaspoonsJobFailedNotification( + testJobId, testUserId, flightContext)); + // verify that the pubsub method was called + verify(pubsubService, times(1)) + .publishMessage( + notificationConfiguration.projectId(), + notificationConfiguration.topicId(), + stringifiedJobFailedNotification); + } + + @Test + void configureAndSendPipelineRunFailedNotificationIOException() + throws IOException, InterruptedException { + Pipeline pipeline = pipelinesService.getPipelineById(1L); + createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.FAILED); + + when(flightContext.getFlightId()).thenReturn(testJobId.toString()); + RawlsServiceApiException rawlsServiceApiException = + new RawlsServiceApiException(testErrorMessage); + StepResult stepResultFailedWithException = + new StepResult(StepStatus.STEP_RESULT_FAILURE_FATAL, rawlsServiceApiException); + when(flightContext.getResult()).thenReturn(stepResultFailedWithException); + + doThrow(new IOException()).when(pubsubService).publishMessage(any(), any(), any()); + + // exception should be caught + assertDoesNotThrow( + () -> + notificationService.configureAndSendPipelineRunFailedNotification( + testJobId, testUserId, flightContext)); + } + + @Test + void configureAndSendPipelineRunFailedNotificationInterruptedException() + throws IOException, InterruptedException { + Pipeline pipeline = pipelinesService.getPipelineById(1L); + createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.FAILED); + + when(flightContext.getFlightId()).thenReturn(testJobId.toString()); + RawlsServiceApiException rawlsServiceApiException = + new RawlsServiceApiException(testErrorMessage); + StepResult stepResultFailedWithException = + new StepResult(StepStatus.STEP_RESULT_FAILURE_FATAL, rawlsServiceApiException); + when(flightContext.getResult()).thenReturn(stepResultFailedWithException); + + doThrow(new InterruptedException()).when(pubsubService).publishMessage(any(), any(), any()); + + // exception should be caught + assertDoesNotThrow( + () -> + notificationService.configureAndSendPipelineRunFailedNotification( + testJobId, testUserId, flightContext)); + } + + /** + * Helper method for tests to create a completed pipeline run in the database. + * + * @param pipeline the pipeline to create the run for + * @param statusEnum the status of the pipeline run + * @return the completed pipeline run + */ + private PipelineRun createCompletedPipelineRunInDb( + Pipeline pipeline, CommonPipelineRunStatusEnum statusEnum) { + PipelineRun completedPipelineRun = + new PipelineRun( + testJobId, + testUserId, + pipeline.getId(), + pipeline.getWdlMethodVersion(), + pipeline.getWorkspaceId(), + pipeline.getWorkspaceBillingProject(), + pipeline.getWorkspaceName(), + pipeline.getWorkspaceStorageContainerName(), + pipeline.getWorkspaceGoogleProject(), + null, // timestamps auto generated by db + null, + statusEnum, + testUserDescription, + testQuotaConsumedByJob); + + return pipelineRunsRepository.save(completedPipelineRun); + } +} diff --git a/service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java b/service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java new file mode 100644 index 00000000..b9335f44 --- /dev/null +++ b/service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java @@ -0,0 +1,8 @@ +package bio.terra.pipelines.notifications; + +import bio.terra.pipelines.testutils.BaseEmbeddedDbTest; +import org.springframework.beans.factory.annotation.Autowired; + +class PubsubServiceTest extends BaseEmbeddedDbTest { + @Autowired PubsubService pubsubService; +} diff --git a/service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotificationTest.java b/service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotificationTest.java new file mode 100644 index 00000000..b172d2aa --- /dev/null +++ b/service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotificationTest.java @@ -0,0 +1,69 @@ +package bio.terra.pipelines.notifications; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import bio.terra.pipelines.testutils.BaseTest; +import org.junit.jupiter.api.Test; + +class TeaspoonsJobFailedNotificationTest extends BaseTest { + String recipientUserId = "test-recipient-user-id"; + String pipelineDisplayName = "test-pipeline-display-name"; + String jobId = "test-job-id"; + String errorMessage = "test-error-message"; + String timeSubmitted = "test-time-submitted"; + String timeCompleted = "test-time-completed"; + String quotaRemaining = "test-quota-remaining"; + String userDescription = "test-user-description"; + + @Test + void teaspoonsJobFailedNotificationShortConstructor() { + BaseTeaspoonsJobNotification baseTeaspoonsJobNotification = + new BaseTeaspoonsJobNotification( + recipientUserId, + pipelineDisplayName, + jobId, + timeSubmitted, + timeCompleted, + quotaRemaining, + userDescription); + + TeaspoonsJobFailedNotification notification = + new TeaspoonsJobFailedNotification(baseTeaspoonsJobNotification, errorMessage); + + assertEquals("TeaspoonsJobFailedNotification", notification.getNotificationType()); + assertEquals(recipientUserId, notification.getRecipientUserId()); + assertEquals(pipelineDisplayName, notification.getPipelineDisplayName()); + assertEquals(jobId, notification.getJobId()); + assertEquals(errorMessage, notification.getErrorMessage()); + assertEquals(timeSubmitted, notification.getTimeSubmitted()); + assertEquals(timeCompleted, notification.getTimeCompleted()); + assertEquals("0", notification.getQuotaConsumedByJob()); + assertEquals(quotaRemaining, notification.getQuotaRemaining()); + assertEquals(userDescription, notification.getUserDescription()); + } + + @Test + void teaspoonsJobFailedNotificationFullConstructor() { + + TeaspoonsJobFailedNotification notification = + new TeaspoonsJobFailedNotification( + recipientUserId, + pipelineDisplayName, + jobId, + errorMessage, + timeSubmitted, + timeCompleted, + quotaRemaining, + userDescription); + assertEquals("TeaspoonsJobFailedNotification", notification.getNotificationType()); + assertEquals(recipientUserId, notification.getRecipientUserId()); + assertEquals(pipelineDisplayName, notification.getPipelineDisplayName()); + assertEquals(jobId, notification.getJobId()); + assertEquals(errorMessage, notification.getErrorMessage()); + assertEquals(timeSubmitted, notification.getTimeSubmitted()); + assertEquals(timeCompleted, notification.getTimeCompleted()); + assertEquals("0", notification.getQuotaConsumedByJob()); + assertEquals(quotaRemaining, notification.getQuotaRemaining()); + assertEquals(userDescription, notification.getUserDescription()); + } +} diff --git a/service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotificationTest.java b/service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotificationTest.java new file mode 100644 index 00000000..309e04a1 --- /dev/null +++ b/service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotificationTest.java @@ -0,0 +1,67 @@ +package bio.terra.pipelines.notifications; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import bio.terra.pipelines.testutils.BaseTest; +import org.junit.jupiter.api.Test; + +class TeaspoonsJobSucceededNotificationTest extends BaseTest { + + String recipientUserId = "test-recipient-user-id"; + String pipelineDisplayName = "test-pipeline-display-name"; + String jobId = "test-job-id"; + String timeSubmitted = "test-time-submitted"; + String timeCompleted = "test-time-completed"; + String quotaConsumedByJob = "test-quota-consumed-by-job"; + String quotaRemaining = "test-quota-remaining"; + String userDescription = "test-user-description"; + + @Test + void teaspoonsJobSucceededNotificationShortConstructor() { + BaseTeaspoonsJobNotification baseTeaspoonsJobNotification = + new BaseTeaspoonsJobNotification( + recipientUserId, + pipelineDisplayName, + jobId, + timeSubmitted, + timeCompleted, + quotaRemaining, + userDescription); + + TeaspoonsJobSucceededNotification notification = + new TeaspoonsJobSucceededNotification(baseTeaspoonsJobNotification, quotaConsumedByJob); + + assertEquals("TeaspoonsJobSucceededNotification", notification.getNotificationType()); + assertEquals(recipientUserId, notification.getRecipientUserId()); + assertEquals(pipelineDisplayName, notification.getPipelineDisplayName()); + assertEquals(jobId, notification.getJobId()); + assertEquals(timeSubmitted, notification.getTimeSubmitted()); + assertEquals(timeCompleted, notification.getTimeCompleted()); + assertEquals(quotaConsumedByJob, notification.getQuotaConsumedByJob()); + assertEquals(quotaRemaining, notification.getQuotaRemaining()); + assertEquals(userDescription, notification.getUserDescription()); + } + + @Test + void teaspoonsJobSucceededNotificationFullConstructor() { + TeaspoonsJobSucceededNotification notification = + new TeaspoonsJobSucceededNotification( + recipientUserId, + pipelineDisplayName, + jobId, + timeSubmitted, + timeCompleted, + quotaConsumedByJob, + quotaRemaining, + userDescription); + assertEquals("TeaspoonsJobSucceededNotification", notification.getNotificationType()); + assertEquals(recipientUserId, notification.getRecipientUserId()); + assertEquals(pipelineDisplayName, notification.getPipelineDisplayName()); + assertEquals(jobId, notification.getJobId()); + assertEquals(timeSubmitted, notification.getTimeSubmitted()); + assertEquals(timeCompleted, notification.getTimeCompleted()); + assertEquals(quotaConsumedByJob, notification.getQuotaConsumedByJob()); + assertEquals(quotaRemaining, notification.getQuotaRemaining()); + assertEquals(userDescription, notification.getUserDescription()); + } +} From 392cb7bfe1dc12252eb6b6813fce020c22598c2f Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Thu, 19 Dec 2024 13:22:57 -0500 Subject: [PATCH 16/28] resolve conflicts --- .../notifications/PubsubService.java | 21 +---- ...rwaySendFailedJobNotificationHookTest.java | 45 +++++----- .../notifications/PubsubServiceTest.java | 13 +++ .../SendJobSucceededNotificationStepTest.java | 82 +++++++++++++++++++ 4 files changed, 118 insertions(+), 43 deletions(-) create mode 100644 service/src/test/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStepTest.java diff --git a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java index b5cfc8bb..ce40d218 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java @@ -5,11 +5,9 @@ import com.google.api.core.ApiFutures; import com.google.api.gax.rpc.ApiException; import com.google.cloud.pubsub.v1.Publisher; -import com.google.cloud.pubsub.v1.TopicAdminClient; import com.google.common.util.concurrent.MoreExecutors; import com.google.protobuf.ByteString; import com.google.pubsub.v1.PubsubMessage; -import com.google.pubsub.v1.Topic; import com.google.pubsub.v1.TopicName; import java.io.IOException; import java.util.concurrent.TimeUnit; @@ -22,26 +20,9 @@ public class PubsubService { private static final Logger logger = LoggerFactory.getLogger(PubsubService.class); - public void createTopic(String projectId, String topicId) throws IOException { - try (TopicAdminClient topicAdminClient = TopicAdminClient.create()) { - TopicName topicName = TopicName.of(projectId, topicId); - if (topicAdminClient.getTopic(topicName) == null) { - Topic topic = topicAdminClient.createTopic(topicName); - logger.info("Created topic: {}", topic.getName()); - } - } catch (IOException e) { - logger.error("Error creating topic", e); - } - } - + /** Publish a message to a Google PubSub topic. */ public void publishMessage(String projectId, String topicId, String message) throws IOException, InterruptedException { - // TopicName topicName = TopicName.of(projectId, topicId); - // var publisher = Publisher.newBuilder(topicName).build(); - // - // publisher.publish(PubsubMessage.newBuilder().setData(ByteString.copyFromUtf8(message)).build()); - // publisher.awaitTermination() - TopicName topicName = TopicName.of(projectId, topicId); Publisher publisher = null; logger.info("Publishing message to Google PubSub projectId {}, topicId {}", projectId, topicId); diff --git a/service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java b/service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java index 4e38a2da..4302acb0 100644 --- a/service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java +++ b/service/src/test/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHookTest.java @@ -1,18 +1,17 @@ package bio.terra.pipelines.common.utils; -import static bio.terra.pipelines.testutils.TestUtils.createNewPipelineRunWithJobId; import static org.junit.jupiter.params.provider.Arguments.arguments; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; -import bio.terra.pipelines.db.entities.PipelineRun; import bio.terra.pipelines.dependencies.stairway.JobMapKeys; import bio.terra.pipelines.notifications.NotificationService; -import bio.terra.pipelines.service.PipelineRunsService; -import bio.terra.pipelines.service.PipelinesService; -import bio.terra.pipelines.service.QuotasService; import bio.terra.pipelines.testutils.BaseEmbeddedDbTest; import bio.terra.pipelines.testutils.StairwayTestUtils; import bio.terra.pipelines.testutils.TestFlightContext; import bio.terra.pipelines.testutils.TestUtils; +import bio.terra.stairway.FlightContext; import bio.terra.stairway.FlightStatus; import java.util.UUID; import java.util.stream.Stream; @@ -20,22 +19,27 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.mockito.InjectMocks; import org.mockito.Mock; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.mock.mockito.MockBean; class StairwaySendFailedJobNotificationHookTest extends BaseEmbeddedDbTest { + @Autowired @InjectMocks StairwaySendFailedJobNotificationHook stairwaySendFailedJobNotificationHook; - @Autowired PipelineRunsService pipelineRunsService; - @Mock NotificationService notificationService; - @Autowired PipelinesService pipelinesService; - @Autowired QuotasService quotasService; + + @MockBean NotificationService notificationService; + + @Mock private FlightContext flightContext; private final UUID testJobId = TestUtils.TEST_NEW_UUID; @BeforeEach void setup() { - // when(notificationService). - + doNothing() + .when(notificationService) + .configureAndSendPipelineRunFailedNotification( + testJobId, TestUtils.TEST_USER_ID_1, flightContext); stairwaySendFailedJobNotificationHook = new StairwaySendFailedJobNotificationHook(notificationService); } @@ -98,9 +102,6 @@ void endFlight( .put(JobMapKeys.DO_SEND_JOB_FAILURE_NOTIFICATION_HOOK, hookKeyValue); } - // write a new pipelineRun to the db - this includes status set to PREPARING - PipelineRun pipelineRun = createNewPipelineRunWithJobId(UUID.fromString(context.getFlightId())); - stairwaySendFailedJobNotificationHook.startFlight(context); // set the end flight status @@ -108,14 +109,12 @@ void endFlight( stairwaySendFailedJobNotificationHook.endFlight(context); - // the flight did not fail, so the pipelineRun status should not have been updated to FAILED - // PipelineRun writtenPipelineRun = - // pipelineRunsRepository.findByJobIdAndUserId(testJobId, - // TestUtils.TEST_USER_ID_1).get(); - // if (shouldSendFailedJobNotification) { - // assertEquals(CommonPipelineRunStatusEnum.FAILED, writtenPipelineRun.getStatus()); - // } else { - // assertNotEquals(CommonPipelineRunStatusEnum.FAILED, writtenPipelineRun.getStatus()); - // } + if (shouldSendFailedJobNotification) { + verify(notificationService) + .configureAndSendPipelineRunFailedNotification( + testJobId, TestUtils.TEST_USER_ID_1, context); + } else { + verifyNoInteractions(notificationService); + } } } diff --git a/service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java b/service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java index b9335f44..1a2d69cc 100644 --- a/service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java +++ b/service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java @@ -1,8 +1,21 @@ package bio.terra.pipelines.notifications; import bio.terra.pipelines.testutils.BaseEmbeddedDbTest; +import java.io.IOException; +import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; class PubsubServiceTest extends BaseEmbeddedDbTest { @Autowired PubsubService pubsubService; + + @Test + void publishMessage() throws IOException, InterruptedException { + // setup + String message = "test message"; + String topicId = "testTopicId"; + String projectId = "testProjectId"; + + // lol this passes the test with no mocking since we catch the exception + pubsubService.publishMessage(projectId, topicId, message); + } } diff --git a/service/src/test/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStepTest.java b/service/src/test/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStepTest.java new file mode 100644 index 00000000..f34bdfb4 --- /dev/null +++ b/service/src/test/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStepTest.java @@ -0,0 +1,82 @@ +package bio.terra.pipelines.stairway.steps.common; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import bio.terra.pipelines.dependencies.stairway.JobMapKeys; +import bio.terra.pipelines.notifications.NotificationService; +import bio.terra.pipelines.testutils.BaseEmbeddedDbTest; +import bio.terra.pipelines.testutils.TestUtils; +import bio.terra.stairway.FlightContext; +import bio.terra.stairway.FlightMap; +import bio.terra.stairway.StepStatus; +import java.util.UUID; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.Mock; + +class SendJobSucceededNotificationStepTest extends BaseEmbeddedDbTest { + @Mock private NotificationService notificationService; + @Mock private FlightContext flightContext; + + private final UUID testJobId = TestUtils.TEST_NEW_UUID; + private final String testUserId = TestUtils.TEST_USER_ID_1; + + @BeforeEach + void setup() { + var inputParameters = new FlightMap(); + var workingMap = new FlightMap(); + + inputParameters.put(JobMapKeys.USER_ID, testUserId); + + when(flightContext.getFlightId()).thenReturn(testJobId.toString()); + when(flightContext.getInputParameters()).thenReturn(inputParameters); + when(flightContext.getWorkingMap()).thenReturn(workingMap); + } + + @Test + void doStepSuccess() { + // assume success + doNothing() + .when(notificationService) + .configureAndSendPipelineRunSucceededNotification(testJobId, testUserId); + + var sendJobSucceededNotificationStep = + new SendJobSucceededNotificationStep(notificationService); + var result = sendJobSucceededNotificationStep.doStep(flightContext); + + assertEquals(StepStatus.STEP_RESULT_SUCCESS, result.getStepStatus()); + verify(notificationService, times(1)) + .configureAndSendPipelineRunSucceededNotification(testJobId, testUserId); + } + + @Test + void doStepFailureStillSuccess() { + // throw exception + doThrow(new RuntimeException()) + .when(notificationService) + .configureAndSendPipelineRunSucceededNotification(testJobId, testUserId); + + var sendJobSucceededNotificationStep = + new SendJobSucceededNotificationStep(notificationService); + var result = sendJobSucceededNotificationStep.doStep(flightContext); + + // step should catch exception and just log the error rather than throwing + assertEquals(StepStatus.STEP_RESULT_SUCCESS, result.getStepStatus()); + verify(notificationService, times(1)) + .configureAndSendPipelineRunSucceededNotification(testJobId, testUserId); + } + + @Test + void undoStepSuccess() { + var sendJobSucceededNotificationStep = + new SendJobSucceededNotificationStep(notificationService); + var result = sendJobSucceededNotificationStep.undoStep(flightContext); + + assertEquals(StepStatus.STEP_RESULT_SUCCESS, result.getStepStatus()); + } +} From f60ac84b72d1357455f5261bee9c8a9d5f0a87eb Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Thu, 19 Dec 2024 13:27:47 -0500 Subject: [PATCH 17/28] resolve conflicts --- .../flights/imputation/RunImputationAzureJobFlight.java | 6 ++++++ .../stairway/steps/common/CompletePipelineRunStep.java | 4 ++-- .../steps/common/SendJobSucceededNotificationStep.java | 2 +- .../stairway/steps/common/CompletePipelineRunStepTest.java | 6 +++--- 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationAzureJobFlight.java b/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationAzureJobFlight.java index 5727163b..e3c0ac5f 100644 --- a/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationAzureJobFlight.java +++ b/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationAzureJobFlight.java @@ -5,9 +5,15 @@ import bio.terra.pipelines.common.utils.FlightUtils; import bio.terra.pipelines.common.utils.PipelinesEnum; import bio.terra.pipelines.dependencies.stairway.JobMapKeys; +<<<<<<< HEAD:service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationAzureJobFlight.java import bio.terra.pipelines.stairway.steps.common.CompletePipelineRunStep; import bio.terra.pipelines.stairway.steps.imputation.PrepareImputationInputsStep; import bio.terra.pipelines.stairway.steps.imputation.azure.*; +======= +import bio.terra.pipelines.stairway.CompletePipelineRunStep; +import bio.terra.pipelines.stairway.imputation.steps.PrepareImputationInputsStep; +import bio.terra.pipelines.stairway.imputation.steps.azure.*; +>>>>>>> 13fe0fb (move files around):service/src/main/java/bio/terra/pipelines/stairway/imputation/RunImputationAzureJobFlight.java import bio.terra.stairway.*; public class RunImputationAzureJobFlight extends Flight { diff --git a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java index 87333051..405a502d 100644 --- a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java +++ b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java @@ -35,10 +35,10 @@ public StepResult doStep(FlightContext flightContext) { // validate and extract parameters from working map var workingMap = flightContext.getWorkingMap(); FlightUtils.validateRequiredEntries( - workingMap, ImputationJobMapKeys.PIPELINE_RUN_OUTPUTS, ImputationJobMapKeys.QUOTA_CONSUMED); + workingMap, ImputationJobMapKeys.PIPELINE_RUN_OUTPUTS, ImputationJobMapKeys.EFFECTIVE_QUOTA_CONSUMED); Map outputsMap = workingMap.get(ImputationJobMapKeys.PIPELINE_RUN_OUTPUTS, Map.class); - int quotaConsumed = workingMap.get(ImputationJobMapKeys.QUOTA_CONSUMED, Integer.class); + int quotaConsumed = workingMap.get(ImputationJobMapKeys.EFFECTIVE_QUOTA_CONSUMED, Integer.class); pipelineRunsService.markPipelineRunSuccessAndWriteOutputs( jobId, userId, outputsMap, quotaConsumed); diff --git a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java index 12b34778..8c26eee1 100644 --- a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java +++ b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java @@ -16,7 +16,7 @@ */ public class SendJobSucceededNotificationStep implements Step { private final NotificationService notificationService; - private final Logger logger = LoggerFactory.getLogger(CompletePipelineRunStep.class); + private final Logger logger = LoggerFactory.getLogger(SendJobSucceededNotificationStep.class); public SendJobSucceededNotificationStep(NotificationService notificationService) { this.notificationService = notificationService; diff --git a/service/src/test/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStepTest.java b/service/src/test/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStepTest.java index 52527ba5..1ea43727 100644 --- a/service/src/test/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStepTest.java +++ b/service/src/test/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStepTest.java @@ -34,7 +34,7 @@ class CompletePipelineRunStepTest extends BaseEmbeddedDbTest { @Mock private FlightContext flightContext; private final UUID testJobId = TestUtils.TEST_NEW_UUID; - private final Integer quotaConsumed = 50; + private final Integer effectiveQuotaConsumed = 500; @BeforeEach void setup() { @@ -42,7 +42,7 @@ void setup() { var workingMap = new FlightMap(); workingMap.put(ImputationJobMapKeys.PIPELINE_RUN_OUTPUTS, TestUtils.TEST_PIPELINE_OUTPUTS); - workingMap.put(ImputationJobMapKeys.QUOTA_CONSUMED, quotaConsumed); + workingMap.put(ImputationJobMapKeys.EFFECTIVE_QUOTA_CONSUMED, effectiveQuotaConsumed); when(flightContext.getInputParameters()).thenReturn(inputParameters); when(flightContext.getWorkingMap()).thenReturn(workingMap); @@ -88,7 +88,7 @@ void doStepSuccess() throws JsonProcessingException { .findByJobIdAndUserId(testJobId, inputParams.get(JobMapKeys.USER_ID, String.class)) .orElseThrow(); assertEquals(CommonPipelineRunStatusEnum.SUCCEEDED, writtenJob.getStatus()); - assertEquals(quotaConsumed, writtenJob.getQuotaConsumed()); + assertEquals(effectiveQuotaConsumed, writtenJob.getQuotaConsumed()); assertTrue(writtenJob.getStatus().isSuccess()); // make sure outputs were written to db From ed60cb6fd12a0f0274e571f6f9cbbf33dba6b318 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Thu, 19 Dec 2024 13:28:56 -0500 Subject: [PATCH 18/28] resolve conflicts --- .../flights/imputation/RunImputationAzureJobFlight.java | 6 ------ .../stairway/steps/common/CompletePipelineRunStep.java | 7 +++++-- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationAzureJobFlight.java b/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationAzureJobFlight.java index e3c0ac5f..5727163b 100644 --- a/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationAzureJobFlight.java +++ b/service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationAzureJobFlight.java @@ -5,15 +5,9 @@ import bio.terra.pipelines.common.utils.FlightUtils; import bio.terra.pipelines.common.utils.PipelinesEnum; import bio.terra.pipelines.dependencies.stairway.JobMapKeys; -<<<<<<< HEAD:service/src/main/java/bio/terra/pipelines/stairway/flights/imputation/RunImputationAzureJobFlight.java import bio.terra.pipelines.stairway.steps.common.CompletePipelineRunStep; import bio.terra.pipelines.stairway.steps.imputation.PrepareImputationInputsStep; import bio.terra.pipelines.stairway.steps.imputation.azure.*; -======= -import bio.terra.pipelines.stairway.CompletePipelineRunStep; -import bio.terra.pipelines.stairway.imputation.steps.PrepareImputationInputsStep; -import bio.terra.pipelines.stairway.imputation.steps.azure.*; ->>>>>>> 13fe0fb (move files around):service/src/main/java/bio/terra/pipelines/stairway/imputation/RunImputationAzureJobFlight.java import bio.terra.stairway.*; public class RunImputationAzureJobFlight extends Flight { diff --git a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java index 405a502d..27574137 100644 --- a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java +++ b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java @@ -35,10 +35,13 @@ public StepResult doStep(FlightContext flightContext) { // validate and extract parameters from working map var workingMap = flightContext.getWorkingMap(); FlightUtils.validateRequiredEntries( - workingMap, ImputationJobMapKeys.PIPELINE_RUN_OUTPUTS, ImputationJobMapKeys.EFFECTIVE_QUOTA_CONSUMED); + workingMap, + ImputationJobMapKeys.PIPELINE_RUN_OUTPUTS, + ImputationJobMapKeys.EFFECTIVE_QUOTA_CONSUMED); Map outputsMap = workingMap.get(ImputationJobMapKeys.PIPELINE_RUN_OUTPUTS, Map.class); - int quotaConsumed = workingMap.get(ImputationJobMapKeys.EFFECTIVE_QUOTA_CONSUMED, Integer.class); + int quotaConsumed = + workingMap.get(ImputationJobMapKeys.EFFECTIVE_QUOTA_CONSUMED, Integer.class); pipelineRunsService.markPipelineRunSuccessAndWriteOutputs( jobId, userId, outputsMap, quotaConsumed); From c7fb87bbbb259586f128dc74eae4d1891ea58f9e Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Wed, 18 Dec 2024 16:21:02 -0500 Subject: [PATCH 19/28] dumb test don't even try --- .../bio/terra/pipelines/notifications/PubsubServiceTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java b/service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java index 1a2d69cc..aa8c14f5 100644 --- a/service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java +++ b/service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java @@ -16,6 +16,6 @@ void publishMessage() throws IOException, InterruptedException { String projectId = "testProjectId"; // lol this passes the test with no mocking since we catch the exception - pubsubService.publishMessage(projectId, topicId, message); + // pubsubService.publishMessage(projectId, topicId, message); } } From eff5b8affbf5808ac02c36b7a225524fb0ff031e Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Thu, 19 Dec 2024 10:34:51 -0500 Subject: [PATCH 20/28] update db changelog, try creating publisher once --- .../notifications/PubsubService.java | 98 ++++++++++++------- 1 file changed, 60 insertions(+), 38 deletions(-) diff --git a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java index ce40d218..35ebe6d6 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java @@ -10,7 +10,6 @@ import com.google.pubsub.v1.PubsubMessage; import com.google.pubsub.v1.TopicName; import java.io.IOException; -import java.util.concurrent.TimeUnit; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.stereotype.Service; @@ -20,54 +19,77 @@ public class PubsubService { private static final Logger logger = LoggerFactory.getLogger(PubsubService.class); + private Publisher publisher; + + private void createOrGetPublisher(TopicName topicName) throws IOException { + if (this.publisher != null) { + logger.info("Publisher already exists for Google PubSub topicName {}", topicName); + return; + } + try { + logger.info("Creating publisher for Google PubSub topicName {}", topicName); + // Create a publisher instance with default settings bound to the topic + this.publisher = Publisher.newBuilder(topicName).build(); + } catch (IOException e) { + logger.error("Error creating publisher for Google PubSub topicName {}", topicName); + throw e; + } + } + /** Publish a message to a Google PubSub topic. */ public void publishMessage(String projectId, String topicId, String message) throws IOException, InterruptedException { TopicName topicName = TopicName.of(projectId, topicId); - Publisher publisher = null; - logger.info("Publishing message to Google PubSub projectId {}, topicId {}", projectId, topicId); + logger.info( + "Publishing message to Google PubSub projectId {}, topicId {}: {}", + projectId, + topicId, + message); - try { - // Create a publisher instance with default settings bound to the topic - publisher = Publisher.newBuilder(topicName).build(); + // try { + createOrGetPublisher(topicName); - ByteString data = ByteString.copyFromUtf8(message); - PubsubMessage pubsubMessage = PubsubMessage.newBuilder().setData(data).build(); + ByteString data = ByteString.copyFromUtf8(message); + PubsubMessage pubsubMessage = PubsubMessage.newBuilder().setData(data).build(); - // Once published, returns a server-assigned message id (unique within the topic) - ApiFuture future = publisher.publish(pubsubMessage); + // Once published, returns a server-assigned message id (unique within the topic) + ApiFuture future = this.publisher.publish(pubsubMessage); - // Add an asynchronous callback to handle success / failure - ApiFutures.addCallback( - future, - new ApiFutureCallback() { + // Add an asynchronous callback to handle success / failure + ApiFutures.addCallback( + future, + new ApiFutureCallback() { - @Override - public void onFailure(Throwable throwable) { - if (throwable instanceof ApiException apiException) { - // details on the API exception - logger.warn( - "Google API exception: status code {}, is retryable: {}", - apiException.getStatusCode().getCode(), - apiException.isRetryable()); - } - logger.error("Error publishing message to Google PubSub: {}", message); + @Override + public void onFailure(Throwable throwable) { + String errorMessage = ""; + if (throwable instanceof ApiException apiException) { + // details on the API exception + errorMessage = + "Google API exception: status code %s, is retryable: %s" + .formatted( + apiException.getStatusCode().getCode(), apiException.isRetryable()); } + logger.error( + "Error publishing message to Google PubSub: {}; {}", + errorMessage, + throwable.getMessage()); + } - @Override - public void onSuccess(String messageId) { - // Once published, returns server-assigned message ids (unique within the topic) - logger.info("Published message ID: {}", messageId); - } - }, - MoreExecutors.directExecutor()); + @Override + public void onSuccess(String messageId) { + // Once published, returns server-assigned message ids (unique within the topic) + logger.info("Published message ID: {}", messageId); + } + }, + MoreExecutors.directExecutor()); - } finally { - if (publisher != null) { - // When finished with the publisher, shutdown to free up resources. - publisher.shutdown(); - publisher.awaitTermination(1, TimeUnit.MINUTES); - } - } + // } finally { + // if (publisher != null) { + // // When finished with the publisher, shutdown to free up resources. + // publisher.shutdown(); + // publisher.awaitTermination(1, TimeUnit.MINUTES); + // } } + // } } From e10a1f36cf3b52222607603df24bbcd4e4359d33 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Thu, 19 Dec 2024 13:13:59 -0500 Subject: [PATCH 21/28] refactor notification classes, other pr comments --- .../BaseTeaspoonsJobNotification.java | 21 +- .../notifications/NotificationService.java | 105 ++++---- .../notifications/PubsubService.java | 76 +++--- .../TeaspoonsJobFailedNotification.java | 22 +- .../TeaspoonsJobSucceededNotification.java | 17 +- .../NotificationServiceTest.java | 237 +++++------------- .../TeaspoonsJobFailedNotificationTest.java | 27 -- ...TeaspoonsJobSucceededNotificationTest.java | 26 -- 8 files changed, 159 insertions(+), 372 deletions(-) diff --git a/service/src/main/java/bio/terra/pipelines/notifications/BaseTeaspoonsJobNotification.java b/service/src/main/java/bio/terra/pipelines/notifications/BaseTeaspoonsJobNotification.java index 9eefcf2e..f91bba07 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/BaseTeaspoonsJobNotification.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/BaseTeaspoonsJobNotification.java @@ -2,17 +2,20 @@ import lombok.Getter; +/** Base class for Teaspoons job notifications. Contains common fields for all job notifications. */ @Getter -public class BaseTeaspoonsJobNotification { - public final String recipientUserId; - public final String pipelineDisplayName; - public final String jobId; - public final String timeSubmitted; - public final String timeCompleted; - public final String quotaRemaining; - public final String userDescription; +public abstract class BaseTeaspoonsJobNotification { + protected String notificationType; + protected String recipientUserId; + protected String pipelineDisplayName; + protected String jobId; + protected String timeSubmitted; + protected String timeCompleted; + protected String quotaRemaining; + protected String quotaConsumedByJob; + protected String userDescription; - public BaseTeaspoonsJobNotification( + protected BaseTeaspoonsJobNotification( String recipientUserId, String pipelineDisplayName, String jobId, diff --git a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java index 5c44236c..26a8279f 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java @@ -57,10 +57,13 @@ public NotificationService( * * @param jobId the job id * @param userId the user id + * @param context the flight context (only needed for failed notifications) + * @param isSuccess whether the notification is for a succeeded job; if false, creates a failed + * notification * @return the base notification object */ - public BaseTeaspoonsJobNotification createBaseTeaspoonsJobNotification( - UUID jobId, String userId) { + private BaseTeaspoonsJobNotification createTeaspoonsJobNotification( + UUID jobId, String userId, FlightContext context, boolean isSuccess) { PipelineRun pipelineRun = pipelineRunsService.getPipelineRun(jobId, userId); Pipeline pipeline = pipelinesService.getPipelineById(pipelineRun.getPipelineId()); String pipelineDisplayName = pipeline.getDisplayName(); @@ -71,14 +74,41 @@ public BaseTeaspoonsJobNotification createBaseTeaspoonsJobNotification( quotasService.getOrCreateQuotaForUserAndPipeline(userId, pipeline.getName()); String quotaRemaining = String.valueOf(userQuota.getQuota() - userQuota.getQuotaConsumed()); - return new BaseTeaspoonsJobNotification( - pipelineRun.getUserId(), - pipelineDisplayName, - pipelineRun.getJobId().toString(), - formatInstantToReadableString(pipelineRun.getCreated()), - formatInstantToReadableString(pipelineRun.getUpdated()), - quotaRemaining, - pipelineRun.getDescription()); + if (isSuccess) { // succeeded + return new TeaspoonsJobSucceededNotification( + userId, + pipelineDisplayName, + jobId.toString(), + formatInstantToReadableString(pipelineRun.getCreated()), + formatInstantToReadableString(pipelineRun.getUpdated()), + pipelineRun.getQuotaConsumed().toString(), + quotaRemaining, + pipelineRun.getDescription()); + } else { // failed + // get exception + Optional exception = context.getResult().getException(); + String errorMessage; + if (exception.isPresent()) { + ApiErrorReport errorReport = + buildApiErrorReport(exception.get()); // use same logic that the status endpoint uses + errorMessage = errorReport.getMessage(); + } else { + logger.error( + "No exception found in flight result for flight {} with status {}", + context.getFlightId(), + context.getFlightStatus()); + errorMessage = "Unknown error"; + } + return new TeaspoonsJobFailedNotification( + userId, + pipelineDisplayName, + jobId.toString(), + errorMessage, + formatInstantToReadableString(pipelineRun.getCreated()), + formatInstantToReadableString(pipelineRun.getUpdated()), + quotaRemaining, + pipelineRun.getDescription()); + } } /** @@ -88,54 +118,10 @@ public BaseTeaspoonsJobNotification createBaseTeaspoonsJobNotification( * @param dateTime the Instant to format * @return the formatted date time string */ - public String formatInstantToReadableString(Instant dateTime) { + protected String formatInstantToReadableString(Instant dateTime) { return dateTime.atZone(ZoneId.of("UTC")).format(DateTimeFormatter.RFC_1123_DATE_TIME); } - /** - * Create a notification object for a failed job. - * - * @param jobId the job id - * @param userId the user id - * @param context the flight context - * @return the notification object - */ - public TeaspoonsJobFailedNotification createTeaspoonsJobFailedNotification( - UUID jobId, String userId, FlightContext context) { - // get exception - Optional exception = context.getResult().getException(); - String errorMessage; - if (exception.isPresent()) { - ApiErrorReport errorReport = - buildApiErrorReport(exception.get()); // use same logic that the status endpoint uses - errorMessage = errorReport.getMessage(); - } else { - logger.error( - "No exception found in flight result for flight {} with status {}", - context.getFlightId(), - context.getFlightStatus()); - errorMessage = "Unknown error"; - } - - return new TeaspoonsJobFailedNotification( - createBaseTeaspoonsJobNotification(jobId, userId), errorMessage); - } - - /** - * Create a notification object for a successful job. - * - * @param jobId the job id - * @param userId the user id - * @return the notification object - */ - public TeaspoonsJobSucceededNotification createTeaspoonsJobSucceededNotification( - UUID jobId, String userId) { - String quotaConsumedByJob = - pipelineRunsService.getPipelineRun(jobId, userId).getQuotaConsumed().toString(); - return new TeaspoonsJobSucceededNotification( - createBaseTeaspoonsJobNotification(jobId, userId), quotaConsumedByJob); - } - /** * Configure and send a notification that a job has succeeded. * @@ -147,8 +133,9 @@ public void configureAndSendPipelineRunSucceededNotification(UUID jobId, String pubsubService.publishMessage( notificationConfiguration.projectId(), notificationConfiguration.topicId(), - objectMapper.writeValueAsString(createTeaspoonsJobSucceededNotification(jobId, userId))); - } catch (IOException | InterruptedException e) { + objectMapper.writeValueAsString( + createTeaspoonsJobNotification(jobId, userId, null, true))); + } catch (IOException e) { logger.error("Error sending pipelineRunSucceeded notification", e); } } @@ -167,8 +154,8 @@ public void configureAndSendPipelineRunFailedNotification( notificationConfiguration.projectId(), notificationConfiguration.topicId(), objectMapper.writeValueAsString( - createTeaspoonsJobFailedNotification(jobId, userId, context))); - } catch (IOException | InterruptedException e) { + createTeaspoonsJobNotification(jobId, userId, context, false))); + } catch (IOException e) { logger.error("Error sending pipelineRunFailed notification", e); } } diff --git a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java index 35ebe6d6..9700abc9 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java @@ -19,17 +19,20 @@ public class PubsubService { private static final Logger logger = LoggerFactory.getLogger(PubsubService.class); - private Publisher publisher; + private static Publisher publisher; - private void createOrGetPublisher(TopicName topicName) throws IOException { - if (this.publisher != null) { + /** + * Initialize a publisher for a Google PubSub topic. Does nothing if the publisher already exists. + */ + private static void initPublisher(TopicName topicName) throws IOException { + if (publisher != null) { logger.info("Publisher already exists for Google PubSub topicName {}", topicName); return; } try { logger.info("Creating publisher for Google PubSub topicName {}", topicName); // Create a publisher instance with default settings bound to the topic - this.publisher = Publisher.newBuilder(topicName).build(); + publisher = Publisher.newBuilder(topicName).build(); } catch (IOException e) { logger.error("Error creating publisher for Google PubSub topicName {}", topicName); throw e; @@ -37,8 +40,7 @@ private void createOrGetPublisher(TopicName topicName) throws IOException { } /** Publish a message to a Google PubSub topic. */ - public void publishMessage(String projectId, String topicId, String message) - throws IOException, InterruptedException { + public void publishMessage(String projectId, String topicId, String message) throws IOException { TopicName topicName = TopicName.of(projectId, topicId); logger.info( "Publishing message to Google PubSub projectId {}, topicId {}: {}", @@ -46,50 +48,44 @@ public void publishMessage(String projectId, String topicId, String message) topicId, message); - // try { - createOrGetPublisher(topicName); + initPublisher(topicName); ByteString data = ByteString.copyFromUtf8(message); PubsubMessage pubsubMessage = PubsubMessage.newBuilder().setData(data).build(); // Once published, returns a server-assigned message id (unique within the topic) - ApiFuture future = this.publisher.publish(pubsubMessage); + ApiFuture future = publisher.publish(pubsubMessage); // Add an asynchronous callback to handle success / failure + // Registers a callback to be run when the ApiFuture's computation is complete or, if the + // computation is already complete, immediately. ApiFutures.addCallback( - future, - new ApiFutureCallback() { + future, handleCompletedPubsubMessagePublishing(), MoreExecutors.directExecutor()); + } - @Override - public void onFailure(Throwable throwable) { - String errorMessage = ""; - if (throwable instanceof ApiException apiException) { - // details on the API exception - errorMessage = - "Google API exception: status code %s, is retryable: %s" - .formatted( - apiException.getStatusCode().getCode(), apiException.isRetryable()); - } - logger.error( - "Error publishing message to Google PubSub: {}; {}", - errorMessage, - throwable.getMessage()); - } + ApiFutureCallback handleCompletedPubsubMessagePublishing() { + return new ApiFutureCallback() { - @Override - public void onSuccess(String messageId) { - // Once published, returns server-assigned message ids (unique within the topic) - logger.info("Published message ID: {}", messageId); - } - }, - MoreExecutors.directExecutor()); + @Override + public void onFailure(Throwable throwable) { + String errorMessage = ""; + if (throwable instanceof ApiException apiException) { + // details on the API exception + errorMessage = + "Google API exception: status code %s, is retryable: %s" + .formatted(apiException.getStatusCode().getCode(), apiException.isRetryable()); + } + logger.error( + "Error publishing message to Google PubSub: {}; {}", + errorMessage, + throwable.getMessage()); + } - // } finally { - // if (publisher != null) { - // // When finished with the publisher, shutdown to free up resources. - // publisher.shutdown(); - // publisher.awaitTermination(1, TimeUnit.MINUTES); - // } + @Override + public void onSuccess(String messageId) { + // Once published, returns server-assigned message ids (unique within the topic) + logger.info("Published message ID: {}", messageId); + } + }; } - // } } diff --git a/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotification.java b/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotification.java index e6867859..712d390f 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotification.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotification.java @@ -3,27 +3,11 @@ import lombok.Getter; @Getter +@SuppressWarnings({"java:S107"}) // Disable "Methods should not have too many parameters" public class TeaspoonsJobFailedNotification extends BaseTeaspoonsJobNotification { private static final String NOTIFICATION_TYPE = "TeaspoonsJobFailedNotification"; private static final String QUOTA_CONSUMED_BY_FAILED_JOB = "0"; - public final String notificationType; - public final String errorMessage; - public final String quotaConsumedByJob; - - public TeaspoonsJobFailedNotification( - BaseTeaspoonsJobNotification baseTeaspoonsJobNotification, String errorMessage) { - super( - baseTeaspoonsJobNotification.recipientUserId, - baseTeaspoonsJobNotification.pipelineDisplayName, - baseTeaspoonsJobNotification.jobId, - baseTeaspoonsJobNotification.timeSubmitted, - baseTeaspoonsJobNotification.timeCompleted, - baseTeaspoonsJobNotification.quotaRemaining, - baseTeaspoonsJobNotification.userDescription); - this.notificationType = NOTIFICATION_TYPE; - this.errorMessage = errorMessage; - this.quotaConsumedByJob = QUOTA_CONSUMED_BY_FAILED_JOB; - } + private final String errorMessage; public TeaspoonsJobFailedNotification( String recipientUserId, @@ -43,7 +27,7 @@ public TeaspoonsJobFailedNotification( quotaRemaining, userDescription); this.notificationType = NOTIFICATION_TYPE; - this.errorMessage = errorMessage; this.quotaConsumedByJob = QUOTA_CONSUMED_BY_FAILED_JOB; + this.errorMessage = errorMessage; } } diff --git a/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotification.java b/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotification.java index 85ba9c05..da41ccba 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotification.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotification.java @@ -3,24 +3,9 @@ import lombok.Getter; @Getter +@SuppressWarnings({"java:S107"}) // Disable "Methods should not have too many parameters" public class TeaspoonsJobSucceededNotification extends BaseTeaspoonsJobNotification { private static final String NOTIFICATION_TYPE = "TeaspoonsJobSucceededNotification"; - public final String notificationType; - public final String quotaConsumedByJob; - - public TeaspoonsJobSucceededNotification( - BaseTeaspoonsJobNotification baseTeaspoonsJobNotification, String quotaConsumedByJob) { - super( - baseTeaspoonsJobNotification.recipientUserId, - baseTeaspoonsJobNotification.pipelineDisplayName, - baseTeaspoonsJobNotification.jobId, - baseTeaspoonsJobNotification.timeSubmitted, - baseTeaspoonsJobNotification.timeCompleted, - baseTeaspoonsJobNotification.quotaRemaining, - baseTeaspoonsJobNotification.userDescription); - this.notificationType = NOTIFICATION_TYPE; - this.quotaConsumedByJob = quotaConsumedByJob; - } public TeaspoonsJobSucceededNotification( String recipientUserId, diff --git a/service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java b/service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java index 0a9bb3ad..8130f807 100644 --- a/service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java +++ b/service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java @@ -51,53 +51,6 @@ class NotificationServiceTest extends BaseEmbeddedDbTest { String testUserDescription = TestUtils.TEST_USER_PROVIDED_DESCRIPTION; String testErrorMessage = "test error message"; - @Test - void createBaseTeaspoonsJobNotification() { - Pipeline pipeline = pipelinesService.getPipelineById(1L); - PipelineRun writtenPipelineRun = - createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.SUCCEEDED); - - // initialize and set user quota - UserQuota userQuota = - quotasService.getOrCreateQuotaForUserAndPipeline(testUserId, pipeline.getName()); - UserQuota updatedUserQuota = - quotasService.updateQuotaConsumed(userQuota, testQuotaConsumedByJob); - int expectedQuotaRemaining = updatedUserQuota.getQuota() - updatedUserQuota.getQuotaConsumed(); - - BaseTeaspoonsJobNotification baseTeaspoonsJobNotification = - notificationService.createBaseTeaspoonsJobNotification(testJobId, testUserId); - - assertEquals(testUserId, baseTeaspoonsJobNotification.getRecipientUserId()); - assertEquals(pipeline.getDisplayName(), baseTeaspoonsJobNotification.getPipelineDisplayName()); - assertEquals(testJobId.toString(), baseTeaspoonsJobNotification.getJobId()); - assertEquals( - notificationService.formatInstantToReadableString(writtenPipelineRun.getCreated()), - baseTeaspoonsJobNotification.getTimeSubmitted()); - assertEquals( - notificationService.formatInstantToReadableString(writtenPipelineRun.getUpdated()), - baseTeaspoonsJobNotification.getTimeCompleted()); - assertEquals( - String.valueOf(expectedQuotaRemaining), baseTeaspoonsJobNotification.getQuotaRemaining()); - assertEquals(testUserDescription, baseTeaspoonsJobNotification.getUserDescription()); - } - - @Test - void createBaseTeaspoonsJobNotificationNoQuota() { - Pipeline pipeline = pipelinesService.getPipelineById(1L); - createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.SUCCEEDED); - - // don't initialize user in user_quota table - int expectedQuotaRemaining = - quotasService.getPipelineQuota(pipeline.getName()).getDefaultQuota(); - - BaseTeaspoonsJobNotification baseTeaspoonsJobNotification = - notificationService.createBaseTeaspoonsJobNotification(testJobId, testUserId); - - assertEquals(testUserId, baseTeaspoonsJobNotification.getRecipientUserId()); - assertEquals( - String.valueOf(expectedQuotaRemaining), baseTeaspoonsJobNotification.getQuotaRemaining()); - } - @Test void formatDateTime() { Instant instant = Instant.parse("2021-08-25T12:34:56.789Z"); @@ -106,70 +59,7 @@ void formatDateTime() { } @Test - void createTeaspoonsJobFailedNotification() { - Pipeline pipeline = pipelinesService.getPipelineById(1L); - PipelineRun writtenPipelineRun = - createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.FAILED); - - // don't initialize user in user_quota table - int expectedQuotaRemaining = - quotasService.getPipelineQuota(pipeline.getName()).getDefaultQuota(); - - when(flightContext.getFlightId()).thenReturn(testJobId.toString()); - RawlsServiceApiException rawlsServiceApiException = - new RawlsServiceApiException(testErrorMessage); - StepResult stepResultFailedWithException = - new StepResult(StepStatus.STEP_RESULT_FAILURE_FATAL, rawlsServiceApiException); - when(flightContext.getResult()).thenReturn(stepResultFailedWithException); - - TeaspoonsJobFailedNotification teaspoonsJobFailedNotification = - notificationService.createTeaspoonsJobFailedNotification( - testJobId, testUserId, flightContext); - - assertEquals(testUserId, teaspoonsJobFailedNotification.getRecipientUserId()); - assertEquals( - pipeline.getDisplayName(), teaspoonsJobFailedNotification.getPipelineDisplayName()); - assertEquals(testJobId.toString(), teaspoonsJobFailedNotification.getJobId()); - assertEquals( - notificationService.formatInstantToReadableString(writtenPipelineRun.getCreated()), - teaspoonsJobFailedNotification.getTimeSubmitted()); - assertEquals( - notificationService.formatInstantToReadableString(writtenPipelineRun.getUpdated()), - teaspoonsJobFailedNotification.getTimeCompleted()); - assertEquals( - String.valueOf(expectedQuotaRemaining), teaspoonsJobFailedNotification.getQuotaRemaining()); - assertEquals(testUserDescription, teaspoonsJobFailedNotification.getUserDescription()); - - // fields specific to failed job notification - assertEquals( - "TeaspoonsJobFailedNotification", teaspoonsJobFailedNotification.getNotificationType()); - assertEquals(testErrorMessage, teaspoonsJobFailedNotification.getErrorMessage()); - assertEquals("0", teaspoonsJobFailedNotification.getQuotaConsumedByJob()); - } - - @Test - void createTeaspoonsJobFailedNotificationNoMessage() { - Pipeline pipeline = pipelinesService.getPipelineById(1L); - createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.FAILED); - - when(flightContext.getFlightId()).thenReturn(testJobId.toString()); - StepResult stepResultFailedNoException = new StepResult(StepStatus.STEP_RESULT_FAILURE_FATAL); - when(flightContext.getResult()).thenReturn(stepResultFailedNoException); - - TeaspoonsJobFailedNotification teaspoonsJobFailedNotification = - notificationService.createTeaspoonsJobFailedNotification( - testJobId, testUserId, flightContext); - - assertEquals(testUserId, teaspoonsJobFailedNotification.getRecipientUserId()); - - // test for message when no exception is present - assertEquals( - "TeaspoonsJobFailedNotification", teaspoonsJobFailedNotification.getNotificationType()); - assertEquals("Unknown error", teaspoonsJobFailedNotification.getErrorMessage()); - } - - @Test - void createTeaspoonsJobSucceededNotification() { + void configureAndSendPipelineRunSucceededNotification() throws IOException { Pipeline pipeline = pipelinesService.getPipelineById(1L); PipelineRun writtenPipelineRun = createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.SUCCEEDED); @@ -181,38 +71,6 @@ void createTeaspoonsJobSucceededNotification() { quotasService.updateQuotaConsumed(userQuota, testQuotaConsumedByJob); int expectedQuotaRemaining = updatedUserQuota.getQuota() - updatedUserQuota.getQuotaConsumed(); - TeaspoonsJobSucceededNotification teaspoonsJobSucceededNotification = - notificationService.createTeaspoonsJobSucceededNotification(testJobId, testUserId); - - assertEquals(testUserId, teaspoonsJobSucceededNotification.getRecipientUserId()); - assertEquals( - pipeline.getDisplayName(), teaspoonsJobSucceededNotification.getPipelineDisplayName()); - assertEquals(testJobId.toString(), teaspoonsJobSucceededNotification.getJobId()); - assertEquals( - notificationService.formatInstantToReadableString(writtenPipelineRun.getCreated()), - teaspoonsJobSucceededNotification.getTimeSubmitted()); - assertEquals( - notificationService.formatInstantToReadableString(writtenPipelineRun.getUpdated()), - teaspoonsJobSucceededNotification.getTimeCompleted()); - assertEquals( - String.valueOf(expectedQuotaRemaining), - teaspoonsJobSucceededNotification.getQuotaRemaining()); - assertEquals(testUserDescription, teaspoonsJobSucceededNotification.getUserDescription()); - - // fields specific to succeeded job notification - assertEquals( - "TeaspoonsJobSucceededNotification", - teaspoonsJobSucceededNotification.getNotificationType()); - assertEquals( - testQuotaConsumedByJob.toString(), - teaspoonsJobSucceededNotification.getQuotaConsumedByJob()); - } - - @Test - void configureAndSendPipelineRunSucceededNotification() throws IOException, InterruptedException { - Pipeline pipeline = pipelinesService.getPipelineById(1L); - createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.SUCCEEDED); - // success is a void method doNothing().when(pubsubService).publishMessage(any(), any(), any()); @@ -220,7 +78,16 @@ void configureAndSendPipelineRunSucceededNotification() throws IOException, Inte String stringifiedJobSucceededNotification = objectMapper.writeValueAsString( - notificationService.createTeaspoonsJobSucceededNotification(testJobId, testUserId)); + new TeaspoonsJobSucceededNotification( + testUserId, + pipeline.getDisplayName(), + testJobId.toString(), + notificationService.formatInstantToReadableString(writtenPipelineRun.getCreated()), + notificationService.formatInstantToReadableString(writtenPipelineRun.getUpdated()), + testQuotaConsumedByJob.toString(), + String.valueOf(expectedQuotaRemaining), + testUserDescription)); + // verify that the pubsub method was called verify(pubsubService, times(1)) .publishMessage( @@ -230,8 +97,7 @@ void configureAndSendPipelineRunSucceededNotification() throws IOException, Inte } @Test - void configureAndSendPipelineRunSucceededNotificationIOException() - throws IOException, InterruptedException { + void configureAndSendPipelineRunSucceededNotificationIOException() throws IOException { Pipeline pipeline = pipelinesService.getPipelineById(1L); createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.SUCCEEDED); @@ -245,24 +111,17 @@ void configureAndSendPipelineRunSucceededNotificationIOException() } @Test - void configureAndSendPipelineRunSucceededNotificationInterruptedException() - throws IOException, InterruptedException { + void configureAndSendPipelineRunFailedNotification() throws IOException { Pipeline pipeline = pipelinesService.getPipelineById(1L); - createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.SUCCEEDED); - - doThrow(new InterruptedException()).when(pubsubService).publishMessage(any(), any(), any()); - - // exception should be caught - assertDoesNotThrow( - () -> - notificationService.configureAndSendPipelineRunSucceededNotification( - testJobId, testUserId)); - } + PipelineRun writtenPipelineRun = + createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.FAILED); - @Test - void configureAndSendPipelineRunFailedNotification() throws IOException, InterruptedException { - Pipeline pipeline = pipelinesService.getPipelineById(1L); - createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.FAILED); + // initialize and set a custom user quota value. this is not quota consumed by the job. + int customUserQuota = 2000; + UserQuota userQuota = + quotasService.getOrCreateQuotaForUserAndPipeline(testUserId, pipeline.getName()); + UserQuota updatedUserQuota = quotasService.updateQuotaConsumed(userQuota, customUserQuota); + int expectedQuotaRemaining = updatedUserQuota.getQuota() - updatedUserQuota.getQuotaConsumed(); when(flightContext.getFlightId()).thenReturn(testJobId.toString()); RawlsServiceApiException rawlsServiceApiException = @@ -279,8 +138,15 @@ void configureAndSendPipelineRunFailedNotification() throws IOException, Interru String stringifiedJobFailedNotification = objectMapper.writeValueAsString( - notificationService.createTeaspoonsJobFailedNotification( - testJobId, testUserId, flightContext)); + new TeaspoonsJobFailedNotification( + testUserId, + pipeline.getDisplayName(), + testJobId.toString(), + testErrorMessage, + notificationService.formatInstantToReadableString(writtenPipelineRun.getCreated()), + notificationService.formatInstantToReadableString(writtenPipelineRun.getUpdated()), + String.valueOf(expectedQuotaRemaining), + testUserDescription)); // verify that the pubsub method was called verify(pubsubService, times(1)) .publishMessage( @@ -290,10 +156,14 @@ void configureAndSendPipelineRunFailedNotification() throws IOException, Interru } @Test - void configureAndSendPipelineRunFailedNotificationIOException() - throws IOException, InterruptedException { + void configureAndSendPipelineRunFailedNotificationNoUserQuota() throws IOException { Pipeline pipeline = pipelinesService.getPipelineById(1L); - createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.FAILED); + PipelineRun writtenPipelineRun = + createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.FAILED); + + // don't initialize user in user_quota table + int expectedQuotaRemaining = + quotasService.getPipelineQuota(pipeline.getName()).getDefaultQuota(); when(flightContext.getFlightId()).thenReturn(testJobId.toString()); RawlsServiceApiException rawlsServiceApiException = @@ -302,18 +172,33 @@ void configureAndSendPipelineRunFailedNotificationIOException() new StepResult(StepStatus.STEP_RESULT_FAILURE_FATAL, rawlsServiceApiException); when(flightContext.getResult()).thenReturn(stepResultFailedWithException); - doThrow(new IOException()).when(pubsubService).publishMessage(any(), any(), any()); + // success is a void method + doNothing().when(pubsubService).publishMessage(any(), any(), any()); - // exception should be caught - assertDoesNotThrow( - () -> - notificationService.configureAndSendPipelineRunFailedNotification( - testJobId, testUserId, flightContext)); + notificationService.configureAndSendPipelineRunFailedNotification( + testJobId, testUserId, flightContext); + + String stringifiedJobFailedNotification = + objectMapper.writeValueAsString( + new TeaspoonsJobFailedNotification( + testUserId, + pipeline.getDisplayName(), + testJobId.toString(), + testErrorMessage, + notificationService.formatInstantToReadableString(writtenPipelineRun.getCreated()), + notificationService.formatInstantToReadableString(writtenPipelineRun.getUpdated()), + String.valueOf(expectedQuotaRemaining), + testUserDescription)); + // verify that the pubsub method was called + verify(pubsubService, times(1)) + .publishMessage( + notificationConfiguration.projectId(), + notificationConfiguration.topicId(), + stringifiedJobFailedNotification); } @Test - void configureAndSendPipelineRunFailedNotificationInterruptedException() - throws IOException, InterruptedException { + void configureAndSendPipelineRunFailedNotificationIOException() throws IOException { Pipeline pipeline = pipelinesService.getPipelineById(1L); createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.FAILED); @@ -324,7 +209,7 @@ void configureAndSendPipelineRunFailedNotificationInterruptedException() new StepResult(StepStatus.STEP_RESULT_FAILURE_FATAL, rawlsServiceApiException); when(flightContext.getResult()).thenReturn(stepResultFailedWithException); - doThrow(new InterruptedException()).when(pubsubService).publishMessage(any(), any(), any()); + doThrow(new IOException()).when(pubsubService).publishMessage(any(), any(), any()); // exception should be caught assertDoesNotThrow( diff --git a/service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotificationTest.java b/service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotificationTest.java index b172d2aa..836b8f92 100644 --- a/service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotificationTest.java +++ b/service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobFailedNotificationTest.java @@ -15,33 +15,6 @@ class TeaspoonsJobFailedNotificationTest extends BaseTest { String quotaRemaining = "test-quota-remaining"; String userDescription = "test-user-description"; - @Test - void teaspoonsJobFailedNotificationShortConstructor() { - BaseTeaspoonsJobNotification baseTeaspoonsJobNotification = - new BaseTeaspoonsJobNotification( - recipientUserId, - pipelineDisplayName, - jobId, - timeSubmitted, - timeCompleted, - quotaRemaining, - userDescription); - - TeaspoonsJobFailedNotification notification = - new TeaspoonsJobFailedNotification(baseTeaspoonsJobNotification, errorMessage); - - assertEquals("TeaspoonsJobFailedNotification", notification.getNotificationType()); - assertEquals(recipientUserId, notification.getRecipientUserId()); - assertEquals(pipelineDisplayName, notification.getPipelineDisplayName()); - assertEquals(jobId, notification.getJobId()); - assertEquals(errorMessage, notification.getErrorMessage()); - assertEquals(timeSubmitted, notification.getTimeSubmitted()); - assertEquals(timeCompleted, notification.getTimeCompleted()); - assertEquals("0", notification.getQuotaConsumedByJob()); - assertEquals(quotaRemaining, notification.getQuotaRemaining()); - assertEquals(userDescription, notification.getUserDescription()); - } - @Test void teaspoonsJobFailedNotificationFullConstructor() { diff --git a/service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotificationTest.java b/service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotificationTest.java index 309e04a1..fd956614 100644 --- a/service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotificationTest.java +++ b/service/src/test/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotificationTest.java @@ -16,32 +16,6 @@ class TeaspoonsJobSucceededNotificationTest extends BaseTest { String quotaRemaining = "test-quota-remaining"; String userDescription = "test-user-description"; - @Test - void teaspoonsJobSucceededNotificationShortConstructor() { - BaseTeaspoonsJobNotification baseTeaspoonsJobNotification = - new BaseTeaspoonsJobNotification( - recipientUserId, - pipelineDisplayName, - jobId, - timeSubmitted, - timeCompleted, - quotaRemaining, - userDescription); - - TeaspoonsJobSucceededNotification notification = - new TeaspoonsJobSucceededNotification(baseTeaspoonsJobNotification, quotaConsumedByJob); - - assertEquals("TeaspoonsJobSucceededNotification", notification.getNotificationType()); - assertEquals(recipientUserId, notification.getRecipientUserId()); - assertEquals(pipelineDisplayName, notification.getPipelineDisplayName()); - assertEquals(jobId, notification.getJobId()); - assertEquals(timeSubmitted, notification.getTimeSubmitted()); - assertEquals(timeCompleted, notification.getTimeCompleted()); - assertEquals(quotaConsumedByJob, notification.getQuotaConsumedByJob()); - assertEquals(quotaRemaining, notification.getQuotaRemaining()); - assertEquals(userDescription, notification.getUserDescription()); - } - @Test void teaspoonsJobSucceededNotificationFullConstructor() { TeaspoonsJobSucceededNotification notification = From 7d35e9b3040be7f71a6ab1399fc08994102509e0 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Thu, 19 Dec 2024 13:33:45 -0500 Subject: [PATCH 22/28] add step expectations of input params and working map --- .../stairway/steps/common/CompletePipelineRunStep.java | 4 ++++ .../steps/common/SendJobSucceededNotificationStep.java | 2 ++ 2 files changed, 6 insertions(+) diff --git a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java index 27574137..ab6b833d 100644 --- a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java +++ b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java @@ -14,6 +14,10 @@ /** * Step to mark a pipeline run as a success and write the outputs and quota consumed to the database + * + *

This step expects the JobMapKeys.USER_ID in the input parameters and + * ImputationJobMapKeys.PIPELINE_RUN_OUTPUTS and ImputationJobMapKeys.EFFECTIVE_QUOTA_CONSUMED in + * the working map. */ public class CompletePipelineRunStep implements Step { private final PipelineRunsService pipelineRunsService; diff --git a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java index 8c26eee1..9959197a 100644 --- a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java +++ b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/SendJobSucceededNotificationStep.java @@ -13,6 +13,8 @@ /** * Step to send an email notification that a job has succeeded. This step cannot fail (we catch all * exceptions). + * + *

This step expects JobMapKeys.USER_ID in the input parameters. */ public class SendJobSucceededNotificationStep implements Step { private final NotificationService notificationService; From 140e677ab113c248c32bcb9191da0e308474da04 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Thu, 19 Dec 2024 14:06:10 -0500 Subject: [PATCH 23/28] add test for initPublisher --- .../notifications/PubsubService.java | 4 +- .../notifications/PubsubServiceTest.java | 37 +++++++++++++++---- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java index 9700abc9..21c4dccc 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java @@ -19,12 +19,12 @@ public class PubsubService { private static final Logger logger = LoggerFactory.getLogger(PubsubService.class); - private static Publisher publisher; + protected static Publisher publisher; /** * Initialize a publisher for a Google PubSub topic. Does nothing if the publisher already exists. */ - private static void initPublisher(TopicName topicName) throws IOException { + protected static void initPublisher(TopicName topicName) throws IOException { if (publisher != null) { logger.info("Publisher already exists for Google PubSub topicName {}", topicName); return; diff --git a/service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java b/service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java index aa8c14f5..42e05eb7 100644 --- a/service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java +++ b/service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java @@ -1,21 +1,44 @@ package bio.terra.pipelines.notifications; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + import bio.terra.pipelines.testutils.BaseEmbeddedDbTest; +import com.google.cloud.pubsub.v1.Publisher; +import com.google.pubsub.v1.TopicName; import java.io.IOException; import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; import org.springframework.beans.factory.annotation.Autowired; class PubsubServiceTest extends BaseEmbeddedDbTest { @Autowired PubsubService pubsubService; @Test - void publishMessage() throws IOException, InterruptedException { - // setup - String message = "test message"; - String topicId = "testTopicId"; - String projectId = "testProjectId"; + void initPublisher() throws IOException { + TopicName topicName = TopicName.of("projectId", "topicId"); + try (MockedStatic mockedStaticPublisher = mockStatic(Publisher.class)) { + Publisher.Builder mockBuilder = mock(Publisher.Builder.class); + mockedStaticPublisher.when(() -> Publisher.newBuilder(topicName)).thenReturn(mockBuilder); + Publisher mockPublisher = mock(Publisher.class); + when(mockBuilder.build()).thenReturn(mockPublisher); + + // to start, publisher should be null + assertNull(PubsubService.publisher); + + // calling init once should set the publisher + PubsubService.initPublisher(topicName); + assertEquals(mockPublisher, PubsubService.publisher); - // lol this passes the test with no mocking since we catch the exception - // pubsubService.publishMessage(projectId, topicId, message); + // calling init again should not call build again + PubsubService.initPublisher(topicName); + assertEquals(mockPublisher, PubsubService.publisher); + verify(mockBuilder, times(1)).build(); + } } } From 61f35257b977a493ece325f14dbf1dcdbdb9d0d2 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Thu, 19 Dec 2024 14:21:13 -0500 Subject: [PATCH 24/28] add test, try to get logging working properly with ApiFutureCallback --- .../notifications/PubsubService.java | 4 +- .../NotificationServiceTest.java | 40 +++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java index 21c4dccc..87cf8570 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java @@ -60,10 +60,10 @@ public void publishMessage(String projectId, String topicId, String message) thr // Registers a callback to be run when the ApiFuture's computation is complete or, if the // computation is already complete, immediately. ApiFutures.addCallback( - future, handleCompletedPubsubMessagePublishing(), MoreExecutors.directExecutor()); + future, handleCompletedPubsubMessagePublishing(logger), MoreExecutors.directExecutor()); } - ApiFutureCallback handleCompletedPubsubMessagePublishing() { + ApiFutureCallback handleCompletedPubsubMessagePublishing(Logger logger) { return new ApiFutureCallback() { @Override diff --git a/service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java b/service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java index 8130f807..7b37e2bd 100644 --- a/service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java +++ b/service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java @@ -197,6 +197,46 @@ void configureAndSendPipelineRunFailedNotificationNoUserQuota() throws IOExcepti stringifiedJobFailedNotification); } + @Test + void configureAndSendPipelineRunFailedNotificationWithoutException() throws IOException { + Pipeline pipeline = pipelinesService.getPipelineById(1L); + PipelineRun writtenPipelineRun = + createCompletedPipelineRunInDb(pipeline, CommonPipelineRunStatusEnum.FAILED); + + // don't initialize user in user_quota table + int expectedQuotaRemaining = + quotasService.getPipelineQuota(pipeline.getName()).getDefaultQuota(); + + when(flightContext.getFlightId()).thenReturn(testJobId.toString()); + StepResult stepResultFailedWithoutException = + new StepResult(StepStatus.STEP_RESULT_FAILURE_FATAL); + when(flightContext.getResult()).thenReturn(stepResultFailedWithoutException); + + // success is a void method + doNothing().when(pubsubService).publishMessage(any(), any(), any()); + + notificationService.configureAndSendPipelineRunFailedNotification( + testJobId, testUserId, flightContext); + + String stringifiedJobFailedNotification = + objectMapper.writeValueAsString( + new TeaspoonsJobFailedNotification( + testUserId, + pipeline.getDisplayName(), + testJobId.toString(), + "Unknown error", + notificationService.formatInstantToReadableString(writtenPipelineRun.getCreated()), + notificationService.formatInstantToReadableString(writtenPipelineRun.getUpdated()), + String.valueOf(expectedQuotaRemaining), + testUserDescription)); + // verify that the pubsub method was called + verify(pubsubService, times(1)) + .publishMessage( + notificationConfiguration.projectId(), + notificationConfiguration.topicId(), + stringifiedJobFailedNotification); + } + @Test void configureAndSendPipelineRunFailedNotificationIOException() throws IOException { Pipeline pipeline = pipelinesService.getPipelineById(1L); From 34815b6021ee710778bb58f6d52e8b60bf746e0a Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Thu, 19 Dec 2024 14:26:47 -0500 Subject: [PATCH 25/28] try another logging thing --- .../bio/terra/pipelines/notifications/PubsubService.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java index 87cf8570..ab747539 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java @@ -60,10 +60,10 @@ public void publishMessage(String projectId, String topicId, String message) thr // Registers a callback to be run when the ApiFuture's computation is complete or, if the // computation is already complete, immediately. ApiFutures.addCallback( - future, handleCompletedPubsubMessagePublishing(logger), MoreExecutors.directExecutor()); + future, handleCompletedPubsubMessagePublishing(), MoreExecutors.directExecutor()); } - ApiFutureCallback handleCompletedPubsubMessagePublishing(Logger logger) { + ApiFutureCallback handleCompletedPubsubMessagePublishing() { return new ApiFutureCallback() { @Override @@ -75,7 +75,7 @@ public void onFailure(Throwable throwable) { "Google API exception: status code %s, is retryable: %s" .formatted(apiException.getStatusCode().getCode(), apiException.isRetryable()); } - logger.error( + PubsubService.logger.error( "Error publishing message to Google PubSub: {}; {}", errorMessage, throwable.getMessage()); @@ -84,7 +84,7 @@ public void onFailure(Throwable throwable) { @Override public void onSuccess(String messageId) { // Once published, returns server-assigned message ids (unique within the topic) - logger.info("Published message ID: {}", messageId); + PubsubService.logger.info("Published message ID: {}", messageId); } }; } From 8bb9531a814812c1da3d3bcf479617654a5a759f Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Thu, 19 Dec 2024 14:31:14 -0500 Subject: [PATCH 26/28] suppress unboxing warning --- .../stairway/steps/common/CompletePipelineRunStep.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java index ab6b833d..87164de9 100644 --- a/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java +++ b/service/src/main/java/bio/terra/pipelines/stairway/steps/common/CompletePipelineRunStep.java @@ -28,6 +28,8 @@ public CompletePipelineRunStep(PipelineRunsService pipelineRunsService) { } @Override + @SuppressWarnings("java:S2259") // suppress warning for possible NPE when unboxing quotaConsumed, + // since we do validate that quotaConsumed is not null in `validateRequiredEntries` public StepResult doStep(FlightContext flightContext) { // validate and extract parameters from input map var inputParameters = flightContext.getInputParameters(); From 7fd29a3e86053933e39b019e02e0475cdb7059f8 Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Thu, 19 Dec 2024 15:28:39 -0500 Subject: [PATCH 27/28] wait up to 30 seconds to retrieve pubsub message publishing result --- .../notifications/PubsubService.java | 55 ++++++++----------- 1 file changed, 22 insertions(+), 33 deletions(-) diff --git a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java index ab747539..c07e8512 100644 --- a/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java +++ b/service/src/main/java/bio/terra/pipelines/notifications/PubsubService.java @@ -1,15 +1,13 @@ package bio.terra.pipelines.notifications; import com.google.api.core.ApiFuture; -import com.google.api.core.ApiFutureCallback; -import com.google.api.core.ApiFutures; import com.google.api.gax.rpc.ApiException; import com.google.cloud.pubsub.v1.Publisher; -import com.google.common.util.concurrent.MoreExecutors; import com.google.protobuf.ByteString; import com.google.pubsub.v1.PubsubMessage; import com.google.pubsub.v1.TopicName; import java.io.IOException; +import java.util.concurrent.TimeUnit; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.stereotype.Service; @@ -56,36 +54,27 @@ public void publishMessage(String projectId, String topicId, String message) thr // Once published, returns a server-assigned message id (unique within the topic) ApiFuture future = publisher.publish(pubsubMessage); - // Add an asynchronous callback to handle success / failure - // Registers a callback to be run when the ApiFuture's computation is complete or, if the - // computation is already complete, immediately. - ApiFutures.addCallback( - future, handleCompletedPubsubMessagePublishing(), MoreExecutors.directExecutor()); - } - - ApiFutureCallback handleCompletedPubsubMessagePublishing() { - return new ApiFutureCallback() { - - @Override - public void onFailure(Throwable throwable) { - String errorMessage = ""; - if (throwable instanceof ApiException apiException) { - // details on the API exception - errorMessage = - "Google API exception: status code %s, is retryable: %s" - .formatted(apiException.getStatusCode().getCode(), apiException.isRetryable()); - } - PubsubService.logger.error( - "Error publishing message to Google PubSub: {}; {}", - errorMessage, - throwable.getMessage()); - } - - @Override - public void onSuccess(String messageId) { - // Once published, returns server-assigned message ids (unique within the topic) - PubsubService.logger.info("Published message ID: {}", messageId); + try { + // Wait on any pending publish requests with a 30-second timeout + String messageId = future.get(30, TimeUnit.SECONDS); + logger.info("Published message ID: {}", messageId); + } catch (Exception e) { + String errorMessage; + if (e instanceof ApiException apiException) { + // details on the API exception + errorMessage = + "Google API exception: status code %s, is retryable: %s" + .formatted(apiException.getStatusCode().getCode(), apiException.isRetryable()); + logger.error( + "Error publishing message to Google PubSub: {}; {}", errorMessage, e.getMessage()); + } else if (e instanceof InterruptedException) { + errorMessage = "Thread was interrupted"; + logger.error( + "Error publishing message to Google PubSub: {}; {}", errorMessage, e.getMessage()); + Thread.currentThread().interrupt(); + } else { + logger.error("Error publishing message to Google PubSub: {}", e.getMessage()); } - }; + } } } From 6e92a56318055795385a781eaab7474c7c5209ab Mon Sep 17 00:00:00 2001 From: Morgan Taylor Date: Fri, 20 Dec 2024 11:56:22 -0500 Subject: [PATCH 28/28] improve tests by removing some any() inputs to mocks --- .../NotificationServiceTest.java | 65 ++++++++++++------- 1 file changed, 42 insertions(+), 23 deletions(-) diff --git a/service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java b/service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java index 7b37e2bd..5bd3d6ad 100644 --- a/service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java +++ b/service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java @@ -71,11 +71,6 @@ void configureAndSendPipelineRunSucceededNotification() throws IOException { quotasService.updateQuotaConsumed(userQuota, testQuotaConsumedByJob); int expectedQuotaRemaining = updatedUserQuota.getQuota() - updatedUserQuota.getQuotaConsumed(); - // success is a void method - doNothing().when(pubsubService).publishMessage(any(), any(), any()); - - notificationService.configureAndSendPipelineRunSucceededNotification(testJobId, testUserId); - String stringifiedJobSucceededNotification = objectMapper.writeValueAsString( new TeaspoonsJobSucceededNotification( @@ -87,6 +82,15 @@ void configureAndSendPipelineRunSucceededNotification() throws IOException { testQuotaConsumedByJob.toString(), String.valueOf(expectedQuotaRemaining), testUserDescription)); + // success is a void method + doNothing() + .when(pubsubService) + .publishMessage( + notificationConfiguration.projectId(), + notificationConfiguration.topicId(), + stringifiedJobSucceededNotification); + + notificationService.configureAndSendPipelineRunSucceededNotification(testJobId, testUserId); // verify that the pubsub method was called verify(pubsubService, times(1)) @@ -130,12 +134,6 @@ void configureAndSendPipelineRunFailedNotification() throws IOException { new StepResult(StepStatus.STEP_RESULT_FAILURE_FATAL, rawlsServiceApiException); when(flightContext.getResult()).thenReturn(stepResultFailedWithException); - // success is a void method - doNothing().when(pubsubService).publishMessage(any(), any(), any()); - - notificationService.configureAndSendPipelineRunFailedNotification( - testJobId, testUserId, flightContext); - String stringifiedJobFailedNotification = objectMapper.writeValueAsString( new TeaspoonsJobFailedNotification( @@ -147,6 +145,17 @@ void configureAndSendPipelineRunFailedNotification() throws IOException { notificationService.formatInstantToReadableString(writtenPipelineRun.getUpdated()), String.valueOf(expectedQuotaRemaining), testUserDescription)); + // success is a void method + doNothing() + .when(pubsubService) + .publishMessage( + notificationConfiguration.projectId(), + notificationConfiguration.topicId(), + stringifiedJobFailedNotification); + + notificationService.configureAndSendPipelineRunFailedNotification( + testJobId, testUserId, flightContext); + // verify that the pubsub method was called verify(pubsubService, times(1)) .publishMessage( @@ -172,12 +181,6 @@ void configureAndSendPipelineRunFailedNotificationNoUserQuota() throws IOExcepti new StepResult(StepStatus.STEP_RESULT_FAILURE_FATAL, rawlsServiceApiException); when(flightContext.getResult()).thenReturn(stepResultFailedWithException); - // success is a void method - doNothing().when(pubsubService).publishMessage(any(), any(), any()); - - notificationService.configureAndSendPipelineRunFailedNotification( - testJobId, testUserId, flightContext); - String stringifiedJobFailedNotification = objectMapper.writeValueAsString( new TeaspoonsJobFailedNotification( @@ -189,6 +192,17 @@ void configureAndSendPipelineRunFailedNotificationNoUserQuota() throws IOExcepti notificationService.formatInstantToReadableString(writtenPipelineRun.getUpdated()), String.valueOf(expectedQuotaRemaining), testUserDescription)); + // success is a void method + doNothing() + .when(pubsubService) + .publishMessage( + notificationConfiguration.projectId(), + notificationConfiguration.topicId(), + stringifiedJobFailedNotification); + + notificationService.configureAndSendPipelineRunFailedNotification( + testJobId, testUserId, flightContext); + // verify that the pubsub method was called verify(pubsubService, times(1)) .publishMessage( @@ -212,12 +226,6 @@ void configureAndSendPipelineRunFailedNotificationWithoutException() throws IOEx new StepResult(StepStatus.STEP_RESULT_FAILURE_FATAL); when(flightContext.getResult()).thenReturn(stepResultFailedWithoutException); - // success is a void method - doNothing().when(pubsubService).publishMessage(any(), any(), any()); - - notificationService.configureAndSendPipelineRunFailedNotification( - testJobId, testUserId, flightContext); - String stringifiedJobFailedNotification = objectMapper.writeValueAsString( new TeaspoonsJobFailedNotification( @@ -229,6 +237,17 @@ void configureAndSendPipelineRunFailedNotificationWithoutException() throws IOEx notificationService.formatInstantToReadableString(writtenPipelineRun.getUpdated()), String.valueOf(expectedQuotaRemaining), testUserDescription)); + // success is a void method + doNothing() + .when(pubsubService) + .publishMessage( + notificationConfiguration.projectId(), + notificationConfiguration.topicId(), + stringifiedJobFailedNotification); + + notificationService.configureAndSendPipelineRunFailedNotification( + testJobId, testUserId, flightContext); + // verify that the pubsub method was called verify(pubsubService, times(1)) .publishMessage(