refactor(clouddriver): InstanceService - Eliminated the usage of Retr…

…ofitError in the catch blocks and added SpinnakerRetrofitErrorHandler in the retrofit config which handles the RetrofitError and throws a customised Spinnaker*Exception
spinnaker · Nov 2, 2023 · 662abe2 · 662abe2
1 parent 2e3c5f0
commit 662abe2
Show file tree

Hide file tree

Showing 11 changed files with 82 additions and 23 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
@@ -26,7 +27,10 @@ import com.netflix.spinnaker.orca.libdiffs.DefaultComparableLooseVersion
 import com.netflix.spinnaker.orca.libdiffs.Library
 import com.netflix.spinnaker.orca.pipeline.model.PipelineExecutionImpl
 import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl
-import retrofit.RetrofitError
+import okhttp3.Headers
+import okhttp3.HttpUrl
+import okhttp3.Request
+import org.springframework.http.HttpMethod
 import retrofit.client.Response
 import retrofit.mime.TypedString
 import spock.lang.Shared
@@ -41,13 +45,19 @@ class JarDiffsTaskSpec extends Specification {
 
   InstanceService instanceService = Mock(InstanceService)
 
+  Headers headers = Headers.of("Content-type", "application/json")
+  Map<String, String> tags = new HashMap<>()
+  Request request = new Request(HttpUrl.parse("http://foo.com"), HttpMethod.GET.name(), headers, null, tags as Map<Class<?>, ? extends Object>)
+
+
   @Shared
   PipelineExecutionImpl pipeline = PipelineExecutionImpl.newPipeline("orca")
 
   def setup() {
     GroovyMock(OortHelper, global: true)
     task.objectMapper = new ObjectMapper()
     task.comparableLooseVersion = new DefaultComparableLooseVersion()
+    tags.put("testKey", "testValue")
   }
 
   Map deployContext = ["availabilityZones" : ["us-west-2": ["us-west-2a"]],"kato.tasks" : [[resultObjects : [[ancestorServerGroupNameByRegion: ["us-west-2" : "myapp-v000"]],[serverGroupNameByRegion : ["us-west-2" : "myapp-v002"]]]]]]
@@ -103,7 +113,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(request)}
     1 * instanceService.getJars() >> jarsResponse
     def result = task.getJarList([foo: [hostName : "bar"], foo2: [hostName : "bar2"]])
 
@@ -121,7 +131,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(request)}
     def result = task.getJarList((1..5).collectEntries { ["${it}": [hostName : "bar"]] })
 
     then:
@@ -138,7 +148,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(request)}
     1 * instanceService.getJars() >> jarsResponse
     def result = task.getJarList([foo: [hostName : "bar"], foo2: [hostName : "bar2"], foo3: [hostName : "bar3"]])
 
@@ -215,7 +225,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(request)}
     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,12 +16,16 @@
 
 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
 import com.netflix.spinnaker.orca.clouddriver.utils.OortHelper
 import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl
-import retrofit.RetrofitError
+import okhttp3.Headers
+import okhttp3.HttpUrl
+import okhttp3.Request
+import org.springframework.http.HttpMethod
 import retrofit.client.Response
 import retrofit.mime.TypedString
 import spock.lang.Specification
@@ -52,6 +56,12 @@ class InstanceHealthCheckTaskSpec extends Specification {
 
     instances.size() * task.createInstanceService(_) >> instanceService
 
+    Headers headers = Headers.of("Content-type", "application/json")
+    Map<String, String> tags = new HashMap<>()
+    tags.put("testKey", "testValue")
+    Request request = new Request(HttpUrl.parse("http://foo.com"), HttpMethod.GET.name(), headers, null, tags as Map<Class<?>, ? extends Object>)
+
+
     when:
     def result = task.execute(stage)
 
@@ -60,7 +70,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(request) }
       }
     }
 

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,11 +17,15 @@
 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
 import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl
-import retrofit.RetrofitError
+import okhttp3.Headers
+import okhttp3.HttpUrl
+import okhttp3.Request
+import org.springframework.http.HttpMethod
 import retrofit.client.Client
 import retrofit.client.Response
 import retrofit.mime.TypedString
