refactor(clouddriver) : InstanceService - Eliminated the usages of Re…

…trofitError exception handling and replaced it with SpinnakerRetrofitErrorHandler (#4580) * refactor(clouddriver): InstanceService - Eliminated the usage of RetrofitError in the catch blocks and added SpinnakerRetrofitErrorHandler in the retrofit config which handles the RetrofitError and throws a customised Spinnaker*Exception * refactor(clouddriver): Code formatted to bring the statements inline with the existing formatting * test(clouddriver): Replaced the 'request' mock bean with RetrofitError to wrap up the customised Spinnaker*Exception * test(clouddriver): Modified the wrapper exception from generic RetrofitError to more specific Retrofit network error * test(clouddriver): Reordered the RetrofitError import
spinnaker · Nov 5, 2023 · a5d151f · a5d151f
1 parent 5385ad2
commit a5d151f
Show file tree

Hide file tree

Showing 11 changed files with 29 additions and 19 deletions.
diff --git a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/JarDiffsTask.groovy b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/JarDiffsTask.groovy
@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.orca.kato.tasks
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
 import com.netflix.spinnaker.orca.clouddriver.model.Instance.InstanceInfo
 
@@ -121,6 +122,7 @@ class JarDiffsTask implements DiffTask {
     RestAdapter restAdapter = new RestAdapter.Builder()
       .setEndpoint(address)
       .setClient(new OkClient(okHttpClient))
+      .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
       .build()
 
     return restAdapter.create(InstanceService.class)

diff --git a/...driver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/AbstractQuipTask.groovy b/...driver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/AbstractQuipTask.groovy
@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.orca.kato.tasks.quip
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler
 import com.netflix.spinnaker.orca.api.pipeline.Task
 import com.netflix.spinnaker.orca.clouddriver.InstanceService
 import com.squareup.okhttp.OkHttpClient
@@ -30,6 +31,7 @@ abstract class AbstractQuipTask implements Task {
       .setEndpoint(address)
       .setClient(new OkClient(new OkHttpClient(retryOnConnectionFailure: false)))
       .setLogLevel(BASIC)
+      .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
       .build()
     return restAdapter.create(InstanceService.class)
   }

diff --git a/...src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/InstanceHealthCheckTask.groovy b/...src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/InstanceHealthCheckTask.groovy
@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.orca.kato.tasks.quip
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.RetryableTask
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
@@ -24,7 +25,6 @@ import com.netflix.spinnaker.orca.clouddriver.model.Instance.InstanceInfo
 import com.netflix.spinnaker.orca.clouddriver.utils.OortHelper
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 
 @Component
 @Deprecated
@@ -61,7 +61,7 @@ class InstanceHealthCheckTask extends AbstractQuipTask implements RetryableTask
       def instanceService = createInstanceService("http://${host}:${healthCheckUrl.port}")
       try { // keep trying until we get a 200 or time out
         instanceService.healthCheck(healthCheckUrl.path.substring(1))
-      } catch (RetrofitError e) {
+      } catch (SpinnakerServerException e) {
         executionStatus = ExecutionStatus.RUNNING
       }
     }

diff --git a/...ddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/MonitorQuipTask.groovy b/...ddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/MonitorQuipTask.groovy
@@ -17,13 +17,13 @@
 package com.netflix.spinnaker.orca.kato.tasks.quip
 
 import com.fasterxml.jackson.databind.ObjectMapper
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.RetryableTask
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 import retrofit.client.Client
 
 @Deprecated
@@ -69,7 +69,7 @@ class MonitorQuipTask extends AbstractQuipTask implements RetryableTask {
         } else {
           throw new RuntimeException("quip task failed for ${hostName} with a result of ${status}, see http://${hostName}:5050/tasks/${taskId}")
         }
-      } catch(RetrofitError e) {
+      } catch(SpinnakerServerException e) {
         result = TaskResult.ofStatus(ExecutionStatus.RUNNING)
       }
     }

diff --git a/...ddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/TriggerQuipTask.groovy b/...ddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/TriggerQuipTask.groovy
@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.orca.kato.tasks.quip
 
 import com.fasterxml.jackson.databind.ObjectMapper
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.RetryableTask
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
@@ -25,7 +26,6 @@ import com.netflix.spinnaker.orca.clouddriver.InstanceService
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 import retrofit.client.Client
 
 @Deprecated
