refactor(deploymentmonitor): Configure SpinnakerRetrofitErrorHandler for DeploymentMonitorService #4614

Pranav-b-7 · 2023-12-08T10:47:26Z

Changes Made

Configured the error handler for the Retrofit client, DeploymentMonitorService, with SpinnakerRetrofitErrorHandler.
Modified catch blocks with RetrofitError to use Spinnaker*Exception, aligning with the upcoming Retrofit version upgrade to retrofit2.x.

Purpose

This PR represents foundational work for upgrading the Retrofit version to retrofit2.x, paving the way for future improvements and compatibility.

Behavioural Changes

The significant modification involves updating catch blocks, resulting in a change in the format of logger messages. This aligns with the anticipated modifications discussed in the related PR: test(MonitoredDeployBaseTask): add tests to verify log message for different types of RetrofitError #4608 , which talks about the broader context related to this change.
Additionally, when the error response body is null, an appropriate error message will be printed in the logger.

kirangodishala · 2023-12-08T12:15:14Z

...lix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy

@@ -45,7 +52,7 @@ class EvaluateDeploymentHealthTaskSpec extends Specification {
 given:


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

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

...va/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java

dbyron-sf · 2023-12-08T21:00:53Z

...va/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java

+ String body =
+ Optional.ofNullable(objectMapper.writeValueAsString(httpException.getResponseBody()))
+ .orElse("");
+ return String.format("headers: %s\nresponse body: %s", httpException.getHeaders(), body);


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.

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.

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

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

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.

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

dbyron-sf · 2023-12-08T21:02:17Z

...va/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java

 } catch (Exception e) {
 log.error(
- "Failed to fully parse retrofit error while reading response from deployment monitor", e);
+ "Failed to fully parse http response while reading response from deployment monitor", e);


I don't think there's any parsing here, nor any response reading. We're serializing some json.

Modified the log statement to Failed to fully read http error response details

dbyron-sf · 2023-12-08T21:03:12Z

...lix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy

@@ -198,6 +205,50 @@ class EvaluateDeploymentHealthTaskSpec extends Specification {
 false | null || ExecutionStatus.FAILED_CONTINUE
 }

+ def "should return status as RUNNING when SpinnakerHttpException is thrown"() {


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

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.

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

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

dbyron-sf · 2023-12-11T16:11:24Z

...va/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java

 } 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);


Suggested change

log.error("Failed to fully read http error response details", e);

log.error("Failed to serialize http error response details", e);

The error log is now is now reword to : "Failed to serialize http error response details"

…RetrofitLogMessage()' This commit sets the stage for forthcoming changes currently in progress under PR : spinnaker#4614 . The primary goal is to compare the behaviour before and after enhancements by introducing test cases for the ‘MonitoredDeployTask’ and the ‘getRetrofitLogMessage()’ method. For ‘MonitoredDeployTask’: - Test Case to simulate networkError and observe behaviour - Test Case to simulate httpError and observe behaviour - Test Case to simulate unexpectedError and observe behaviour - Test Case to simulate conversionError and observe behaviour For ‘getRetrofitLogMessage()’: - Test Cases to verify behaviour during HTTP error details parsing when exceptions occur Additionally, a Mockito dependency ('org.mockito:mockito-inline:2.13.0') has been added to support spying/mocking on the final class . This resolves the issue encountered during testing where Mockito couldn't mock/spy the final class, preventing the following error: org.mockito.exceptions.base.MockitoException: Cannot mock/spy class retrofit.client.Response Mockito cannot mock/spy because : - final class at com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy.MonitoredDeployBaseTaskTest.shouldReturnOnlyStatusWhenExceptionThrownWhileParsingHttpErrorBody(MonitoredDeployBaseTaskTest.java:217) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:568) at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:688) at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60) at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:131)

…RetrofitLogMessage()' This commit sets the stage for forthcoming changes currently in progress under PR : spinnaker#4614 . The primary goal is to compare the behaviour before and after enhancements by introducing test cases for the ‘MonitoredDeployTask’ and the ‘getRetrofitLogMessage()’ method. For ‘MonitoredDeployTask’: - Test Case to simulate networkError and observe behaviour - Test Case to simulate httpError and observe behaviour - Test Case to simulate unexpectedError and observe behaviour - Test Case to simulate conversionError and observe behaviour For ‘getRetrofitLogMessage()’: - Test Cases to verify behaviour during HTTP error details parsing when exceptions occur Additionally, a Mockito dependency ('org.mockito:mockito-inline:2.13.0') has been added to support spying/mocking on the final class 'retrofit.client.Response'. This resolves the issue encountered during testing where Mockito couldn't mock/spy the final class, preventing the following error: org.mockito.exceptions.base.MockitoException: Cannot mock/spy class retrofit.client.Response Mockito cannot mock/spy because : - final class at com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy.MonitoredDeployBaseTaskTest.shouldReturnOnlyStatusWhenExceptionThrownWhileParsingHttpErrorBody(MonitoredDeployBaseTaskTest.java:217) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:568) at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:688) at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60) at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:131)

