From 6ae6207c91fb370f2ca26a95ba58b8d34ffbff22 Mon Sep 17 00:00:00 2001 From: Kiran Godishala <53332225+kirangodishala@users.noreply.github.com> Date: Tue, 19 Sep 2023 20:03:05 +0530 Subject: [PATCH] refactor(retrofit): use SpinnakerRetrofitErrorHandler as a step towards simplifying exception handling (#4529) - add retry support to createBake() method in CreateBakeTask - add retry support to bakeManifest() method in CreateBakeManifestTask --- orca-bakery/orca-bakery.gradle | 1 + .../bakery/config/BakeryConfiguration.groovy | 2 + .../orca/bakery/tasks/CreateBakeTask.groovy | 44 ++++++++++--------- .../manifests/CreateBakeManifestTask.java | 11 ++++- .../orca/bakery/api/BakeryServiceSpec.groovy | 4 +- .../bakery/tasks/CreateBakeTaskSpec.groovy | 9 ++-- 6 files changed, 45 insertions(+), 26 deletions(-) diff --git a/orca-bakery/orca-bakery.gradle b/orca-bakery/orca-bakery.gradle index 05166f4247..30d5ef446d 100644 --- a/orca-bakery/orca-bakery.gradle +++ b/orca-bakery/orca-bakery.gradle @@ -26,6 +26,7 @@ dependencies { implementation("com.fasterxml.jackson.datatype:jackson-datatype-guava") implementation("io.spinnaker.fiat:fiat-core:$fiatVersion") implementation("org.codehaus.groovy:groovy-datetime") + implementation("io.spinnaker.kork:kork-retrofit") api("io.spinnaker.kork:kork-web") diff --git a/orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/config/BakeryConfiguration.groovy b/orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/config/BakeryConfiguration.groovy index 225a827426..df212ead71 100644 --- a/orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/config/BakeryConfiguration.groovy +++ b/orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/config/BakeryConfiguration.groovy @@ -16,6 +16,7 @@ package com.netflix.spinnaker.orca.bakery.config +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler import com.netflix.spinnaker.orca.bakery.BakerySelector import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression import org.springframework.boot.context.properties.EnableConfigurationProperties @@ -80,6 +81,7 @@ class BakeryConfiguration { .setClient(retrofitClient) .setLogLevel(retrofitLogLevel) .setLog(new RetrofitSlf4jLog(BakeryService)) + .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) .build() .create(BakeryService) } diff --git a/orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/tasks/CreateBakeTask.groovy b/orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/tasks/CreateBakeTask.groovy index bf33000e29..f725aca9e0 100644 --- a/orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/tasks/CreateBakeTask.groovy +++ b/orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/tasks/CreateBakeTask.groovy @@ -19,6 +19,7 @@ package com.netflix.spinnaker.orca.bakery.tasks import com.fasterxml.jackson.databind.ObjectMapper import com.netflix.spinnaker.kork.artifacts.model.Artifact 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.RetryableTask import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution @@ -35,12 +36,14 @@ import com.netflix.spinnaker.orca.pipeline.util.PackageInfo import com.netflix.spinnaker.orca.pipeline.util.PackageType import groovy.transform.CompileDynamic import groovy.transform.CompileStatic +import java.time.Duration import org.slf4j.Logger import org.slf4j.LoggerFactory import org.springframework.beans.factory.annotation.Autowired import org.springframework.stereotype.Component import retrofit.RetrofitError + import static com.netflix.spinnaker.kork.web.selector.v2.SelectableService.* @Component @@ -61,7 +64,7 @@ class CreateBakeTask implements RetryableTask { @Autowired(required = false) Front50Service front50Service - RetrySupport retrySupport = new RetrySupport() + private final RetrySupport retrySupport = new RetrySupport() private final Logger log = LoggerFactory.getLogger(getClass()) @@ -76,7 +79,8 @@ class CreateBakeTask implements RetryableTask { try { if (front50Service != null) { String appName = stage.execution.application - Application application = retrySupport.retry({return front50Service.get(appName)}, 5, 2000, false) + Application application = + retrySupport.retry({ return front50Service.get(appName) }, 5, Duration.ofMillis(2000), false) String user = application.email if (user != null && user != "") { stage.context.user = user @@ -100,12 +104,16 @@ class CreateBakeTask implements RetryableTask { bake.amiSuffix = stage.context.amiSuffix } - def bakeStatus = bakery.service.createBake(stage.context.region as String, bake, rebake) + def bakeStatus = retrySupport.retry( + { return bakery.service.createBake(stage.context.region as String, bake, rebake) }, + 5, + Duration.ofMillis(2000), + false) def stageOutputs = [ - status : bakeStatus, - bakePackageName: bake.packageName ?: "", - previouslyBaked: bakeStatus.state == BakeStatus.State.COMPLETED + status : bakeStatus, + bakePackageName: bake.packageName ?: "", + previouslyBaked: bakeStatus.state == BakeStatus.State.COMPLETED ] as Map if (bake.buildInfoUrl) { @@ -114,9 +122,9 @@ class CreateBakeTask implements RetryableTask { if (bake.buildHost) { stageOutputs << [ - buildHost : bake.buildHost, - job : bake.job, - buildNumber: bake.buildNumber + buildHost : bake.buildHost, + job : bake.job, + buildNumber: bake.buildNumber ] if (bake.commitHash) { @@ -125,19 +133,13 @@ class CreateBakeTask implements RetryableTask { } TaskResult.builder(ExecutionStatus.SUCCEEDED).context(stageOutputs).build() - } catch (RetrofitError e) { - if (e.response?.status && e.response.status == 404) { - try { - def exceptionResult = mapper.readValue(e.response.body.in().text, Map) - def exceptionMessages = (exceptionResult.messages ?: []) as List - if (exceptionMessages) { - throw new IllegalStateException(exceptionMessages[0]) - } - } catch (IOException ignored) { - // do nothing + } catch (SpinnakerHttpException e) { + if (e.responseCode == 404) { + def exceptionMessages = + (e.responseBody?.messages ?: e.responseBody?.message ? [e.responseBody.message] : []) as List + if (exceptionMessages) { + throw new IllegalStateException(exceptionMessages[0]) } - - return TaskResult.ofStatus(ExecutionStatus.RUNNING) } throw e } diff --git a/orca-bakery/src/main/java/com/netflix/spinnaker/orca/bakery/tasks/manifests/CreateBakeManifestTask.java b/orca-bakery/src/main/java/com/netflix/spinnaker/orca/bakery/tasks/manifests/CreateBakeManifestTask.java index d386ec4743..a005080d8a 100644 --- a/orca-bakery/src/main/java/com/netflix/spinnaker/orca/bakery/tasks/manifests/CreateBakeManifestTask.java +++ b/orca-bakery/src/main/java/com/netflix/spinnaker/orca/bakery/tasks/manifests/CreateBakeManifestTask.java @@ -19,6 +19,7 @@ import com.netflix.spinnaker.kork.artifacts.model.Artifact; import com.netflix.spinnaker.kork.artifacts.model.ExpectedArtifact; +import com.netflix.spinnaker.kork.core.RetrySupport; 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; @@ -30,6 +31,7 @@ import com.netflix.spinnaker.orca.bakery.api.manifests.kustomize.KustomizeBakeManifestRequest; import com.netflix.spinnaker.orca.pipeline.util.ArtifactUtils; import com.netflix.spinnaker.orca.pipeline.util.ContextParameterProcessor; +import java.time.Duration; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -62,6 +64,8 @@ public long getTimeout() { private final ContextParameterProcessor contextParameterProcessor; + private final RetrySupport retrySupport = new RetrySupport(); + @Autowired public CreateBakeManifestTask( ArtifactUtils artifactUtils, @@ -146,7 +150,12 @@ public TaskResult execute(@Nonnull StageExecution stage) { "Invalid template renderer " + context.getTemplateRenderer()); } - Artifact result = bakery.bakeManifest(request.getTemplateRenderer(), request); + Artifact result = + retrySupport.retry( + () -> bakery.bakeManifest(request.getTemplateRenderer(), request), + 5, + Duration.ofMillis(2000), + false); Map outputs = new HashMap<>(); outputs.put("artifacts", Collections.singleton(result)); diff --git a/orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/api/BakeryServiceSpec.groovy b/orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/api/BakeryServiceSpec.groovy index 846cee166e..7338b0adeb 100644 --- a/orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/api/BakeryServiceSpec.groovy +++ b/orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/api/BakeryServiceSpec.groovy @@ -17,6 +17,7 @@ package com.netflix.spinnaker.orca.bakery.api import com.github.tomakehurst.wiremock.WireMockServer +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException import com.netflix.spinnaker.orca.bakery.config.BakeryConfiguration import com.netflix.spinnaker.orca.jackson.OrcaObjectMapper import retrofit.RequestInterceptor @@ -105,6 +106,7 @@ class BakeryServiceSpec extends Specification { .willReturn( aResponse() .withStatus(HTTP_NOT_FOUND) + .withBody("{\"message\": \"error\"}") ) ) @@ -112,7 +114,7 @@ class BakeryServiceSpec extends Specification { bakery.lookupStatus(region, statusId) then: - def ex = thrown(RetrofitError) + def ex = thrown(SpinnakerHttpException) ex.response.status == HTTP_NOT_FOUND } diff --git a/orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/tasks/CreateBakeTaskSpec.groovy b/orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/tasks/CreateBakeTaskSpec.groovy index 90e7963b30..20c1b115ae 100644 --- a/orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/tasks/CreateBakeTaskSpec.groovy +++ b/orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/tasks/CreateBakeTaskSpec.groovy @@ -17,6 +17,7 @@ package com.netflix.spinnaker.orca.bakery.tasks import com.netflix.spinnaker.kork.artifacts.model.Artifact +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException import com.netflix.spinnaker.kork.web.selector.v2.SelectableService import com.netflix.spinnaker.orca.api.pipeline.models.Trigger import com.netflix.spinnaker.orca.bakery.BakerySelector @@ -30,6 +31,7 @@ import com.netflix.spinnaker.orca.pipeline.util.ArtifactUtils import com.netflix.spinnaker.orca.pipeline.util.PackageType import retrofit.RetrofitError import retrofit.client.Response +import retrofit.converter.JacksonConverter import retrofit.mime.TypedString import spock.lang.Shared import spock.lang.Specification @@ -245,9 +247,10 @@ class CreateBakeTaskSpec extends Specification { def error404 = RetrofitError.httpError( null, new Response("", HTTP_NOT_FOUND, "Not Found", [], new TypedString("{ \"messages\": [\"Error Message\"]}")), - null, + new JacksonConverter(), null ) + def httpError404 = new SpinnakerHttpException(error404) def setup() { task.mapper = mapper @@ -332,8 +335,8 @@ class CreateBakeTaskSpec extends Specification { task.execute(bakeStage) then: - 1 * bakery.createBake(bakeConfig.region, _ as BakeRequest, null) >> { - throw error404 + 5 * bakery.createBake(bakeConfig.region, _ as BakeRequest, null) >> { + throw httpError404 } IllegalStateException e = thrown() e.message == "Error Message"