refactor(igor): use SpinnakerRetrofitErrorHandler with IgorService (#…

…4666) * refactor(igor): use SpinnakerRetrofitErrorHandler with IgorService This PR lays the foundational work for upgrading the retrofit version to 2.x, specifically focusing on refactoring the exception handling for IgorService. There are no behaviour changes detected with these code changes and hence no additional tests added to demonstrate the functionalities. Few existing tests are modified to align it with the new changes. * refactor(keel): Cleanup RetrofitError in ImportDeliveryConfigTask There are 2 APIs invoked inside the try block: 1. Keel Service API - keelService.publishDeliveryConfig() : With the addition of the commit - c417922, the keelService uses SpinnakerRetrofitErrorHandler. 2. Igor Service API - scmService.getDeliveryConfigManifest() : With the addition of the commit - d430b75, the igor service uses SpinnakerRetrofitErrorHandler. and hence with the above mentioned commits the catch block with RetrofitError and the handler logics associated with it becomes irrelevant.
spinnaker · Mar 7, 2024 · 38e426e · 38e426e
1 parent e2896cb
commit 38e426e
Show file tree

Hide file tree

Showing 20 changed files with 103 additions and 147 deletions.
diff --git a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/config/IgorConfiguration.groovy b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/config/IgorConfiguration.groovy
@@ -20,6 +20,7 @@ import com.fasterxml.jackson.databind.ObjectMapper
 import com.jakewharton.retrofit.Ok3Client
 import com.netflix.spinnaker.config.DefaultServiceEndpoint
 import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler
 import com.netflix.spinnaker.orca.igor.IgorService
 import com.netflix.spinnaker.orca.retrofit.RetrofitConfiguration
 import com.netflix.spinnaker.orca.retrofit.logging.RetrofitSlf4jLog
@@ -34,7 +35,6 @@ import org.springframework.context.annotation.Import
 import retrofit.Endpoint
 import retrofit.RequestInterceptor
 import retrofit.RestAdapter
-import retrofit.client.Client
 import retrofit.converter.JacksonConverter
 import static retrofit.Endpoints.newFixedEndpoint
 