…DeployTask and 'getRetrofitLogMessage()' This commit sets the stage for forthcoming changes currently in progress under PR : spinnaker#4614 . The primary goal is to compare the behaviour before and after enhancements by introducing test cases for the ‘MonitoredDeployTask’ and the ‘getRetrofitLogMessage()’ method. For ‘MonitoredDeployTask’: - Test Case to simulate networkError and observe behaviour - Test Case to simulate httpError and observe behaviour - Test Case to simulate unexpectedError and observe behaviour - Test Case to simulate conversionError and observe behaviour For ‘getRetrofitLogMessage()’: - Test Cases to verify behaviour during HTTP error details parsing when exceptions occur Additionally, a Mockito dependency ('org.mockito:mockito-inline:2.13.0') has been added to support spying/mocking on the final class 'retrofit.client.Response'. This resolves the issue encountered during testing where Mockito couldn't mock/spy the final class, preventing the following error: org.mockito.exceptions.base.MockitoException: Cannot mock/spy class retrofit.client.Response Mockito cannot mock/spy because : - final class at com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy.MonitoredDeployBaseTaskTest.shouldReturnOnlyStatusWhenExceptionThrownWhileParsingHttpErrorBody(MonitoredDeployBaseTaskTest.java:217) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:568) at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:688) at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60) at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:131)

…DeployTask and 'getRetrofitLogMessage()' This commit sets the stage for forthcoming changes currently in progress under PR : spinnaker#4614 . The primary goal is to compare the behaviour before and after enhancements by introducing test cases for the ‘MonitoredDeployTask’ and the ‘getRetrofitLogMessage()’ method. For ‘MonitoredDeployTask’: - Test Case to simulate networkError and observe behaviour - Test Case to simulate httpError and observe behaviour - Test Case to simulate unexpectedError and observe behaviour - Test Case to simulate conversionError and observe behaviour For ‘getRetrofitLogMessage()’: - Test Cases to verify behaviour during HTTP error details parsing when exceptions occur Additionally, a Mockito dependency ('org.mockito:mockito-inline') has been added to support spying/mocking on the final class 'retrofit.client.Response'. This resolves the issue encountered during testing where Mockito couldn't mock/spy the final class, preventing the following error: org.mockito.exceptions.base.MockitoException: Cannot mock/spy class retrofit.client.Response Mockito cannot mock/spy because : - final class at com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy.MonitoredDeployBaseTaskTest.shouldReturnOnlyStatusWhenExceptionThrownWhileParsingHttpErrorBody(MonitoredDeployBaseTaskTest.java:217) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:568) at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:688) at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60) at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:131)

dbyron-sf · 2024-01-02T23:20:33Z

...lix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy

@@ -198,6 +203,50 @@ class EvaluateDeploymentHealthTaskSpec extends Specification {
 false | null || ExecutionStatus.FAILED_CONTINUE
 }

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


Since the code changes are happening in MonitoredDeployBaseTask, is it possible to add additional coverage to MonitoredDeployBaseTaskTest? Is there anything specific to EvaluateDeploymentHealthTaskSpec that's important?
As well, MonitoredDeployBaseTaskTest is java and I'd much rather add more java code than groovy.

The EvaluateDeploymentHealthTask is one of the child class of MonitoredDeployBaseTask .
The executeInternal() method is overridden in this child class , and the there is also a call to evaluateHealth API in this method.
This API call is what we are mocking in the test case. So, I think this test case is more specific to EvaluateDeploymentHealthTask.
Moving this to MonitoredDeployBaseTaskTest would make it more complex , because it requires lots of mocking

