Skip to content

Commit

Permalink
refactor(deploymentmonitor): Configure SpinnakerRetrofitErrorHandler …
Browse files Browse the repository at this point in the history 
…for DeploymentMonitorService

Update the error handler for the retrofit client, DeploymentMonitorService, to use SpinnakerRetrofitErrorHandler. Additionally, modify catch blocks with RetrofitError to use Spinnaker*Exception.

Purpose : This commit is foundational work for upgrading the Retrofit version to retrofit2.x

Behavioural change : The modification includes updating catch blocks, leading to a change in the format of logger messages. This aligns with the upcoming modifications discussed in the PR: #4608
  • Loading branch information
@Pranav-b-7
Pranav-b-7 committed Jan 2, 2024
1 parent d481b84 commit ec0c677
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 46 deletions.
39 changes: 16 additions & 23 deletions ...com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java
View file
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -52,6 +48,7 @@ public class MonitoredDeployBaseTask implements RetryableTask {
private DeploymentMonitorServiceProvider deploymentMonitorServiceProvider;
private final Map<EvaluateHealthResponse.NextStepDirective, String> summaryMapping =
new HashMap<>();
private final ObjectMapper objectMapper = new ObjectMapper();

MonitoredDeployBaseTask(
DeploymentMonitorServiceProvider deploymentMonitorServiceProvider, Registry registry) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 "<NO RESPONSE>";
}

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) {
Expand Down
11 changes: 6 additions & 5 deletions .../spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy
View file
Expand Up @@ -19,6 +19,8 @@ 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
Expand Down Expand Up @@ -46,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()))
}
}

Expand Down Expand Up @@ -203,8 +205,7 @@ class EvaluateDeploymentHealthTaskSpec extends Specification {
false | null || ExecutionStatus.FAILED_CONTINUE
}

def "should return status as RUNNING when Retrofit http error is thrown"() {

def "should return status as RUNNING when SpinnakerHttpException is thrown"() {
def converter = new JacksonConverter(new ObjectMapper())

Response response =
Expand All @@ -224,7 +225,7 @@ class EvaluateDeploymentHealthTaskSpec extends Specification {
given:
def monitorServiceStub = Stub(DeploymentMonitorService) {
evaluateHealth(_) >> {
throw RetrofitError.httpError("https://foo.com/deployment/evaluateHealth", response, converter, null)
throw new SpinnakerHttpException(RetrofitError.httpError("https://foo.com/deployment/evaluateHealth", response, converter, null))
}
}

Expand Down
43 changes: 25 additions & 18 deletions ...netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTaskTest.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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("<NO RESPONSE>");
}

@Test
Expand All @@ -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("<NO RESPONSE>");
assertThat(logMessageOnSpinNetworkException).isEqualTo("<NO RESPONSE>");
}

@Test
Expand All @@ -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("<NO RESPONSE>");
assertThat(logMessageOnSpinServerException).isEqualTo("<NO RESPONSE>");
}

static class MockTypedInput implements TypedInput {
Expand Down
1 change: 1 addition & 0 deletions orca-deploymentmonitor/orca-deploymentmonitor.gradle
View file
Expand Up @@ -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")

Expand Down
2 changes: 2 additions & 0 deletions ...n/java/com/netflix/spinnaker/orca/deploymentmonitor/DeploymentMonitorServiceProvider.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand Down

0 comments on commit ec0c677

Please sign in to comment.