refactor(*): Replace Hystrix with Resilience4j (#775)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
spinnaker · Jun 9, 2020 · e82e97b · e82e97b
1 parent 091fd79
commit e82e97b
Show file tree

Hide file tree

Showing 14 changed files with 170 additions and 255 deletions.
diff --git a/igor-web/igor-web.gradle b/igor-web/igor-web.gradle
@@ -75,7 +75,6 @@ dependencies {
     implementation "com.netflix.spinnaker.kork:kork-web"
     implementation "com.netflix.spinnaker.kork:kork-jedis"
     implementation "com.netflix.spinnaker.kork:kork-telemetry"
-    implementation "com.netflix.spinnaker.kork:kork-hystrix"
     implementation "com.netflix.spinnaker.kork:kork-secrets-aws"
     implementation "com.netflix.spinnaker.kork:kork-secrets-gcp"
     implementation "com.netflix.spinnaker.kork:kork-plugins"

diff --git a/igor-web/src/main/groovy/com/netflix/spinnaker/igor/Main.groovy b/igor-web/src/main/groovy/com/netflix/spinnaker/igor/Main.groovy
@@ -16,7 +16,6 @@
 
 package com.netflix.spinnaker.igor
 
-import com.netflix.config.ConfigurationManager
 import com.netflix.spinnaker.kork.boot.DefaultPropertiesBuilder
 import com.netflix.spinnaker.kork.configserver.ConfigServerBootstrap
 import org.springframework.boot.autoconfigure.EnableAutoConfiguration
@@ -48,7 +47,6 @@ class Main extends SpringBootServletInitializer {
      */
     InetAddressCachePolicy.cachePolicy = InetAddressCachePolicy.NEVER
     Security.setProperty('networkaddress.cache.ttl', '0')
-    ConfigurationManager.loadCascadedPropertiesFromResources("hystrix")
   }
 
   static void main(String... args) {

diff --git a/igor-web/src/main/groovy/com/netflix/spinnaker/igor/config/IgorConfig.groovy b/igor-web/src/main/groovy/com/netflix/spinnaker/igor/config/IgorConfig.groovy
@@ -16,7 +16,6 @@
 
 package com.netflix.spinnaker.igor.config
 
-import com.netflix.hystrix.exception.HystrixRuntimeException
 import com.netflix.spectator.api.Registry
 import com.netflix.spinnaker.fiat.shared.EnableFiatAutoConfig
 import com.netflix.spinnaker.filters.AuthenticatedRequestFilter
@@ -38,15 +37,9 @@ import org.springframework.context.annotation.Bean
 import org.springframework.context.annotation.Configuration
 import org.springframework.context.annotation.Import
 import org.springframework.core.Ordered
-import org.springframework.http.HttpStatus
 import org.springframework.security.web.firewall.StrictHttpFirewall
-import org.springframework.web.bind.annotation.ControllerAdvice
-import org.springframework.web.bind.annotation.ExceptionHandler
-import org.springframework.web.bind.annotation.ResponseBody
-import org.springframework.web.bind.annotation.ResponseStatus
 import org.springframework.web.servlet.config.annotation.InterceptorRegistry
 import org.springframework.web.servlet.config.annotation.WebMvcConfigurerAdapter
-import retrofit.RetrofitError
 
 import java.util.concurrent.ExecutorService
 import java.util.concurrent.Executors
@@ -105,12 +98,10 @@ class IgorConfig extends WebMvcConfigurerAdapter {
         Executors.newCachedThreadPool()
     }
 
-    @Bean
-    HystrixRuntimeExceptionHandler hystrixRuntimeExceptionHandler() {
-        return new HystrixRuntimeExceptionHandler()
-    }
-
-    @Bean
+  /**
+   * TODO: Replace with R4J
+   */
+  @Bean
     RetrySupport retrySupport() {
         return new RetrySupport()
     }
@@ -125,27 +116,4 @@ class IgorConfig extends WebMvcConfigurerAdapter {
     JinjaArtifactExtractor.Factory jinjaArtifactExtractorFactory(JinjavaFactory jinjavaFactory) {
         return new JinjaArtifactExtractor.Factory(jinjavaFactory);
     }
-
-    @ControllerAdvice
-    static class HystrixRuntimeExceptionHandler {
-        @ResponseStatus(HttpStatus.TOO_MANY_REQUESTS)
-        @ResponseBody
-        @ExceptionHandler(HystrixRuntimeException)
-        public Map handleHystrix(HystrixRuntimeException exception) {
-            def failureCause = exception.cause
-            if (failureCause instanceof RetrofitError) {
-                failureCause = failureCause.cause ?: failureCause
-            }
-
-            return [
-                fallbackException: exception.fallbackException.toString(),
-                failureType: exception.failureType,
-                failureCause: failureCause.toString(),
-                error: "Hystrix Failure",
-                message: exception.message,
-                status: HttpStatus.TOO_MANY_REQUESTS.value(),
-                timestamp: System.currentTimeMillis()
-            ]
-        }
-    }
 }
diff --git a/igor-web/src/main/groovy/com/netflix/spinnaker/igor/config/JenkinsConfig.groovy b/igor-web/src/main/groovy/com/netflix/spinnaker/igor/config/JenkinsConfig.groovy
@@ -35,6 +35,7 @@ import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger
 import com.squareup.okhttp.OkHttpClient
 import groovy.transform.CompileStatic
 import groovy.util.logging.Slf4j
+import io.github.resilience4j.circuitbreaker.CircuitBreakerRegistry
 import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty
 import org.springframework.boot.context.properties.EnableConfigurationProperties
@@ -86,7 +87,8 @@ class JenkinsConfig {
                                                @Valid JenkinsProperties jenkinsProperties,
                                                JenkinsOkHttpClientProvider jenkinsOkHttpClientProvider,
                                                JenkinsRetrofitRequestInterceptorProvider jenkinsRetrofitRequestInterceptorProvider,
-                                               Registry registry) {
+                                               Registry registry,
+                                               CircuitBreakerRegistry circuitBreakerRegistry) {
         log.info "creating jenkinsMasters"
         Map<String, JenkinsService> jenkinsMasters = jenkinsProperties?.masters?.collectEntries { JenkinsProperties.JenkinsHost host ->
             log.info "bootstrapping ${host.address} as ${host.name}"
@@ -100,16 +102,23 @@ class JenkinsConfig {
                     igorConfigurationProperties.client.timeout
                 ),
                 host.csrf,
-                host.permissions.build()
+                host.permissions.build(),
+                circuitBreakerRegistry
             )]
         }
 
         buildServices.addServices(jenkinsMasters)
         jenkinsMasters
     }
 
-    static JenkinsService jenkinsService(String jenkinsHostId, JenkinsClient jenkinsClient, Boolean csrf, Permissions permissions) {
-        return new JenkinsService(jenkinsHostId, jenkinsClient, csrf, permissions)
+    static JenkinsService jenkinsService(
+      String jenkinsHostId,
+      JenkinsClient jenkinsClient,
+      Boolean csrf,
+      Permissions permissions,
+      CircuitBreakerRegistry circuitBreakerRegistry
+    ) {
+        return new JenkinsService(jenkinsHostId, jenkinsClient, csrf, permissions, circuitBreakerRegistry)
     }
 
     static ObjectMapper getObjectMapper() {

diff --git a/igor-web/src/main/groovy/com/netflix/spinnaker/igor/jenkins/service/JenkinsService.java b/igor-web/src/main/groovy/com/netflix/spinnaker/igor/jenkins/service/JenkinsService.java
@@ -22,7 +22,6 @@
 import com.fasterxml.jackson.core.type.TypeReference;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.netflix.spinnaker.fiat.model.resources.Permissions;
-import com.netflix.spinnaker.hystrix.SimpleJava8HystrixCommand;
 import com.netflix.spinnaker.igor.build.model.GenericBuild;
 import com.netflix.spinnaker.igor.build.model.GenericGitRevision;
 import com.netflix.spinnaker.igor.exceptions.ArtifactNotFoundException;
@@ -45,6 +44,8 @@
 import com.netflix.spinnaker.kork.core.RetrySupport;
 import com.netflix.spinnaker.kork.exceptions.SpinnakerException;
 import com.netflix.spinnaker.kork.web.exceptions.NotFoundException;
+import io.github.resilience4j.circuitbreaker.CircuitBreaker;
+import io.github.resilience4j.circuitbreaker.CircuitBreakerRegistry;
 import java.io.InputStream;
 import java.time.Duration;
 import java.util.HashMap;
@@ -65,20 +66,24 @@
 @Slf4j
 public class JenkinsService implements BuildOperations, BuildProperties {
   private final ObjectMapper objectMapper = new ObjectMapper();
-  private final String groupKey;
   private final String serviceName;
   private final JenkinsClient jenkinsClient;
   private final Boolean csrf;
   private final RetrySupport retrySupport = new RetrySupport();
   private final Permissions permissions;
+  private final CircuitBreaker circuitBreaker;
 
   public JenkinsService(
-      String jenkinsHostId, JenkinsClient jenkinsClient, Boolean csrf, Permissions permissions) {
+      String jenkinsHostId,
+      JenkinsClient jenkinsClient,
+      Boolean csrf,
+      Permissions permissions,
+      CircuitBreakerRegistry circuitBreakerRegistry) {
     this.serviceName = jenkinsHostId;
-    this.groupKey = "jenkins-" + jenkinsHostId;
     this.jenkinsClient = jenkinsClient;
     this.csrf = csrf;
     this.permissions = permissions;
+    this.circuitBreaker = circuitBreakerRegistry.circuitBreaker("jenkins-" + jenkinsHostId);
   }
 
   @Override
@@ -91,21 +96,21 @@ private String encode(String uri) {
   }
 
   public ProjectsList getProjects() {
-    ProjectsList projectsList =
-        new SimpleJava8HystrixCommand<>(
-                groupKey, buildCommandKey("getProjects"), jenkinsClient::getProjects)
-            .execute();
+    return circuitBreaker.executeSupplier(
+        () -> {
+          ProjectsList projectsList = jenkinsClient.getProjects();
 
-    if (projectsList == null || projectsList.getList() == null) {
-      return new ProjectsList();
-    }
-    List<Project> projects =
-        projectsList.getList().stream()
-            .flatMap(this::recursiveGetProjects)
-            .collect(Collectors.toList());
-    ProjectsList projectList = new ProjectsList();
-    projectList.setList(projects);
-    return projectList;
+          if (projectsList == null || projectsList.getList() == null) {
+            return new ProjectsList();
+          }
+          List<Project> projects =
+              projectsList.getList().stream()
+                  .flatMap(this::recursiveGetProjects)
+                  .collect(Collectors.toList());
+          ProjectsList projectList = new ProjectsList();
+          projectList.setList(projects);
+          return projectList;
+        });
   }
 
   private Stream<Project> recursiveGetProjects(Project project) {
@@ -122,43 +127,35 @@ private Stream<Project> recursiveGetProjects(Project project, String prefix) {
   }
 
   public JobList getJobs() {
-    return new SimpleJava8HystrixCommand<>(
-            groupKey, buildCommandKey("getJobs"), jenkinsClient::getJobs)
-        .execute();
+    return circuitBreaker.executeSupplier(jenkinsClient::getJobs);
   }
 
   public String getCrumb() {
     if (csrf) {
-      Crumb crumb =
-          new SimpleJava8HystrixCommand<>(
-                  groupKey, buildCommandKey("getCrumb"), jenkinsClient::getCrumb)
-              .execute();
-      if (crumb != null) {
-        return crumb.getCrumb();
-      }
+      return circuitBreaker.executeSupplier(
+          () -> {
+            Crumb crumb = jenkinsClient.getCrumb();
+            if (crumb != null) {
+              return crumb.getCrumb();
+            }
+            return null;
+          });
     }
     return null;
   }
 
   @Override
   public List<Build> getBuilds(String jobName) {
-    return new SimpleJava8HystrixCommand<>(
-            groupKey,
-            buildCommandKey("getBuildList"),
-            () -> jenkinsClient.getBuilds(encode(jobName)).getList())
-        .execute();
+    return circuitBreaker.executeSupplier(() -> jenkinsClient.getBuilds(encode(jobName)).getList());
   }
 
   public BuildDependencies getDependencies(String jobName) {
-    return new SimpleJava8HystrixCommand<>(
-            groupKey,
-            buildCommandKey("getDependencies"),
-            () -> jenkinsClient.getDependencies(encode(jobName)))
-        .execute();
+    return circuitBreaker.executeSupplier(() -> jenkinsClient.getDependencies(encode(jobName)));
   }
 
   public Build getBuild(String jobName, Integer buildNumber) {
-    return jenkinsClient.getBuild(encode(jobName), buildNumber);
+    return circuitBreaker.executeSupplier(
+        () -> jenkinsClient.getBuild(encode(jobName), buildNumber));
   }
 
   @Override
@@ -196,45 +193,33 @@ public Permissions getPermissions() {
 
   private ScmDetails getGitDetails(String jobName, Integer buildNumber) {
     return retrySupport.retry(
-        () ->
-            new SimpleJava8HystrixCommand<>(
-                    groupKey,
-                    buildCommandKey("getGitDetails"),
-                    () -> {
-                      try {
-                        return jenkinsClient.getGitDetails(encode(jobName), buildNumber);
-                      } catch (RetrofitError e) {
-                        // assuming that a conversion error is unlikely to succeed on retry
-                        if (e.getKind() == RetrofitError.Kind.CONVERSION) {
-                          log.warn(
-                              "Unable to deserialize git details for build "
-                                  + buildNumber
-                                  + " of "
-                                  + jobName,
-                              e);
-                          return null;
-                        } else {
-                          throw e;
-                        }
-                      }
-                    })
-                .execute(),
+        () -> {
+          try {
+            return jenkinsClient.getGitDetails(encode(jobName), buildNumber);
+          } catch (RetrofitError e) {
+            // assuming that a conversion error is unlikely to succeed on retry
+            if (e.getKind() == RetrofitError.Kind.CONVERSION) {
+              log.warn(
+                  "Unable to deserialize git details for build " + buildNumber + " of " + jobName,
+                  e);
+              return null;
+            } else {
+              throw e;
+            }
+          }
+        },
         10,
         1000,
         false);
   }
 
   public Build getLatestBuild(String jobName) {
-    return new SimpleJava8HystrixCommand<>(
-            groupKey,
-            buildCommandKey("getLatestBuild"),
-            () -> jenkinsClient.getLatestBuild(encode(jobName)))
-        .execute();
+    return circuitBreaker.executeSupplier(() -> jenkinsClient.getLatestBuild(encode(jobName)));
   }
 
   public QueuedJob queuedBuild(Integer item) {
     try {
-      return jenkinsClient.getQueuedItem(item);
+      return circuitBreaker.executeSupplier(() -> jenkinsClient.getQueuedItem(item));
     } catch (RetrofitError e) {
       if (e.getResponse() != null && e.getResponse().getStatus() == NOT_FOUND.value()) {
         throw new NotFoundException("Queued job '${item}' not found for master '${master}'.");
@@ -244,16 +229,18 @@ public QueuedJob queuedBuild(Integer item) {
   }
 
   public Response build(String jobName) {
-    return jenkinsClient.build(encode(jobName), "", getCrumb());
+    return circuitBreaker.executeSupplier(
+        () -> jenkinsClient.build(encode(jobName), "", getCrumb()));
   }
 
   public Response buildWithParameters(String jobName, Map<String, String> queryParams) {
-    return jenkinsClient.buildWithParameters(encode(jobName), queryParams, "", getCrumb());
+    return circuitBreaker.executeSupplier(
+        () -> jenkinsClient.buildWithParameters(encode(jobName), queryParams, "", getCrumb()));
   }
 
   @Override
   public JobConfig getJobConfig(String jobName) {
-    return jenkinsClient.getJobConfig(encode(jobName));
+    return circuitBreaker.executeSupplier(() -> jenkinsClient.getJobConfig(encode(jobName)));
   }
 
   @Override
@@ -332,18 +319,13 @@ private Response getPropertyFile(String jobName, Integer buildNumber, String fil
   }
 
   public Response stopRunningBuild(String jobName, Integer buildNumber) {
-    return jenkinsClient.stopRunningBuild(encode(jobName), buildNumber, "", getCrumb());
+    return circuitBreaker.executeSupplier(
+        () -> jenkinsClient.stopRunningBuild(encode(jobName), buildNumber, "", getCrumb()));
   }
 
   public Response stopQueuedBuild(String queuedBuild) {
-    return jenkinsClient.stopQueuedBuild(queuedBuild, "", getCrumb());
-  }
-
-  /**
-   * A CommandKey should be unique per group (to ensure broken circuits do not span Jenkins masters)
-   */
-  private String buildCommandKey(String id) {
-    return groupKey + "-" + id;
+    return circuitBreaker.executeSupplier(
+        () -> jenkinsClient.stopQueuedBuild(queuedBuild, "", getCrumb()));
   }
 
   @Override