@@ -118,6 +122,10 @@ class MonitorQuipTaskSpec extends Specification {
     def stage = new StageExecutionImpl(pipe, 'monitorQuip', [:])
     stage.context.instances = instances
     stage.context.taskIds = taskIds
+    Headers headers = Headers.of("Content-type", "application/json")
+    Map<String, String> tags = new HashMap<>()
+    tags.put("testKey", "testValue")
+    Request request = new Request(HttpUrl.parse("http://foo.com"), HttpMethod.GET.name(), headers, null, tags as Map<Class<?>, ? extends Object>)
 
     task.createInstanceService(_) >> instanceService
 
@@ -126,7 +134,7 @@ class MonitorQuipTaskSpec extends Specification {
 
     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(request)}
     }
     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,14 +16,20 @@
 
 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 okhttp3.Headers
+import okhttp3.HttpUrl
+import okhttp3.Request
+import org.springframework.http.HttpMethod
+
 import java.nio.charset.Charset
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import com.netflix.spinnaker.orca.clouddriver.InstanceService
 import com.netflix.spinnaker.orca.pipeline.model.PipelineExecutionImpl
 import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl
-import retrofit.RetrofitError
 import retrofit.client.Client
 import retrofit.client.Response
 import retrofit.mime.TypedByteArray
@@ -168,6 +174,12 @@ class TriggerQuipTaskSpec extends Specification {
     stage.context.instances = instances
     instances.size() * task.createInstanceService(_) >> instanceService
 
+    Headers headers = Headers.of("Content-type", "application/json")
+    Map<String, String> tags = new HashMap<>()
+    tags.put("testKey", "testValue")
+    Request request = new Request(HttpUrl.parse("http://foo.com"), HttpMethod.GET.name(), headers, null, tags as Map<Class<?>, ? extends Object>)
+
+
     when:
     TaskResult result = task.execute(stage)
 
@@ -176,7 +188,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(request)
         }
       } else {
         1 * instanceService.patchInstance(app, patchVersion, "") >> instanceResponse
@@ -245,12 +257,18 @@ class TriggerQuipTaskSpec extends Specification {
     task.instanceVersionSleep = 1
     task.createInstanceService(_) >> instanceService
 
+    Headers headers = Headers.of("Content-type", "application/json")
+    Map<String, String> tags = new HashMap<>()
+    tags.put("testKey", "testValue")
+    Request request = new Request(HttpUrl.parse("http://foo.com"), HttpMethod.GET.name(), headers, null, tags as Map<Class<?>, ? extends Object>)
+
+
     when:
     TaskResult result = task.execute(stage)
 
     then:
     2 * instanceService.getCurrentVersion(app) >> {
-      throw RetrofitError.networkError('http://foo', new IOException('failed'))
+      throw new SpinnakerNetworkException(new RuntimeException(), request)
     } >> 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,11 +17,15 @@
 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
 import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl
-import retrofit.RetrofitError
+import okhttp3.Headers
+import okhttp3.HttpUrl
+import okhttp3.Request
+import org.springframework.http.HttpMethod
 import retrofit.client.Client
 import retrofit.client.Response
 import retrofit.mime.TypedString
@@ -226,14 +230,19 @@ class VerifyQuipTaskSpec extends Specification {
 
     Response instanceResponse = new Response('http://oort', 200, 'OK', [], new TypedString(instance))
 
+    Headers headers = Headers.of("Content-type", "application/json")
+    Map<String, String> tags = new HashMap<>()
+    tags.put("testKey", "testValue")
+    Request request = new Request(HttpUrl.parse("http://foo.com"), HttpMethod.GET.name(), headers, null, tags as Map<Class<?>, ? extends Object>)
+
     when:
     TaskResult result = task.execute(stage)
 
     then:
     2 * task.createInstanceService(_) >> instanceService
     1 * instanceService.listTasks() >> instanceResponse
     1 * instanceService.listTasks() >> {
-      throw new RetrofitError(null, null, null, null, null, null, null)
+      throw new SpinnakerServerException(request)
     }
     !result?.context
     thrown(RuntimeException)