From d430b75efeced7eb1f8d64eff6f0a160210eb2e6 Mon Sep 17 00:00:00 2001 From: Pranav-b-7 Date: Wed, 6 Mar 2024 17:00:58 +0530 Subject: [PATCH 1/2] 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. --- .../orca/igor/config/IgorConfiguration.groovy | 3 ++- .../igor/tasks/MonitorJenkinsJobTask.groovy | 8 +++--- .../tasks/MonitorQueuedJenkinsJobTask.groovy | 8 +++--- .../tasks/MonitorWerckerJobStartedTask.groovy | 9 +++---- .../igor/tasks/StartJenkinsJobTask.groovy | 11 ++++---- .../orca/igor/tasks/StartScriptTask.groovy | 10 +++---- .../orca/igor/tasks/RetryableIgorTask.java | 17 ++++++------ .../GetAwsCodeBuildArtifactsTaskSpec.groovy | 9 ++++--- .../tasks/GetBuildPropertiesTaskSpec.groovy | 5 +++- .../orca/igor/tasks/GetCommitsTaskSpec.groovy | 5 ++-- ...etGoogleCloudBuildArtifactsTaskSpec.groovy | 9 ++++--- .../tasks/MonitorAwsCodeBuildTaskSpec.groovy | 9 ++++--- .../MonitorGoogleCloudBuildTaskSpec.groovy | 9 ++++--- .../tasks/MonitorJenkinsJobTaskSpec.groovy | 10 ++++--- .../igor/tasks/RetryableIgorTaskSpec.groovy | 26 ++++++++++--------- .../igor/tasks/StartJenkinsJobTaskSpec.groovy | 9 ++++--- .../keel/ImportDeliveryConfigTaskTests.kt | 13 +++++----- orca-web/orca-web.gradle | 1 + .../controllers/OperationsController.groovy | 14 +++++++--- 19 files changed, 103 insertions(+), 82 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 index 8176a2c0e5..2fc638d15a 100644 --- 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 index 6aecaaecfa..10b5225c2f 100644 --- 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/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorQueuedJenkinsJobTask.groovy b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorQueuedJenkinsJobTask.groovy index 3a479d8f5b..7db9cd9c58 100644 --- a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorQueuedJenkinsJobTask.groovy +++ b/orca-igor/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/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorWerckerJobStartedTask.groovy b/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorWerckerJobStartedTask.groovy index 9fe96b056c..5fac717cf1 100644 --- a/orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorWerckerJobStartedTask.groovy +++ b/orca-igor/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 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 index 5138cfea44..24b3958413 100644 --- 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 index c41bf144f7..5811b072d2 100644 --- 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,13 +18,14 @@ 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 @@ -32,7 +33,6 @@ 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 index 97e004b198..c7abee72ec 100644 --- 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 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/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetAwsCodeBuildArtifactsTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetAwsCodeBuildArtifactsTaskSpec.groovy index fe160d668f..6166474ebb 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetAwsCodeBuildArtifactsTaskSpec.groovy +++ b/orca-igor/src/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/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTaskSpec.groovy index 43a57d7eaa..c5ca50c3e2 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTaskSpec.groovy +++ b/orca-igor/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 index ff13049e64..91256a84d4 100644 --- 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/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetGoogleCloudBuildArtifactsTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetGoogleCloudBuildArtifactsTaskSpec.groovy index a72aca200d..abc8e6fb7b 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetGoogleCloudBuildArtifactsTaskSpec.groovy +++ b/orca-igor/src/test/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/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorAwsCodeBuildTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorAwsCodeBuildTaskSpec.groovy index f9fcf3191a..fdd7e36efd 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorAwsCodeBuildTaskSpec.groovy +++ b/orca-igor/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 - } + }) } } diff --git a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorGoogleCloudBuildTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorGoogleCloudBuildTaskSpec.groovy index 68c7917d9a..74057951f6 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorGoogleCloudBuildTaskSpec.groovy +++ b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorGoogleCloudBuildTaskSpec.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 @@ -85,14 +86,14 @@ class MonitorGoogleCloudBuildTaskSpec extends Specification { TaskResult result = task.execute(stage) then: - 1 * igorService.getGoogleCloudBuild(ACCOUNT, BUILD_ID) >> { throw stubRetrofitError() } + 1 * igorService.getGoogleCloudBuild(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/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorJenkinsJobTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorJenkinsJobTaskSpec.groovy index 3929879b3c..45323115f7 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorJenkinsJobTaskSpec.groovy +++ b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/MonitorJenkinsJobTaskSpec.groovy @@ -16,6 +16,8 @@ package com.netflix.spinnaker.orca.igor.tasks +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus import com.netflix.spinnaker.orca.clouddriver.pipeline.job.model.JobStatus import com.netflix.spinnaker.orca.igor.BuildService @@ -111,8 +113,10 @@ class MonitorJenkinsJobTaskSpec extends Specification { getResponse() >> new Response('', httpStatus, '', [], null) } + def httpException = new SpinnakerHttpException(exception) + task.buildService = Stub(BuildService) { - getBuild(stage.context.buildNumber, stage.context.master, stage.context.job) >> { throw exception } + getBuild(stage.context.buildNumber, stage.context.master, stage.context.job) >> { throw httpException } } when: @@ -120,12 +124,12 @@ class MonitorJenkinsJobTaskSpec extends Specification { def thrownException = null try { result = task.execute(stage) - } catch (RetrofitError e) { + } catch (SpinnakerHttpException e) { thrownException = e } then: - thrownException ? thrownException == exception : result.status == expectedExecutionStatus + thrownException ? thrownException == httpException : result.status == expectedExecutionStatus where: httpStatus || expectedExecutionStatus diff --git a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTaskSpec.groovy index 409def020b..a40e2431f6 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTaskSpec.groovy +++ b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTaskSpec.groovy @@ -16,6 +16,8 @@ package com.netflix.spinnaker.orca.igor.tasks +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException +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.model.RetryableStageDefinition @@ -51,7 +53,7 @@ class RetryableIgorTaskSpec extends Specification { def result = task.execute(stage) then: - 1 * task.tryExecute(jobRequest) >> { throw stubRetrofitError(500) } + 1 * task.tryExecute(jobRequest) >> { throw stubHttpError(500) } jobRequest.getConsecutiveErrors() >> 0 result.status == ExecutionStatus.RUNNING result.context.get("consecutiveErrors") == 1 @@ -62,7 +64,7 @@ class RetryableIgorTaskSpec extends Specification { def result = task.execute(stage) then: - 1 * task.tryExecute(jobRequest) >> { throw stubRetrofitNetworkError() } + 1 * task.tryExecute(jobRequest) >> { throw stubNetworkError() } jobRequest.getConsecutiveErrors() >> 0 result.status == ExecutionStatus.RUNNING result.context.get("consecutiveErrors") == 1 @@ -73,9 +75,9 @@ class RetryableIgorTaskSpec extends Specification { def result = task.execute(stage) then: - 1 * task.tryExecute(jobRequest) >> { throw stubRetrofitError(404) } + 1 * task.tryExecute(jobRequest) >> { throw stubHttpError(404) } jobRequest.getConsecutiveErrors() >> 0 - thrown RetrofitError + thrown SpinnakerHttpException } def "should propagate the error we have reached the retry limit"() { @@ -83,9 +85,9 @@ class RetryableIgorTaskSpec extends Specification { def result = task.execute(stage) then: - 1 * task.tryExecute(jobRequest) >> { throw stubRetrofitError(500) } + 1 * task.tryExecute(jobRequest) >> { throw stubHttpError(500) } jobRequest.getConsecutiveErrors() >> 5 - thrown RetrofitError + thrown SpinnakerHttpException } def "should propagate a non-successful task status"() { @@ -109,16 +111,16 @@ class RetryableIgorTaskSpec extends Specification { result.context.get("consecutiveErrors") == 0 } - def stubRetrofitError(int statusCode) { - return Stub(RetrofitError) { + def stubHttpError(int statusCode) { + return new SpinnakerHttpException(Stub(RetrofitError) { getKind() >> RetrofitError.Kind.HTTP getResponse() >> new Response("", statusCode, "", Collections.emptyList(), null) - } + }) } - def stubRetrofitNetworkError() { - return Stub(RetrofitError) { + def stubNetworkError() { + return new SpinnakerNetworkException(Stub(RetrofitError) { getKind() >> RetrofitError.Kind.NETWORK - } + }) } } diff --git a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/StartJenkinsJobTaskSpec.groovy b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/StartJenkinsJobTaskSpec.groovy index 6aa397e5d9..a49f2f7f48 100644 --- a/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/StartJenkinsJobTaskSpec.groovy +++ b/orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/StartJenkinsJobTaskSpec.groovy @@ -17,11 +17,12 @@ package com.netflix.spinnaker.orca.igor.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.igor.BuildService import com.netflix.spinnaker.orca.pipeline.model.PipelineExecutionImpl import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl -import com.netflix.spinnaker.orca.retrofit.exceptions.RetrofitExceptionHandler +import com.netflix.spinnaker.orca.retrofit.exceptions.SpinnakerServerExceptionHandler import retrofit.RetrofitError import retrofit.client.Response import retrofit.mime.TypedString @@ -39,7 +40,7 @@ class StartJenkinsJobTaskSpec extends Specification { convertValue(_,_) >> [:] } - task.retrofitExceptionHandler = new RetrofitExceptionHandler() + task.spinnakerServerExceptionHandler = new SpinnakerServerExceptionHandler() } @Shared @@ -85,14 +86,14 @@ class StartJenkinsJobTaskSpec extends Specification { and: task.buildService = Stub(BuildService) { - build(stage.context.master, stage.context.job, stage.context.parameters, stage.startTime.toString()) >> {throw RetrofitError.unexpectedError("http://test", new RuntimeException())} + build(stage.context.master, stage.context.job, stage.context.parameters, stage.startTime.toString()) >> {throw new SpinnakerServerException(RetrofitError.unexpectedError("http://test", new RuntimeException()))} } when: task.execute(stage) then: - thrown(RetrofitError) + thrown(SpinnakerServerException) } def "handle 202 response from igor"() { diff --git a/orca-keel/src/test/kotlin/com/netflix/spinnaker/orca/keel/ImportDeliveryConfigTaskTests.kt b/orca-keel/src/test/kotlin/com/netflix/spinnaker/orca/keel/ImportDeliveryConfigTaskTests.kt index 39ea5b5efa..4caba7797b 100644 --- a/orca-keel/src/test/kotlin/com/netflix/spinnaker/orca/keel/ImportDeliveryConfigTaskTests.kt +++ b/orca-keel/src/test/kotlin/com/netflix/spinnaker/orca/keel/ImportDeliveryConfigTaskTests.kt @@ -18,6 +18,7 @@ package com.netflix.spinnaker.orca.keel import com.fasterxml.jackson.databind.ObjectMapper import com.fasterxml.jackson.module.kotlin.convertValue +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException import com.netflix.spinnaker.orca.KeelService import com.netflix.spinnaker.orca.api.pipeline.TaskResult @@ -288,11 +289,11 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests { manifestLocation.manifest, manifestLocation.ref ) - } throws RetrofitError.httpError( + } throws SpinnakerHttpException(RetrofitError.httpError( "http://igor", Response("http://igor", 404, "", emptyList(), null), null, null - ) + )) } } @@ -314,11 +315,11 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests { manifestLocation.manifest, manifestLocation.ref ) - } throws RetrofitError.httpError( + } throws SpinnakerHttpException(RetrofitError.httpError( "http://igor", Response("http://igor", 401, "", emptyList(), null), null, null - ) + )) } } @@ -341,11 +342,11 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests { manifestLocation.manifest, manifestLocation.ref ) - } throws RetrofitError.httpError( + } throws SpinnakerHttpException(RetrofitError.httpError( "http://igor", Response("http://igor", 503, "", emptyList(), null), null, null - ) + )) } } diff --git a/orca-web/orca-web.gradle b/orca-web/orca-web.gradle index 82ad37be6c..c9e6a4c1c5 100644 --- a/orca-web/orca-web.gradle +++ b/orca-web/orca-web.gradle @@ -63,6 +63,7 @@ dependencies { implementation("net.logstash.logback:logstash-logback-encoder") implementation("io.spinnaker.fiat:fiat-api:$fiatVersion") implementation("io.spinnaker.fiat:fiat-core:$fiatVersion") + implementation("io.spinnaker.kork:kork-retrofit") runtimeOnly("io.spinnaker.kork:kork-runtime") diff --git a/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy b/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy index c0a6cf9a64..1352c43fae 100644 --- a/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy +++ b/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy @@ -23,6 +23,8 @@ import com.netflix.spinnaker.fiat.shared.FiatService import com.netflix.spinnaker.fiat.shared.FiatStatus import com.netflix.spinnaker.kork.exceptions.ConfigurationException import com.netflix.spinnaker.kork.exceptions.SpinnakerException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution import com.netflix.spinnaker.orca.api.pipeline.models.Trigger import com.netflix.spinnaker.orca.clouddriver.service.JobService @@ -327,12 +329,14 @@ class OperationsController { def buildInfo try { buildInfo = buildService.getBuild(trigger.buildNumber, trigger.master, trigger.job) - } catch (RetrofitError e) { - if (e.response?.status == 404) { + } catch (SpinnakerHttpException e) { + if (e.responseCode == 404) { throw new ConfigurationException("Build ${trigger.buildNumber} of ${trigger.master}/${trigger.job} not found") } else { throw new OperationFailedException("Failed to get build ${trigger.buildNumber} of ${trigger.master}/${trigger.job}", e) } + } catch (SpinnakerServerException e){ + throw new OperationFailedException("Failed to get build ${trigger.buildNumber} of ${trigger.master}/${trigger.job}", e) } if (buildInfo?.artifacts) { if (trigger.type == "manual") { @@ -348,12 +352,14 @@ class OperationsController { trigger.master as String, trigger.job as String ) - } catch (RetrofitError e) { - if (e.response?.status == 404) { + } catch (SpinnakerHttpException e) { + if (e.responseCode == 404) { throw new ConfigurationException("Expected properties file " + trigger.propertyFile + " (configured on trigger), but it was missing") } else { throw new OperationFailedException("Failed to get properties file ${trigger.propertyFile}", e) } + } catch (SpinnakerServerException e){ + throw new OperationFailedException("Failed to get properties file ${trigger.propertyFile}", e) } } } From 32e6571ad0578b9745a30fde75a85e8ac0d2b6b1 Mon Sep 17 00:00:00 2001 From: Pranav-b-7 Date: Thu, 7 Mar 2024 17:10:30 +0530 Subject: [PATCH 2/2] 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 - c41792295d4b2bba289ab131699cc7b061ec627e, the keelService uses SpinnakerRetrofitErrorHandler. 2. Igor Service API - scmService.getDeliveryConfigManifest() : With the addition of the commit - d430b75efeced7eb1f8d64eff6f0a160210eb2e6, 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. --- .../keel/task/ImportDeliveryConfigTask.kt | 65 ------------------- 1 file changed, 65 deletions(-) diff --git a/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTask.kt b/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTask.kt index dd28ffb86e..186d8fb547 100644 --- a/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTask.kt +++ b/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTask.kt @@ -18,7 +18,6 @@ package com.netflix.spinnaker.orca.keel.task import com.fasterxml.jackson.databind.ObjectMapper import com.fasterxml.jackson.module.kotlin.convertValue -import com.fasterxml.jackson.module.kotlin.readValue import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException @@ -40,7 +39,6 @@ import java.time.Instant import java.util.concurrent.TimeUnit import org.slf4j.LoggerFactory import org.springframework.stereotype.Component -import retrofit.RetrofitError /** * Task that retrieves a Managed Delivery config manifest from source control via igor, then publishes it to keel, @@ -78,8 +76,6 @@ constructor( keelService.publishDeliveryConfig(deliveryConfig) TaskResult.builder(ExecutionStatus.SUCCEEDED).context(emptyMap()).build() - } catch (e: RetrofitError) { - handleRetryableFailures(e, context) } catch (e: SpinnakerServerException) { handleRetryableFailures(e, context) } catch (e: Exception) { @@ -221,48 +217,6 @@ constructor( } } - /** - * Handle (potentially) retryable failures by looking at the retrofit error type or HTTP status code. A few 40x errors - * are handled as special cases to provide more friendly error messages to the UI. - */ - private fun handleRetryableFailures(error: RetrofitError, context: ImportDeliveryConfigContext): TaskResult { - return when { - error.kind == RetrofitError.Kind.NETWORK -> { - // retry if unable to connect - buildRetry( - context, - "Network error talking to downstream service, attempt ${context.attempt} of ${context.maxRetries}: ${error.friendlyMessage}" - ) - } - error.response?.status in 400..499 -> { - val response = error.response!! - // just give up on 4xx errors, which are unlikely to resolve with retries, but give users a hint about 401 - // errors from igor/scm, and attempt to parse keel errors (which are typically more informative) - buildError( - if (error.fromIgor && response.status == 401) { - UNAUTHORIZED_SCM_ACCESS_MESSAGE - } else if (error.fromKeel && response.body.length() > 0) { - // keel's errors should use the standard Spring format, so we try to parse them - try { - objectMapper.readValue(response.body.`in`()) - } catch (_: Exception) { - "Non-retryable HTTP response ${error.response?.status} received from downstream service: ${error.friendlyMessage}" - } - } else { - "Non-retryable HTTP response ${error.response?.status} received from downstream service: ${error.friendlyMessage}" - } - ) - } - else -> { - // retry on other status codes - buildRetry( - context, - "Retryable HTTP response ${error.response?.status} received from downstream service: ${error.friendlyMessage}" - ) - } - } - } - /** * Builds a [TaskResult] that indicates the task is still running, so that we will try again in the next execution loop. */ @@ -301,13 +255,6 @@ constructor( override fun getTimeout() = TimeUnit.SECONDS.toMillis(180) - val RetrofitError.friendlyMessage: String - get() = if (kind == RetrofitError.Kind.HTTP) { - "HTTP ${response.status} ${response.url}: ${cause?.message ?: message}" - } else { - "$message: ${cause?.message ?: ""}" - } - val SpinnakerHttpException.httpErrorMessage: String get() { return "HTTP ${responseCode} ${url}: ${cause?.message ?: message}" @@ -323,24 +270,12 @@ constructor( return "$message" } - val RetrofitError.fromIgor: Boolean - get() { - val parsedUrl = URL(url) - return parsedUrl.host.contains("igor") || parsedUrl.port == 8085 - } - val SpinnakerServerException.fromIgor: Boolean get() { val parsedUrl = URL(url) return parsedUrl.host.contains("igor") || parsedUrl.port == 8085 } - val RetrofitError.fromKeel: Boolean - get() { - val parsedUrl = URL(url) - return parsedUrl.host.contains("keel") || parsedUrl.port == 8087 - } - val SpinnakerServerException.fromKeel: Boolean get() { val parsedUrl = URL(url)