From 22e912efe7bc465dfd7881639b8d53a141a83116 Mon Sep 17 00:00:00 2001 From: Jesse Suen Date: Fri, 29 Sep 2023 15:01:46 -0700 Subject: [PATCH] fix: prevent hot loop when fully promoted rollout is aborted (#3064) * fix: prevent hot loop when fully promoted rollout is aborted Signed-off-by: Jesse Suen * test: change expectations of abort tests Signed-off-by: Jesse Suen --------- Signed-off-by: Jesse Suen Signed-off-by: Philip Clark --- experiments/controller_test.go | 6 ++-- experiments/experiment_test.go | 2 +- experiments/replicaset_test.go | 4 +-- rollout/analysis_test.go | 64 +++++++++++++++++----------------- rollout/bluegreen_test.go | 16 ++++----- rollout/canary_test.go | 51 +++++++++++++-------------- rollout/controller.go | 5 +++ rollout/controller_test.go | 4 +-- rollout/experiment_test.go | 14 ++++---- rollout/service_test.go | 4 +-- test/e2e/istio_test.go | 4 +-- 11 files changed, 89 insertions(+), 85 deletions(-) diff --git a/experiments/controller_test.go b/experiments/controller_test.go index 26587b6346..0103e03ce1 100644 --- a/experiments/controller_test.go +++ b/experiments/controller_test.go @@ -805,7 +805,7 @@ func TestAddInvalidSpec(t *testing.T) { "status":{ } }`, nil, cond) - assert.Equal(t, expectedPatch, patch) + assert.JSONEq(t, expectedPatch, patch) } func TestKeepInvalidSpec(t *testing.T) { @@ -852,7 +852,7 @@ func TestUpdateInvalidSpec(t *testing.T) { "status":{ } }`, nil, cond) - assert.Equal(t, expectedPatch, patch) + assert.JSONEq(t, expectedPatch, patch) } @@ -892,7 +892,7 @@ func TestRemoveInvalidSpec(t *testing.T) { "status":{ } }`, templateStatus, cond) - assert.Equal(t, expectedPatch, patch) + assert.JSONEq(t, expectedPatch, patch) } func TestRun(t *testing.T) { diff --git a/experiments/experiment_test.go b/experiments/experiment_test.go index e047082cee..21fbeb530a 100644 --- a/experiments/experiment_test.go +++ b/experiments/experiment_test.go @@ -282,7 +282,7 @@ func TestSuccessAfterDurationPasses(t *testing.T) { "phase": "Successful" } }`, templateStatuses, cond) - assert.Equal(t, expectedPatch, patch) + assert.JSONEq(t, expectedPatch, patch) } // TestDontRequeueWithoutDuration verifies we don't requeue if an experiment does not have diff --git a/experiments/replicaset_test.go b/experiments/replicaset_test.go index e4d1cdf231..030414f2df 100644 --- a/experiments/replicaset_test.go +++ b/experiments/replicaset_test.go @@ -42,7 +42,7 @@ func TestCreateMultipleRS(t *testing.T) { "status":{ } }`, templateStatus, cond) - assert.Equal(t, expectedPatch, patch) + assert.JSONEq(t, expectedPatch, patch) } func TestCreateMissingRS(t *testing.T) { @@ -72,7 +72,7 @@ func TestCreateMissingRS(t *testing.T) { generateTemplatesStatus("bar", 0, 0, v1alpha1.TemplateStatusProgressing, now()), generateTemplatesStatus("baz", 0, 0, v1alpha1.TemplateStatusProgressing, now()), } - assert.Equal(t, calculatePatch(e, expectedPatch, templateStatuses, cond), patch) + assert.JSONEq(t, calculatePatch(e, expectedPatch, templateStatuses, cond), patch) } func TestTemplateHasMultipleRS(t *testing.T) { diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index f0313d7ea0..de9a5e1db3 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -180,7 +180,7 @@ func TestCreateBackgroundAnalysisRun(t *testing.T) { } } }` - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, expectedArName)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, expectedArName)), patch) } func TestCreateBackgroundAnalysisRunWithTemplates(t *testing.T) { @@ -241,7 +241,7 @@ func TestCreateBackgroundAnalysisRunWithTemplates(t *testing.T) { } } }` - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, expectedArName)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, expectedArName)), patch) } func TestCreateBackgroundAnalysisRunWithClusterTemplates(t *testing.T) { @@ -303,7 +303,7 @@ func TestCreateBackgroundAnalysisRunWithClusterTemplates(t *testing.T) { } } }` - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, expectedArName)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, expectedArName)), patch) } func TestInvalidSpecMissingClusterTemplatesBackgroundAnalysis(t *testing.T) { @@ -339,7 +339,7 @@ func TestInvalidSpecMissingClusterTemplatesBackgroundAnalysis(t *testing.T) { expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, progressingCond, string(invalidSpecBytes), strings.ReplaceAll(errmsg, "\"", "\\\"")) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r, expectedPatch), patch) } func TestCreateBackgroundAnalysisRunWithClusterTemplatesAndTemplate(t *testing.T) { @@ -416,7 +416,7 @@ func TestCreateBackgroundAnalysisRunWithClusterTemplatesAndTemplate(t *testing.T } } }` - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, expectedArName)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, expectedArName)), patch) } // TestCreateAnalysisRunWithCollision ensures we will create an new analysis run with a new name @@ -487,7 +487,7 @@ func TestCreateAnalysisRunWithCollision(t *testing.T) { } } }` - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, expectedAR.Name)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, expectedAR.Name)), patch) } // TestCreateAnalysisRunWithCollisionAndSemanticEquality will ensure we do not create an extra @@ -550,7 +550,7 @@ func TestCreateAnalysisRunWithCollisionAndSemanticEquality(t *testing.T) { } } }` - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, ar.Name)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, ar.Name)), patch) } func TestCreateAnalysisRunOnAnalysisStep(t *testing.T) { @@ -611,7 +611,7 @@ func TestCreateAnalysisRunOnAnalysisStep(t *testing.T) { } } }` - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, expectedArName)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, expectedArName)), patch) } func TestFailCreateStepAnalysisRunIfInvalidTemplateRef(t *testing.T) { @@ -653,7 +653,7 @@ func TestFailCreateStepAnalysisRunIfInvalidTemplateRef(t *testing.T) { expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, progressingCond, string(invalidSpecBytes), strings.ReplaceAll(errmsg, "\"", "\\\"")) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r, expectedPatch), patch) } func TestFailCreateBackgroundAnalysisRunIfInvalidTemplateRef(t *testing.T) { @@ -698,7 +698,7 @@ func TestFailCreateBackgroundAnalysisRunIfInvalidTemplateRef(t *testing.T) { expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, progressingCond, string(invalidSpecBytes), strings.ReplaceAll(errmsg, "\"", "\\\"")) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r, expectedPatch), patch) } func TestFailCreateBackgroundAnalysisRunIfMetricRepeated(t *testing.T) { @@ -745,7 +745,7 @@ func TestFailCreateBackgroundAnalysisRunIfMetricRepeated(t *testing.T) { expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, progressingCond, string(invalidSpecBytes), strings.ReplaceAll(errmsg, "\"", "\\\"")) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r, expectedPatch), patch) } func TestDoNothingWithAnalysisRunsWhileBackgroundAnalysisRunRunning(t *testing.T) { @@ -798,7 +798,7 @@ func TestDoNothingWithAnalysisRunsWhileBackgroundAnalysisRunRunning(t *testing.T patchIndex := f.expectPatchRolloutAction(r2) f.run(getKey(r2, t)) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) + assert.JSONEq(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) } func TestDoNothingWhileStepBasedAnalysisRunRunning(t *testing.T) { @@ -847,7 +847,7 @@ func TestDoNothingWhileStepBasedAnalysisRunRunning(t *testing.T) { patchIndex := f.expectPatchRolloutAction(r2) f.run(getKey(r2, t)) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) + assert.JSONEq(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) } func TestCancelOlderAnalysisRuns(t *testing.T) { @@ -915,7 +915,7 @@ func TestCancelOlderAnalysisRuns(t *testing.T) { } } }` - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestDeleteAnalysisRunsWithNoMatchingRS(t *testing.T) { @@ -971,7 +971,7 @@ func TestDeleteAnalysisRunsWithNoMatchingRS(t *testing.T) { deletedAr := f.getDeletedAnalysisRun(deletedIndex) assert.Equal(t, deletedAr, arWithDiffPodHash.Name) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) + assert.JSONEq(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) } func TestDeleteAnalysisRunsAfterRSDelete(t *testing.T) { @@ -1083,7 +1083,7 @@ func TestIncrementStepAfterSuccessfulAnalysisRun(t *testing.T) { }` condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false, "", false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition)), patch) } func TestPausedOnInconclusiveBackgroundAnalysisRun(t *testing.T) { @@ -1152,7 +1152,7 @@ func TestPausedOnInconclusiveBackgroundAnalysisRun(t *testing.T) { }` condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, v1alpha1.PauseReasonInconclusiveAnalysis, now, v1alpha1.PauseReasonInconclusiveAnalysis)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, v1alpha1.PauseReasonInconclusiveAnalysis, now, v1alpha1.PauseReasonInconclusiveAnalysis)), patch) } func TestPausedStepAfterInconclusiveAnalysisRun(t *testing.T) { @@ -1215,7 +1215,7 @@ func TestPausedStepAfterInconclusiveAnalysisRun(t *testing.T) { } }` condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, v1alpha1.PauseReasonInconclusiveAnalysis, now, v1alpha1.PauseReasonInconclusiveAnalysis)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, v1alpha1.PauseReasonInconclusiveAnalysis, now, v1alpha1.PauseReasonInconclusiveAnalysis)), patch) } func TestErrorConditionAfterErrorAnalysisRunStep(t *testing.T) { @@ -1282,7 +1282,7 @@ func TestErrorConditionAfterErrorAnalysisRunStep(t *testing.T) { errmsg := fmt.Sprintf(conditions.RolloutAbortedMessage, 2) + ": " + ar.Status.Message condition := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, errmsg, false) expectedPatch = fmt.Sprintf(expectedPatch, condition, now, errmsg) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestErrorConditionAfterErrorAnalysisRunBackground(t *testing.T) { @@ -1358,7 +1358,7 @@ func TestErrorConditionAfterErrorAnalysisRunBackground(t *testing.T) { condition := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, "", false) now := timeutil.Now().UTC().Format(time.RFC3339) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, now, errmsg)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, now, errmsg)), patch) } func TestCancelAnalysisRunsWhenAborted(t *testing.T) { @@ -1419,7 +1419,7 @@ func TestCancelAnalysisRunsWhenAborted(t *testing.T) { }` errmsg := fmt.Sprintf(conditions.RolloutAbortedMessage, 2) now := timeutil.Now().UTC().Format(time.RFC3339) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, newConditions, now, errmsg)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, newConditions, now, errmsg)), patch) } func TestCancelBackgroundAnalysisRunWhenRolloutIsCompleted(t *testing.T) { @@ -1521,7 +1521,7 @@ func TestDoNotCreateBackgroundAnalysisRunAfterInconclusiveRun(t *testing.T) { f.run(getKey(r2, t)) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) + assert.JSONEq(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) } func TestDoNotCreateBackgroundAnalysisRunOnNewCanaryRollout(t *testing.T) { @@ -1647,7 +1647,7 @@ func TestCreatePrePromotionAnalysisRun(t *testing.T) { } } }`, ar.Name) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } // TestDoNotCreatePrePromotionAnalysisProgressedRollout ensures a pre-promotion analysis is not created after a Rollout @@ -1771,7 +1771,7 @@ func TestDoNotCreatePrePromotionAnalysisRunOnNotReadyReplicaSet(t *testing.T) { f.run(getKey(r2, t)) patch := f.getPatchedRollout(patchRolloutIndex) - assert.Equal(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) + assert.JSONEq(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) } func TestRolloutPrePromotionAnalysisBecomesInconclusive(t *testing.T) { @@ -1841,7 +1841,7 @@ func TestRolloutPrePromotionAnalysisBecomesInconclusive(t *testing.T) { } } }`, now, now) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestRolloutPrePromotionAnalysisSwitchServiceAfterSuccess(t *testing.T) { @@ -1905,7 +1905,7 @@ func TestRolloutPrePromotionAnalysisSwitchServiceAfterSuccess(t *testing.T) { "message": null } }`, rs2PodHash, rs2PodHash, rs2PodHash) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestRolloutPrePromotionAnalysisHonorAutoPromotionSeconds(t *testing.T) { @@ -1971,7 +1971,7 @@ func TestRolloutPrePromotionAnalysisHonorAutoPromotionSeconds(t *testing.T) { "message": null } }`, rs2PodHash, rs2PodHash, rs2PodHash) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestRolloutPrePromotionAnalysisDoNothingOnInconclusiveAnalysis(t *testing.T) { @@ -2096,7 +2096,7 @@ func TestAbortRolloutOnErrorPrePromotionAnalysis(t *testing.T) { now := timeutil.MetaNow().UTC().Format(time.RFC3339) progressingFalseAborted, _ := newProgressingCondition(conditions.RolloutAbortedReason, r2, "") newConditions := updateConditionsPatch(*r2, progressingFalseAborted) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, now, newConditions, conditions.RolloutAbortedReason, progressingFalseAborted.Message)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, now, newConditions, conditions.RolloutAbortedReason, progressingFalseAborted.Message)), patch) } func TestCreatePostPromotionAnalysisRun(t *testing.T) { @@ -2143,7 +2143,7 @@ func TestCreatePostPromotionAnalysisRun(t *testing.T) { } } }`, ar.Name) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestRolloutPostPromotionAnalysisSuccess(t *testing.T) { @@ -2199,7 +2199,7 @@ func TestRolloutPostPromotionAnalysisSuccess(t *testing.T) { "message": null } }`, rs2PodHash) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } // TestPostPromotionAnalysisRunHandleInconclusive ensures that the Rollout does not scale down a old ReplicaSet if @@ -2264,7 +2264,7 @@ func TestPostPromotionAnalysisRunHandleInconclusive(t *testing.T) { "message": "InconclusiveAnalysisRun" } }`) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestAbortRolloutOnErrorPostPromotionAnalysis(t *testing.T) { @@ -2334,7 +2334,7 @@ func TestAbortRolloutOnErrorPostPromotionAnalysis(t *testing.T) { now := timeutil.MetaNow().UTC().Format(time.RFC3339) progressingFalseAborted, _ := newProgressingCondition(conditions.RolloutAbortedReason, r2, "") newConditions := updateConditionsPatch(*r2, progressingFalseAborted) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, now, newConditions, conditions.RolloutAbortedReason, progressingFalseAborted.Message)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, now, newConditions, conditions.RolloutAbortedReason, progressingFalseAborted.Message)), patch) } func TestCreateAnalysisRunWithCustomAnalysisRunMetadataAndROCopyLabels(t *testing.T) { diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 42b521a565..cff894e8ca 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -290,7 +290,7 @@ func TestBlueGreenHandlePause(t *testing.T) { f.run(getKey(r2, t)) patch := f.getPatchedRollout(patchRolloutIndex) - assert.Equal(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) + assert.JSONEq(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) }) t.Run("AddPause", func(t *testing.T) { f := newFixture(t) @@ -334,7 +334,7 @@ func TestBlueGreenHandlePause(t *testing.T) { } }` now := timeutil.Now().UTC().Format(time.RFC3339) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, v1alpha1.PauseReasonBlueGreenPause, now)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, v1alpha1.PauseReasonBlueGreenPause, now)), patch) }) @@ -376,7 +376,7 @@ func TestBlueGreenHandlePause(t *testing.T) { } }` addedConditions := generateConditionsPatchWithPause(true, conditions.RolloutPausedReason, rs2, true, "", true, false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, addedConditions)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, addedConditions)), patch) }) t.Run("NoActionsAfterPausing", func(t *testing.T) { @@ -417,7 +417,7 @@ func TestBlueGreenHandlePause(t *testing.T) { patchIndex := f.expectPatchRolloutActionWithPatch(r2, OnlyObservedGenerationPatch) f.run(getKey(r2, t)) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) + assert.JSONEq(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) }) t.Run("NoActionsAfterPausedOnInconclusiveRun", func(t *testing.T) { @@ -468,7 +468,7 @@ func TestBlueGreenHandlePause(t *testing.T) { patchIndex := f.expectPatchRolloutActionWithPatch(r2, OnlyObservedGenerationPatch) f.run(getKey(r2, t)) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) + assert.JSONEq(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) }) t.Run("NoAutoPromoteBeforeDelayTimePasses", func(t *testing.T) { @@ -509,7 +509,7 @@ func TestBlueGreenHandlePause(t *testing.T) { patchIndex := f.expectPatchRolloutActionWithPatch(r2, OnlyObservedGenerationPatch) f.run(getKey(r2, t)) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) + assert.JSONEq(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) }) t.Run("AutoPromoteAfterDelayTimePasses", func(t *testing.T) { @@ -813,7 +813,7 @@ func TestBlueGreenHandlePause(t *testing.T) { "conditions": %s } }` - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedUnpausePatch, unpauseConditions)), unpausePatch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedUnpausePatch, unpauseConditions)), unpausePatch) generatedConditions := generateConditionsPatchWithCompleted(true, conditions.ReplicaSetUpdatedReason, rs2, true, "", true) expected2ndPatchWithoutSubs := `{ @@ -1453,7 +1453,7 @@ func TestBlueGreenAbort(t *testing.T) { } }`, rs1PodHash, expectedConditions, rs1PodHash, conditions.RolloutAbortedReason, fmt.Sprintf(conditions.RolloutAbortedMessage, 2)) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestBlueGreenHandlePauseAutoPromoteWithConditions(t *testing.T) { diff --git a/rollout/canary_test.go b/rollout/canary_test.go index e3bffff2dd..4adca3fcb9 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -182,7 +182,7 @@ func TestCanaryRolloutEnterPauseState(t *testing.T) { now := timeutil.MetaNow().UTC().Format(time.RFC3339) expectedPatchWithoutObservedGen := fmt.Sprintf(expectedPatchTemplate, v1alpha1.PauseReasonCanaryPauseStep, now, conditions, v1alpha1.PauseReasonCanaryPauseStep) expectedPatch := calculatePatch(r2, expectedPatchWithoutObservedGen) - assert.Equal(t, expectedPatch, patch) + assert.JSONEq(t, expectedPatch, patch) } func TestCanaryRolloutNoProgressWhilePaused(t *testing.T) { @@ -257,7 +257,7 @@ func TestCanaryRolloutUpdatePauseConditionWhilePaused(t *testing.T) { f.run(getKey(r2, t)) patch := f.getPatchedRollout(addPausedConditionPatch) - assert.Equal(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) + assert.JSONEq(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) } func TestCanaryRolloutResetProgressDeadlineOnRetry(t *testing.T) { @@ -300,7 +300,7 @@ func TestCanaryRolloutResetProgressDeadlineOnRetry(t *testing.T) { "message": "more replicas need to be updated" } }`, retryCondition) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestCanaryRolloutIncrementStepAfterUnPaused(t *testing.T) { @@ -342,7 +342,7 @@ func TestCanaryRolloutIncrementStepAfterUnPaused(t *testing.T) { }` generatedConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false, "", false) expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchTemplate, generatedConditions)) - assert.Equal(t, expectedPatch, patch) + assert.JSONEq(t, expectedPatch, patch) } func TestCanaryRolloutUpdateStatusWhenAtEndOfSteps(t *testing.T) { @@ -383,7 +383,7 @@ func TestCanaryRolloutUpdateStatusWhenAtEndOfSteps(t *testing.T) { }` expectedPatch := fmt.Sprintf(expectedPatchWithoutStableRS, expectedStableRS, generateConditionsPatchWithCompleted(true, conditions.ReplicaSetUpdatedReason, rs2, false, "", true)) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestResetCurrentStepIndexOnStepChange(t *testing.T) { @@ -426,7 +426,7 @@ func TestResetCurrentStepIndexOnStepChange(t *testing.T) { }` newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) expectedPatch := fmt.Sprintf(expectedPatchWithoutPodHash, expectedCurrentPodHash, expectedCurrentStepHash, newConditions) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestResetCurrentStepIndexOnPodSpecChange(t *testing.T) { @@ -467,7 +467,7 @@ func TestResetCurrentStepIndexOnPodSpecChange(t *testing.T) { newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) expectedPatch := fmt.Sprintf(expectedPatchWithoutPodHash, expectedCurrentPodHash, newConditions) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestCanaryRolloutCreateFirstReplicasetNoSteps(t *testing.T) { @@ -505,7 +505,7 @@ func TestCanaryRolloutCreateFirstReplicasetNoSteps(t *testing.T) { newConditions := generateConditionsPatchWithCompleted(false, conditions.ReplicaSetUpdatedReason, rs, false, "", true) - assert.Equal(t, calculatePatch(r, fmt.Sprintf(expectedPatch, newConditions)), patch) + assert.JSONEq(t, calculatePatch(r, fmt.Sprintf(expectedPatch, newConditions)), patch) } func TestCanaryRolloutCreateFirstReplicasetWithSteps(t *testing.T) { @@ -545,7 +545,7 @@ func TestCanaryRolloutCreateFirstReplicasetWithSteps(t *testing.T) { }` expectedPatch := fmt.Sprintf(expectedPatchWithSub, generateConditionsPatchWithCompleted(false, conditions.ReplicaSetUpdatedReason, rs, false, "", true)) - assert.Equal(t, calculatePatch(r, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r, expectedPatch), patch) } func TestCanaryRolloutCreateNewReplicaWithCorrectWeight(t *testing.T) { @@ -843,7 +843,7 @@ func TestRollBackToStable(t *testing.T) { newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs1, false, "", true) expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, hash.ComputePodTemplateHash(&r2.Spec.Template, r2.Status.CollisionCount), newConditions) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestRollBackToActiveReplicaSetWithinWindow(t *testing.T) { @@ -935,7 +935,7 @@ func TestGradualShiftToNewStable(t *testing.T) { newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, newConditions) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestRollBackToStableAndStepChange(t *testing.T) { @@ -983,7 +983,7 @@ func TestRollBackToStableAndStepChange(t *testing.T) { newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs1, false, "", true) expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, newPodHash, newStepHash, newConditions) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestCanaryRolloutIncrementStepIfSetWeightsAreCorrect(t *testing.T) { @@ -1019,7 +1019,7 @@ func TestCanaryRolloutIncrementStepIfSetWeightsAreCorrect(t *testing.T) { } }` newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs3, false, "", false) - assert.Equal(t, calculatePatch(r3, fmt.Sprintf(expectedPatch, newConditions)), patch) + assert.JSONEq(t, calculatePatch(r3, fmt.Sprintf(expectedPatch, newConditions)), patch) } func TestSyncRolloutWaitAddToQueue(t *testing.T) { @@ -1171,7 +1171,7 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) { "currentStepIndex":2 } }` - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestCanaryRolloutStatusHPAStatusFields(t *testing.T) { @@ -1215,7 +1215,7 @@ func TestCanaryRolloutStatusHPAStatusFields(t *testing.T) { f.run(getKey(r2, t)) patch := f.getPatchedRolloutWithoutConditions(index) - assert.Equal(t, calculatePatch(r2, expectedPatchWithSub), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatchWithSub), patch) } func TestCanaryRolloutWithCanaryService(t *testing.T) { @@ -1656,7 +1656,7 @@ func TestCanaryRolloutScaleWhilePaused(t *testing.T) { patch := f.getPatchedRolloutWithoutConditions(patchIndex) expectedPatch := calculatePatch(r2, OnlyObservedGenerationPatch) - assert.Equal(t, expectedPatch, patch) + assert.JSONEq(t, expectedPatch, patch) } func TestResumeRolloutAfterPauseDuration(t *testing.T) { @@ -1756,7 +1756,7 @@ func TestNoResumeAfterPauseDurationIfUserPaused(t *testing.T) { "message": "manually paused" } }` - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r2, expectedPatch), patch) } func TestHandleNilNewRSOnScaleAndImageChange(t *testing.T) { @@ -1803,7 +1803,7 @@ func TestHandleNilNewRSOnScaleAndImageChange(t *testing.T) { patchIndex := f.expectPatchRolloutAction(r2) f.run(getKey(r2, t)) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) + assert.JSONEq(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) } func TestHandleCanaryAbort(t *testing.T) { @@ -1850,10 +1850,10 @@ func TestHandleCanaryAbort(t *testing.T) { }` errmsg := fmt.Sprintf(conditions.RolloutAbortedMessage, 2) newConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, "", false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, newConditions, conditions.RolloutAbortedReason, errmsg)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, newConditions, conditions.RolloutAbortedReason, errmsg)), patch) }) - t.Run("Do not reset currentStepCount if newRS is stableRS", func(t *testing.T) { + t.Run("Do not reset currentStepCount and reset abort if newRS is stableRS", func(t *testing.T) { f := newFixture(t) defer f.Close() @@ -1881,13 +1881,12 @@ func TestHandleCanaryAbort(t *testing.T) { patch := f.getPatchedRollout(patchIndex) expectedPatch := `{ "status":{ - "conditions": %s, - "phase": "Degraded", - "message": "%s: %s" + "abort": null, + "abortedAt": null, + "conditions": %s } }` - errmsg := fmt.Sprintf(conditions.RolloutAbortedMessage, 1) - newConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r1, false, "", true) - assert.Equal(t, calculatePatch(r1, fmt.Sprintf(expectedPatch, newConditions, conditions.RolloutAbortedReason, errmsg)), patch) + newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r1, false, "", true) + assert.JSONEq(t, calculatePatch(r1, fmt.Sprintf(expectedPatch, newConditions)), patch) }) } diff --git a/rollout/controller.go b/rollout/controller.go index 5824512271..ebc17c1303 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -56,6 +56,7 @@ import ( logutil "github.com/argoproj/argo-rollouts/utils/log" "github.com/argoproj/argo-rollouts/utils/record" replicasetutil "github.com/argoproj/argo-rollouts/utils/replicaset" + rolloututil "github.com/argoproj/argo-rollouts/utils/rollout" serviceutil "github.com/argoproj/argo-rollouts/utils/service" timeutil "github.com/argoproj/argo-rollouts/utils/time" unstructuredutil "github.com/argoproj/argo-rollouts/utils/unstructured" @@ -520,6 +521,10 @@ func (c *Controller) newRolloutContext(rollout *v1alpha1.Rollout) (*rolloutConte }, reconcilerBase: c.reconcilerBase, } + if rolloututil.IsFullyPromoted(rollout) && roCtx.pauseContext.IsAborted() { + logCtx.Warnf("Removing abort condition from fully promoted rollout") + roCtx.pauseContext.RemoveAbort() + } // carry over existing recorded weights roCtx.newStatus.Canary.Weights = rollout.Status.Canary.Weights return &roCtx, nil diff --git a/rollout/controller_test.go b/rollout/controller_test.go index d37cdc24cd..a637d68f29 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -1346,7 +1346,7 @@ func TestSwitchInvalidSpecMessage(t *testing.T) { expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, progressingCond, string(invalidSpecBytes), conditions.InvalidSpecReason, strings.ReplaceAll(errmsg, "\"", "\\\"")) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r, expectedPatch), patch) } // TestPodTemplateHashEquivalence verifies the hash is computed consistently when there are slight @@ -1549,7 +1549,7 @@ func TestSwitchBlueGreenToCanary(t *testing.T) { "selector": "foo=bar" } }`, addedConditions, conditions.ComputeStepHash(r)) - assert.Equal(t, calculatePatch(r, expectedPatch), patch) + assert.JSONEq(t, calculatePatch(r, expectedPatch), patch) } func newInvalidSpecCondition(reason string, resourceObj runtime.Object, optionalMessage string) (v1alpha1.RolloutCondition, string) { diff --git a/rollout/experiment_test.go b/rollout/experiment_test.go index bcd10cad92..233dd16ca5 100644 --- a/rollout/experiment_test.go +++ b/rollout/experiment_test.go @@ -69,7 +69,7 @@ func TestRolloutCreateExperiment(t *testing.T) { } }` conds := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, ex.Name, conds)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, ex.Name, conds)), patch) } func TestRolloutCreateClusterTemplateExperiment(t *testing.T) { @@ -126,7 +126,7 @@ func TestRolloutCreateClusterTemplateExperiment(t *testing.T) { } }` conds := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, ex.Name, conds)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, ex.Name, conds)), patch) } func TestCreateExperimentWithCollision(t *testing.T) { @@ -178,7 +178,7 @@ func TestCreateExperimentWithCollision(t *testing.T) { } }` conds := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, createdEx.Name, conds)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, createdEx.Name, conds)), patch) } func TestCreateExperimentWithCollisionAndSemanticEquality(t *testing.T) { @@ -229,7 +229,7 @@ func TestCreateExperimentWithCollisionAndSemanticEquality(t *testing.T) { } }` conds := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, ex.Name, conds)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, ex.Name, conds)), patch) } func TestRolloutExperimentProcessingDoNothing(t *testing.T) { @@ -267,7 +267,7 @@ func TestRolloutExperimentProcessingDoNothing(t *testing.T) { f.run(getKey(r2, t)) patch := f.getPatchedRollout(patchIndex) - assert.Equal(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) + assert.JSONEq(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) } @@ -314,7 +314,7 @@ func TestAbortRolloutAfterFailedExperiment(t *testing.T) { }` now := timeutil.Now().UTC().Format(time.RFC3339) generatedConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, "", false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, now, generatedConditions, conditions.RolloutAbortedReason, fmt.Sprintf(conditions.RolloutAbortedMessage, 2))), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, now, generatedConditions, conditions.RolloutAbortedReason, fmt.Sprintf(conditions.RolloutAbortedMessage, 2))), patch) } func TestPauseRolloutAfterInconclusiveExperiment(t *testing.T) { @@ -481,7 +481,7 @@ func TestRolloutExperimentFinishedIncrementStep(t *testing.T) { }` generatedConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false, "", false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, generatedConditions)), patch) + assert.JSONEq(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, generatedConditions)), patch) } func TestRolloutDoNotCreateExperimentWithoutStableRS(t *testing.T) { diff --git a/rollout/service_test.go b/rollout/service_test.go index 393faf87a0..e29ee53b4a 100644 --- a/rollout/service_test.go +++ b/rollout/service_test.go @@ -144,7 +144,7 @@ func TestActiveServiceNotFound(t *testing.T) { } }` _, pausedCondition := newInvalidSpecCondition(conditions.InvalidSpecReason, notUsedActiveSvc, errmsg) - assert.Equal(t, calculatePatch(r, fmt.Sprintf(expectedPatch, pausedCondition, conditions.InvalidSpecReason, strings.ReplaceAll(errmsg, "\"", "\\\""))), patch) + assert.JSONEq(t, calculatePatch(r, fmt.Sprintf(expectedPatch, pausedCondition, conditions.InvalidSpecReason, strings.ReplaceAll(errmsg, "\"", "\\\""))), patch) } func TestPreviewServiceNotFound(t *testing.T) { @@ -173,7 +173,7 @@ func TestPreviewServiceNotFound(t *testing.T) { } }` _, pausedCondition := newInvalidSpecCondition(conditions.InvalidSpecReason, notUsedPreviewSvc, errmsg) - assert.Equal(t, calculatePatch(r, fmt.Sprintf(expectedPatch, pausedCondition, conditions.InvalidSpecReason, strings.ReplaceAll(errmsg, "\"", "\\\""))), patch) + assert.JSONEq(t, calculatePatch(r, fmt.Sprintf(expectedPatch, pausedCondition, conditions.InvalidSpecReason, strings.ReplaceAll(errmsg, "\"", "\\\""))), patch) } diff --git a/test/e2e/istio_test.go b/test/e2e/istio_test.go index 2f993f09bc..7ecbd66fdf 100644 --- a/test/e2e/istio_test.go +++ b/test/e2e/istio_test.go @@ -303,7 +303,7 @@ func (s *IstioSuite) TestIstioAbortUpdate() { Then(). When(). AbortRollout(). - WaitForRolloutStatus("Degraded"). + WaitForRolloutStatus("Healthy"). Then(). ExpectRevisionPodCount("1", 1). When(). @@ -316,7 +316,7 @@ func (s *IstioSuite) TestIstioAbortUpdate() { Then(). When(). AbortRollout(). - WaitForRolloutStatus("Degraded"). + WaitForRolloutStatus("Healthy"). Then(). ExpectRevisionPodCount("2", 1) }