From ccdbea7e2b454c5f161c7e054b364e92c4e45bce Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 28 Mar 2023 08:33:24 -0700 Subject: [PATCH] Fix/blue green deploy (backport #4414) (#4420) * Fix/blue green deploy (#4414) * fix(blue-green-deploy): removing old failed manifests * fix(blue-green-deploy): refactoring * fix(blue-green-deploy): refactoring * fix(blue-green-deploy): added docs * fix(blue-green-deploy): added test to validate new behaviour (cherry picked from commit 5156eecb38de478a1723ccfa6cfcf4aabb6c3baf) # Conflicts: # orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/pipeline/manifest/DeployManifestStage.java # orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/pipeline/manifest/DeployManifestStageTest.java * chore(conflict): resolved conflicts (#4423) --------- Co-authored-by: Krzysztof Kotula Co-authored-by: ovidiupopa07 <105648914+ovidiupopa07@users.noreply.github.com> --- .../manifest/DeployManifestStage.java | 311 +++++++++++++----- .../manifest/DeployManifestStageTest.java | 146 ++++++-- 2 files changed, 355 insertions(+), 102 deletions(-) diff --git a/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/pipeline/manifest/DeployManifestStage.java b/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/pipeline/manifest/DeployManifestStage.java index 4b2494a7d8..eecd610053 100644 --- a/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/pipeline/manifest/DeployManifestStage.java +++ b/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/pipeline/manifest/DeployManifestStage.java @@ -31,27 +31,31 @@ import com.netflix.spinnaker.orca.clouddriver.tasks.artifacts.CleanupArtifactsTask; import com.netflix.spinnaker.orca.clouddriver.tasks.manifest.*; import com.netflix.spinnaker.orca.clouddriver.tasks.manifest.DeployManifestContext.TrafficManagement; -import com.netflix.spinnaker.orca.clouddriver.tasks.manifest.ResolveDeploySourceManifestTask; import com.netflix.spinnaker.orca.pipeline.ExpressionAwareStageDefinitionBuilder; import com.netflix.spinnaker.orca.pipeline.tasks.artifacts.BindProducedArtifactsTask; import com.netflix.spinnaker.orca.pipeline.util.ContextParameterProcessor; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.function.BiConsumer; import java.util.stream.Collectors; import javax.annotation.Nonnull; -import org.springframework.beans.factory.annotation.Autowired; +import lombok.Getter; +import lombok.RequiredArgsConstructor; +import org.jetbrains.annotations.NotNull; import org.springframework.stereotype.Component; @Component +@RequiredArgsConstructor public class DeployManifestStage extends ExpressionAwareStageDefinitionBuilder { + public static final String PIPELINE_CONFIG_TYPE = "deployManifest"; - private final OortService oortService; + private final OldManifestActionAppender oldManifestActionAppender; - @Autowired - public DeployManifestStage(OortService oortService) { - this.oortService = oortService; + private static boolean shouldRemoveStageOutputs(@NotNull StageExecution stage) { + return stage.getContext().getOrDefault("noOutput", "false").toString().equals("true"); } @Override @@ -68,102 +72,253 @@ public void taskGraph(@Nonnull StageExecution stage, @Nonnull TaskNode.Builder b .withTask(BindProducedArtifactsTask.TASK_NAME, BindProducedArtifactsTask.class); } + @Override + public boolean processExpressions( + @Nonnull StageExecution stage, + @Nonnull ContextParameterProcessor contextParameterProcessor, + @Nonnull ExpressionEvaluationSummary summary) { + DeployManifestContext context = stage.mapTo(DeployManifestContext.class); + if (context.isSkipExpressionEvaluation()) { + processDefaultEntries( + stage, contextParameterProcessor, summary, Collections.singletonList("manifests")); + return false; + } + return true; + } + + @Override public void afterStages(@Nonnull StageExecution stage, @Nonnull StageGraphBuilder graph) { TrafficManagement trafficManagement = stage.mapTo(DeployManifestContext.class).getTrafficManagement(); if (trafficManagement.isEnabled()) { switch (trafficManagement.getOptions().getStrategy()) { case RED_BLACK: - disableOldManifests(stage.getContext(), graph); + oldManifestActionAppender.deleteOrDisableOldManifest(stage.getContext(), graph); break; case HIGHLANDER: - disableOldManifests(stage.getContext(), graph); - deleteOldManifests(stage.getContext(), graph); + oldManifestActionAppender.disableOldManifest(stage.getContext(), graph); + oldManifestActionAppender.deleteOldManifest(stage.getContext(), graph); break; case NONE: // do nothing } } - if (stage.getContext().getOrDefault("noOutput", "false").toString().equals("true")) { + if (shouldRemoveStageOutputs(stage)) { stage.setOutputs(emptyMap()); } } - private void disableOldManifests(Map parentContext, StageGraphBuilder graph) { - addStagesForOldManifests(parentContext, graph, DisableManifestStage.PIPELINE_CONFIG_TYPE); - } + /** {@code OldManifestActionAppender} appends new stages to old manifests */ + @Component + @RequiredArgsConstructor + static class OldManifestActionAppender { - private void deleteOldManifests(Map parentContext, StageGraphBuilder graph) { - addStagesForOldManifests(parentContext, graph, DeleteManifestStage.PIPELINE_CONFIG_TYPE); - } + private final GetDeployedManifests deployedManifests; + private final ManifestOperationsHelper manifestOperationsHelper; - private void addStagesForOldManifests( - Map parentContext, StageGraphBuilder graph, String stageType) { - List> deployedManifests = getNewManifests(parentContext); - String account = (String) parentContext.get("account"); - Map manifestMoniker = (Map) parentContext.get("moniker"); - String application = (String) manifestMoniker.get("app"); - - deployedManifests.forEach( - manifest -> { - Map manifestMetadata = (Map) manifest.get("metadata"); - String manifestName = - String.format("replicaSet %s", (String) manifestMetadata.get("name")); - String namespace = (String) manifestMetadata.get("namespace"); - Map annotations = (Map) manifestMetadata.get("annotations"); - String clusterName = (String) annotations.get("moniker.spinnaker.io/cluster"); - String cloudProvider = "kubernetes"; - - ImmutableList previousManifestNames = - getOldManifestNames(application, account, clusterName, namespace, manifestName); - previousManifestNames.forEach( - name -> { - graph.append( - (stage) -> { - stage.setType(stageType); - Map context = stage.getContext(); - context.put("account", account); - context.put("app", application); - context.put("cloudProvider", cloudProvider); - context.put("manifestName", name); - context.put("location", namespace); - }); - }); - }); - } + /** + * Appends delete stages to already deployed manifests that preceded the current stage manifest + * + * @param parentContext of currently executed stage + * @param graph stage execution graph + */ + private void deleteOldManifest(Map parentContext, StageGraphBuilder graph) { + applyAction( + parentContext, + (name, manifest) -> + appendStage(graph, manifest, name, DeleteManifestStage.PIPELINE_CONFIG_TYPE)); + } + + /** + * Appends disable stages to already deployed manifests that preceded the current stage manifest + * + * @param parentContext of currently executed stage + * @param graph stage execution graph + */ + private void disableOldManifest(Map parentContext, StageGraphBuilder graph) { + applyAction( + parentContext, + (name, manifest) -> + appendStage(graph, manifest, name, DisableManifestStage.PIPELINE_CONFIG_TYPE)); + } + + /** + * Appends disable or delete stages to already deployed manifests that preceded the current + * stage manifest. The specific stage that will be appended depends on the status of the + * previous deployment. + * + * @param parentContext of currently executed stage + * @param graph stage execution graph + */ + private void deleteOrDisableOldManifest( + Map parentContext, StageGraphBuilder graph) { + applyAction( + parentContext, + (name, manifest) -> { + var oldManifestIsUnstable = + this.manifestOperationsHelper.previousDeploymentNeitherStableNorFailed( + manifest.getAccount(), name); + var nextStageType = + oldManifestIsUnstable + ? DeleteManifestStage.PIPELINE_CONFIG_TYPE + : DisableManifestStage.PIPELINE_CONFIG_TYPE; + appendStage(graph, manifest, name, nextStageType); + }); + } - private List> getNewManifests(Map parentContext) { - List> manifests = (List>) parentContext.get("outputs.manifests"); - return manifests.stream() - .filter(manifest -> manifest.get("kind").equals("ReplicaSet")) - .collect(Collectors.toList()); + private void applyAction( + Map parentContext, BiConsumer action) { + this.deployedManifests + .get(parentContext) + .forEach( + currentlyDeployedManifest -> + manifestOperationsHelper + .getOldManifestNames(currentlyDeployedManifest) + .forEach( + oldManifestName -> + action.accept(oldManifestName, currentlyDeployedManifest))); + } + + private void appendStage( + StageGraphBuilder graph, DeployedManifest manifest, String name, String stageType) { + graph.append( + (stage) -> { + stage.setType(stageType); + Map context = stage.getContext(); + context.put("account", manifest.getAccount()); + context.put("app", manifest.getApplication()); + context.put("cloudProvider", manifest.getCloudProvider()); + context.put("manifestName", name); + context.put("location", manifest.getNamespace()); + }); + } } - private ImmutableList getOldManifestNames( - String application, - String account, - String clusterName, - String namespace, - String newManifestName) { - return oortService - .getClusterManifests(account, namespace, "replicaSet", application, clusterName) - .stream() - .filter(m -> !m.getFullResourceName().equals(newManifestName)) - .map(ManifestCoordinates::getFullResourceName) - .collect(toImmutableList()); + /** + * Delegate class to handle all manifest-related actions in this file such as fetching manifest + * from external service or extracting manifest params from parentContext + */ + @Component + @RequiredArgsConstructor + static class ManifestOperationsHelper { + + private static final String REPLICA_SET = "replicaSet"; + private static final String KIND = "kind"; + private static final String OUTPUTS_MANIFEST = "outputs.manifests"; + + private final OortService oortService; + + /** + * This returns all replicaSet manifests from the cluster. The search is performed in an + * external service, and search parameters match manifests deployed in the current stage. + * + * @param dm - deployment manifest of current stage + * @return list of all manifest already deployed to the cluster + */ + ImmutableList getOldManifestNames(DeployedManifest dm) { + return oortService + .getClusterManifests( + dm.getAccount(), + dm.getNamespace(), + REPLICA_SET, + dm.getApplication(), + dm.getClusterName()) + .stream() + .filter(m -> !m.getFullResourceName().equals(dm.getManifestName())) + .map(ManifestCoordinates::getFullResourceName) + .collect(toImmutableList()); + } + + /** + * Returns replicaSet manifests from the {@code parentContext} + * + * @param parentContext of currently processed stage + * @return list of replicaSet manifests deployed in the cluster - obtained directly from the + * {@code parentContext} + */ + List> getNewManifests(Map parentContext) { + var manifestsFromParentContext = (List>) parentContext.get(OUTPUTS_MANIFEST); + return manifestsFromParentContext.stream() + .filter(manifest -> REPLICA_SET.equalsIgnoreCase((String) manifest.get(KIND))) + .collect(Collectors.toList()); + } + + /** + * During a B/G deployment, if the blue deployment fails to create pods (due to issues such as + * an incorrect image name), the deployment will not fail, but will wait indefinitely to achieve + * stability. This is indicated by status.failed=false and status.stable=false. This method + * checks for such a situation. + * + * @param account used to run deployment + * @param name of the manifest + * @return true, if manifest was not deployed correctly and waits to get stable, false otherwise + */ + boolean previousDeploymentNeitherStableNorFailed(String account, String name) { + var oldManifest = this.oortService.getManifest(account, name, false); + + var status = oldManifest.getStatus(); + var notStable = !status.getStable().isState(); + var notFailed = !status.getFailed().isState(); + + return notFailed && notStable; + } } - @Override - public boolean processExpressions( - @Nonnull StageExecution stage, - @Nonnull ContextParameterProcessor contextParameterProcessor, - @Nonnull ExpressionEvaluationSummary summary) { - DeployManifestContext context = stage.mapTo(DeployManifestContext.class); - if (context.isSkipExpressionEvaluation()) { - processDefaultEntries( - stage, contextParameterProcessor, summary, Collections.singletonList("manifests")); - return false; + /** Delegate class for fetching and mapping manifests deployed in the cluster */ + @Component + @RequiredArgsConstructor + static class GetDeployedManifests { + + private final ManifestOperationsHelper manifestOperationsHelper; + + /** + * This method encapsulates fetching deployed manifests and mapping them to a new designated + * type, {@code DeployedManifest} + * + * @param parentContext is the context of currently processed stage + * @return list of replicaSet manifests deployed in currently processed stage + */ + List get(Map parentContext) { + + var deployedManifests = new ArrayList(); + var account = (String) parentContext.get("account"); + var manifestMoniker = (Map) parentContext.get("moniker"); + var application = (String) manifestMoniker.get("app"); + + this.manifestOperationsHelper + .getNewManifests(parentContext) + .forEach( + manifest -> { + var manifestMetadata = (Map) manifest.get("metadata"); + var annotations = (Map) manifestMetadata.get("annotations"); + + deployedManifests.add( + new DeployedManifest( + account, + manifestMoniker, + application, + (Map) manifest.get("metadata"), + String.format("replicaSet %s", manifestMetadata.get("name")), + (String) manifestMetadata.get("namespace"), + (Map) manifestMetadata.get("annotations"), + (String) annotations.get("moniker.spinnaker.io/cluster"), + "kubernetes")); + }); + return deployedManifests; } - return true; + } + + @Getter + @RequiredArgsConstructor + static class DeployedManifest { + private final String account; + private final Map manifestMoniker; + private final String application; + private final Map manifestMetadata; + private final String manifestName; + private final String namespace; + private final Map annotations; + private final String clusterName; + private final String cloudProvider; } } diff --git a/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/pipeline/manifest/DeployManifestStageTest.java b/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/pipeline/manifest/DeployManifestStageTest.java index 29d674ebd4..bd737be2e3 100644 --- a/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/pipeline/manifest/DeployManifestStageTest.java +++ b/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/pipeline/manifest/DeployManifestStageTest.java @@ -17,6 +17,9 @@ package com.netflix.spinnaker.orca.clouddriver.pipeline.manifest; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import com.fasterxml.jackson.core.type.TypeReference; @@ -26,7 +29,11 @@ import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionType; import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution; import com.netflix.spinnaker.orca.clouddriver.OortService; +import com.netflix.spinnaker.orca.clouddriver.model.Manifest; import com.netflix.spinnaker.orca.clouddriver.model.ManifestCoordinates; +import com.netflix.spinnaker.orca.clouddriver.pipeline.manifest.DeployManifestStage.GetDeployedManifests; +import com.netflix.spinnaker.orca.clouddriver.pipeline.manifest.DeployManifestStage.ManifestOperationsHelper; +import com.netflix.spinnaker.orca.clouddriver.pipeline.manifest.DeployManifestStage.OldManifestActionAppender; import com.netflix.spinnaker.orca.clouddriver.tasks.manifest.DeployManifestContext; import com.netflix.spinnaker.orca.clouddriver.tasks.manifest.DeployManifestContext.TrafficManagement.ManifestStrategyType; import com.netflix.spinnaker.orca.jackson.OrcaObjectMapper; @@ -35,9 +42,9 @@ import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl; import java.util.Map; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; @ExtendWith(MockitoExtension.class) @@ -48,12 +55,43 @@ final class DeployManifestStageTest { private static final String CLUSTER = "replicaSet my-rs"; private static final String NAMESAPCE = "my-ns"; - @Mock private OortService oortService; + private OortService oortService; + private ManifestOperationsHelper manifestOperationsHelper; + private GetDeployedManifests getDeployedManifests; private DeployManifestStage deployManifestStage; + private OldManifestActionAppender oldManifestActionAppender; + + private static Map getContext(DeployManifestContext deployManifestContext) { + Map context = + objectMapper.convertValue( + deployManifestContext, new TypeReference>() {}); + context.put("moniker", ImmutableMap.of("app", APPLICATION)); + context.put("account", ACCOUNT); + context.put( + "outputs.manifests", + ImmutableList.of( + ImmutableMap.of( + "kind", + "ReplicaSet", + "metadata", + ImmutableMap.of( + "name", + "my-rs-v001", + "namespace", + NAMESAPCE, + "annotations", + ImmutableMap.of("moniker.spinnaker.io/cluster", CLUSTER))))); + return context; + } @BeforeEach void setUp() { - deployManifestStage = new DeployManifestStage(oortService); + oortService = mock(OortService.class); + manifestOperationsHelper = new ManifestOperationsHelper(oortService); + getDeployedManifests = new GetDeployedManifests(manifestOperationsHelper); + oldManifestActionAppender = + new OldManifestActionAppender(getDeployedManifests, manifestOperationsHelper); + deployManifestStage = new DeployManifestStage(oldManifestActionAppender); } @Test @@ -70,6 +108,7 @@ void rolloutStrategyDisabled() { @Test void rolloutStrategyRedBlack() { + givenManifestIsStable(); when(oortService.getClusterManifests(ACCOUNT, NAMESAPCE, "replicaSet", APPLICATION, CLUSTER)) .thenReturn( ImmutableList.of( @@ -117,6 +156,57 @@ void rolloutStrategyRedBlack() { .containsExactly("replicaSet my-rs-v000"); } + @Test + @DisplayName("red/black deployment should trigger old manifest deletion if it was failed") + void rolloutBlueGreenStrategyDeletesOldManifest() { + givenManifestIsNotStable(); + when(oortService.getClusterManifests(ACCOUNT, NAMESAPCE, "replicaSet", APPLICATION, CLUSTER)) + .thenReturn( + ImmutableList.of( + ManifestCoordinates.builder() + .name("my-rs-v000") + .kind("replicaSet") + .namespace(NAMESAPCE) + .build(), + ManifestCoordinates.builder() + .name("my-rs-v001") + .kind("replicaSet") + .namespace(NAMESAPCE) + .build())); + Map context = + getContext( + DeployManifestContext.builder() + .trafficManagement( + DeployManifestContext.TrafficManagement.builder() + .enabled(true) + .options( + DeployManifestContext.TrafficManagement.Options.builder() + .strategy(ManifestStrategyType.RED_BLACK) + .build()) + .build()) + .build()); + StageExecutionImpl stage = + new StageExecutionImpl( + new PipelineExecutionImpl(ExecutionType.PIPELINE, APPLICATION), + DeployManifestStage.PIPELINE_CONFIG_TYPE, + context); + assertThat(getAfterStages(stage)) + .extracting(StageExecution::getType) + .containsExactly(DeleteManifestStage.PIPELINE_CONFIG_TYPE); + assertThat(getAfterStages(stage)) + .extracting(s -> s.getContext().get("account")) + .containsExactly(ACCOUNT); + assertThat(getAfterStages(stage)) + .extracting(s -> s.getContext().get("app")) + .containsExactly(APPLICATION); + assertThat(getAfterStages(stage)) + .extracting(s -> s.getContext().get("location")) + .containsExactly(NAMESAPCE); + assertThat(getAfterStages(stage)) + .extracting(s -> s.getContext().get("manifestName")) + .containsExactly("replicaSet my-rs-v000"); + } + @Test void rolloutStrategyHighlander() { when(oortService.getClusterManifests(ACCOUNT, NAMESAPCE, "replicaSet", APPLICATION, CLUSTER)) @@ -203,26 +293,34 @@ private Iterable getAfterStages(StageExecutionImpl stage) { return graph.build(); } - private static Map getContext(DeployManifestContext deployManifestContext) { - Map context = - objectMapper.convertValue( - deployManifestContext, new TypeReference>() {}); - context.put("moniker", ImmutableMap.of("app", APPLICATION)); - context.put("account", ACCOUNT); - context.put( - "outputs.manifests", - ImmutableList.of( - ImmutableMap.of( - "kind", - "ReplicaSet", - "metadata", - ImmutableMap.of( - "name", - "my-rs-v001", - "namespace", - NAMESAPCE, - "annotations", - ImmutableMap.of("moniker.spinnaker.io/cluster", CLUSTER))))); - return context; + private void givenManifestIsStable() { + + var manifest = + Manifest.builder() + .status( + Manifest.Status.builder() + .stable(Manifest.Condition.emptyTrue()) + .failed(Manifest.Condition.emptyFalse()) + .build()) + .build(); + + givenManifestIs(manifest); + } + + private void givenManifestIsNotStable() { + var manifest = + Manifest.builder() + .status( + Manifest.Status.builder() + .stable(Manifest.Condition.emptyFalse()) + .failed(Manifest.Condition.emptyFalse()) + .build()) + .build(); + + givenManifestIs(manifest); + } + + private void givenManifestIs(Manifest manifest) { + when(oortService.getManifest(anyString(), anyString(), anyBoolean())).thenReturn(manifest); } }