diff --git a/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java b/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java index 373012c87ae..d3e8e3067a9 100644 --- a/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java +++ b/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java @@ -16,10 +16,12 @@ package com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy; -import com.google.common.io.CharStreams; +import com.fasterxml.jackson.databind.ObjectMapper; import com.netflix.spectator.api.Registry; import com.netflix.spinnaker.config.DeploymentMonitorDefinition; import com.netflix.spinnaker.kork.annotations.VisibleForTesting; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException; import com.netflix.spinnaker.orca.api.pipeline.RetryableTask; import com.netflix.spinnaker.orca.api.pipeline.TaskResult; import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus; @@ -31,19 +33,13 @@ import com.netflix.spinnaker.orca.deploymentmonitor.models.EvaluateHealthResponse; import com.netflix.spinnaker.orca.deploymentmonitor.models.MonitoredDeployInternalStageData; import com.netflix.spinnaker.orca.deploymentmonitor.models.StatusExplanation; -import java.io.InputStreamReader; -import java.nio.charset.StandardCharsets; import java.time.Duration; import java.util.*; import java.util.concurrent.TimeUnit; -import java.util.stream.Collectors; import javax.annotation.Nonnull; import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import retrofit.RetrofitError; -import retrofit.client.Header; -import retrofit.client.Response; public class MonitoredDeployBaseTask implements RetryableTask { static final int MAX_RETRY_COUNT = 3; @@ -52,6 +48,7 @@ public class MonitoredDeployBaseTask implements RetryableTask { private DeploymentMonitorServiceProvider deploymentMonitorServiceProvider; private final Map summaryMapping = new HashMap<>(); + private final ObjectMapper objectMapper = new ObjectMapper(); MonitoredDeployBaseTask( DeploymentMonitorServiceProvider deploymentMonitorServiceProvider, Registry registry) { @@ -138,12 +135,12 @@ public long getDynamicTimeout(StageExecution stage) { try { return executeInternal(stage, monitorDefinition); - } catch (RetrofitError e) { + } catch (SpinnakerServerException e) { log.warn( "HTTP Error encountered while talking to {}->{}, {}}", monitorDefinition, e.getUrl(), - getRetrofitLogMessage(e.getResponse()), + getErrorMessage(e), e); return handleError(stage, e, true, monitorDefinition); @@ -269,28 +266,24 @@ private MonitoredDeployStageData getStageContext(StageExecution stage) { } @VisibleForTesting - String getRetrofitLogMessage(Response response) { - if (response == null) { + String getErrorMessage(SpinnakerServerException spinnakerException) { + if (!(spinnakerException instanceof SpinnakerHttpException)) { return ""; } - String body = ""; - String status = ""; - String headers = ""; + SpinnakerHttpException httpException = (SpinnakerHttpException) spinnakerException; try { - status = String.format("%d (%s)", response.getStatus(), response.getReason()); - body = - CharStreams.toString( - new InputStreamReader(response.getBody().in(), StandardCharsets.UTF_8)); - headers = - response.getHeaders().stream().map(Header::toString).collect(Collectors.joining("\n")); + String body = ""; + if (httpException.getResponseBody() != null) { + body = objectMapper.writeValueAsString(httpException.getResponseBody()); + } + return String.format("headers: %s\nresponse body: %s", httpException.getHeaders(), body); } catch (Exception e) { - log.error( - "Failed to fully parse retrofit error while reading response from deployment monitor", e); + log.error("Failed to fully read http error response details", e); } - return String.format("status: %s\nheaders: %s\nresponse body: %s", status, headers, body); + return "headers: \nresponse body: "; } private boolean shouldFailOnError(StageExecution stage, DeploymentMonitorDefinition definition) { diff --git a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy index b26591e1d72..111a85a8017 100644 --- a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy +++ b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy @@ -16,10 +16,14 @@ package com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy +import com.fasterxml.jackson.databind.ObjectMapper import com.netflix.spectator.api.NoopRegistry import com.netflix.spinnaker.config.DeploymentMonitorDefinition +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.api.pipeline.TaskResult +import com.netflix.spinnaker.orca.clouddriver.MortServiceSpec import com.netflix.spinnaker.orca.deploymentmonitor.DeploymentMonitorService import com.netflix.spinnaker.orca.deploymentmonitor.models.DeploymentMonitorStageConfig import com.netflix.spinnaker.orca.deploymentmonitor.models.DeploymentStep @@ -27,7 +31,10 @@ import com.netflix.spinnaker.orca.deploymentmonitor.models.EvaluateHealthRespons import com.netflix.spinnaker.orca.deploymentmonitor.models.MonitoredDeployInternalStageData import com.netflix.spinnaker.orca.pipeline.model.PipelineExecutionImpl import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl +import org.springframework.http.HttpStatus import retrofit.RetrofitError +import retrofit.client.Response +import retrofit.converter.JacksonConverter import spock.lang.Specification import com.netflix.spinnaker.orca.deploymentmonitor.DeploymentMonitorServiceProvider import spock.lang.Unroll @@ -41,11 +48,11 @@ class EvaluateDeploymentHealthTaskSpec extends Specification { PipelineExecutionImpl pipe = pipeline { } - def "should retry retrofit errors"() { + def "should retry on SpinnakerServerException"() { given: def monitorServiceStub = Stub(DeploymentMonitorService) { evaluateHealth(_) >> { - throw RetrofitError.networkError("url", new IOException()) + throw new SpinnakerServerException(RetrofitError.networkError("url", new IOException())) } } @@ -198,6 +205,50 @@ class EvaluateDeploymentHealthTaskSpec extends Specification { false | null || ExecutionStatus.FAILED_CONTINUE } + def "should return status as RUNNING when SpinnakerHttpException is thrown"() { + + def converter = new JacksonConverter(new ObjectMapper()) + + Response response = + new Response( + "/deployment/evaluateHealth", + HttpStatus.BAD_REQUEST.value(), + "bad-request", + Collections.emptyList(), + new MortServiceSpec.MockTypedInput(converter, [ + accountName: "account", + description: "simple description", + name: "sg1", + region: "region", + type: "openstack" + ])) + + given: + def monitorServiceStub = Stub(DeploymentMonitorService) { + evaluateHealth(_) >> { + throw new SpinnakerHttpException(RetrofitError.httpError("https://foo.com/deployment/evaluateHealth", response, converter, null)) + } + } + + def serviceProviderStub = getServiceProviderStub(monitorServiceStub) + + def task = new EvaluateDeploymentHealthTask(serviceProviderStub, new NoopRegistry()) + + MonitoredDeployInternalStageData stageData = new MonitoredDeployInternalStageData() + stageData.deploymentMonitor = new DeploymentMonitorStageConfig() + stageData.deploymentMonitor.id = "LogMonitorId" + + def stage = new StageExecutionImpl(pipe, "evaluateDeploymentHealth", stageData.toContextMap() + [application: pipe.application]) + stage.startTime = Instant.now().toEpochMilli() + + when: 'we can still retry' + TaskResult result = task.execute(stage) + + then: 'should retry' + result.status == ExecutionStatus.RUNNING + result.context.deployMonitorHttpRetryCount == 1 + } + private getServiceProviderStub(monitorServiceStub) { return getServiceProviderStub(monitorServiceStub, {}) } diff --git a/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTaskTest.java b/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTaskTest.java index 784241c377d..1297c9d6ad1 100644 --- a/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTaskTest.java +++ b/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTaskTest.java @@ -21,6 +21,10 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.netflix.spectator.api.NoopRegistry; import com.netflix.spinnaker.config.DeploymentMonitorDefinition; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerConversionException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException; import com.netflix.spinnaker.orca.deploymentmonitor.DeploymentMonitorServiceProvider; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; @@ -93,14 +97,14 @@ public void shouldParseHttpErrorResponseDetailsWhenHttpErrorHasOccurred() { RetrofitError.httpError( "https://foo.com/deployment/evaluateHealth", response, new JacksonConverter(), null); - String logMessageOnHttpError = - monitoredDeployBaseTask.getRetrofitLogMessage(httpError.getResponse()); - String status = HttpStatus.BAD_REQUEST.value() + " (" + HttpStatus.BAD_REQUEST.name() + ")"; + SpinnakerHttpException httpException = new SpinnakerHttpException(httpError); + + String logMessageOnSpinHttpException = monitoredDeployBaseTask.getErrorMessage(httpException); String body = "{\"error\":\"400 - Bad request, application name cannot be empty\"}"; - assertThat(logMessageOnHttpError) + assertThat(logMessageOnSpinHttpException) .isEqualTo( - String.format("status: %s\nheaders: %s\nresponse body: %s", status, header, body)); + String.format("headers: %s\nresponse body: %s", httpException.getHeaders(), body)); } @Test @@ -130,14 +134,13 @@ public void shouldParseHttpErrorResponseDetailsWhenConversionErrorHasOccurred() null, new ConversionException("Failed to parse response")); - String status = HttpStatus.BAD_REQUEST.value() + " (" + HttpStatus.BAD_REQUEST.name() + ")"; - String body = "{\"error\":\"400 - Bad request, application name cannot be empty\"}"; - String logMessageOnConversionError = - monitoredDeployBaseTask.getRetrofitLogMessage(conversionError.getResponse()); + SpinnakerConversionException conversionException = + new SpinnakerConversionException(conversionError); - assertThat(logMessageOnConversionError) - .isEqualTo( - String.format("status: %s\nheaders: %s\nresponse body: %s", status, header, body)); + String logMessageOnSpinConversionException = + monitoredDeployBaseTask.getErrorMessage(conversionException); + + assertThat(logMessageOnSpinConversionException).isEqualTo(""); } @Test @@ -148,10 +151,12 @@ void shouldReturnDefaultLogMsgWhenNetworkErrorHasOccurred() { "https://foo.com/deployment/evaluateHealth", new IOException("Failed to connect to the host : foo.com")); - String logMessageOnNetworkError = - monitoredDeployBaseTask.getRetrofitLogMessage(networkError.getResponse()); + SpinnakerNetworkException networkException = new SpinnakerNetworkException(networkError); + + String logMessageOnSpinNetworkException = + monitoredDeployBaseTask.getErrorMessage(networkException); - assertThat(logMessageOnNetworkError).isEqualTo(""); + assertThat(logMessageOnSpinNetworkException).isEqualTo(""); } @Test @@ -162,10 +167,12 @@ void shouldReturnDefaultLogMsgWhenUnexpectedErrorHasOccurred() { "https://foo.com/deployment/evaluateHealth", new IOException("Failed to connect to the host : foo.com")); - String logMessageOnUnexpectedError = - monitoredDeployBaseTask.getRetrofitLogMessage(unexpectedError.getResponse()); + SpinnakerServerException serverException = new SpinnakerServerException(unexpectedError); + + String logMessageOnSpinServerException = + monitoredDeployBaseTask.getErrorMessage(serverException); - assertThat(logMessageOnUnexpectedError).isEqualTo(""); + assertThat(logMessageOnSpinServerException).isEqualTo(""); } static class MockTypedInput implements TypedInput { diff --git a/orca-deploymentmonitor/orca-deploymentmonitor.gradle b/orca-deploymentmonitor/orca-deploymentmonitor.gradle index 14491fc63f8..8c940777bb7 100644 --- a/orca-deploymentmonitor/orca-deploymentmonitor.gradle +++ b/orca-deploymentmonitor/orca-deploymentmonitor.gradle @@ -19,6 +19,7 @@ apply from: "$rootDir/gradle/spock.gradle" dependencies { implementation(project(":orca-core")) implementation(project(":orca-retrofit")) + implementation("io.spinnaker.kork:kork-retrofit") implementation("org.springframework.boot:spring-boot-autoconfigure") diff --git a/orca-deploymentmonitor/src/main/java/com/netflix/spinnaker/orca/deploymentmonitor/DeploymentMonitorServiceProvider.java b/orca-deploymentmonitor/src/main/java/com/netflix/spinnaker/orca/deploymentmonitor/DeploymentMonitorServiceProvider.java index f3bcd02c225..f4a6a7750eb 100644 --- a/orca-deploymentmonitor/src/main/java/com/netflix/spinnaker/orca/deploymentmonitor/DeploymentMonitorServiceProvider.java +++ b/orca-deploymentmonitor/src/main/java/com/netflix/spinnaker/orca/deploymentmonitor/DeploymentMonitorServiceProvider.java @@ -18,6 +18,7 @@ import com.netflix.spinnaker.config.DeploymentMonitorDefinition; import com.netflix.spinnaker.kork.exceptions.UserException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler; import com.netflix.spinnaker.orca.retrofit.logging.RetrofitSlf4jLog; import java.util.HashMap; import java.util.List; @@ -81,6 +82,7 @@ private synchronized DeploymentMonitorService getServiceByDefinition( .setLogLevel(retrofitLogLevel) .setLog(new RetrofitSlf4jLog(DeploymentMonitorService.class)) .setConverter(new JacksonConverter()) + .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) .build() .create(DeploymentMonitorService.class);