fix(front50): teach MonitorPipelineTask to handle missing/null execut…

…ion ids (#4555) give the task terminal status in this case since that feels closer to the truth than succeeded even though the error is likely elsewhere (e.g. in a StartPipelineTask).
spinnaker · Oct 9, 2023 · 7523f5d · 7523f5d
1 parent cd9781f
commit 7523f5d
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 3 deletions.
diff --git a/...ont50/src/main/groovy/com/netflix/spinnaker/orca/front50/tasks/MonitorPipelineTask.groovy b/...ont50/src/main/groovy/com/netflix/spinnaker/orca/front50/tasks/MonitorPipelineTask.groovy
@@ -57,12 +57,20 @@ class MonitorPipelineTask implements OverridableTimeoutRetryableTask {
     MonitorPipelineStage.StageParameters stageData = stage.mapTo(MonitorPipelineStage.StageParameters.class)
 
     if (stage.type == MonitorPipelineStage.PIPELINE_CONFIG_TYPE) {
-      pipelineIds = stageData.executionIds
+      // Use - null to remove null elements
+      pipelineIds = stageData.executionIds ? stageData.executionIds - null : Collections.emptyList()
     } else {
-      pipelineIds = Collections.singletonList(stageData.executionId)
+      pipelineIds = stageData.executionId ? Collections.singletonList(stageData.executionId) : Collections.emptyList()
       isLegacyStage = true
     }
 
+    if (pipelineIds.size() == 0) {
+      String message = "no pipeline execution ids to monitor";
+      log.info(message)
+      stage.appendErrorMessage(message)
+      return TaskResult.ofStatus(ExecutionStatus.TERMINAL)
+    }
+
     HashMap<String, MonitorPipelineStage.ChildPipelineStatusDetails> pipelineStatuses = new HashMap<>(pipelineIds.size())
     PipelineExecution firstPipeline
 

diff --git a/...0/src/test/groovy/com/netflix/spinnaker/orca/front50/tasks/MonitorPipelineTaskSpec.groovy b/...0/src/test/groovy/com/netflix/spinnaker/orca/front50/tasks/MonitorPipelineTaskSpec.groovy
@@ -28,6 +28,7 @@ import spock.lang.Specification
 import spock.lang.Subject
 import spock.lang.Unroll
 
+import static com.netflix.spinnaker.orca.api.pipeline.models.ExecutionType.PIPELINE
 import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.pipeline
 import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.pipeline
 import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.pipeline
@@ -36,13 +37,14 @@ import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.stage
 class MonitorPipelineTaskSpec extends Specification {
 
   ExecutionRepository repo = Mock(ExecutionRepository)
-  StageExecutionImpl stage = new StageExecutionImpl(type: "whatever")
+  StageExecutionImpl stage = new StageExecutionImpl()
 
   @Subject
   MonitorPipelineTask task = new MonitorPipelineTask(repo, new ObjectMapper())
 
   def setup() {
     stage.context.executionId = 'abc'
+    stage.type = 'whatever'
   }
 
   @Unroll
@@ -309,4 +311,40 @@ class MonitorPipelineTaskSpec extends Specification {
     MonitorPipelineStage.MonitorBehavior.FailFast               || ExecutionStatus.TERMINAL
     MonitorPipelineStage.MonitorBehavior.WaitForAllToComplete   || ExecutionStatus.RUNNING
   }
+
+  @Unroll
+  def "handles a missing execution id (stage type: #stageType)"() {
+    given:
+    stage.context.remove('executionId')
+    stage.type = stageType
+
+    when:
+    task.execute(stage)
+
+    then:
+    noExceptionThrown()
+    0 * repo.retrieve(PIPELINE, _)
+
+    where:
+    stageType << [ MonitorPipelineStage.PIPELINE_CONFIG_TYPE, PipelineStage.PIPELINE_CONFIG_TYPE ]
+  }
+
+  @Unroll
+  def "handles a null execution id (stage type: #stageType)"() {
+    given:
+    stage.context.executionId = null
+    stage.context.executionIds = [null]
+    stage.type = stageType
+
+    when:
+    task.execute(stage)
+
+    then:
+    noExceptionThrown()
+    0 * repo.retrieve(PIPELINE, _)
+
+    where:
+    stageType << [ MonitorPipelineStage.PIPELINE_CONFIG_TYPE, PipelineStage.PIPELINE_CONFIG_TYPE ]
+  }
+
 }