@@ -75,7 +75,7 @@ class TriggerQuipTask extends AbstractQuipTask implements RetryableTask {
             def ref = objectMapper.readValue(instanceResponse.body.in().text, Map).ref
             taskIdMap.put(instanceHostName, ref.substring(1 + ref.lastIndexOf('/')))
             patchedInstanceIds << instanceId
-          } catch (RetrofitError e) {
+          } catch (SpinnakerServerException e) {
             log.warn("Error in Quip request: {}", e.message)
           }
         }
@@ -108,7 +108,7 @@ class TriggerQuipTask extends AbstractQuipTask implements RetryableTask {
         if (version && !version.isEmpty()) {
           return version
         }
-      } catch (RetrofitError e) {
+      } catch (SpinnakerServerException e) {
         //retry
       }
       sleep(instanceVersionSleep)

diff --git a/...uddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/VerifyQuipTask.groovy b/...uddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/VerifyQuipTask.groovy
@@ -17,13 +17,13 @@
 package com.netflix.spinnaker.orca.kato.tasks.quip
 
 import com.fasterxml.jackson.databind.ObjectMapper
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 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 org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
-import retrofit.RetrofitError
 import retrofit.client.Client
 
 import javax.annotation.Nonnull
@@ -68,7 +68,7 @@ class VerifyQuipTask extends AbstractQuipTask implements Task {
       def instanceService = createInstanceService("http://${hostName}:5050")
       try {
         instanceService.listTasks()
-      } catch(RetrofitError e) {
+      } catch(SpinnakerServerException e) {
         allInstancesHaveQuip = false
       }
     }

diff --git a/...clouddriver/src/test/groovy/com/netflix/spinnaker/orca/kato/tasks/JarDiffsTaskSpec.groovy b/...clouddriver/src/test/groovy/com/netflix/spinnaker/orca/kato/tasks/JarDiffsTaskSpec.groovy
@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.orca.kato.tasks
 
 import com.fasterxml.jackson.databind.ObjectMapper
