From 5f4b2edbe72ee5d7003edb71e7477592c1e928b0 Mon Sep 17 00:00:00 2001 From: Mark Vulfson Date: Sun, 9 Aug 2020 13:59:36 -0700 Subject: [PATCH] fix(exceptions): add exception handler for exceptional situations (#3845) * fix(exceptions): add exceptionhandler for non-excpetional situations When a user submits a bad pipeline e.g. * with jenkins trigger that has an invalid properties file or job that doesn't exist) * with a broken pipeline template we don't want to 500 because: a) the calling service (echo in most cases here) will keep retrying which makes no sense b) it's an erroneous HTTP 500 metric that is noise * fix(exceptions): fix various exception issues 1. Don't log task failures on `UserException` 2. Don't fail retrieving properties file if the build wasn't started * fixup! Merge branch 'master' into mark/exceptionhandlers Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- .../orca/pipeline/ExecutionLauncher.java | 11 ++++++++++- .../orca/igor/tasks/GetBuildPropertiesTask.java | 16 +++++++++++++++- .../orca/igor/tasks/RetryableIgorTask.java | 10 ++++++---- .../igor/tasks/GetBuildPropertiesTaskSpec.groovy | 3 ++- .../spinnaker/orca/q/handler/RunTaskHandler.kt | 8 ++++++-- .../orca/controllers/OperationsController.groovy | 7 +++---- .../V2PipelineTemplateController.java | 5 +---- .../exceptions/OperationFailedException.java | 4 +++- 8 files changed, 46 insertions(+), 18 deletions(-) diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/ExecutionLauncher.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/ExecutionLauncher.java index a659e4e117..a8eb04d51b 100644 --- a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/ExecutionLauncher.java +++ b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/ExecutionLauncher.java @@ -24,6 +24,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.netflix.spectator.api.Registry; +import com.netflix.spinnaker.kork.exceptions.UserException; import com.netflix.spinnaker.kork.web.exceptions.ValidationException; import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus; import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionType; @@ -181,7 +182,15 @@ private PipelineExecution handleStartupFailure(PipelineExecution execution, Thro } } - log.error("Failed to start {} {}", execution.getType(), execution.getId(), failure); + if (failure instanceof UserException) { + log.warn( + "Failed to start {} {} due to user error or misconfiguration", + execution.getType(), + execution.getId(), + failure); + } else { + log.error("Failed to start {} {}", execution.getType(), execution.getId(), failure); + } executionRepository.updateStatus(execution.getType(), execution.getId(), status); executionRepository.cancel(execution.getType(), execution.getId(), canceledBy, reason); return executionRepository.retrieve(execution.getType(), execution.getId()); diff --git a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTask.java b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTask.java index 1f85fdcbe0..cd97cdc26d 100644 --- a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTask.java +++ b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTask.java @@ -16,6 +16,8 @@ package com.netflix.spinnaker.orca.igor.tasks; +import com.netflix.spinnaker.kork.exceptions.ConfigurationException; +import com.netflix.spinnaker.kork.exceptions.UserException; import com.netflix.spinnaker.orca.api.pipeline.TaskResult; import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus; import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution; @@ -41,6 +43,18 @@ public class GetBuildPropertiesTask extends RetryableIgorTask return TaskResult.SUCCEEDED; } + if ((stageDefinition.getBuildNumber() == null)) { + throw new UserException( + "Can't retrieve property file because the build number is not available, check for build start failures"); + } + + if ((stageDefinition.getPropertyFile() == null) + || (stageDefinition.getMaster() == null) + || (stageDefinition.getJob() == null)) { + throw new UserException( + "Can't retrieve property file: propertyFile, master, and job can't be null, check for build start failures"); + } + Map properties = buildService.getPropertyFile( stageDefinition.getBuildNumber(), @@ -48,7 +62,7 @@ public class GetBuildPropertiesTask extends RetryableIgorTask stageDefinition.getMaster(), stageDefinition.getJob()); if (properties.size() == 0) { - throw new IllegalStateException( + throw new ConfigurationException( String.format( "Expected properties file %s but it was either missing, empty or contained invalid syntax", stageDefinition.getPropertyFile())); diff --git a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTask.java b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTask.java index eae40904cd..97e004b198 100644 --- a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTask.java +++ b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTask.java @@ -89,10 +89,12 @@ private boolean isRetryable(RetrofitError retrofitError) { return true; } - int status = retrofitError.getResponse().getStatus(); - if (status == 500 || status == 503) { - log.warn(String.format("Received HTTP %s response from igor, retrying...", status)); - return true; + if (retrofitError.getResponse() != null) { + int status = retrofitError.getResponse().getStatus(); + if (status == 500 || status == 503) { + log.warn(String.format("Received HTTP %s response from igor, retrying...", status)); + return true; + } } return false; } diff --git a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTaskSpec.groovy index 788d61dfb2..b4e7acde2a 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTaskSpec.groovy +++ b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTaskSpec.groovy @@ -18,6 +18,7 @@ package com.netflix.spinnaker.orca.igor.tasks import com.fasterxml.jackson.databind.ObjectMapper import com.netflix.spinnaker.kork.artifacts.model.Artifact +import com.netflix.spinnaker.kork.exceptions.ConfigurationException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.api.pipeline.TaskResult import com.netflix.spinnaker.orca.igor.BuildService @@ -141,7 +142,7 @@ class GetBuildPropertiesTaskSpec extends Specification { task.execute(stage) then: - IllegalStateException e = thrown IllegalStateException + ConfigurationException e = thrown ConfigurationException e.message == "Expected properties file $PROPERTY_FILE but it was either missing, empty or contained invalid syntax" } 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 index 1add4b22f4..d4e486798f 100644 --- 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 @@ -170,8 +170,12 @@ class RunTaskHandler( } else if (e is TimeoutException && stage.context["markSuccessfulOnTimeout"] == true) { queue.push(CompleteTask(message, SUCCEEDED)) } else { - if (e !is TimeoutException && e !is UserException) { - log.error("Error running ${message.taskType.simpleName} for ${message.executionType}[${message.executionId}]", e) + if (e !is TimeoutException) { + if (e is UserException) { + log.warn("${message.taskType.simpleName} for ${message.executionType}[${message.executionId}] failed, likely due to user error", e) + } else { + log.error("Error running ${message.taskType.simpleName} for ${message.executionType}[${message.executionId}]", e) + } } val status = stage.failureStatus(default = TERMINAL) diff --git a/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy b/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy index 188bfc5065..1b38a00193 100644 --- a/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy +++ b/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy @@ -23,8 +23,7 @@ import com.netflix.spinnaker.fiat.shared.FiatService import com.netflix.spinnaker.fiat.shared.FiatStatus import com.netflix.spinnaker.kork.exceptions.ConfigurationException import com.netflix.spinnaker.kork.exceptions.SpinnakerException -import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException -import com.netflix.spinnaker.kork.web.exceptions.ValidationException +import com.netflix.spinnaker.kork.exceptions.UserException import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution import com.netflix.spinnaker.orca.clouddriver.service.JobService import com.netflix.spinnaker.orca.exceptions.OperationFailedException @@ -330,7 +329,7 @@ class OperationsController { buildInfo = buildService.getBuild(trigger.buildNumber, trigger.master, trigger.job) } catch (RetrofitError e) { if (e.response?.status == 404) { - throw new IllegalStateException("Build ${trigger.buildNumber} of ${trigger.master}/${trigger.job} not found") + throw new ConfigurationException("Build ${trigger.buildNumber} of ${trigger.master}/${trigger.job} not found") } else { throw new OperationFailedException("Failed to get build ${trigger.buildNumber} of ${trigger.master}/${trigger.job}", e) } @@ -351,7 +350,7 @@ class OperationsController { ) } catch (RetrofitError e) { if (e.response?.status == 404) { - throw new IllegalStateException("Expected properties file " + trigger.propertyFile + " (configured on trigger), but it was missing") + throw new ConfigurationException("Expected properties file " + trigger.propertyFile + " (configured on trigger), but it was missing") } else { throw new OperationFailedException("Failed to get properties file ${trigger.propertyFile}", e) } diff --git a/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/V2PipelineTemplateController.java b/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/V2PipelineTemplateController.java index 81c21b447f..a51c2e6949 100644 --- a/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/V2PipelineTemplateController.java +++ b/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/V2PipelineTemplateController.java @@ -25,10 +25,7 @@ import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression; -import org.springframework.web.bind.annotation.RequestBody; -import org.springframework.web.bind.annotation.RequestMapping; -import org.springframework.web.bind.annotation.RequestMethod; -import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.bind.annotation.*; @RestController @RequestMapping(value = "/v2/pipelineTemplates") diff --git a/orca-web/src/main/groovy/com/netflix/spinnaker/orca/exceptions/OperationFailedException.java b/orca-web/src/main/groovy/com/netflix/spinnaker/orca/exceptions/OperationFailedException.java index ad2e4399ca..05493069c5 100644 --- a/orca-web/src/main/groovy/com/netflix/spinnaker/orca/exceptions/OperationFailedException.java +++ b/orca-web/src/main/groovy/com/netflix/spinnaker/orca/exceptions/OperationFailedException.java @@ -1,6 +1,8 @@ package com.netflix.spinnaker.orca.exceptions; -public class OperationFailedException extends RuntimeException { +import com.netflix.spinnaker.kork.exceptions.IntegrationException; + +public class OperationFailedException extends IntegrationException { public OperationFailedException(String message) { super(message); }