fix(exceptions): Don't let errors in trackResult leak out (spinnake…

…r#3843) It's possible for trackResult metrics call to fail and we don't want that failure to leak out and fail orca tasks Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Kaixiang-AWS · Aug 6, 2020 · 9aa0b3c · 9aa0b3c
1 parent 3fa29ca
commit 9aa0b3c
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 22 deletions.
diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/clouddriver/utils/CloudProviderAware.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/clouddriver/utils/CloudProviderAware.java
@@ -58,9 +58,12 @@ default String getCredentials(Map<String, Object> context) {
 
   // may return a list with 0, 1 or more regions (no guarantees on the ordering)
   default List<String> getRegions(Map<String, Object> context) {
-    String region = (String) context.getOrDefault("region", null);
+    Object region = context.getOrDefault("region", null);
     if (region != null) {
-      return ImmutableList.of(region);
+      if (region instanceof List) {
+        return ImmutableList.copyOf((List<String>) region);
+      }
+      return ImmutableList.of(region.toString());
     }
 
     try {

diff --git a/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/CompleteTaskHandler.kt b/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/CompleteTaskHandler.kt
@@ -39,6 +39,7 @@ import com.netflix.spinnaker.orca.q.StartTask
 import com.netflix.spinnaker.orca.q.get
 import com.netflix.spinnaker.orca.q.metrics.MetricsTagHelper
 import com.netflix.spinnaker.q.Queue
+import java.lang.Exception
 import java.time.Clock
 import java.util.concurrent.TimeUnit
 import org.springframework.beans.factory.annotation.Qualifier
@@ -118,18 +119,21 @@ class CompleteTaskHandler(
   }
 
   private fun trackResult(stage: StageExecution, taskModel: TaskExecution, status: ExecutionStatus) {
-    val commonTags = MetricsTagHelper.commonTags(stage, taskModel, status)
-    val detailedTags = MetricsTagHelper.detailedTaskTags(stage, taskModel, status)
+    try {
+      val commonTags = MetricsTagHelper.commonTags(stage, taskModel, status)
+      val detailedTags = MetricsTagHelper.detailedTaskTags(stage, taskModel, status)
 
-    // we are looking at the time it took to complete the whole execution, not just one invocation
-    val elapsedMillis = clock.millis() - (taskModel.startTime ?: 0)
+      // we are looking at the time it took to complete the whole execution, not just one invocation
+      val elapsedMillis = clock.millis() - (taskModel.startTime ?: 0)
 
-    hashMapOf(
-      "task.completions.duration" to commonTags + BasicTag("application", stage.execution.application),
-      "task.completions.duration.withType" to commonTags + detailedTags
-    ).forEach {
-      name, tags ->
-      registry.timer(name, tags).record(elapsedMillis, TimeUnit.MILLISECONDS)
+      hashMapOf(
+        "task.completions.duration" to commonTags + BasicTag("application", stage.execution.application),
+        "task.completions.duration.withType" to commonTags + detailedTags
+      ).forEach { name, tags ->
+        registry.timer(name, tags).record(elapsedMillis, TimeUnit.MILLISECONDS)
+      }
+    } catch (e: Exception) {
+      log.warn("Failed to track result for stage: ${stage.id}, task: ${taskModel.id}", e)
     }
   }
 }
diff --git a/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/RunTaskHandler.kt b/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/RunTaskHandler.kt
@@ -19,6 +19,7 @@ package com.netflix.spinnaker.orca.q.handler
 import com.netflix.spectator.api.BasicTag
 import com.netflix.spectator.api.Registry
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
+import com.netflix.spinnaker.kork.exceptions.UserException
 import com.netflix.spinnaker.orca.TaskExecutionInterceptor
 import com.netflix.spinnaker.orca.TaskResolver
 import com.netflix.spinnaker.orca.api.pipeline.OverridableTimeoutRetryableTask
@@ -169,7 +170,7 @@ class RunTaskHandler(
             } else if (e is TimeoutException && stage.context["markSuccessfulOnTimeout"] == true) {
               queue.push(CompleteTask(message, SUCCEEDED))
             } else {
-              if (e !is TimeoutException) {
+              if (e !is TimeoutException && e !is UserException) {
                 log.error("Error running ${message.taskType.simpleName} for ${message.executionType}[${message.executionId}]", e)
               }
 
@@ -186,17 +187,20 @@ class RunTaskHandler(
   }
 
   private fun trackResult(stage: StageExecution, thisInvocationStartTimeMs: Long, taskModel: TaskExecution, status: ExecutionStatus) {
-    val commonTags = MetricsTagHelper.commonTags(stage, taskModel, status)
-    val detailedTags = MetricsTagHelper.detailedTaskTags(stage, taskModel, status)
+    try {
+      val commonTags = MetricsTagHelper.commonTags(stage, taskModel, status)
+      val detailedTags = MetricsTagHelper.detailedTaskTags(stage, taskModel, status)
 
-    val elapsedMillis = clock.millis() - thisInvocationStartTimeMs
+      val elapsedMillis = clock.millis() - thisInvocationStartTimeMs
 
-    hashMapOf(
-      "task.invocations.duration" to commonTags + BasicTag("application", stage.execution.application),
-      "task.invocations.duration.withType" to commonTags + detailedTags
-    ).forEach {
-      name, tags ->
-      registry.timer(name, tags).record(elapsedMillis, TimeUnit.MILLISECONDS)
+      hashMapOf(
+        "task.invocations.duration" to commonTags + BasicTag("application", stage.execution.application),
+        "task.invocations.duration.withType" to commonTags + detailedTags
+      ).forEach { name, tags ->
+        registry.timer(name, tags).record(elapsedMillis, TimeUnit.MILLISECONDS)
+      }
+    } catch (e: java.lang.Exception) {
+      log.warn("Failed to track result for stage: ${stage.id}, task: ${taskModel.id}", e)
     }
   }