+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.InstanceService
@@ -103,7 +104,7 @@ class JarDiffsTaskSpec extends Specification {
     Response jarsResponse = new Response('http://foo.com', 200, 'OK', [], new TypedString(sourceJarsResponse))
 
     when:
-    1 * instanceService.getJars() >> {throw new RetrofitError(null, null, null, null, null, null, null)}
+    1 * instanceService.getJars() >> {throw new SpinnakerServerException(new RetrofitError(null, null, null, null, null, null, null))}
     1 * instanceService.getJars() >> jarsResponse
     def result = task.getJarList([foo: [hostName : "bar"], foo2: [hostName : "bar2"]])
 
@@ -121,7 +122,7 @@ class JarDiffsTaskSpec extends Specification {
     5 * task.createInstanceService("http://bar:8077") >> instanceService
 
     when:
-    5 * instanceService.getJars() >> {throw new RetrofitError(null, null, null, null, null, null, null)}
+    5 * instanceService.getJars() >> {throw new SpinnakerServerException(new RetrofitError(null, null, null, null, null, null, null))}
     def result = task.getJarList((1..5).collectEntries { ["${it}": [hostName : "bar"]] })
 
     then:
@@ -138,7 +139,7 @@ class JarDiffsTaskSpec extends Specification {
     Response jarsResponse = new Response('http://foo.com', 200, 'OK', [], new TypedString(sourceJarsResponse))
 
     when:
-    1 * instanceService.getJars() >> {throw new RetrofitError(null, null, null, null, null, null, null)}
+    1 * instanceService.getJars() >> {throw new SpinnakerServerException(new RetrofitError(null, null, null, null, null, null, null))}
     1 * instanceService.getJars() >> jarsResponse
     def result = task.getJarList([foo: [hostName : "bar"], foo2: [hostName : "bar2"], foo3: [hostName : "bar3"]])
 
@@ -215,7 +216,7 @@ class JarDiffsTaskSpec extends Specification {
     task.oortHelper = oortHelper
     1 * oortHelper.getInstancesForCluster(stage.context, "myapp-v000", false) >> sourceExpectedInstances
     1 * oortHelper.getInstancesForCluster(stage.context, "myapp-v002", false) >> targetExpectedInstances
-    1 * instanceService.getJars() >> {throw new RetrofitError(null, null, null, null, null, null, null)}
+    1 * instanceService.getJars() >> {throw new SpinnakerServerException(new RetrofitError(null, null, null, null, null, null, null))}
     1 * instanceService.getJars() >> targetResponse
 
     TaskResult result = task.execute(stage)

diff --git a/...test/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/InstanceHealthCheckTaskSpec.groovy b/...test/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/InstanceHealthCheckTaskSpec.groovy
@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.orca.kato.tasks.quip
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.clouddriver.InstanceService
 import com.netflix.spinnaker.orca.clouddriver.ModelUtils
@@ -60,7 +61,7 @@ class InstanceHealthCheckTaskSpec extends Specification {
       if (responseCode.get(i) == 200) {
         1 * instanceService.healthCheck("healthCheck") >> responses.get(i)
       } else {
-        1 * instanceService.healthCheck("healthCheck") >> { throw new RetrofitError(null, null, null, null, null, null, null) }
+        1 * instanceService.healthCheck("healthCheck") >> { throw new SpinnakerServerException(new RetrofitError(null, null, null, null, null, null, null)) }
       }
     }
 

diff --git a/...ver/src/test/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/MonitorQuipTaskSpec.groovy b/...ver/src/test/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/MonitorQuipTaskSpec.groovy
@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.orca.kato.tasks.quip
 
 import com.fasterxml.jackson.databind.ObjectMapper
+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.InstanceService
@@ -118,15 +119,14 @@ class MonitorQuipTaskSpec extends Specification {
     def stage = new StageExecutionImpl(pipe, 'monitorQuip', [:])
     stage.context.instances = instances
     stage.context.taskIds = taskIds
-
     task.createInstanceService(_) >> instanceService
 
     when:
     TaskResult result = task.execute(stage)
 
     then:
     taskIds.eachWithIndex { def entry, int i ->
-      instanceService.listTask(entry.value) >> { throw new RetrofitError(null, null, null, null, null, null, null)}
+      instanceService.listTask(entry.value) >> { throw new SpinnakerServerException(new RetrofitError(null, null, null, null, null, null, null))}
     }
     result.status == ExecutionStatus.RUNNING
 

diff --git a/...ver/src/test/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/TriggerQuipTaskSpec.groovy b/...ver/src/test/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/TriggerQuipTaskSpec.groovy
@@ -16,6 +16,9 @@
 
 package com.netflix.spinnaker.orca.kato.tasks.quip
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
+
 import java.nio.charset.Charset
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
@@ -176,7 +179,7 @@ class TriggerQuipTaskSpec extends Specification {
       // need to do this since I can't stick exceptions on the data table
       if (it) {
         1 * instanceService.patchInstance(app, patchVersion, "") >> {
-          throw new RetrofitError(null, null, null, null, null, null, null)
+          throw new SpinnakerServerException(new RetrofitError(null, null, null, null, null, null, null))
         }
       } else {
         1 * instanceService.patchInstance(app, patchVersion, "") >> instanceResponse
@@ -250,7 +253,7 @@ class TriggerQuipTaskSpec extends Specification {
 
     then:
     2 * instanceService.getCurrentVersion(app) >> {
-      throw RetrofitError.networkError('http://foo', new IOException('failed'))
+      throw new SpinnakerNetworkException(RetrofitError.networkError('http://foo', new IOException('failed')))
     } >> mkResponse([version: patchVersion])
 
     result.context.skippedInstances.keySet() == ["i-1234"] as Set

diff --git a/...iver/src/test/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/VerifyQuipTaskSpec.groovy b/...iver/src/test/groovy/com/netflix/spinnaker/orca/kato/tasks/quip/VerifyQuipTaskSpec.groovy
@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.orca.kato.tasks.quip
 
 import com.fasterxml.jackson.databind.ObjectMapper
+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.InstanceService
@@ -233,7 +234,7 @@ class VerifyQuipTaskSpec extends Specification {
     2 * task.createInstanceService(_) >> instanceService
     1 * instanceService.listTasks() >> instanceResponse
     1 * instanceService.listTasks() >> {
-      throw new RetrofitError(null, null, null, null, null, null, null)
+      throw new SpinnakerServerException(new RetrofitError(null, null, null, null, null, null, null))
     }
     !result?.context
     thrown(RuntimeException)