@@ -61,6 +61,7 @@ class IgorConfiguration {
       .setEndpoint(igorEndpoint)
       .setClient(new Ok3Client(clientProvider.getClient(new DefaultServiceEndpoint("igor", igorEndpoint.url))))
       .setLogLevel(retrofitLogLevel)
+      .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
       .setRequestInterceptor(spinnakerRequestInterceptor)
       .setLog(new RetrofitSlf4jLog(IgorService))
       .setConverter(new JacksonConverter(mapper))

diff --git a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorJenkinsJobTask.groovy b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorJenkinsJobTask.groovy
@@ -18,6 +18,7 @@ package com.netflix.spinnaker.orca.igor.tasks
 
 import com.google.common.base.Enums
 import com.netflix.spinnaker.kork.core.RetrySupport
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.OverridableTimeoutRetryableTask
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
@@ -26,7 +27,6 @@ import com.netflix.spinnaker.orca.igor.BuildService
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 
 import java.util.concurrent.TimeUnit
 
@@ -81,9 +81,9 @@ class MonitorJenkinsJobTask implements OverridableTimeoutRetryableTask {
       }
 
       return TaskResult.builder(taskStatus).context(outputs).outputs(outputs).build()
-    } catch (RetrofitError e) {
-      if ([503, 500, 404].contains(e.response?.status)) {
-        log.warn("Http ${e.response.status} received from `igor`, retrying...")
+    } catch (SpinnakerHttpException e) {
+      if ([503, 500, 404].contains(e.responseCode)) {
+        log.warn("Http ${e.responseCode} received from `igor`, retrying...")
         return TaskResult.ofStatus(ExecutionStatus.RUNNING)
       }
 

diff --git a/.../src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorQueuedJenkinsJobTask.groovy b/.../src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorQueuedJenkinsJobTask.groovy
@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.orca.igor.tasks
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
 import com.netflix.spinnaker.orca.igor.IgorService
 import org.springframework.beans.factory.annotation.Value
@@ -28,7 +29,6 @@ import com.netflix.spinnaker.orca.igor.BuildService
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 
 @Slf4j
 @Component
@@ -56,9 +56,9 @@ class MonitorQueuedJenkinsJobTask implements OverridableTimeoutRetryableTask {
         createBacklink(stage, jenkinsController, jobName, build.number as Integer)
         return TaskResult.builder(ExecutionStatus.SUCCEEDED).context([buildNumber: build.number]).build()
       }
-    } catch (RetrofitError e) {
-      if ([503, 500, 404].contains(e.response?.status)) {
-        log.warn("Http ${e.response.status} received from `igor`, retrying...")
+    } catch (SpinnakerHttpException e) {
+      if ([503, 500, 404].contains(e.responseCode)) {
+        log.warn("Http ${e.responseCode} received from `igor`, retrying...")
         return TaskResult.ofStatus(ExecutionStatus.RUNNING)
       }
       throw e

diff --git a/...src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorWerckerJobStartedTask.groovy b/...src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorWerckerJobStartedTask.groovy
@@ -8,6 +8,7 @@
  */
 package com.netflix.spinnaker.orca.igor.tasks
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.OverridableTimeoutRetryableTask
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
@@ -16,7 +17,6 @@ import com.netflix.spinnaker.orca.igor.BuildService
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 
 import javax.annotation.Nonnull
 import java.util.concurrent.TimeUnit
@@ -38,7 +38,6 @@ class MonitorWerckerJobStartedTask implements OverridableTimeoutRetryableTask {
 
     try {
       Map<String, Object> build = buildService.getBuild(buildNumber, master, job)
-      Map outputs = [:]
       if ("not_built".equals(build?.result) || build?.number == null) {
         //The build has not yet started, so the job started monitoring task needs to be re-run
         return TaskResult.ofStatus(ExecutionStatus.RUNNING)
@@ -47,9 +46,9 @@ class MonitorWerckerJobStartedTask implements OverridableTimeoutRetryableTask {
         return TaskResult.builder(ExecutionStatus.SUCCEEDED).context([buildNumber: build.number]).build()
       }
 
-    } catch (RetrofitError e) {
-      if ([503, 500, 404].contains(e.response?.status)) {
-        log.warn("Http ${e.response.status} received from `igor`, retrying...")
+    } catch (SpinnakerHttpException e) {
+      if ([503, 500, 404].contains(e.responseCode)) {
+        log.warn("Http ${e.responseCode} received from `igor`, retrying...")
         return TaskResult.ofStatus(ExecutionStatus.RUNNING)
       }
 

diff --git a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/StartJenkinsJobTask.groovy b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/StartJenkinsJobTask.groovy
@@ -18,20 +18,19 @@ package com.netflix.spinnaker.orca.igor.tasks
 
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.kork.exceptions.SystemException
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.orca.api.pipeline.RetryableTask
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
-import com.netflix.spinnaker.orca.api.pipeline.Task
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import com.netflix.spinnaker.orca.exceptions.ExceptionHandler
 import com.netflix.spinnaker.orca.igor.BuildService
-import com.netflix.spinnaker.orca.retrofit.exceptions.RetrofitExceptionHandler
+import com.netflix.spinnaker.orca.retrofit.exceptions.SpinnakerServerExceptionHandler
 import org.slf4j.Logger
 import org.slf4j.LoggerFactory
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.http.HttpStatus
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 import retrofit.client.Response
 
 import javax.annotation.Nonnull
@@ -48,7 +47,7 @@ class StartJenkinsJobTask implements RetryableTask {
   ObjectMapper objectMapper
 
   @Autowired
-  RetrofitExceptionHandler retrofitExceptionHandler
+  SpinnakerServerExceptionHandler spinnakerServerExceptionHandler
 
   @Nonnull
   @Override
@@ -72,9 +71,9 @@ class StartJenkinsJobTask implements RetryableTask {
             .build()
       }
     }
-    catch (RetrofitError e) {
+    catch (SpinnakerServerException e){
       // This igor call is idempotent so we can retry despite it being PUT/POST
-      ExceptionHandler.Response exceptionResponse = retrofitExceptionHandler.handle("StartJenkinsJob", e)
+      ExceptionHandler.Response exceptionResponse = spinnakerServerExceptionHandler.handle("StartJenkinsJob", e)
 
       if (exceptionResponse.shouldRetry) {
         log.warn("Failure communicating with igor to start a jenkins job, will retry", e)

diff --git a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/StartScriptTask.groovy b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/StartScriptTask.groovy
@@ -18,21 +18,21 @@ package com.netflix.spinnaker.orca.igor.tasks
 
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.kork.exceptions.SystemException
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.orca.api.pipeline.RetryableTask
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import com.netflix.spinnaker.orca.exceptions.ExceptionHandler
 import com.netflix.spinnaker.orca.igor.BuildService
-import com.netflix.spinnaker.orca.retrofit.exceptions.RetrofitExceptionHandler
+import com.netflix.spinnaker.orca.retrofit.exceptions.SpinnakerServerExceptionHandler
 import org.slf4j.Logger
 import org.slf4j.LoggerFactory
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.beans.factory.annotation.Value
 import org.springframework.core.env.Environment
 import org.springframework.http.HttpStatus
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 import retrofit.client.Response
 
 import javax.annotation.Nonnull
@@ -49,7 +49,7 @@ class StartScriptTask implements RetryableTask {
   ObjectMapper objectMapper
 
   @Autowired
-  RetrofitExceptionHandler retrofitExceptionHandler
+  SpinnakerServerExceptionHandler spinnakerServerExceptionHandler
 
   @Autowired
   Environment environment
@@ -113,9 +113,9 @@ class StartScriptTask implements RetryableTask {
             .build()
       }
     }
-    catch (RetrofitError e) {
+    catch (SpinnakerServerException e) {
       // This igor call is idempotent so we can retry despite it being PUT/POST
-      ExceptionHandler.Response exceptionResponse = retrofitExceptionHandler.handle("StartJenkinsJob", e)
+      ExceptionHandler.Response exceptionResponse = spinnakerServerExceptionHandler.handle("StartJenkinsJob", e)
 
       if (exceptionResponse.shouldRetry) {
         log.warn("Failure communicating with igor to start a jenkins job, will retry", e)

diff --git a/orca-igor/src/main/java/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTask.java b/orca-igor/src/main/java/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTask.java
@@ -17,6 +17,9 @@
 package com.netflix.spinnaker.orca.igor.tasks;
 
 import com.google.common.collect.ImmutableMap;
+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.api.pipeline.RetryableTask;
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult;
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus;
@@ -28,7 +31,6 @@
 import javax.annotation.Nonnull;
 import lombok.RequiredArgsConstructor;
 import lombok.extern.slf4j.Slf4j;
-import retrofit.RetrofitError;
 
 @RequiredArgsConstructor
 @Slf4j
@@ -53,7 +55,7 @@ protected int getMaxConsecutiveErrors() {
     try {
       TaskResult result = tryExecute(stageDefinition);
       return resetErrorCount(result);
-    } catch (RetrofitError e) {
+    } catch (SpinnakerServerException e) {
       if (stageDefinition.getConsecutiveErrors() < getMaxConsecutiveErrors() && isRetryable(e)) {
         return TaskResult.builder(ExecutionStatus.RUNNING)
             .context(errorContext(errors + 1))
@@ -83,14 +85,13 @@ private Map<String, Integer> errorContext(int errors) {
     return Collections.singletonMap("consecutiveErrors", errors);
   }
 
-  private boolean isRetryable(RetrofitError retrofitError) {
-    if (retrofitError.getKind() == RetrofitError.Kind.NETWORK) {
+  private boolean isRetryable(SpinnakerServerException exception) {
+    if (exception instanceof SpinnakerNetworkException) {
       log.warn("Failed to communicate with igor, retrying...");
       return true;
-    }
-
-    if (retrofitError.getResponse() != null) {
-      int status = retrofitError.getResponse().getStatus();
+    } else if (exception instanceof SpinnakerHttpException) {
+      var httpException = (SpinnakerHttpException) exception;
+      int status = httpException.getResponseCode();
       if (status == 500 || status == 503) {
         log.warn(String.format("Received HTTP %s response from igor, retrying...", status));
         return true;

diff --git a/...test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetAwsCodeBuildArtifactsTaskSpec.groovy b/...test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetAwsCodeBuildArtifactsTaskSpec.groovy
@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.orca.igor.tasks
 
 import com.netflix.spinnaker.kork.artifacts.model.Artifact
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import com.netflix.spinnaker.orca.igor.IgorService
@@ -73,14 +74,14 @@ class GetAwsCodeBuildArtifactsTaskSpec extends Specification {
     TaskResult result = task.execute(stage)
 
     then:
-    1 * igorService.getAwsCodeBuildArtifacts(ACCOUNT, BUILD_ID) >> { throw stubRetrofitError() }
+    1 * igorService.getAwsCodeBuildArtifacts(ACCOUNT, BUILD_ID) >> { throw stubNetworkError() }
     0 * igorService._
     result.getStatus() == ExecutionStatus.RUNNING
   }
 
-  def stubRetrofitError() {
-    return Stub(RetrofitError) {
+  def stubNetworkError() {
+    return new SpinnakerNetworkException(Stub(RetrofitError) {
       getKind() >> RetrofitError.Kind.NETWORK
-    }
+    })
   }
 }
diff --git a/...r/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTaskSpec.groovy b/...r/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTaskSpec.groovy
@@ -19,6 +19,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.kork.retrofit.exceptions.SpinnakerHttpException
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import com.netflix.spinnaker.orca.igor.BuildService
@@ -120,9 +121,11 @@ class GetBuildPropertiesTaskSpec extends Specification {
       getResponse() >> new Response("", 500, "", Collections.emptyList(), null)
     }
 
+    def httpException = new SpinnakerHttpException(igorError)
+
     and:
     1 * buildService.getPropertyFile(BUILD_NUMBER, PROPERTY_FILE, MASTER, JOB) >>
-      { throw igorError }
+      { throw httpException }
 
     when:
     TaskResult result = task.execute(stage)

diff --git a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetCommitsTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetCommitsTaskSpec.groovy
@@ -260,8 +260,7 @@ class GetCommitsTaskSpec extends Specification {
 
     then:
     1 * scmService.compareCommits("stash", "projectKey", "repositorySlug", ['to': '186605b', 'from': 'a86305d', 'limit': 100]) >> {
-      throw new RetrofitError(null, null,
-          new Response("http://stash.com", 500, "test reason", [], null), null, null, null, null)
+      throw new SpinnakerHttpException(RetrofitError.httpError("http://stash.com", new Response("http://stash.com", 500, "test reason", [], null), null, null))
     }
     result.status == taskStatus
 
@@ -450,7 +449,7 @@ class GetCommitsTaskSpec extends Specification {
     and:
     task.scmService = Stub(ScmService) {
       compareCommits("stash", "projectKey", "repositorySlug", ['to': '186605b', 'from': 'a86305d', 'limit': 100]) >> {
-        throw new RetrofitError(null, null, new Response("http://stash.com", 404, "test reason", [], null), null, null, null, null)
+        throw new SpinnakerHttpException(RetrofitError.httpError("http://stash.com", new Response("http://stash.com", 404, "test reason", [], null), null, null))
       }
     }
 

diff --git a/.../groovy/com/netflix/spinnaker/orca/igor/tasks/GetGoogleCloudBuildArtifactsTaskSpec.groovy b/.../groovy/com/netflix/spinnaker/orca/igor/tasks/GetGoogleCloudBuildArtifactsTaskSpec.groovy
@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.orca.igor.tasks
 
 import com.netflix.spinnaker.kork.artifacts.model.Artifact
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import com.netflix.spinnaker.orca.igor.IgorService
@@ -72,14 +73,14 @@ class GetGoogleCloudBuildArtifactsTaskSpec extends Specification {
     TaskResult result = task.execute(stage)
 
     then:
-    1 * igorService.getGoogleCloudBuildArtifacts(ACCOUNT, BUILD_ID) >> { throw stubRetrofitError() }
+    1 * igorService.getGoogleCloudBuildArtifacts(ACCOUNT, BUILD_ID) >> { throw stubNetworkError() }
     0 * igorService._
     result.getStatus() == ExecutionStatus.RUNNING
   }
 
-  def stubRetrofitError() {
-    return Stub(RetrofitError) {
+  def stubNetworkError() {
+    return new SpinnakerNetworkException(Stub(RetrofitError) {
       getKind() >> RetrofitError.Kind.NETWORK
-    }
+    })
   }
 }
diff --git a/.../src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorAwsCodeBuildTaskSpec.groovy b/.../src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorAwsCodeBuildTaskSpec.groovy
@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.orca.igor.tasks
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import com.netflix.spinnaker.orca.igor.IgorService
@@ -82,14 +83,14 @@ class MonitorAwsCodeBuildTaskSpec extends Specification {
     TaskResult result = task.execute(stage)
 
     then:
-    1 * igorService.getAwsCodeBuildExecution(ACCOUNT, BUILD_ID) >> { throw stubRetrofitError() }
+    1 * igorService.getAwsCodeBuildExecution(ACCOUNT, BUILD_ID) >> { throw stubNetworkError() }
     0 * igorService._
     result.getStatus() == ExecutionStatus.RUNNING
   }
 
-  def stubRetrofitError() {
-    return Stub(RetrofitError) {
+  def stubNetworkError() {
+    return new SpinnakerNetworkException(Stub(RetrofitError) {
       getKind() >> RetrofitError.Kind.NETWORK
-    }
+    })
   }
 }