Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(deploymentmonitor): Configure SpinnakerRetrofitErrorHandler for DeploymentMonitorService #4614

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

refactor(deploymentmonitor): Configure SpinnakerRetrofitErrorHandler for DeploymentMonitorService #4614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
11438e9
test(deploymentmonitor): Add Test Case for EvaluateDeploymentTask
Pranav-b-7 Jan 2, 2024
52e528f
refactor(deploymentmonitor): Configure SpinnakerRetrofitErrorHandler …
Pranav-b-7 Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 18 additions & 23 deletions ...com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
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<>();
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,26 @@ 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());
} else {
body = "Failed to serialize the error response body";
}
return String.format("headers: %s\nresponse body: %s", httpException.getHeaders(), body);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somehow nervous about including an empty body when there might not be one. If the response was present, but wasn't json, I'd like to make that clear somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even before this PR, the code would still return empty body headers and status, in case of any parse/read failures.

If we are planning to change this behaviour, I think we have to add a test case before this modifications to demonstrate the behavioural change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't about parse/read failures though. This is about a non-json response. We're guaranteed a behavior change here.

Copy link
Contributor Author

@Pranav-b-7 Pranav-b-7 Jan 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new specific exception handler is introduced to manage JsonProcessing during the serialization of the HTTP error body.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm talking about something different. With SpinnakerHttpException, getResponseBody is null if the response isn't json whereas RetrofitError provided access to the response body in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes added : when the error response body is null, an appropriate error message will be printed in the logger.

} catch (Exception e) {
log.error(
"Failed to fully parse retrofit error while reading response from deployment monitor", e);
log.error("Failed to fully serialize 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
54 changes: 52 additions & 2 deletions .../spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,25 @@

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
import com.netflix.spinnaker.orca.deploymentmonitor.models.EvaluateHealthResponse
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
Expand All @@ -41,11 +48,11 @@ class EvaluateDeploymentHealthTaskSpec extends Specification {
PipelineExecutionImpl pipe = pipeline {
}

def "should retry retrofit errors"() {
def "should retry on SpinnakerServerException"() {
given:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though underlying exceptions at present are retrofit, wouldn't it be meaningful to change "retrofit errors" to SpinnakerServerExceptions in the test name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats true.. I have modified the test name to : should retry on SpinnakerServerException

def monitorServiceStub = Stub(DeploymentMonitorService) {
evaluateHealth(_) >> {
throw RetrofitError.networkError("url", new IOException())
throw new SpinnakerServerException(RetrofitError.networkError("url", new IOException()))
}
}

Expand Down Expand Up @@ -198,6 +205,49 @@ class EvaluateDeploymentHealthTaskSpec extends Specification {
false | null || ExecutionStatus.FAILED_CONTINUE
}

def "should return status as RUNNING when SpinnakerHttpException is thrown"() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible to add this test before the code change so we can see any change in behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is no change in the behaviour here. This test case is to verify if the execute() method returns status as RUNNING when SpinnakerHttpException is thrown. This was the same case even with RetrofitError as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without having the test before the code change, it's hard to see that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With reference to this comment : #4617 (comment) , moved this test case to a separate commit for a better understanding .

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, {})
}
Expand Down
80 changes: 61 additions & 19 deletions ...netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTaskTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,15 @@
package com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;

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 @@ -50,6 +55,8 @@ public class MonitoredDeployBaseTaskTest {

private final ObjectMapper objectMapper = new ObjectMapper();

private DeploymentMonitorServiceProvider deploymentMonitorServiceProvider;

@BeforeEach
void setup() {
OkClient okClient = new OkClient();
Expand All @@ -63,7 +70,7 @@ void setup() {
var deploymentMonitorDefinitions = new ArrayList<DeploymentMonitorDefinition>();
deploymentMonitorDefinitions.add(deploymentMonitorDefinition);

DeploymentMonitorServiceProvider deploymentMonitorServiceProvider =
deploymentMonitorServiceProvider =
new DeploymentMonitorServiceProvider(
okClient, retrofitLogLevel, requestInterceptor, deploymentMonitorDefinitions);
monitoredDeployBaseTask =
Expand Down Expand Up @@ -93,14 +100,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 +137,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 +154,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 +170,44 @@ 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(logMessageOnSpinServerException).isEqualTo("<NO RESPONSE>");
}

@Test
void shouldReturnHeadersAndErrorMessageWhenErrorResponseBodyIsNull() {

var converter = new JacksonConverter(objectMapper);
var headers = new ArrayList<Header>();
var header = new Header(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE);
monitoredDeployBaseTask.objectMapper = mock(ObjectMapper.class);

headers.add(header);

assertThat(logMessageOnUnexpectedError).isEqualTo("<NO RESPONSE>");
Response response =
new Response(
"/deployment/evaluateHealth",
HttpStatus.BAD_REQUEST.value(),
HttpStatus.BAD_REQUEST.name(),
headers,
new MockTypedInput(converter, null));

RetrofitError httpError =
RetrofitError.httpError(
"https://foo.com/deployment/evaluateHealth", response, converter, null);

SpinnakerHttpException httpException = new SpinnakerHttpException(httpError);

String logMessageOnSpinHttpException = monitoredDeployBaseTask.getErrorMessage(httpException);
String body = "Failed to serialize the error response body";

assertThat(logMessageOnSpinHttpException)
.isEqualTo(
String.format("headers: %s\nresponse body: %s", httpException.getHeaders(), body));
}

static class MockTypedInput implements TypedInput {
Expand Down
1 change: 1 addition & 0 deletions orca-deploymentmonitor/orca-deploymentmonitor.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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