This commit introduces a test case aimed at demonstrating the existing behaviour of the ‘EvaluateDeploymentTask’ task. The primary objective of this test case is to verify the proper handling of ‘RetrofitError’ thrown by the retrofit client when interacting with DeploymentMonitorService APIs. Details: - Added a dedicated test case to assess the behaviour of ‘EvaluateDeploymentTask’. - The test case specifically focuses on handling ‘RetrofitError’ instances thrown by DeploymentMonitorService APIs. - This step is essential for understanding and validating the current behaviour of the task in response to potential errors during API interactions. This test case serves as a baseline for future modifications and enhancements related to the ‘EvaluateDeploymentTask’.

…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: spinnaker#4608 Additionally, when the error response body is null, an appropriate error message will be printed in the logger.

kirangodishala reviewed Dec 8, 2023

View reviewed changes

Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch from 4a5ee9a to f2541dc Compare December 8, 2023 15:59

Pranav-b-7 requested a review from kirangodishala December 8, 2023 16:07

dbyron-sf reviewed Dec 8, 2023

View reviewed changes

...va/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java Outdated Show resolved Hide resolved

dbyron-sf reviewed Dec 8, 2023

View reviewed changes

Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch from f2541dc to 53aa376 Compare December 11, 2023 10:41

Pranav-b-7 requested a review from dbyron-sf December 11, 2023 15:41

dbyron-sf reviewed Dec 11, 2023

View reviewed changes

Pranav-b-7 mentioned this pull request Dec 12, 2023

test(clouddriver): Test Suite Refinement: MonitoredDeployTask and getRetrofitLogMessage() Behaviour #4617

Open

Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch from cc39981 to 36d3cf0 Compare December 29, 2023 11:45

Pranav-b-7 closed this Dec 29, 2023

Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch from 36d3cf0 to adc81ac Compare December 29, 2023 12:18

Pranav-b-7 reopened this Jan 2, 2024

Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch 4 times, most recently from ec0c677 to ce45162 Compare January 2, 2024 11:19

Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch from ce45162 to b681cef Compare January 2, 2024 11:43

Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch from b681cef to 133f1c5 Compare January 2, 2024 12:08

dbyron-sf force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch from 133f1c5 to d92a982 Compare January 2, 2024 22:57

dbyron-sf reviewed Jan 2, 2024

View reviewed changes

Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch 3 times, most recently from 977bd17 to 0ca8d19 Compare January 3, 2024 07:50

Pranav-b-7 marked this pull request as ready for review January 3, 2024 07:51

Pranav-b-7 requested a review from dbyron-sf January 3, 2024 07:51

Pranav-b-7 added 2 commits January 4, 2024 09:40

Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch from 0ca8d19 to 52e528f Compare January 4, 2024 04:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(deploymentmonitor): Configure SpinnakerRetrofitErrorHandler for DeploymentMonitorService #4614

refactor(deploymentmonitor): Configure SpinnakerRetrofitErrorHandler for DeploymentMonitorService #4614

Pranav-b-7 commented Dec 8, 2023 •

edited

kirangodishala Dec 8, 2023

Pranav-b-7 Dec 8, 2023

dbyron-sf Dec 8, 2023

Pranav-b-7 Dec 11, 2023

dbyron-sf Dec 11, 2023

Pranav-b-7 Jan 2, 2024 •

edited

dbyron-sf Jan 2, 2024

Pranav-b-7 Jan 3, 2024

dbyron-sf Dec 8, 2023

Pranav-b-7 Dec 11, 2023

dbyron-sf Dec 8, 2023

Pranav-b-7 Dec 11, 2023

dbyron-sf Dec 11, 2023

Pranav-b-7 Jan 2, 2024

dbyron-sf Dec 11, 2023

Pranav-b-7 Jan 2, 2024

dbyron-sf Jan 2, 2024

Pranav-b-7 Jan 3, 2024

		@@ -45,7 +52,7 @@ class EvaluateDeploymentHealthTaskSpec extends Specification {
		given:

	log.error("Failed to fully read http error response details", e);
	log.error("Failed to serialize http error response details", e);

refactor(deploymentmonitor): Configure SpinnakerRetrofitErrorHandler for DeploymentMonitorService #4614

Are you sure you want to change the base?

refactor(deploymentmonitor): Configure SpinnakerRetrofitErrorHandler for DeploymentMonitorService #4614

Conversation

Pranav-b-7 commented Dec 8, 2023 • edited

Changes Made

Purpose

Behavioural Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pranav-b-7 Jan 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pranav-b-7 commented Dec 8, 2023 •

edited

Pranav-b-7 Jan 2, 2024 •

edited