From cfdff34e6b55eee0a12842eb2c6793faff20287c Mon Sep 17 00:00:00 2001 From: zachaller Date: Thu, 18 Aug 2022 20:54:55 -0500 Subject: [PATCH 01/27] feat: add healthy event/condition and fix completed event/condition Signed-off-by: zachaller --- pkg/apis/rollouts/v1alpha1/types.go | 3 +++ rollout/bluegreen_test.go | 8 +++--- rollout/controller_test.go | 10 ++++---- rollout/service_test.go | 12 +++++---- rollout/sync.go | 26 ++++++++++++-------- utils/conditions/conditions.go | 38 ++++++++++++++++++++++++++--- utils/conditions/rollouts_test.go | 2 +- 7 files changed, 71 insertions(+), 28 deletions(-) diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index b5124b2f2d..98357e09d5 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -989,6 +989,9 @@ const ( RolloutPaused RolloutConditionType = "Paused" // RolloutCompleted means that rollout is in a completed state. It is still progressing at this point. RolloutCompleted RolloutConditionType = "Completed" + + // RolloutHealthy means that rollout is in a completed state. It is still progressing at this point. + RolloutHealthy RolloutConditionType = "Healthy" ) // RolloutCondition describes the state of a rollout at a certain point. diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 5cae668da3..0743737081 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -45,7 +45,7 @@ func TestBlueGreenComplateRolloutRestart(t *testing.T) { r := newBlueGreenRollout("foo", 1, nil, "active", "preview") r.Status.Conditions = []v1alpha1.RolloutCondition{} - completedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) + completedCond := conditions.NewRolloutCondition(v1alpha1.RolloutHealthy, corev1.ConditionTrue, conditions.RolloutHealthyReason, conditions.RolloutHealthyReason) conditions.SetRolloutCondition(&r.Status, *completedCond) f.rolloutLister = append(f.rolloutLister, r) @@ -1162,7 +1162,7 @@ func TestBlueGreenRolloutCompletedFalse(t *testing.T) { defer f.Close() r1 := newBlueGreenRollout("foo", 1, nil, "bar", "") - completedCondition, _ := newCompletedCondition(true) + completedCondition, _ := newHealthyCondition(true) conditions.SetRolloutCondition(&r1.Status, completedCondition) r2 := bumpVersion(r1) @@ -1198,7 +1198,7 @@ func TestBlueGreenRolloutCompletedFalse(t *testing.T) { assert.NoError(t, err) index := len(rolloutPatch.Status.Conditions) - 2 - assert.Equal(t, v1alpha1.RolloutCompleted, rolloutPatch.Status.Conditions[index].Type) + assert.Equal(t, v1alpha1.RolloutHealthy, rolloutPatch.Status.Conditions[index].Type) assert.Equal(t, corev1.ConditionFalse, rolloutPatch.Status.Conditions[index].Status) } @@ -1522,7 +1522,7 @@ func TestBlueGreenAddScaleDownDelay(t *testing.T) { rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 2, 1, false, true) - completedCondition, _ := newCompletedCondition(true) + completedCondition, _ := newHealthyCondition(true) conditions.SetRolloutCondition(&r2.Status, completedCondition) progressingCondition, _ := newProgressingCondition(conditions.NewRSAvailableReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) diff --git a/rollout/controller_test.go b/rollout/controller_test.go index 7f324ea10b..5172d5043f 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -193,7 +193,7 @@ func newPausedCondition(isPaused bool) (v1alpha1.RolloutCondition, string) { return condition, string(conditionBytes) } -func newCompletedCondition(isCompleted bool) (v1alpha1.RolloutCondition, string) { +func newHealthyCondition(isCompleted bool) (v1alpha1.RolloutCondition, string) { status := corev1.ConditionTrue if !isCompleted { status = corev1.ConditionFalse @@ -201,10 +201,10 @@ func newCompletedCondition(isCompleted bool) (v1alpha1.RolloutCondition, string) condition := v1alpha1.RolloutCondition{ LastTransitionTime: timeutil.MetaNow(), LastUpdateTime: timeutil.MetaNow(), - Message: conditions.RolloutCompletedReason, - Reason: conditions.RolloutCompletedReason, + Message: conditions.RolloutHealthyReason, + Reason: conditions.RolloutHealthyReason, Status: status, - Type: v1alpha1.RolloutCompleted, + Type: v1alpha1.RolloutHealthy, } conditionBytes, err := json.Marshal(condition) if err != nil { @@ -337,7 +337,7 @@ func generateConditionsPatchWithPause(available bool, progressingReason string, func generateConditionsPatchWithComplete(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isCompleted bool) string { _, availableCondition := newAvailableCondition(available) _, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage) - _, completeCondition := newCompletedCondition(isCompleted) + _, completeCondition := newHealthyCondition(isCompleted) if availableConditionFirst { return fmt.Sprintf("[%s, %s, %s]", availableCondition, completeCondition, progressingCondition) } diff --git a/rollout/service_test.go b/rollout/service_test.go index aa4f1d020b..2ebe578dc3 100644 --- a/rollout/service_test.go +++ b/rollout/service_test.go @@ -305,7 +305,7 @@ func TestBlueGreenAWSVerifyTargetGroupsNotYetReady(t *testing.T) { r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 3, 3, 6, 3, false, true) r2.Status.Message = "" r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) - completedCondition, _ := newCompletedCondition(true) + completedCondition, _ := newHealthyCondition(true) conditions.SetRolloutCondition(&r2.Status, completedCondition) progressingCondition, _ := newProgressingCondition(conditions.NewRSAvailableReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) @@ -388,10 +388,12 @@ func TestBlueGreenAWSVerifyTargetGroupsReady(t *testing.T) { r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 3, 3, 6, 3, false, true) r2.Status.Message = "waiting for post-promotion verification to complete" r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) - completedCondition, _ := newCompletedCondition(true) + completedCondition, _ := newHealthyCondition(true) conditions.SetRolloutCondition(&r2.Status, completedCondition) progressingCondition, _ := newProgressingCondition(conditions.NewRSAvailableReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) + completedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) + conditions.SetRolloutCondition(&r2.Status, *completedCond) f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2, tgb) @@ -489,7 +491,7 @@ func TestCanaryAWSVerifyTargetGroupsNotYetReady(t *testing.T) { r2.Status.StableRS = rs2PodHash availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) - completedCondition, _ := newCompletedCondition(false) + completedCondition, _ := newHealthyCondition(false) conditions.SetRolloutCondition(&r2.Status, completedCondition) progressingCondition, _ := newProgressingCondition(conditions.NewRSAvailableReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) @@ -585,7 +587,7 @@ func TestCanaryAWSVerifyTargetGroupsReady(t *testing.T) { r2.Status.StableRS = rs2PodHash availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) - completedCondition, _ := newCompletedCondition(false) + completedCondition, _ := newHealthyCondition(false) conditions.SetRolloutCondition(&r2.Status, completedCondition) progressingCondition, _ := newProgressingCondition(conditions.NewRSAvailableReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) @@ -646,7 +648,7 @@ func TestCanaryAWSVerifyTargetGroupsSkip(t *testing.T) { r2.Status.StableRS = rs2PodHash availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) - completedCondition, _ := newCompletedCondition(false) + completedCondition, _ := newHealthyCondition(false) conditions.SetRolloutCondition(&r2.Status, completedCondition) progressingCondition, _ := newProgressingCondition(conditions.NewRSAvailableReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) diff --git a/rollout/sync.go b/rollout/sync.go index fc260f8633..47ce065113 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -548,14 +548,16 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt isAborted := c.pauseContext.IsAborted() var becameIncomplete bool // remember if we transitioned from completed - completeCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutCompleted) - if !isPaused && conditions.RolloutComplete(c.rollout, &newStatus) { - updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) - conditions.SetRolloutCondition(&newStatus, *updateCompletedCond) + completeCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutHealthy) + if !isPaused && conditions.RolloutHealthyAndComplete(c.rollout, &newStatus) { + updateHealthyCond := conditions.NewRolloutCondition(v1alpha1.RolloutHealthy, corev1.ConditionTrue, conditions.RolloutHealthyReason, conditions.RolloutHealthyReason) + conditions.SetRolloutCondition(&newStatus, *updateHealthyCond) + c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.RolloutHealthyReason}, conditions.RolloutHealthyMessage) } else { if completeCond != nil { - updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionFalse, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) - becameIncomplete = conditions.SetRolloutCondition(&newStatus, *updateCompletedCond) + updateHealthyCond := conditions.NewRolloutCondition(v1alpha1.RolloutHealthy, corev1.ConditionFalse, conditions.RolloutHealthyReason, conditions.RolloutHealthyReason) + becameIncomplete = conditions.SetRolloutCondition(&newStatus, *updateHealthyCond) + c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.RolloutHealthyReason}, conditions.RolloutNotHealthyMessage) } } @@ -576,11 +578,11 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt // In such a case, we should simply not estimate any progress for this rollout. currentCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutProgressing) - isCompleteRollout := newStatus.Replicas == newStatus.AvailableReplicas && currentCond != nil && currentCond.Reason == conditions.NewRSAvailableReason && currentCond.Type != v1alpha1.RolloutProgressing + isHealthyRollout := newStatus.Replicas == newStatus.AvailableReplicas && currentCond != nil && currentCond.Reason == conditions.NewRSAvailableReason && currentCond.Type != v1alpha1.RolloutProgressing // Check for progress. Only do this if the latest rollout hasn't completed yet and it is not aborted - if !isCompleteRollout && !isAborted { + if !isHealthyRollout && !isAborted { switch { - case conditions.RolloutComplete(c.rollout, &newStatus): + case conditions.RolloutHealthyAndComplete(c.rollout, &newStatus): // Update the rollout conditions with a message for the new replica set that // was successfully deployed. If the condition already exists, we ignore this update. rsName := "" @@ -754,7 +756,7 @@ func (c *rolloutContext) requeueStuckRollout(newStatus v1alpha1.RolloutStatus) t } // No need to estimate progress if the rollout is complete or already timed out. isPaused := len(c.rollout.Status.PauseConditions) > 0 || c.rollout.Spec.Paused - if conditions.RolloutComplete(c.rollout, &newStatus) || currentCond.Reason == conditions.TimedOutReason || isPaused || c.rollout.Status.Abort || isIndefiniteStep(c.rollout) { + if conditions.RolloutHealthyAndComplete(c.rollout, &newStatus) || currentCond.Reason == conditions.TimedOutReason || isPaused || c.rollout.Status.Abort || isIndefiniteStep(c.rollout) { return time.Duration(-1) } // If there is no sign of progress at this point then there is a high chance that the @@ -926,6 +928,10 @@ func (c *rolloutContext) promoteStable(newStatus *v1alpha1.RolloutStatus, reason revision, _ := replicasetutil.Revision(c.rollout) c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason) + if conditions.RolloutComplete(c.rollout, newStatus) { + updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) + conditions.SetRolloutCondition(newStatus, *updateCompletedCond) + } } return nil } diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go index 99b6d59ddf..b69042ec53 100644 --- a/utils/conditions/conditions.go +++ b/utils/conditions/conditions.go @@ -69,6 +69,13 @@ const ( // RolloutCompletedMessage is added when the rollout is completed RolloutCompletedMessage = "Rollout completed update to revision %d (%s): %s" + // RolloutHealthyReason is added in a rollout when it is completed. + RolloutHealthyReason = "RolloutHealthy" + // RolloutHealthyMessage is added when the rollout is healthy + RolloutHealthyMessage = "Rollout is healthy" + // RolloutNotHealthyMessage is added when the rollout is not healthy + RolloutNotHealthyMessage = "Rollout is not healthy" + // RolloutAbortedReason indicates that the rollout was aborted RolloutAbortedReason = "RolloutAborted" // RolloutAbortedMessage indicates that the rollout was aborted @@ -125,7 +132,7 @@ const ( // TimedOutReason is added in a rollout when its newest replica set fails to show any progress // within the given deadline (progressDeadlineSeconds). TimedOutReason = "ProgressDeadlineExceeded" - // RolloutTimeOutMessage is is added in a rollout when the rollout fails to show any progress + // RolloutTimeOutMessage is added in a rollout when the rollout fails to show any progress // within the given deadline (progressDeadlineSeconds). RolloutTimeOutMessage = "Rollout %q has timed out progressing." @@ -257,9 +264,9 @@ func RolloutProgressing(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutSt strategySpecificProgress } -// RolloutComplete considers a rollout to be complete once all of its desired replicas +// RolloutHealthyAndComplete considers a rollout to be complete once all of its desired replicas // are updated, available, and receiving traffic from the active service, and no old pods are running. -func RolloutComplete(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) bool { +func RolloutHealthyAndComplete(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) bool { completedStrategy := true replicas := defaults.GetReplicasOrDefault(rollout.Spec.Replicas) @@ -288,6 +295,31 @@ func RolloutComplete(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatu completedStrategy } +// RolloutComplete considers a rollout to be complete once +func RolloutComplete(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) bool { + completedStrategy := true + + if rollout.Spec.Strategy.BlueGreen != nil { + activeSelectorComplete := newStatus.BlueGreen.ActiveSelector == newStatus.CurrentPodHash + previewSelectorComplete := true + if rollout.Spec.Strategy.BlueGreen.PreviewService != "" { + previewSelectorComplete = newStatus.BlueGreen.PreviewSelector == newStatus.CurrentPodHash + } + completedStrategy = activeSelectorComplete && previewSelectorComplete + } + if rollout.Spec.Strategy.Canary != nil { + stepCount := len(rollout.Spec.Strategy.Canary.Steps) + executedAllSteps := true + if stepCount > 0 && newStatus.CurrentStepIndex != nil { + executedAllSteps = int32(stepCount) == *newStatus.CurrentStepIndex + } + currentRSIsStable := newStatus.StableRS != "" && newStatus.StableRS == newStatus.CurrentPodHash + completedStrategy = executedAllSteps && currentRSIsStable + } + + return rollout.Status.ObservedGeneration == strconv.Itoa(int(rollout.Generation)) && completedStrategy +} + // ComputeStepHash returns a hash value calculated from the Rollout's steps. The hash will // be safe encoded to avoid bad words. func ComputeStepHash(rollout *v1alpha1.Rollout) string { diff --git a/utils/conditions/rollouts_test.go b/utils/conditions/rollouts_test.go index f6e3e02550..7678c0c1dd 100644 --- a/utils/conditions/rollouts_test.go +++ b/utils/conditions/rollouts_test.go @@ -475,7 +475,7 @@ func TestRolloutComplete(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - assert.Equal(t, test.expected, RolloutComplete(test.r, &test.r.Status)) + assert.Equal(t, test.expected, RolloutHealthyAndComplete(test.r, &test.r.Status)) }) } From 4f6c55ae31b598e0c9dd991a52338986ee80abb9 Mon Sep 17 00:00:00 2001 From: zachaller Date: Thu, 18 Aug 2022 21:16:30 -0500 Subject: [PATCH 02/27] fix unit tests Signed-off-by: zachaller --- rollout/service_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/rollout/service_test.go b/rollout/service_test.go index 2ebe578dc3..565ad2c33a 100644 --- a/rollout/service_test.go +++ b/rollout/service_test.go @@ -324,6 +324,7 @@ func TestBlueGreenAWSVerifyTargetGroupsNotYetReady(t *testing.T) { assert.Equal(t, expectedPatch, patch) f.assertEvents([]string{ conditions.TargetGroupUnverifiedReason, + conditions.RolloutHealthyReason, }) } @@ -410,6 +411,7 @@ func TestBlueGreenAWSVerifyTargetGroupsReady(t *testing.T) { f.assertEvents([]string{ conditions.TargetGroupVerifiedReason, conditions.RolloutCompletedReason, + conditions.RolloutHealthyReason, }) } @@ -507,6 +509,7 @@ func TestCanaryAWSVerifyTargetGroupsNotYetReady(t *testing.T) { f.run(getKey(r2, t)) f.assertEvents([]string{ conditions.TargetGroupUnverifiedReason, + conditions.RolloutHealthyReason, }) } @@ -605,6 +608,7 @@ func TestCanaryAWSVerifyTargetGroupsReady(t *testing.T) { f.verifyPatchedReplicaSet(scaleDownRSIndex, 30) f.assertEvents([]string{ conditions.TargetGroupVerifiedReason, + conditions.RolloutHealthyReason, }) } @@ -661,7 +665,9 @@ func TestCanaryAWSVerifyTargetGroupsSkip(t *testing.T) { f.ingressLister = append(f.ingressLister, ingressutil.NewLegacyIngress(ing)) f.run(getKey(r2, t)) // there should be no api calls - f.assertEvents(nil) + f.assertEvents([]string{ + conditions.RolloutHealthyReason, + }) } // TestShouldVerifyTargetGroups returns whether or not we should verify the target group From a126e36b8c935d762fe80dba1951d1f2088c5900 Mon Sep 17 00:00:00 2001 From: zachaller Date: Thu, 18 Aug 2022 22:02:24 -0500 Subject: [PATCH 03/27] rename vars to make more sense and remove healthy event becase it will never be consistent Signed-off-by: zachaller --- pkg/apis/rollouts/v1alpha1/types.go | 5 ++--- rollout/bluegreen_test.go | 4 ++-- rollout/controller_test.go | 6 +++--- rollout/service_test.go | 10 +++++----- rollout/sync.go | 10 +++++----- utils/conditions/conditions.go | 12 ++++++------ 6 files changed, 23 insertions(+), 24 deletions(-) diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index 98357e09d5..826ded984b 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -989,9 +989,8 @@ const ( RolloutPaused RolloutConditionType = "Paused" // RolloutCompleted means that rollout is in a completed state. It is still progressing at this point. RolloutCompleted RolloutConditionType = "Completed" - - // RolloutHealthy means that rollout is in a completed state. It is still progressing at this point. - RolloutHealthy RolloutConditionType = "Healthy" + // HealthyAndCompleted means that rollout is in a completed state and is healthy. + HealthyAndCompleted RolloutConditionType = "HealthyAndCompleted" ) // RolloutCondition describes the state of a rollout at a certain point. diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 0743737081..ab9af61bb2 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -45,7 +45,7 @@ func TestBlueGreenComplateRolloutRestart(t *testing.T) { r := newBlueGreenRollout("foo", 1, nil, "active", "preview") r.Status.Conditions = []v1alpha1.RolloutCondition{} - completedCond := conditions.NewRolloutCondition(v1alpha1.RolloutHealthy, corev1.ConditionTrue, conditions.RolloutHealthyReason, conditions.RolloutHealthyReason) + completedCond := conditions.NewRolloutCondition(v1alpha1.HealthyAndCompleted, corev1.ConditionTrue, conditions.RolloutHealthyAndCompletedReason, conditions.RolloutHealthyAndCompletedReason) conditions.SetRolloutCondition(&r.Status, *completedCond) f.rolloutLister = append(f.rolloutLister, r) @@ -1198,7 +1198,7 @@ func TestBlueGreenRolloutCompletedFalse(t *testing.T) { assert.NoError(t, err) index := len(rolloutPatch.Status.Conditions) - 2 - assert.Equal(t, v1alpha1.RolloutHealthy, rolloutPatch.Status.Conditions[index].Type) + assert.Equal(t, v1alpha1.HealthyAndCompleted, rolloutPatch.Status.Conditions[index].Type) assert.Equal(t, corev1.ConditionFalse, rolloutPatch.Status.Conditions[index].Status) } diff --git a/rollout/controller_test.go b/rollout/controller_test.go index 5172d5043f..a2bcbed2a0 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -201,10 +201,10 @@ func newHealthyCondition(isCompleted bool) (v1alpha1.RolloutCondition, string) { condition := v1alpha1.RolloutCondition{ LastTransitionTime: timeutil.MetaNow(), LastUpdateTime: timeutil.MetaNow(), - Message: conditions.RolloutHealthyReason, - Reason: conditions.RolloutHealthyReason, + Message: conditions.RolloutHealthyAndCompletedReason, + Reason: conditions.RolloutHealthyAndCompletedReason, Status: status, - Type: v1alpha1.RolloutHealthy, + Type: v1alpha1.HealthyAndCompleted, } conditionBytes, err := json.Marshal(condition) if err != nil { diff --git a/rollout/service_test.go b/rollout/service_test.go index 565ad2c33a..371b2faeab 100644 --- a/rollout/service_test.go +++ b/rollout/service_test.go @@ -324,7 +324,7 @@ func TestBlueGreenAWSVerifyTargetGroupsNotYetReady(t *testing.T) { assert.Equal(t, expectedPatch, patch) f.assertEvents([]string{ conditions.TargetGroupUnverifiedReason, - conditions.RolloutHealthyReason, + conditions.RolloutHealthyAndCompletedReason, }) } @@ -411,7 +411,7 @@ func TestBlueGreenAWSVerifyTargetGroupsReady(t *testing.T) { f.assertEvents([]string{ conditions.TargetGroupVerifiedReason, conditions.RolloutCompletedReason, - conditions.RolloutHealthyReason, + conditions.RolloutHealthyAndCompletedReason, }) } @@ -509,7 +509,7 @@ func TestCanaryAWSVerifyTargetGroupsNotYetReady(t *testing.T) { f.run(getKey(r2, t)) f.assertEvents([]string{ conditions.TargetGroupUnverifiedReason, - conditions.RolloutHealthyReason, + conditions.RolloutHealthyAndCompletedReason, }) } @@ -608,7 +608,7 @@ func TestCanaryAWSVerifyTargetGroupsReady(t *testing.T) { f.verifyPatchedReplicaSet(scaleDownRSIndex, 30) f.assertEvents([]string{ conditions.TargetGroupVerifiedReason, - conditions.RolloutHealthyReason, + conditions.RolloutHealthyAndCompletedReason, }) } @@ -666,7 +666,7 @@ func TestCanaryAWSVerifyTargetGroupsSkip(t *testing.T) { f.run(getKey(r2, t)) // there should be no api calls f.assertEvents([]string{ - conditions.RolloutHealthyReason, + conditions.RolloutHealthyAndCompletedReason, }) } diff --git a/rollout/sync.go b/rollout/sync.go index 47ce065113..53dfdce1c6 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -548,16 +548,16 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt isAborted := c.pauseContext.IsAborted() var becameIncomplete bool // remember if we transitioned from completed - completeCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutHealthy) + completeCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.HealthyAndCompleted) if !isPaused && conditions.RolloutHealthyAndComplete(c.rollout, &newStatus) { - updateHealthyCond := conditions.NewRolloutCondition(v1alpha1.RolloutHealthy, corev1.ConditionTrue, conditions.RolloutHealthyReason, conditions.RolloutHealthyReason) + updateHealthyCond := conditions.NewRolloutCondition(v1alpha1.HealthyAndCompleted, corev1.ConditionTrue, conditions.RolloutHealthyAndCompletedReason, conditions.RolloutHealthyAndCompletedMessage) conditions.SetRolloutCondition(&newStatus, *updateHealthyCond) - c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.RolloutHealthyReason}, conditions.RolloutHealthyMessage) + //c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.RolloutHealthyAndCompletedReason}, conditions.RolloutHealthyAndCompletedMessage) } else { if completeCond != nil { - updateHealthyCond := conditions.NewRolloutCondition(v1alpha1.RolloutHealthy, corev1.ConditionFalse, conditions.RolloutHealthyReason, conditions.RolloutHealthyReason) + updateHealthyCond := conditions.NewRolloutCondition(v1alpha1.HealthyAndCompleted, corev1.ConditionFalse, conditions.RolloutHealthyAndCompletedReason, conditions.RolloutNotHealthyAndCompletedMessage) becameIncomplete = conditions.SetRolloutCondition(&newStatus, *updateHealthyCond) - c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.RolloutHealthyReason}, conditions.RolloutNotHealthyMessage) + //c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.RolloutHealthyAndCompletedReason}, conditions.RolloutNotHealthyAndCompletedMessage) } } diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go index b69042ec53..31068f256a 100644 --- a/utils/conditions/conditions.go +++ b/utils/conditions/conditions.go @@ -69,12 +69,12 @@ const ( // RolloutCompletedMessage is added when the rollout is completed RolloutCompletedMessage = "Rollout completed update to revision %d (%s): %s" - // RolloutHealthyReason is added in a rollout when it is completed. - RolloutHealthyReason = "RolloutHealthy" - // RolloutHealthyMessage is added when the rollout is healthy - RolloutHealthyMessage = "Rollout is healthy" - // RolloutNotHealthyMessage is added when the rollout is not healthy - RolloutNotHealthyMessage = "Rollout is not healthy" + // RolloutHealthyAndCompletedReason is added in a rollout when it is healthy. + RolloutHealthyAndCompletedReason = "HealthyAndCompleted" + // RolloutHealthyAndCompletedMessage is added when the rollout is completed and is healthy or not. + RolloutHealthyAndCompletedMessage = "Rollout is healthy and completed" + // RolloutNotHealthyAndCompletedMessage is added when the rollout is completed and is healthy or not. + RolloutNotHealthyAndCompletedMessage = "Rollout is not healthy and completed" // RolloutAbortedReason indicates that the rollout was aborted RolloutAbortedReason = "RolloutAborted" From 7a4492caa3ec3467839ee2e4d398ef94efb4d4e1 Mon Sep 17 00:00:00 2001 From: zachaller Date: Fri, 19 Aug 2022 09:00:53 -0500 Subject: [PATCH 04/27] fix unit tests Signed-off-by: zachaller --- rollout/controller_test.go | 4 +++- rollout/service_test.go | 8 +------- rollout/sync.go | 8 +++++--- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/rollout/controller_test.go b/rollout/controller_test.go index a2bcbed2a0..412d8ba0ce 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -195,13 +195,15 @@ func newPausedCondition(isPaused bool) (v1alpha1.RolloutCondition, string) { func newHealthyCondition(isCompleted bool) (v1alpha1.RolloutCondition, string) { status := corev1.ConditionTrue + msg := conditions.RolloutHealthyAndCompletedMessage if !isCompleted { status = corev1.ConditionFalse + msg = conditions.RolloutNotHealthyAndCompletedMessage } condition := v1alpha1.RolloutCondition{ LastTransitionTime: timeutil.MetaNow(), LastUpdateTime: timeutil.MetaNow(), - Message: conditions.RolloutHealthyAndCompletedReason, + Message: msg, Reason: conditions.RolloutHealthyAndCompletedReason, Status: status, Type: v1alpha1.HealthyAndCompleted, diff --git a/rollout/service_test.go b/rollout/service_test.go index 371b2faeab..2ebe578dc3 100644 --- a/rollout/service_test.go +++ b/rollout/service_test.go @@ -324,7 +324,6 @@ func TestBlueGreenAWSVerifyTargetGroupsNotYetReady(t *testing.T) { assert.Equal(t, expectedPatch, patch) f.assertEvents([]string{ conditions.TargetGroupUnverifiedReason, - conditions.RolloutHealthyAndCompletedReason, }) } @@ -411,7 +410,6 @@ func TestBlueGreenAWSVerifyTargetGroupsReady(t *testing.T) { f.assertEvents([]string{ conditions.TargetGroupVerifiedReason, conditions.RolloutCompletedReason, - conditions.RolloutHealthyAndCompletedReason, }) } @@ -509,7 +507,6 @@ func TestCanaryAWSVerifyTargetGroupsNotYetReady(t *testing.T) { f.run(getKey(r2, t)) f.assertEvents([]string{ conditions.TargetGroupUnverifiedReason, - conditions.RolloutHealthyAndCompletedReason, }) } @@ -608,7 +605,6 @@ func TestCanaryAWSVerifyTargetGroupsReady(t *testing.T) { f.verifyPatchedReplicaSet(scaleDownRSIndex, 30) f.assertEvents([]string{ conditions.TargetGroupVerifiedReason, - conditions.RolloutHealthyAndCompletedReason, }) } @@ -665,9 +661,7 @@ func TestCanaryAWSVerifyTargetGroupsSkip(t *testing.T) { f.ingressLister = append(f.ingressLister, ingressutil.NewLegacyIngress(ing)) f.run(getKey(r2, t)) // there should be no api calls - f.assertEvents([]string{ - conditions.RolloutHealthyAndCompletedReason, - }) + f.assertEvents(nil) } // TestShouldVerifyTargetGroups returns whether or not we should verify the target group diff --git a/rollout/sync.go b/rollout/sync.go index 53dfdce1c6..80c109b108 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -926,10 +926,12 @@ func (c *rolloutContext) promoteStable(newStatus *v1alpha1.RolloutStatus, reason } newStatus.StableRS = newStatus.CurrentPodHash revision, _ := replicasetutil.Revision(c.rollout) - c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, - conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason) if conditions.RolloutComplete(c.rollout, newStatus) { - updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) + c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, + conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason) + + updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, + conditions.RolloutCompletedReason, fmt.Sprintf(conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason)) conditions.SetRolloutCondition(newStatus, *updateCompletedCond) } } From 46d7ffd2951c46cf4f148fa113e8609a8311ca95 Mon Sep 17 00:00:00 2001 From: zachaller Date: Fri, 19 Aug 2022 09:29:05 -0500 Subject: [PATCH 05/27] possible fix for e2e Signed-off-by: zachaller --- test/e2e/functional_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/e2e/functional_test.go b/test/e2e/functional_test.go index f4380c3562..a10a7c8933 100644 --- a/test/e2e/functional_test.go +++ b/test/e2e/functional_test.go @@ -1164,15 +1164,15 @@ spec: When(). UpdateSpec(). Then(). - ExpectRollout("Completed=False", func(r *v1alpha1.Rollout) bool { - cmd := exec.Command("kubectl", "wait", "--for=condition=Completed=False", fmt.Sprintf("rollout/%s", r.Name)) + ExpectRollout("HealthyAndCompleted=False", func(r *v1alpha1.Rollout) bool { + cmd := exec.Command("kubectl", "wait", "--for=condition=HealthyAndCompleted=False", fmt.Sprintf("rollout/%s", r.Name)) out, err := cmd.CombinedOutput() return err == nil && strings.Contains(string(out), fmt.Sprintf("rollout.argoproj.io/%s condition met", r.Name)) }). ExpectRolloutStatus("Progressing"). ExpectActiveRevision("1"). - ExpectRollout("Completed=True", func(r *v1alpha1.Rollout) bool { - cmd := exec.Command("kubectl", "wait", "--for=condition=Completed=True", fmt.Sprintf("rollout/%s", r.Name)) + ExpectRollout("HealthyAndCompleted=True", func(r *v1alpha1.Rollout) bool { + cmd := exec.Command("kubectl", "wait", "--for=condition=HealthyAndCompleted=True", fmt.Sprintf("rollout/%s", r.Name)) out, err := cmd.CombinedOutput() return err == nil && strings.Contains(string(out), fmt.Sprintf("rollout.argoproj.io/%s condition met", r.Name)) }). From 0da4c16976ace11b390472893e8f7debc9393179 Mon Sep 17 00:00:00 2001 From: zachaller Date: Fri, 19 Aug 2022 10:17:28 -0500 Subject: [PATCH 06/27] fix e2e Signed-off-by: zachaller --- test/e2e/functional_test.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/test/e2e/functional_test.go b/test/e2e/functional_test.go index a10a7c8933..a5b5f75a42 100644 --- a/test/e2e/functional_test.go +++ b/test/e2e/functional_test.go @@ -702,13 +702,12 @@ func (s *FunctionalSuite) TestBlueGreenUpdate() { "RolloutUpdated", // Rollout updated to revision 1 "NewReplicaSetCreated", // Created ReplicaSet bluegreen-7dcd8f8869 (revision 1) "ScalingReplicaSet", // Scaled up ReplicaSet bluegreen-7dcd8f8869 (revision 1) from 0 to 3 - "RolloutCompleted", // Rollout completed update to revision 1 (7dcd8f8869): Initial deploy - "SwitchService", // Switched selector for service 'bluegreen' from '' to '7dcd8f8869' - "RolloutUpdated", // Rollout updated to revision 2 - "NewReplicaSetCreated", // Created ReplicaSet bluegreen-5498785cd6 (revision 2) - "ScalingReplicaSet", // Scaled up ReplicaSet bluegreen-5498785cd6 (revision 2) from 0 to 3 - "SwitchService", // Switched selector for service 'bluegreen' from '7dcd8f8869' to '6c779b88b6' - "RolloutCompleted", // Rollout completed update to revision 2 (6c779b88b6): Completed blue-green update + "SwitchService", // Rollout completed update to revision 1 (7dcd8f8869): Initial deploy + "RolloutUpdated", // Switched selector for service 'bluegreen' from '' to '7dcd8f8869' + "NewReplicaSetCreated", // Rollout updated to revision 2 + "ScalingReplicaSet", // Created ReplicaSet bluegreen-5498785cd6 (revision 2) + "SwitchService", // Scaled up ReplicaSet bluegreen-5498785cd6 (revision 2) from 0 to 3 + "RolloutCompleted", // Switched selector for service 'bluegreen' from '7dcd8f8869' to '6c779b88b6' }) } From 447c30a826aae47ff1e2220067c6d56c6342dc53 Mon Sep 17 00:00:00 2001 From: zachaller Date: Fri, 19 Aug 2022 10:45:08 -0500 Subject: [PATCH 07/27] unit test for complete function Signed-off-by: zachaller --- utils/conditions/rollouts_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/utils/conditions/rollouts_test.go b/utils/conditions/rollouts_test.go index 7678c0c1dd..a134b4728f 100644 --- a/utils/conditions/rollouts_test.go +++ b/utils/conditions/rollouts_test.go @@ -350,7 +350,7 @@ func TestRolloutProgressing(t *testing.T) { } -func TestRolloutComplete(t *testing.T) { +func TestRolloutHealthyComplete(t *testing.T) { rollout := func(desired, current, updated, available int32, correctObservedGeneration bool) *v1alpha1.Rollout { r := &v1alpha1.Rollout{ Spec: v1alpha1.RolloutSpec{ @@ -475,6 +475,7 @@ func TestRolloutComplete(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.expected, RolloutComplete(test.r, &test.r.Status)) assert.Equal(t, test.expected, RolloutHealthyAndComplete(test.r, &test.r.Status)) }) } From 326689c6fdbf75381ca3c1fb640d821790a6addb Mon Sep 17 00:00:00 2001 From: zachaller Date: Fri, 19 Aug 2022 11:37:53 -0500 Subject: [PATCH 08/27] small cleanup and changes to not check generation Signed-off-by: zachaller --- rollout/sync.go | 5 ++++- utils/conditions/conditions.go | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/rollout/sync.go b/rollout/sync.go index 80c109b108..4ac5783b3f 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -925,8 +925,11 @@ func (c *rolloutContext) promoteStable(newStatus *v1alpha1.RolloutStatus, reason } } newStatus.StableRS = newStatus.CurrentPodHash - revision, _ := replicasetutil.Revision(c.rollout) + //revision, _ := replicasetutil.Revision(c.rollout) + //c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, + // conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason) if conditions.RolloutComplete(c.rollout, newStatus) { + revision, _ := replicasetutil.Revision(c.rollout) c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason) diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go index 31068f256a..279eca6008 100644 --- a/utils/conditions/conditions.go +++ b/utils/conditions/conditions.go @@ -317,7 +317,7 @@ func RolloutComplete(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatu completedStrategy = executedAllSteps && currentRSIsStable } - return rollout.Status.ObservedGeneration == strconv.Itoa(int(rollout.Generation)) && completedStrategy + return completedStrategy } // ComputeStepHash returns a hash value calculated from the Rollout's steps. The hash will From 114f073afc525ee535f948f219d5bf3ebe86946c Mon Sep 17 00:00:00 2001 From: zachaller Date: Fri, 19 Aug 2022 13:18:27 -0500 Subject: [PATCH 09/27] fix unit test and proper behavior Signed-off-by: zachaller --- rollout/analysis_test.go | 5 ++++- rollout/bluegreen_test.go | 18 +++++++++------- rollout/canary_test.go | 6 +++--- rollout/controller_test.go | 38 +++++++++++++++++++++++++++++++--- rollout/sync.go | 6 ++---- utils/conditions/conditions.go | 2 +- 6 files changed, 55 insertions(+), 20 deletions(-) diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index ed21a4c848..bc87990040 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -1661,7 +1661,7 @@ func TestDoNotCreatePrePromotionAnalysisAfterPromotionRollout(t *testing.T) { f.run(getKey(r2, t)) - newConditions := generateConditionsPatchWithComplete(true, conditions.NewRSAvailableReason, rs2, true, "", true) + newConditions := generateConditionsPatchWithHealthy(true, conditions.NewRSAvailableReason, rs2, true, "", true) expectedPatch := fmt.Sprintf(`{ "status":{ "conditions":%s @@ -2141,6 +2141,9 @@ func TestRolloutPostPromotionAnalysisSuccess(t *testing.T) { activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} activeSvc := newService("active", 80, activeSelector, r2) + cond, _ := newCompleteCondition(true) + conditions.SetRolloutCondition(&r2.Status, cond) + f.objects = append(f.objects, r2, at, ar) f.kubeobjects = append(f.kubeobjects, activeSvc, rs1, rs2) f.rolloutLister = append(f.rolloutLister, r2) diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index ab9af61bb2..b69071741a 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -57,7 +57,7 @@ func TestBlueGreenComplateRolloutRestart(t *testing.T) { rs := newReplicaSet(r, 1) rsPodHash := rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - generatedConditions := generateConditionsPatchWithComplete(false, conditions.ReplicaSetNotAvailableReason, rs, false, "", false) + generatedConditions := generateConditionsPatchWithHealthy(false, conditions.ReplicaSetNotAvailableReason, rs, false, "", false) f.expectCreateReplicaSetAction(rs) servicePatchIndex := f.expectPatchServiceAction(previewSvc, rsPodHash) @@ -635,7 +635,7 @@ func TestBlueGreenHandlePause(t *testing.T) { servicePatchIndex := f.expectPatchServiceAction(activeSvc, rs2PodHash) - generatedConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, true, "") + generatedConditions := generateConditionsPatchWithComplete(true, conditions.ReplicaSetUpdatedReason, rs2, true, "", true) newSelector := metav1.FormatLabelSelector(rs2.Spec.Selector) expectedPatchWithoutSubs := `{ "status": { @@ -739,7 +739,7 @@ func TestBlueGreenHandlePause(t *testing.T) { } }` - generateConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs1, false, "") + generateConditions := generateConditionsPatchWithComplete(true, conditions.ReplicaSetUpdatedReason, rs1, false, "", true) newSelector := metav1.FormatLabelSelector(rs1.Spec.Selector) expectedPatch := calculatePatch(r1, fmt.Sprintf(expectedPatchWithoutSubs, rs1PodHash, rs1PodHash, generateConditions, newSelector)) patchRolloutIndex := f.expectPatchRolloutActionWithPatch(r1, expectedPatch) @@ -796,7 +796,7 @@ func TestBlueGreenHandlePause(t *testing.T) { }` assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedUnpausePatch, unpauseConditions)), unpausePatch) - generatedConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, true, "") + generatedConditions := generateConditionsPatchWithComplete(true, conditions.ReplicaSetUpdatedReason, rs2, true, "", true) expected2ndPatchWithoutSubs := `{ "status": { "blueGreen": { @@ -858,7 +858,7 @@ func TestBlueGreenAddScaleDownDelayToPreviousActiveReplicaSet(t *testing.T) { } }` newSelector := metav1.FormatLabelSelector(rs2.Spec.Selector) - expectedCondition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, true, "") + expectedCondition := generateConditionsPatchWithComplete(true, conditions.ReplicaSetUpdatedReason, rs2, true, "", true) expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, rs2PodHash, rs2PodHash, expectedCondition, newSelector)) assert.Equal(t, expectedPatch, patch) } @@ -1147,7 +1147,7 @@ func TestBlueGreenRolloutCompleted(t *testing.T) { f.run(getKey(r2, t)) - newConditions := generateConditionsPatchWithComplete(true, conditions.NewRSAvailableReason, rs2, true, "", true) + newConditions := generateConditionsPatchWithHealthy(true, conditions.NewRSAvailableReason, rs2, true, "", true) expectedPatch := fmt.Sprintf(`{ "status":{ "conditions":%s @@ -1481,7 +1481,7 @@ func TestBlueGreenHandlePauseAutoPromoteWithConditions(t *testing.T) { "blueGreen": { "activeSelector": "%s" }, - "conditions": [%s, %s, %s], + "conditions": [%s, %s, %s, %s], "stableRS": "%s", "pauseConditions": null, "controllerPause": null, @@ -1497,7 +1497,9 @@ func TestBlueGreenHandlePauseAutoPromoteWithConditions(t *testing.T) { assert.Nil(t, err) pausedCondBytes, err := json.Marshal(r2.Status.Conditions[2]) assert.Nil(t, err) - expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, rs2PodHash, string(availableCondBytes), string(pausedCondBytes), string(progressingCondBytes), rs2PodHash, rs2PodHash)) + completeCond, _ := newCompleteCondition(true) + completeCondBytes, err := json.Marshal(completeCond) + expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, rs2PodHash, string(availableCondBytes), string(pausedCondBytes), string(completeCondBytes), string(progressingCondBytes), rs2PodHash, rs2PodHash)) f.expectPatchServiceAction(activeSvc, rs2PodHash) patchRolloutIndex := f.expectPatchRolloutActionWithPatch(r2, expectedPatch) f.run(getKey(r2, t)) diff --git a/rollout/canary_test.go b/rollout/canary_test.go index 8cc1c3b0df..de42068963 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -379,7 +379,7 @@ func TestCanaryRolloutUpdateStatusWhenAtEndOfSteps(t *testing.T) { } }` - expectedPatch := fmt.Sprintf(expectedPatchWithoutStableRS, expectedStableRS, generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false, "")) + expectedPatch := fmt.Sprintf(expectedPatchWithoutStableRS, expectedStableRS, generateConditionsPatchWithComplete(true, conditions.ReplicaSetUpdatedReason, rs2, false, "", true)) assert.Equal(t, calculatePatch(r2, expectedPatch), patch) } @@ -502,7 +502,7 @@ func TestCanaryRolloutCreateFirstReplicasetNoSteps(t *testing.T) { } }` - newConditions := generateConditionsPatch(false, conditions.ReplicaSetUpdatedReason, rs, false, "") + newConditions := generateConditionsPatchWithComplete(false, conditions.ReplicaSetUpdatedReason, rs, false, "", true) assert.Equal(t, calculatePatch(r, fmt.Sprintf(expectedPatch, newConditions)), patch) } @@ -542,7 +542,7 @@ func TestCanaryRolloutCreateFirstReplicasetWithSteps(t *testing.T) { "conditions": %s } }` - expectedPatch := fmt.Sprintf(expectedPatchWithSub, generateConditionsPatch(false, conditions.ReplicaSetUpdatedReason, rs, false, "")) + expectedPatch := fmt.Sprintf(expectedPatchWithSub, generateConditionsPatchWithComplete(false, conditions.ReplicaSetUpdatedReason, rs, false, "", true)) assert.Equal(t, calculatePatch(r, expectedPatch), patch) } diff --git a/rollout/controller_test.go b/rollout/controller_test.go index 412d8ba0ce..8fe1314030 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -193,10 +193,10 @@ func newPausedCondition(isPaused bool) (v1alpha1.RolloutCondition, string) { return condition, string(conditionBytes) } -func newHealthyCondition(isCompleted bool) (v1alpha1.RolloutCondition, string) { +func newHealthyCondition(isHealthy bool) (v1alpha1.RolloutCondition, string) { status := corev1.ConditionTrue msg := conditions.RolloutHealthyAndCompletedMessage - if !isCompleted { + if !isHealthy { status = corev1.ConditionFalse msg = conditions.RolloutNotHealthyAndCompletedMessage } @@ -215,6 +215,26 @@ func newHealthyCondition(isCompleted bool) (v1alpha1.RolloutCondition, string) { return condition, string(conditionBytes) } +func newCompleteCondition(isCompleted bool) (v1alpha1.RolloutCondition, string) { + status := corev1.ConditionTrue + if !isCompleted { + status = corev1.ConditionFalse + } + condition := v1alpha1.RolloutCondition{ + LastTransitionTime: timeutil.MetaNow(), + LastUpdateTime: timeutil.MetaNow(), + Message: conditions.RolloutCompletedReason, + Reason: conditions.RolloutCompletedReason, + Status: status, + Type: v1alpha1.RolloutCompleted, + } + conditionBytes, err := json.Marshal(condition) + if err != nil { + panic(err) + } + return condition, string(conditionBytes) +} + func newProgressingCondition(reason string, resourceObj runtime.Object, optionalMessage string) (v1alpha1.RolloutCondition, string) { status := corev1.ConditionTrue msg := "" @@ -336,10 +356,20 @@ func generateConditionsPatchWithPause(available bool, progressingReason string, return fmt.Sprintf("[%s, %s, %s]", progressingCondition, pauseCondition, availableCondition) } +func generateConditionsPatchWithHealthy(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isHealthy bool) string { + _, availableCondition := newAvailableCondition(available) + _, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage) + _, completeCondition := newHealthyCondition(isHealthy) + if availableConditionFirst { + return fmt.Sprintf("[%s, %s, %s]", availableCondition, completeCondition, progressingCondition) + } + return fmt.Sprintf("[%s, %s, %s]", completeCondition, progressingCondition, availableCondition) +} + func generateConditionsPatchWithComplete(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isCompleted bool) string { _, availableCondition := newAvailableCondition(available) _, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage) - _, completeCondition := newHealthyCondition(isCompleted) + _, completeCondition := newCompleteCondition(isCompleted) if availableConditionFirst { return fmt.Sprintf("[%s, %s, %s]", availableCondition, completeCondition, progressingCondition) } @@ -365,6 +395,8 @@ func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active, stable s newRollout.Status.StableRS = stable cond, _ := newAvailableCondition(available) newRollout.Status.Conditions = append(newRollout.Status.Conditions, cond) + //completeCond, _ := newCompleteCondition(isCompleted) + //newRollout.Status.Conditions = append(newRollout.Status.Conditions, completeCond) if pause { now := timeutil.MetaNow() cond := v1alpha1.PauseCondition{ diff --git a/rollout/sync.go b/rollout/sync.go index 4ac5783b3f..10f67d98de 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -925,16 +925,14 @@ func (c *rolloutContext) promoteStable(newStatus *v1alpha1.RolloutStatus, reason } } newStatus.StableRS = newStatus.CurrentPodHash - //revision, _ := replicasetutil.Revision(c.rollout) - //c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, - // conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason) + if conditions.RolloutComplete(c.rollout, newStatus) { revision, _ := replicasetutil.Revision(c.rollout) c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason) updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, - conditions.RolloutCompletedReason, fmt.Sprintf(conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason)) + conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) conditions.SetRolloutCondition(newStatus, *updateCompletedCond) } } diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go index 279eca6008..1904973e84 100644 --- a/utils/conditions/conditions.go +++ b/utils/conditions/conditions.go @@ -317,7 +317,7 @@ func RolloutComplete(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatu completedStrategy = executedAllSteps && currentRSIsStable } - return completedStrategy + return completedStrategy // && rollout.Status.ObservedGeneration == strconv.Itoa(int(rollout.Generation)) } // ComputeStepHash returns a hash value calculated from the Rollout's steps. The hash will From 3f96a3c6576c58f570da34ee09c50c92db39af11 Mon Sep 17 00:00:00 2001 From: zachaller Date: Fri, 19 Aug 2022 13:39:44 -0500 Subject: [PATCH 10/27] Fix e2e Signed-off-by: zachaller --- rollout/bluegreen_test.go | 1 + utils/conditions/rollouts_test.go | 89 ++++++++++++++++++------------- 2 files changed, 52 insertions(+), 38 deletions(-) diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index b69071741a..d41e28920e 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -1499,6 +1499,7 @@ func TestBlueGreenHandlePauseAutoPromoteWithConditions(t *testing.T) { assert.Nil(t, err) completeCond, _ := newCompleteCondition(true) completeCondBytes, err := json.Marshal(completeCond) + assert.Nil(t, err) expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, rs2PodHash, string(availableCondBytes), string(pausedCondBytes), string(completeCondBytes), string(progressingCondBytes), rs2PodHash, rs2PodHash)) f.expectPatchServiceAction(activeSvc, rs2PodHash) patchRolloutIndex := f.expectPatchRolloutActionWithPatch(r2, expectedPatch) diff --git a/utils/conditions/rollouts_test.go b/utils/conditions/rollouts_test.go index a134b4728f..d9f4cc95d6 100644 --- a/utils/conditions/rollouts_test.go +++ b/utils/conditions/rollouts_test.go @@ -404,79 +404,92 @@ func TestRolloutHealthyComplete(t *testing.T) { } tests := []struct { - name string - r *v1alpha1.Rollout - expected bool + name string + r *v1alpha1.Rollout + expectedHealthy bool + expectedComplete bool }{ { name: "BlueGreen complete", // update hash to status.CurrentPodHash after k8s library update - r: blueGreenRollout(5, 5, 5, 5, true, "76bbb58f74", "76bbb58f74"), - expected: true, + r: blueGreenRollout(5, 5, 5, 5, true, "76bbb58f74", "76bbb58f74"), + expectedHealthy: true, + expectedComplete: true, }, { name: "BlueGreen complete with extra old replicas", // update hash to status.CurrentPodHash after k8s library update - r: blueGreenRollout(5, 6, 5, 5, true, "76bbb58f74", "76bbb58f74"), - expected: true, + r: blueGreenRollout(5, 6, 5, 5, true, "76bbb58f74", "76bbb58f74"), + expectedHealthy: true, + expectedComplete: true, }, { - name: "BlueGreen not completed: active service does not point at updated rs", - r: blueGreenRollout(1, 1, 1, 1, true, "not-active", ""), - expected: false, + name: "BlueGreen not completed: active service does not point at updated rs", + r: blueGreenRollout(1, 1, 1, 1, true, "not-active", ""), + expectedHealthy: false, + expectedComplete: false, }, { name: "BlueGreen not completed: preview service does not point at updated rs", // update hash to status.CurrentPodHash after k8s library update - r: blueGreenRollout(1, 1, 1, 1, true, "6cb88c6bcf", ""), - expected: false, + r: blueGreenRollout(1, 1, 1, 1, true, "6cb88c6bcf", ""), + expectedHealthy: false, + expectedComplete: false, }, { - name: "CanaryWithSteps Completed", - r: canaryRollout(1, 1, 1, 1, true, "active", true, pointer.Int32Ptr(1)), - expected: false, + name: "CanaryWithSteps Completed", + r: canaryRollout(1, 1, 1, 1, true, "active", true, pointer.Int32Ptr(1)), + expectedHealthy: false, + expectedComplete: false, }, { - name: "CanaryWithSteps Not Completed: Steps left", - r: canaryRollout(1, 1, 1, 1, true, "active", true, pointer.Int32Ptr(0)), - expected: false, + name: "CanaryWithSteps Not Completed: Steps left", + r: canaryRollout(1, 1, 1, 1, true, "active", true, pointer.Int32Ptr(0)), + expectedHealthy: false, + expectedComplete: false, }, { - name: "CanaryNoSteps Completed", - r: canaryRollout(1, 1, 1, 1, true, "active", false, nil), - expected: false, + name: "CanaryNoSteps Completed", + r: canaryRollout(1, 1, 1, 1, true, "active", false, nil), + expectedHealthy: false, + expectedComplete: false, }, { - name: "Canary Not Completed: Diff stableRs", - r: canaryRollout(1, 1, 1, 1, true, "not-active", false, nil), - expected: false, + name: "Canary Not Completed: Diff stableRs", + r: canaryRollout(1, 1, 1, 1, true, "not-active", false, nil), + expectedHealthy: false, + expectedComplete: false, }, { - name: "not complete: min but not all pods become available", - r: rollout(5, 5, 5, 4, true), - expected: false, + name: "not complete: min but not all pods become available", + r: rollout(5, 5, 5, 4, true), + expectedHealthy: false, + expectedComplete: true, }, { - name: "not complete: all pods are available but not all active", - r: rollout(5, 5, 4, 5, true), - expected: false, + name: "not complete: all pods are available but not all active", + r: rollout(5, 5, 4, 5, true), + expectedHealthy: false, + expectedComplete: true, }, { - name: "Canary not complete: still running old pods", - r: rollout(1, 2, 1, 1, true), - expected: false, + name: "Canary not complete: still running old pods", + r: rollout(1, 2, 1, 1, true), + expectedHealthy: false, + expectedComplete: true, }, { - name: "not complete: Mismatching ObservedGeneration", - r: rollout(1, 2, 1, 1, false), - expected: false, + name: "not complete: Mismatching ObservedGeneration", + r: rollout(1, 2, 1, 1, false), + expectedHealthy: false, + expectedComplete: true, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - assert.Equal(t, test.expected, RolloutComplete(test.r, &test.r.Status)) - assert.Equal(t, test.expected, RolloutHealthyAndComplete(test.r, &test.r.Status)) + assert.Equal(t, test.expectedComplete, RolloutComplete(test.r, &test.r.Status)) + assert.Equal(t, test.expectedHealthy, RolloutHealthyAndComplete(test.r, &test.r.Status)) }) } From 995d9738f1e041e1d983f28c59dfaf89d37bd670 Mon Sep 17 00:00:00 2001 From: zachaller Date: Fri, 19 Aug 2022 15:37:00 -0500 Subject: [PATCH 11/27] rename and fix one unit test Signed-off-by: zachaller --- rollout/analysis_test.go | 2 +- rollout/bluegreen_test.go | 14 ++++++++------ rollout/controller_test.go | 23 +++++++++++++++++------ rollout/sync.go | 2 ++ test/e2e/functional_test.go | 13 +++++++------ utils/conditions/conditions.go | 22 +--------------------- 6 files changed, 36 insertions(+), 40 deletions(-) diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index bc87990040..88ae21bbad 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -2141,7 +2141,7 @@ func TestRolloutPostPromotionAnalysisSuccess(t *testing.T) { activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} activeSvc := newService("active", 80, activeSelector, r2) - cond, _ := newCompleteCondition(true) + cond, _ := newCompletedCondition(true) conditions.SetRolloutCondition(&r2.Status, cond) f.objects = append(f.objects, r2, at, ar) diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index d41e28920e..f7c3ab9c11 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -38,15 +38,17 @@ func newBlueGreenRollout(name string, replicas int, revisionHistoryLimit *int32, return rollout } -func TestBlueGreenComplateRolloutRestart(t *testing.T) { +func TestBlueGreenCompletedRolloutRestart(t *testing.T) { f := newFixture(t) defer f.Close() r := newBlueGreenRollout("foo", 1, nil, "active", "preview") r.Status.Conditions = []v1alpha1.RolloutCondition{} - completedCond := conditions.NewRolloutCondition(v1alpha1.HealthyAndCompleted, corev1.ConditionTrue, conditions.RolloutHealthyAndCompletedReason, conditions.RolloutHealthyAndCompletedReason) - conditions.SetRolloutCondition(&r.Status, *completedCond) + completedHealthyCond := conditions.NewRolloutCondition(v1alpha1.HealthyAndCompleted, corev1.ConditionFalse, conditions.RolloutHealthyAndCompletedReason, conditions.RolloutNotHealthyAndCompletedMessage) + conditions.SetRolloutCondition(&r.Status, *completedHealthyCond) + completedCond, _ := newCompletedCondition(true) + conditions.SetRolloutCondition(&r.Status, completedCond) f.rolloutLister = append(f.rolloutLister, r) f.objects = append(f.objects, r) @@ -57,7 +59,7 @@ func TestBlueGreenComplateRolloutRestart(t *testing.T) { rs := newReplicaSet(r, 1) rsPodHash := rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - generatedConditions := generateConditionsPatchWithHealthy(false, conditions.ReplicaSetNotAvailableReason, rs, false, "", false) + generatedConditions := generateConditionsPatchWithCompletedHealthy(false, conditions.ReplicaSetUpdatedReason, rs, false, "", false, true) f.expectCreateReplicaSetAction(rs) servicePatchIndex := f.expectPatchServiceAction(previewSvc, rsPodHash) @@ -107,7 +109,7 @@ func TestBlueGreenCreatesReplicaSet(t *testing.T) { rs := newReplicaSet(r, 1) rsPodHash := rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - generatedConditions := generateConditionsPatch(false, conditions.ReplicaSetUpdatedReason, rs, false, "") + generatedConditions := generateConditionsPatchWithComplete(false, conditions.ReplicaSetUpdatedReason, rs, false, "", true) f.expectCreateReplicaSetAction(rs) servicePatchIndex := f.expectPatchServiceAction(previewSvc, rsPodHash) @@ -1497,7 +1499,7 @@ func TestBlueGreenHandlePauseAutoPromoteWithConditions(t *testing.T) { assert.Nil(t, err) pausedCondBytes, err := json.Marshal(r2.Status.Conditions[2]) assert.Nil(t, err) - completeCond, _ := newCompleteCondition(true) + completeCond, _ := newCompletedCondition(true) completeCondBytes, err := json.Marshal(completeCond) assert.Nil(t, err) expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, rs2PodHash, string(availableCondBytes), string(pausedCondBytes), string(completeCondBytes), string(progressingCondBytes), rs2PodHash, rs2PodHash)) diff --git a/rollout/controller_test.go b/rollout/controller_test.go index 8fe1314030..b87c25958f 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -215,7 +215,7 @@ func newHealthyCondition(isHealthy bool) (v1alpha1.RolloutCondition, string) { return condition, string(conditionBytes) } -func newCompleteCondition(isCompleted bool) (v1alpha1.RolloutCondition, string) { +func newCompletedCondition(isCompleted bool) (v1alpha1.RolloutCondition, string) { status := corev1.ConditionTrue if !isCompleted { status = corev1.ConditionFalse @@ -359,23 +359,34 @@ func generateConditionsPatchWithPause(available bool, progressingReason string, func generateConditionsPatchWithHealthy(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isHealthy bool) string { _, availableCondition := newAvailableCondition(available) _, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage) - _, completeCondition := newHealthyCondition(isHealthy) + _, healthyCondition := newHealthyCondition(isHealthy) if availableConditionFirst { - return fmt.Sprintf("[%s, %s, %s]", availableCondition, completeCondition, progressingCondition) + return fmt.Sprintf("[%s, %s, %s]", availableCondition, healthyCondition, progressingCondition) } - return fmt.Sprintf("[%s, %s, %s]", completeCondition, progressingCondition, availableCondition) + return fmt.Sprintf("[%s, %s, %s]", healthyCondition, progressingCondition, availableCondition) } func generateConditionsPatchWithComplete(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isCompleted bool) string { _, availableCondition := newAvailableCondition(available) _, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage) - _, completeCondition := newCompleteCondition(isCompleted) + _, completeCondition := newCompletedCondition(isCompleted) if availableConditionFirst { return fmt.Sprintf("[%s, %s, %s]", availableCondition, completeCondition, progressingCondition) } return fmt.Sprintf("[%s, %s, %s]", completeCondition, progressingCondition, availableCondition) } +func generateConditionsPatchWithCompletedHealthy(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isHealthy bool, isCompleted bool) string { + _, completedCondition := newCompletedCondition(isCompleted) + _, availableCondition := newAvailableCondition(available) + _, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage) + _, healthyCondition := newHealthyCondition(isHealthy) + if availableConditionFirst { + return fmt.Sprintf("[%s, %s, %s, %s]", availableCondition, healthyCondition, completedCondition, progressingCondition) + } + return fmt.Sprintf("[%s, %s, %s, %s]", healthyCondition, completedCondition, progressingCondition, availableCondition) +} + func updateConditionsPatch(r v1alpha1.Rollout, newCondition v1alpha1.RolloutCondition) string { conditions.SetRolloutCondition(&r.Status, newCondition) conditionsBytes, _ := json.Marshal(r.Status.Conditions) @@ -395,7 +406,7 @@ func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active, stable s newRollout.Status.StableRS = stable cond, _ := newAvailableCondition(available) newRollout.Status.Conditions = append(newRollout.Status.Conditions, cond) - //completeCond, _ := newCompleteCondition(isCompleted) + //completeCond, _ := newCompletedCondition(isCompleted) //newRollout.Status.Conditions = append(newRollout.Status.Conditions, completeCond) if pause { now := timeutil.MetaNow() diff --git a/rollout/sync.go b/rollout/sync.go index 10f67d98de..4a9c3f0f67 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -552,6 +552,8 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt if !isPaused && conditions.RolloutHealthyAndComplete(c.rollout, &newStatus) { updateHealthyCond := conditions.NewRolloutCondition(v1alpha1.HealthyAndCompleted, corev1.ConditionTrue, conditions.RolloutHealthyAndCompletedReason, conditions.RolloutHealthyAndCompletedMessage) conditions.SetRolloutCondition(&newStatus, *updateHealthyCond) + // If we ever wanted to emit a healthy event here it would be noisy and somewhat unpredictable for tests and so should probably skip + // checking in e2e tests. //c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.RolloutHealthyAndCompletedReason}, conditions.RolloutHealthyAndCompletedMessage) } else { if completeCond != nil { diff --git a/test/e2e/functional_test.go b/test/e2e/functional_test.go index a5b5f75a42..a10a7c8933 100644 --- a/test/e2e/functional_test.go +++ b/test/e2e/functional_test.go @@ -702,12 +702,13 @@ func (s *FunctionalSuite) TestBlueGreenUpdate() { "RolloutUpdated", // Rollout updated to revision 1 "NewReplicaSetCreated", // Created ReplicaSet bluegreen-7dcd8f8869 (revision 1) "ScalingReplicaSet", // Scaled up ReplicaSet bluegreen-7dcd8f8869 (revision 1) from 0 to 3 - "SwitchService", // Rollout completed update to revision 1 (7dcd8f8869): Initial deploy - "RolloutUpdated", // Switched selector for service 'bluegreen' from '' to '7dcd8f8869' - "NewReplicaSetCreated", // Rollout updated to revision 2 - "ScalingReplicaSet", // Created ReplicaSet bluegreen-5498785cd6 (revision 2) - "SwitchService", // Scaled up ReplicaSet bluegreen-5498785cd6 (revision 2) from 0 to 3 - "RolloutCompleted", // Switched selector for service 'bluegreen' from '7dcd8f8869' to '6c779b88b6' + "RolloutCompleted", // Rollout completed update to revision 1 (7dcd8f8869): Initial deploy + "SwitchService", // Switched selector for service 'bluegreen' from '' to '7dcd8f8869' + "RolloutUpdated", // Rollout updated to revision 2 + "NewReplicaSetCreated", // Created ReplicaSet bluegreen-5498785cd6 (revision 2) + "ScalingReplicaSet", // Scaled up ReplicaSet bluegreen-5498785cd6 (revision 2) from 0 to 3 + "SwitchService", // Switched selector for service 'bluegreen' from '7dcd8f8869' to '6c779b88b6' + "RolloutCompleted", // Rollout completed update to revision 2 (6c779b88b6): Completed blue-green update }) } diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go index 1904973e84..7c8a091212 100644 --- a/utils/conditions/conditions.go +++ b/utils/conditions/conditions.go @@ -297,27 +297,7 @@ func RolloutHealthyAndComplete(rollout *v1alpha1.Rollout, newStatus *v1alpha1.Ro // RolloutComplete considers a rollout to be complete once func RolloutComplete(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) bool { - completedStrategy := true - - if rollout.Spec.Strategy.BlueGreen != nil { - activeSelectorComplete := newStatus.BlueGreen.ActiveSelector == newStatus.CurrentPodHash - previewSelectorComplete := true - if rollout.Spec.Strategy.BlueGreen.PreviewService != "" { - previewSelectorComplete = newStatus.BlueGreen.PreviewSelector == newStatus.CurrentPodHash - } - completedStrategy = activeSelectorComplete && previewSelectorComplete - } - if rollout.Spec.Strategy.Canary != nil { - stepCount := len(rollout.Spec.Strategy.Canary.Steps) - executedAllSteps := true - if stepCount > 0 && newStatus.CurrentStepIndex != nil { - executedAllSteps = int32(stepCount) == *newStatus.CurrentStepIndex - } - currentRSIsStable := newStatus.StableRS != "" && newStatus.StableRS == newStatus.CurrentPodHash - completedStrategy = executedAllSteps && currentRSIsStable - } - - return completedStrategy // && rollout.Status.ObservedGeneration == strconv.Itoa(int(rollout.Generation)) + return newStatus.StableRS != "" && newStatus.StableRS == newStatus.CurrentPodHash } // ComputeStepHash returns a hash value calculated from the Rollout's steps. The hash will From c0890874fa70a76bdab7c2f438194adda223a528 Mon Sep 17 00:00:00 2001 From: zachaller Date: Fri, 19 Aug 2022 15:50:32 -0500 Subject: [PATCH 12/27] fix unit tests Signed-off-by: zachaller --- utils/conditions/rollouts_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/utils/conditions/rollouts_test.go b/utils/conditions/rollouts_test.go index d9f4cc95d6..c0958f186f 100644 --- a/utils/conditions/rollouts_test.go +++ b/utils/conditions/rollouts_test.go @@ -470,7 +470,7 @@ func TestRolloutHealthyComplete(t *testing.T) { name: "not complete: all pods are available but not all active", r: rollout(5, 5, 4, 5, true), expectedHealthy: false, - expectedComplete: true, + expectedComplete: false, }, { name: "Canary not complete: still running old pods", @@ -482,7 +482,7 @@ func TestRolloutHealthyComplete(t *testing.T) { name: "not complete: Mismatching ObservedGeneration", r: rollout(1, 2, 1, 1, false), expectedHealthy: false, - expectedComplete: true, + expectedComplete: false, }, } From 7f5b2873169b3a2f6057302897a90023090d5737 Mon Sep 17 00:00:00 2001 From: zachaller Date: Fri, 19 Aug 2022 16:36:06 -0500 Subject: [PATCH 13/27] fix unit test Signed-off-by: zachaller --- utils/conditions/rollouts_test.go | 54 ++++++++++++++++--------------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/utils/conditions/rollouts_test.go b/utils/conditions/rollouts_test.go index c0958f186f..a0c566abf5 100644 --- a/utils/conditions/rollouts_test.go +++ b/utils/conditions/rollouts_test.go @@ -351,7 +351,7 @@ func TestRolloutProgressing(t *testing.T) { } func TestRolloutHealthyComplete(t *testing.T) { - rollout := func(desired, current, updated, available int32, correctObservedGeneration bool) *v1alpha1.Rollout { + rollout := func(desired, current, updated, available int32, correctObservedGeneration bool, isCompleted bool) *v1alpha1.Rollout { r := &v1alpha1.Rollout{ Spec: v1alpha1.RolloutSpec{ Replicas: &desired, @@ -365,11 +365,14 @@ func TestRolloutHealthyComplete(t *testing.T) { r.Generation = 123 podHash := hash.ComputePodTemplateHash(&r.Spec.Template, r.Status.CollisionCount) r.Status.CurrentPodHash = podHash + if isCompleted { + r.Status.StableRS = r.Status.CurrentPodHash + } return r } - blueGreenRollout := func(desired, current, updated, available int32, correctObservedGeneration bool, activeSelector, previewSelector string) *v1alpha1.Rollout { - r := rollout(desired, current, updated, available, correctObservedGeneration) + blueGreenRollout := func(desired, current, updated, available int32, correctObservedGeneration bool, activeSelector, previewSelector string, isCompleted bool) *v1alpha1.Rollout { + r := rollout(desired, current, updated, available, correctObservedGeneration, isCompleted) r.Spec.Strategy = v1alpha1.RolloutStrategy{ BlueGreen: &v1alpha1.BlueGreenStrategy{ PreviewService: "preview", @@ -384,8 +387,8 @@ func TestRolloutHealthyComplete(t *testing.T) { return r } - canaryRollout := func(desired, current, updated, available int32, correctObservedGeneration bool, stableRS string, hasSteps bool, stepIndex *int32) *v1alpha1.Rollout { - r := rollout(desired, current, updated, available, correctObservedGeneration) + canaryRollout := func(desired, current, updated, available int32, correctObservedGeneration bool, hasSteps bool, stepIndex *int32, isCompleted bool) *v1alpha1.Rollout { + r := rollout(desired, current, updated, available, correctObservedGeneration, isCompleted) steps := []v1alpha1.CanaryStep{} if hasSteps { steps = append(steps, v1alpha1.CanaryStep{SetWeight: pointer.Int32Ptr(30)}) @@ -395,7 +398,6 @@ func TestRolloutHealthyComplete(t *testing.T) { Steps: steps, }, } - r.Status.StableRS = stableRS r.Status.CurrentStepIndex = stepIndex if correctObservedGeneration { r.Status.ObservedGeneration = strconv.Itoa(int(r.Generation)) @@ -412,77 +414,77 @@ func TestRolloutHealthyComplete(t *testing.T) { { name: "BlueGreen complete", // update hash to status.CurrentPodHash after k8s library update - r: blueGreenRollout(5, 5, 5, 5, true, "76bbb58f74", "76bbb58f74"), + r: blueGreenRollout(5, 5, 5, 5, true, "76bbb58f74", "76bbb58f74", true), expectedHealthy: true, expectedComplete: true, }, { name: "BlueGreen complete with extra old replicas", // update hash to status.CurrentPodHash after k8s library update - r: blueGreenRollout(5, 6, 5, 5, true, "76bbb58f74", "76bbb58f74"), + r: blueGreenRollout(5, 6, 5, 5, true, "76bbb58f74", "76bbb58f74", true), expectedHealthy: true, expectedComplete: true, }, { name: "BlueGreen not completed: active service does not point at updated rs", - r: blueGreenRollout(1, 1, 1, 1, true, "not-active", ""), + r: blueGreenRollout(1, 1, 1, 1, true, "not-active", "", false), expectedHealthy: false, expectedComplete: false, }, { name: "BlueGreen not completed: preview service does not point at updated rs", // update hash to status.CurrentPodHash after k8s library update - r: blueGreenRollout(1, 1, 1, 1, true, "6cb88c6bcf", ""), + r: blueGreenRollout(1, 1, 1, 1, true, "6cb88c6bcf", "", false), expectedHealthy: false, expectedComplete: false, }, { - name: "CanaryWithSteps Completed", - r: canaryRollout(1, 1, 1, 1, true, "active", true, pointer.Int32Ptr(1)), - expectedHealthy: false, - expectedComplete: false, + name: "CanaryWithSteps Completed and Healthy", + r: canaryRollout(1, 1, 1, 1, true, true, pointer.Int32Ptr(1), true), + expectedHealthy: true, + expectedComplete: true, }, { - name: "CanaryWithSteps Not Completed: Steps left", - r: canaryRollout(1, 1, 1, 1, true, "active", true, pointer.Int32Ptr(0)), + name: "CanaryWithSteps Not Healthy and Not completed: Steps left", + r: canaryRollout(1, 1, 1, 1, true, true, pointer.Int32Ptr(0), false), expectedHealthy: false, expectedComplete: false, }, { name: "CanaryNoSteps Completed", - r: canaryRollout(1, 1, 1, 1, true, "active", false, nil), - expectedHealthy: false, - expectedComplete: false, + r: canaryRollout(1, 1, 1, 1, true, false, nil, true), + expectedHealthy: true, + expectedComplete: true, }, { name: "Canary Not Completed: Diff stableRs", - r: canaryRollout(1, 1, 1, 1, true, "not-active", false, nil), + r: canaryRollout(1, 1, 1, 1, true, false, nil, false), expectedHealthy: false, expectedComplete: false, }, { name: "not complete: min but not all pods become available", - r: rollout(5, 5, 5, 4, true), + r: rollout(5, 5, 5, 4, true, true), expectedHealthy: false, expectedComplete: true, }, { name: "not complete: all pods are available but not all active", - r: rollout(5, 5, 4, 5, true), + r: rollout(5, 5, 4, 5, true, true), expectedHealthy: false, - expectedComplete: false, + expectedComplete: true, }, { name: "Canary not complete: still running old pods", - r: rollout(1, 2, 1, 1, true), + r: rollout(1, 2, 1, 1, true, true), expectedHealthy: false, expectedComplete: true, }, { name: "not complete: Mismatching ObservedGeneration", - r: rollout(1, 2, 1, 1, false), + r: rollout(1, 2, 1, 1, false, true), expectedHealthy: false, - expectedComplete: false, + expectedComplete: true, }, } From 58caf4d4555d4f9d709a45c240aa8d18edf82430 Mon Sep 17 00:00:00 2001 From: zachaller Date: Fri, 19 Aug 2022 16:45:14 -0500 Subject: [PATCH 14/27] add seperate test for TestRolloutComplete Signed-off-by: zachaller --- utils/conditions/rollouts_test.go | 128 ++++++++++++++++-------------- 1 file changed, 68 insertions(+), 60 deletions(-) diff --git a/utils/conditions/rollouts_test.go b/utils/conditions/rollouts_test.go index a0c566abf5..075e1d2a44 100644 --- a/utils/conditions/rollouts_test.go +++ b/utils/conditions/rollouts_test.go @@ -350,8 +350,8 @@ func TestRolloutProgressing(t *testing.T) { } -func TestRolloutHealthyComplete(t *testing.T) { - rollout := func(desired, current, updated, available int32, correctObservedGeneration bool, isCompleted bool) *v1alpha1.Rollout { +func TestRolloutHealthyAndComplete(t *testing.T) { + rollout := func(desired, current, updated, available int32, correctObservedGeneration bool) *v1alpha1.Rollout { r := &v1alpha1.Rollout{ Spec: v1alpha1.RolloutSpec{ Replicas: &desired, @@ -365,14 +365,11 @@ func TestRolloutHealthyComplete(t *testing.T) { r.Generation = 123 podHash := hash.ComputePodTemplateHash(&r.Spec.Template, r.Status.CollisionCount) r.Status.CurrentPodHash = podHash - if isCompleted { - r.Status.StableRS = r.Status.CurrentPodHash - } return r } - blueGreenRollout := func(desired, current, updated, available int32, correctObservedGeneration bool, activeSelector, previewSelector string, isCompleted bool) *v1alpha1.Rollout { - r := rollout(desired, current, updated, available, correctObservedGeneration, isCompleted) + blueGreenRollout := func(desired, current, updated, available int32, correctObservedGeneration bool, activeSelector, previewSelector string) *v1alpha1.Rollout { + r := rollout(desired, current, updated, available, correctObservedGeneration) r.Spec.Strategy = v1alpha1.RolloutStrategy{ BlueGreen: &v1alpha1.BlueGreenStrategy{ PreviewService: "preview", @@ -387,8 +384,8 @@ func TestRolloutHealthyComplete(t *testing.T) { return r } - canaryRollout := func(desired, current, updated, available int32, correctObservedGeneration bool, hasSteps bool, stepIndex *int32, isCompleted bool) *v1alpha1.Rollout { - r := rollout(desired, current, updated, available, correctObservedGeneration, isCompleted) + canaryRollout := func(desired, current, updated, available int32, correctObservedGeneration bool, stableRS string, hasSteps bool, stepIndex *int32) *v1alpha1.Rollout { + r := rollout(desired, current, updated, available, correctObservedGeneration) steps := []v1alpha1.CanaryStep{} if hasSteps { steps = append(steps, v1alpha1.CanaryStep{SetWeight: pointer.Int32Ptr(30)}) @@ -398,6 +395,7 @@ func TestRolloutHealthyComplete(t *testing.T) { Steps: steps, }, } + r.Status.StableRS = stableRS r.Status.CurrentStepIndex = stepIndex if correctObservedGeneration { r.Status.ObservedGeneration = strconv.Itoa(int(r.Generation)) @@ -406,97 +404,107 @@ func TestRolloutHealthyComplete(t *testing.T) { } tests := []struct { - name string - r *v1alpha1.Rollout - expectedHealthy bool - expectedComplete bool + name string + r *v1alpha1.Rollout + expected bool }{ { name: "BlueGreen complete", // update hash to status.CurrentPodHash after k8s library update - r: blueGreenRollout(5, 5, 5, 5, true, "76bbb58f74", "76bbb58f74", true), - expectedHealthy: true, - expectedComplete: true, + r: blueGreenRollout(5, 5, 5, 5, true, "76bbb58f74", "76bbb58f74"), + expected: true, }, { name: "BlueGreen complete with extra old replicas", // update hash to status.CurrentPodHash after k8s library update - r: blueGreenRollout(5, 6, 5, 5, true, "76bbb58f74", "76bbb58f74", true), - expectedHealthy: true, - expectedComplete: true, + r: blueGreenRollout(5, 6, 5, 5, true, "76bbb58f74", "76bbb58f74"), + expected: true, }, { - name: "BlueGreen not completed: active service does not point at updated rs", - r: blueGreenRollout(1, 1, 1, 1, true, "not-active", "", false), - expectedHealthy: false, - expectedComplete: false, + name: "BlueGreen not completed: active service does not point at updated rs", + r: blueGreenRollout(1, 1, 1, 1, true, "not-active", ""), + expected: false, }, { name: "BlueGreen not completed: preview service does not point at updated rs", // update hash to status.CurrentPodHash after k8s library update - r: blueGreenRollout(1, 1, 1, 1, true, "6cb88c6bcf", "", false), - expectedHealthy: false, - expectedComplete: false, + r: blueGreenRollout(1, 1, 1, 1, true, "6cb88c6bcf", ""), + expected: false, }, { - name: "CanaryWithSteps Completed and Healthy", - r: canaryRollout(1, 1, 1, 1, true, true, pointer.Int32Ptr(1), true), - expectedHealthy: true, - expectedComplete: true, + name: "CanaryWithSteps Completed", + r: canaryRollout(1, 1, 1, 1, true, "active", true, pointer.Int32Ptr(1)), + expected: false, }, { - name: "CanaryWithSteps Not Healthy and Not completed: Steps left", - r: canaryRollout(1, 1, 1, 1, true, true, pointer.Int32Ptr(0), false), - expectedHealthy: false, - expectedComplete: false, + name: "CanaryWithSteps Not Completed: Steps left", + r: canaryRollout(1, 1, 1, 1, true, "active", true, pointer.Int32Ptr(0)), + expected: false, }, { - name: "CanaryNoSteps Completed", - r: canaryRollout(1, 1, 1, 1, true, false, nil, true), - expectedHealthy: true, - expectedComplete: true, + name: "CanaryNoSteps Completed", + r: canaryRollout(1, 1, 1, 1, true, "active", false, nil), + expected: false, }, { - name: "Canary Not Completed: Diff stableRs", - r: canaryRollout(1, 1, 1, 1, true, false, nil, false), - expectedHealthy: false, - expectedComplete: false, + name: "Canary Not Completed: Diff stableRs", + r: canaryRollout(1, 1, 1, 1, true, "not-active", false, nil), + expected: false, }, { - name: "not complete: min but not all pods become available", - r: rollout(5, 5, 5, 4, true, true), - expectedHealthy: false, - expectedComplete: true, + name: "not complete: min but not all pods become available", + r: rollout(5, 5, 5, 4, true), + expected: false, }, { - name: "not complete: all pods are available but not all active", - r: rollout(5, 5, 4, 5, true, true), - expectedHealthy: false, - expectedComplete: true, + name: "not complete: all pods are available but not all active", + r: rollout(5, 5, 4, 5, true), + expected: false, }, { - name: "Canary not complete: still running old pods", - r: rollout(1, 2, 1, 1, true, true), - expectedHealthy: false, - expectedComplete: true, + name: "Canary not complete: still running old pods", + r: rollout(1, 2, 1, 1, true), + expected: false, }, { - name: "not complete: Mismatching ObservedGeneration", - r: rollout(1, 2, 1, 1, false, true), - expectedHealthy: false, - expectedComplete: true, + name: "not complete: Mismatching ObservedGeneration", + r: rollout(1, 2, 1, 1, false), + expected: false, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - assert.Equal(t, test.expectedComplete, RolloutComplete(test.r, &test.r.Status)) - assert.Equal(t, test.expectedHealthy, RolloutHealthyAndComplete(test.r, &test.r.Status)) + assert.Equal(t, test.expected, RolloutHealthyAndComplete(test.r, &test.r.Status)) }) } } +func TestRolloutComplete(t *testing.T) { + rollout := func(desired, current, updated, available int32) *v1alpha1.Rollout { + r := &v1alpha1.Rollout{ + Spec: v1alpha1.RolloutSpec{ + Replicas: &desired, + }, + Status: v1alpha1.RolloutStatus{ + Replicas: current, + UpdatedReplicas: updated, + AvailableReplicas: available, + }, + } + podHash := hash.ComputePodTemplateHash(&r.Spec.Template, r.Status.CollisionCount) + r.Status.CurrentPodHash = podHash + r.Status.StableRS = podHash + return r + } + r := rollout(5, 5, 5, 5) + assert.Equal(t, true, RolloutComplete(r, &r.Status)) + + r.Status.StableRS = "not-current-pod-hash" + assert.Equal(t, false, RolloutComplete(r, &r.Status)) +} + func TestRolloutTimedOut(t *testing.T) { before := metav1.Time{ From 42dc1396228e94c71425436a2da535ef3c242953 Mon Sep 17 00:00:00 2001 From: zachaller Date: Fri, 19 Aug 2022 17:49:36 -0500 Subject: [PATCH 15/27] renames Signed-off-by: zachaller --- rollout/sync.go | 24 ++++++++++++------------ utils/conditions/conditions.go | 8 ++++---- utils/conditions/rollouts_test.go | 6 +++--- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/rollout/sync.go b/rollout/sync.go index 4a9c3f0f67..204b57b628 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -547,18 +547,18 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt isPaused := len(c.rollout.Status.PauseConditions) > 0 || c.rollout.Spec.Paused isAborted := c.pauseContext.IsAborted() - var becameIncomplete bool // remember if we transitioned from completed + var becameIncompleteUnhealthy bool // remember if we transitioned from completed completeCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.HealthyAndCompleted) - if !isPaused && conditions.RolloutHealthyAndComplete(c.rollout, &newStatus) { + if !isPaused && conditions.RolloutHealthyAndCompleted(c.rollout, &newStatus) { updateHealthyCond := conditions.NewRolloutCondition(v1alpha1.HealthyAndCompleted, corev1.ConditionTrue, conditions.RolloutHealthyAndCompletedReason, conditions.RolloutHealthyAndCompletedMessage) conditions.SetRolloutCondition(&newStatus, *updateHealthyCond) - // If we ever wanted to emit a healthy event here it would be noisy and somewhat unpredictable for tests and so should probably skip - // checking in e2e tests. + // If we ever wanted to emit a healthy event here it would be noisy and somewhat unpredictable for tests and so should probably be skipped + // when checking in e2e and unit tests. //c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.RolloutHealthyAndCompletedReason}, conditions.RolloutHealthyAndCompletedMessage) } else { if completeCond != nil { updateHealthyCond := conditions.NewRolloutCondition(v1alpha1.HealthyAndCompleted, corev1.ConditionFalse, conditions.RolloutHealthyAndCompletedReason, conditions.RolloutNotHealthyAndCompletedMessage) - becameIncomplete = conditions.SetRolloutCondition(&newStatus, *updateHealthyCond) + becameIncompleteUnhealthy = conditions.SetRolloutCondition(&newStatus, *updateHealthyCond) //c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.RolloutHealthyAndCompletedReason}, conditions.RolloutNotHealthyAndCompletedMessage) } } @@ -580,11 +580,11 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt // In such a case, we should simply not estimate any progress for this rollout. currentCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutProgressing) - isHealthyRollout := newStatus.Replicas == newStatus.AvailableReplicas && currentCond != nil && currentCond.Reason == conditions.NewRSAvailableReason && currentCond.Type != v1alpha1.RolloutProgressing + isHealthyAndCompletedRollout := newStatus.Replicas == newStatus.AvailableReplicas && currentCond != nil && currentCond.Reason == conditions.NewRSAvailableReason && currentCond.Type != v1alpha1.RolloutProgressing // Check for progress. Only do this if the latest rollout hasn't completed yet and it is not aborted - if !isHealthyRollout && !isAborted { + if !isHealthyAndCompletedRollout && !isAborted { switch { - case conditions.RolloutHealthyAndComplete(c.rollout, &newStatus): + case conditions.RolloutHealthyAndCompleted(c.rollout, &newStatus): // Update the rollout conditions with a message for the new replica set that // was successfully deployed. If the condition already exists, we ignore this update. rsName := "" @@ -594,7 +594,7 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt msg := fmt.Sprintf(conditions.ReplicaSetCompletedMessage, rsName) progressingCondition := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionTrue, conditions.NewRSAvailableReason, msg) conditions.SetRolloutCondition(&newStatus, *progressingCondition) - case conditions.RolloutProgressing(c.rollout, &newStatus) || becameIncomplete: + case conditions.RolloutProgressing(c.rollout, &newStatus) || becameIncompleteUnhealthy: // If there is any progress made, continue by not checking if the rollout failed. This // behavior emulates the rolling updater progressDeadline check. msg := fmt.Sprintf(conditions.RolloutProgressingMessage, c.rollout.Name) @@ -603,7 +603,7 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt } var reason string - if newStatus.StableRS == newStatus.CurrentPodHash && becameIncomplete { + if newStatus.StableRS == newStatus.CurrentPodHash && becameIncompleteUnhealthy { // When a fully promoted rollout becomes Incomplete, e.g., due to the ReplicaSet status changes like // pod restarts, evicted -> recreated, we'll need to reset the rollout's condition to `PROGRESSING` to // avoid any timeouts. @@ -758,7 +758,7 @@ func (c *rolloutContext) requeueStuckRollout(newStatus v1alpha1.RolloutStatus) t } // No need to estimate progress if the rollout is complete or already timed out. isPaused := len(c.rollout.Status.PauseConditions) > 0 || c.rollout.Spec.Paused - if conditions.RolloutHealthyAndComplete(c.rollout, &newStatus) || currentCond.Reason == conditions.TimedOutReason || isPaused || c.rollout.Status.Abort || isIndefiniteStep(c.rollout) { + if conditions.RolloutHealthyAndCompleted(c.rollout, &newStatus) || currentCond.Reason == conditions.TimedOutReason || isPaused || c.rollout.Status.Abort || isIndefiniteStep(c.rollout) { return time.Duration(-1) } // If there is no sign of progress at this point then there is a high chance that the @@ -928,7 +928,7 @@ func (c *rolloutContext) promoteStable(newStatus *v1alpha1.RolloutStatus, reason } newStatus.StableRS = newStatus.CurrentPodHash - if conditions.RolloutComplete(c.rollout, newStatus) { + if conditions.RolloutCompleted(c.rollout, newStatus) { revision, _ := replicasetutil.Revision(c.rollout) c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason) diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go index 7c8a091212..16597fe272 100644 --- a/utils/conditions/conditions.go +++ b/utils/conditions/conditions.go @@ -264,9 +264,9 @@ func RolloutProgressing(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutSt strategySpecificProgress } -// RolloutHealthyAndComplete considers a rollout to be complete once all of its desired replicas +// RolloutHealthyAndCompleted considers a rollout to be complete once all of its desired replicas // are updated, available, and receiving traffic from the active service, and no old pods are running. -func RolloutHealthyAndComplete(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) bool { +func RolloutHealthyAndCompleted(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) bool { completedStrategy := true replicas := defaults.GetReplicasOrDefault(rollout.Spec.Replicas) @@ -295,8 +295,8 @@ func RolloutHealthyAndComplete(rollout *v1alpha1.Rollout, newStatus *v1alpha1.Ro completedStrategy } -// RolloutComplete considers a rollout to be complete once -func RolloutComplete(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) bool { +// RolloutCompleted considers a rollout to be complete once StableRS == CurrentPodHash +func RolloutCompleted(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) bool { return newStatus.StableRS != "" && newStatus.StableRS == newStatus.CurrentPodHash } diff --git a/utils/conditions/rollouts_test.go b/utils/conditions/rollouts_test.go index 075e1d2a44..4985b8f728 100644 --- a/utils/conditions/rollouts_test.go +++ b/utils/conditions/rollouts_test.go @@ -475,7 +475,7 @@ func TestRolloutHealthyAndComplete(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - assert.Equal(t, test.expected, RolloutHealthyAndComplete(test.r, &test.r.Status)) + assert.Equal(t, test.expected, RolloutHealthyAndCompleted(test.r, &test.r.Status)) }) } @@ -499,10 +499,10 @@ func TestRolloutComplete(t *testing.T) { return r } r := rollout(5, 5, 5, 5) - assert.Equal(t, true, RolloutComplete(r, &r.Status)) + assert.Equal(t, true, RolloutCompleted(r, &r.Status)) r.Status.StableRS = "not-current-pod-hash" - assert.Equal(t, false, RolloutComplete(r, &r.Status)) + assert.Equal(t, false, RolloutCompleted(r, &r.Status)) } func TestRolloutTimedOut(t *testing.T) { From 09cd520e0faa98c81da2cb81ccec3c986580dd45 Mon Sep 17 00:00:00 2001 From: zachaller Date: Fri, 19 Aug 2022 22:05:33 -0500 Subject: [PATCH 16/27] fix e2e Signed-off-by: zachaller --- test/e2e/functional_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/functional_test.go b/test/e2e/functional_test.go index a10a7c8933..1171f57ebf 100644 --- a/test/e2e/functional_test.go +++ b/test/e2e/functional_test.go @@ -959,7 +959,7 @@ spec: Then(). ExpectRevisionPodCount("2", 0). ExpectRollout("Abort=True", func(r *v1alpha1.Rollout) bool { - return r.Status.Abort == true && len(r.Status.Conditions) == 3 + return r.Status.Abort == true && len(r.Status.Conditions) == 4 }) } From dded8f5270e9be27207dff654c0381d0080f93d6 Mon Sep 17 00:00:00 2001 From: zachaller Date: Mon, 22 Aug 2022 11:35:40 -0500 Subject: [PATCH 17/27] Set Completed to false Signed-off-by: zachaller --- rollout/sync.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/rollout/sync.go b/rollout/sync.go index 204b57b628..d18d052a1f 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -676,6 +676,12 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt } else { conditions.RemoveRolloutCondition(&newStatus, v1alpha1.RolloutReplicaFailure) } + + if !conditions.RolloutCompleted(c.rollout, &newStatus) { + updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionFalse, + conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) + conditions.SetRolloutCondition(&newStatus, *updateCompletedCond) + } return newStatus } From 1ff9a32773f44db7ec84bfd569f1205fec592606 Mon Sep 17 00:00:00 2001 From: zachaller Date: Mon, 22 Aug 2022 12:28:16 -0500 Subject: [PATCH 18/27] Add event Signed-off-by: zachaller --- rollout/sync.go | 4 ++++ utils/conditions/conditions.go | 2 ++ 2 files changed, 6 insertions(+) diff --git a/rollout/sync.go b/rollout/sync.go index d18d052a1f..008ca1fe0f 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -681,6 +681,10 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionFalse, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) conditions.SetRolloutCondition(&newStatus, *updateCompletedCond) + + revision, _ := replicasetutil.Revision(c.rollout) + c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, + conditions.RolloutNotCompletedMessage, revision, newStatus.CurrentPodHash) } return newStatus } diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go index 16597fe272..d5bded8da3 100644 --- a/utils/conditions/conditions.go +++ b/utils/conditions/conditions.go @@ -68,6 +68,8 @@ const ( RolloutCompletedReason = "RolloutCompleted" // RolloutCompletedMessage is added when the rollout is completed RolloutCompletedMessage = "Rollout completed update to revision %d (%s): %s" + // RolloutNotCompletedMessage is added when the rollout is completed + RolloutNotCompletedMessage = "Rollout went to not completed state started update to revision %d (%s)" // RolloutHealthyAndCompletedReason is added in a rollout when it is healthy. RolloutHealthyAndCompletedReason = "HealthyAndCompleted" From d9f49c8dcb8c8ebcb3e1d56ae7d521027746d5f1 Mon Sep 17 00:00:00 2001 From: zachaller Date: Mon, 22 Aug 2022 14:37:18 -0500 Subject: [PATCH 19/27] fix e2e Signed-off-by: zachaller --- rollout/sync.go | 20 ++++++++++---------- test/e2e/functional_test.go | 3 +++ utils/conditions/conditions.go | 2 +- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/rollout/sync.go b/rollout/sync.go index 008ca1fe0f..e772459fb3 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -680,11 +680,11 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt if !conditions.RolloutCompleted(c.rollout, &newStatus) { updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionFalse, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) - conditions.SetRolloutCondition(&newStatus, *updateCompletedCond) - - revision, _ := replicasetutil.Revision(c.rollout) - c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, - conditions.RolloutNotCompletedMessage, revision, newStatus.CurrentPodHash) + if conditions.SetRolloutCondition(&newStatus, *updateCompletedCond) { + revision, _ := replicasetutil.Revision(c.rollout) + c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: "RolloutNotCompleted"}, + conditions.RolloutNotCompletedMessage, revision+1, newStatus.CurrentPodHash) + } } return newStatus } @@ -939,13 +939,13 @@ func (c *rolloutContext) promoteStable(newStatus *v1alpha1.RolloutStatus, reason newStatus.StableRS = newStatus.CurrentPodHash if conditions.RolloutCompleted(c.rollout, newStatus) { - revision, _ := replicasetutil.Revision(c.rollout) - c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, - conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason) - updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) - conditions.SetRolloutCondition(newStatus, *updateCompletedCond) + if conditions.SetRolloutCondition(newStatus, *updateCompletedCond) { + revision, _ := replicasetutil.Revision(c.rollout) + c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, + conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason) + } } } return nil diff --git a/test/e2e/functional_test.go b/test/e2e/functional_test.go index 1171f57ebf..7d24d9655a 100644 --- a/test/e2e/functional_test.go +++ b/test/e2e/functional_test.go @@ -90,10 +90,12 @@ spec: ExpectRevisionPodCount("1", 0). ExpectRevisionPodCount("2", 1). ExpectRolloutEvents([]string{ + "RolloutNotCompleted", // Rollout not completed, started update to revision 0 (7fd9b5545c) "RolloutUpdated", // Rollout updated to revision 1 "NewReplicaSetCreated", // Created ReplicaSet abort-retry-promote-698fbfb9dc (revision 1) "ScalingReplicaSet", // Scaled up ReplicaSet abort-retry-promote-698fbfb9dc (revision 1) from 0 to 1 "RolloutCompleted", // Rollout completed update to revision 1 (698fbfb9dc): Initial deploy + "RolloutNotCompleted", "RolloutUpdated", // Rollout updated to revision 2 "NewReplicaSetCreated", // Created ReplicaSet abort-retry-promote-75dcb5ddd6 (revision 2) "ScalingReplicaSet", // Scaled up ReplicaSet abort-retry-promote-75dcb5ddd6 (revision 2) from 0 to 1 @@ -706,6 +708,7 @@ func (s *FunctionalSuite) TestBlueGreenUpdate() { "SwitchService", // Switched selector for service 'bluegreen' from '' to '7dcd8f8869' "RolloutUpdated", // Rollout updated to revision 2 "NewReplicaSetCreated", // Created ReplicaSet bluegreen-5498785cd6 (revision 2) + "RolloutNotCompleted", // Rollout went to not completed state started update to revision 2 (85c6899) "ScalingReplicaSet", // Scaled up ReplicaSet bluegreen-5498785cd6 (revision 2) from 0 to 3 "SwitchService", // Switched selector for service 'bluegreen' from '7dcd8f8869' to '6c779b88b6' "RolloutCompleted", // Rollout completed update to revision 2 (6c779b88b6): Completed blue-green update diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go index d5bded8da3..4c6d8ad91f 100644 --- a/utils/conditions/conditions.go +++ b/utils/conditions/conditions.go @@ -69,7 +69,7 @@ const ( // RolloutCompletedMessage is added when the rollout is completed RolloutCompletedMessage = "Rollout completed update to revision %d (%s): %s" // RolloutNotCompletedMessage is added when the rollout is completed - RolloutNotCompletedMessage = "Rollout went to not completed state started update to revision %d (%s)" + RolloutNotCompletedMessage = "Rollout not completed, started update to revision %d (%s)" // RolloutHealthyAndCompletedReason is added in a rollout when it is healthy. RolloutHealthyAndCompletedReason = "HealthyAndCompleted" From 16ee38a7f5a94dc92621e0c6b2e495799d7c1c19 Mon Sep 17 00:00:00 2001 From: zachaller Date: Mon, 22 Aug 2022 16:29:43 -0500 Subject: [PATCH 20/27] refactor Signed-off-by: zachaller --- rollout/sync.go | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/rollout/sync.go b/rollout/sync.go index e772459fb3..dd9a8e94ca 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -677,7 +677,11 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt conditions.RemoveRolloutCondition(&newStatus, v1alpha1.RolloutReplicaFailure) } - if !conditions.RolloutCompleted(c.rollout, &newStatus) { + if conditions.RolloutCompleted(c.rollout, &newStatus) { + updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, + conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) + conditions.SetRolloutCondition(&newStatus, *updateCompletedCond) + } else { updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionFalse, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) if conditions.SetRolloutCondition(&newStatus, *updateCompletedCond) { @@ -686,6 +690,7 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt conditions.RolloutNotCompletedMessage, revision+1, newStatus.CurrentPodHash) } } + return newStatus } @@ -938,15 +943,19 @@ func (c *rolloutContext) promoteStable(newStatus *v1alpha1.RolloutStatus, reason } newStatus.StableRS = newStatus.CurrentPodHash - if conditions.RolloutCompleted(c.rollout, newStatus) { - updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, - conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) - if conditions.SetRolloutCondition(newStatus, *updateCompletedCond) { - revision, _ := replicasetutil.Revision(c.rollout) - c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, - conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason) - } - } + revision, _ := replicasetutil.Revision(c.rollout) + c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, + conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason) + + //if conditions.RolloutCompleted(c.rollout, newStatus) { + // updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, + // conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) + // if conditions.SetRolloutCondition(newStatus, *updateCompletedCond) { + // revision, _ := replicasetutil.Revision(c.rollout) + // c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, + // conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason) + // } + //} } return nil } From 53215930d94bfa67707e49a3e9a94b98b62cb66b Mon Sep 17 00:00:00 2001 From: zachaller Date: Tue, 23 Aug 2022 13:56:05 -0500 Subject: [PATCH 21/27] Fix all but one unit test Signed-off-by: zachaller --- rollout/analysis_test.go | 78 +++++++++++++++++++------ rollout/bluegreen_test.go | 103 +++++++++++++++++++-------------- rollout/canary_test.go | 38 +++++++----- rollout/controller_test.go | 52 +++++++++++------ rollout/experiment_test.go | 14 +++-- rollout/service_test.go | 28 +++++---- rollout/trafficrouting_test.go | 2 + 7 files changed, 207 insertions(+), 108 deletions(-) diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index 88ae21bbad..46fe961fec 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -153,6 +153,8 @@ func TestCreateBackgroundAnalysisRun(t *testing.T) { conditions.SetRolloutCondition(&r2.Status, progressingCondition) availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completeCond, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completeCond) f.rolloutLister = append(f.rolloutLister, r2) f.analysisTemplateLister = append(f.analysisTemplateLister, at) @@ -212,6 +214,8 @@ func TestCreateBackgroundAnalysisRunWithTemplates(t *testing.T) { conditions.SetRolloutCondition(&r2.Status, progressingCondition) availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completeCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completeCondition) f.rolloutLister = append(f.rolloutLister, r2) f.analysisTemplateLister = append(f.analysisTemplateLister, at) @@ -272,6 +276,8 @@ func TestCreateBackgroundAnalysisRunWithClusterTemplates(t *testing.T) { conditions.SetRolloutCondition(&r2.Status, progressingCondition) availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) f.rolloutLister = append(f.rolloutLister, r2) f.clusterAnalysisTemplateLister = append(f.clusterAnalysisTemplateLister, cat) @@ -381,6 +387,8 @@ func TestCreateBackgroundAnalysisRunWithClusterTemplatesAndTemplate(t *testing.T conditions.SetRolloutCondition(&r2.Status, progressingCondition) availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) f.rolloutLister = append(f.rolloutLister, r2) f.clusterAnalysisTemplateLister = append(f.clusterAnalysisTemplateLister, cat) @@ -446,6 +454,8 @@ func TestCreateAnalysisRunWithCollision(t *testing.T) { conditions.SetRolloutCondition(&r2.Status, progressingCondition) availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) ar.Status.Phase = v1alpha1.AnalysisPhaseFailed @@ -514,6 +524,8 @@ func TestCreateAnalysisRunWithCollisionAndSemanticEquality(t *testing.T) { conditions.SetRolloutCondition(&r2.Status, progressingCondition) availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) f.rolloutLister = append(f.rolloutLister, r2) f.analysisRunLister = append(f.analysisRunLister, ar) @@ -573,6 +585,8 @@ func TestCreateAnalysisRunOnAnalysisStep(t *testing.T) { conditions.SetRolloutCondition(&r2.Status, progressingCondition) availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) f.rolloutLister = append(f.rolloutLister, r2) f.analysisTemplateLister = append(f.analysisTemplateLister, at) @@ -768,6 +782,8 @@ func TestDoNothingWithAnalysisRunsWhileBackgroundAnalysisRunRunning(t *testing.T conditions.SetRolloutCondition(&r2.Status, progressingCondition) availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) r2.Status.Canary.CurrentBackgroundAnalysisRunStatus = &v1alpha1.RolloutAnalysisRunStatus{ Name: ar.Name, Status: v1alpha1.AnalysisPhaseRunning, @@ -816,6 +832,8 @@ func TestDoNothingWhileStepBasedAnalysisRunRunning(t *testing.T) { conditions.SetRolloutCondition(&r2.Status, progressingCondition) availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) r2.Status.Canary.CurrentStepAnalysisRunStatus = &v1alpha1.RolloutAnalysisRunStatus{ Name: ar.Name, Status: v1alpha1.AnalysisPhaseRunning, @@ -866,6 +884,8 @@ func TestCancelOlderAnalysisRuns(t *testing.T) { conditions.SetRolloutCondition(&r2.Status, progressingCondition) availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) r2.Status.Canary.CurrentStepAnalysisRunStatus = &v1alpha1.RolloutAnalysisRunStatus{ Name: ar.Name, Status: "", @@ -933,6 +953,8 @@ func TestDeleteAnalysisRunsWithNoMatchingRS(t *testing.T) { conditions.SetRolloutCondition(&r2.Status, progressingCondition) availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) r2.Status.Canary.CurrentStepAnalysisRunStatus = &v1alpha1.RolloutAnalysisRunStatus{ Name: ar.Name, } @@ -1059,7 +1081,7 @@ func TestIncrementStepAfterSuccessfulAnalysisRun(t *testing.T) { "conditions": %s } }` - condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false, "") + condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false, "", false) assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition)), patch) } @@ -1128,7 +1150,7 @@ func TestPausedOnInconclusiveBackgroundAnalysisRun(t *testing.T) { "message": "%s" } }` - condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "") + condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, v1alpha1.PauseReasonInconclusiveAnalysis, now, v1alpha1.PauseReasonInconclusiveAnalysis)), patch) } @@ -1192,7 +1214,7 @@ func TestPausedStepAfterInconclusiveAnalysisRun(t *testing.T) { "message": "%s" } }` - condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "") + condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, v1alpha1.PauseReasonInconclusiveAnalysis, now, v1alpha1.PauseReasonInconclusiveAnalysis)), patch) } @@ -1258,7 +1280,7 @@ func TestErrorConditionAfterErrorAnalysisRunStep(t *testing.T) { }` now := timeutil.MetaNow().UTC().Format(time.RFC3339) errmsg := fmt.Sprintf(conditions.RolloutAbortedMessage, 2) + ": " + ar.Status.Message - condition := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, errmsg) + condition := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, errmsg, false) expectedPatch = fmt.Sprintf(expectedPatch, condition, now, errmsg) assert.Equal(t, calculatePatch(r2, expectedPatch), patch) } @@ -1333,7 +1355,7 @@ func TestErrorConditionAfterErrorAnalysisRunBackground(t *testing.T) { } }` errmsg := fmt.Sprintf(conditions.RolloutAbortedMessage, 2) - condition := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, "") + 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) @@ -1386,7 +1408,7 @@ func TestCancelAnalysisRunsWhenAborted(t *testing.T) { assert.True(t, f.verifyPatchedAnalysisRun(cancelOldAr, olderAr)) assert.True(t, f.verifyPatchedAnalysisRun(cancelCurrentAr, ar)) patch := f.getPatchedRollout(patchIndex) - newConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, "") + newConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, "", false) expectedPatch := `{ "status": { "conditions": %s, @@ -1488,6 +1510,9 @@ func TestDoNotCreateBackgroundAnalysisRunAfterInconclusiveRun(t *testing.T) { availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) + f.rolloutLister = append(f.rolloutLister, r2) f.analysisTemplateLister = append(f.analysisTemplateLister, at) f.objects = append(f.objects, r2, at) @@ -1586,13 +1611,16 @@ func TestCreatePrePromotionAnalysisRun(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true, false) progressingCondition, _ := newProgressingCondition(conditions.RolloutPausedReason, r2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) + previewSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} previewSvc := newService("preview", 80, previewSelector, r2) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} @@ -1650,7 +1678,7 @@ func TestDoNotCreatePrePromotionAnalysisAfterPromotionRollout(t *testing.T) { f.analysisTemplateLister = append(f.analysisTemplateLister, at) f.objects = append(f.objects, at) - r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 1, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 1, 1, false, true, true) r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) f.rolloutLister = append(f.rolloutLister, r2) @@ -1661,7 +1689,7 @@ func TestDoNotCreatePrePromotionAnalysisAfterPromotionRollout(t *testing.T) { f.run(getKey(r2, t)) - newConditions := generateConditionsPatchWithHealthy(true, conditions.NewRSAvailableReason, rs2, true, "", true) + newConditions := generateConditionsPatchWithHealthy(true, conditions.NewRSAvailableReason, rs2, true, "", true, true) expectedPatch := fmt.Sprintf(`{ "status":{ "conditions":%s @@ -1723,7 +1751,7 @@ func TestDoNotCreatePrePromotionAnalysisRunOnNotReadyReplicaSet(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 2, 2, 4, 2, false, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 2, 2, 4, 2, false, true, false) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} activeSvc := newService("active", 80, activeSelector, r2) @@ -1766,7 +1794,7 @@ func TestRolloutPrePromotionAnalysisBecomesInconclusive(t *testing.T) { rs2 := newReplicaSetWithStatus(r2, 1, 1) rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true, false) r2.Status.BlueGreen.PrePromotionAnalysisRunStatus = &v1alpha1.RolloutAnalysisRunStatus{ Name: ar.Name, Status: v1alpha1.AnalysisPhaseRunning, @@ -1777,6 +1805,9 @@ func TestRolloutPrePromotionAnalysisBecomesInconclusive(t *testing.T) { pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) + activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} activeSvc := newService("active", 80, activeSelector, r2) @@ -1834,7 +1865,7 @@ func TestRolloutPrePromotionAnalysisSwitchServiceAfterSuccess(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true, false) r2.Status.BlueGreen.PrePromotionAnalysisRunStatus = &v1alpha1.RolloutAnalysisRunStatus{ Name: ar.Name, Status: v1alpha1.AnalysisPhaseRunning, @@ -1903,7 +1934,7 @@ func TestRolloutPrePromotionAnalysisHonorAutoPromotionSeconds(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true, false) now := metav1.NewTime(timeutil.MetaNow().Add(-10 * time.Second)) r2.Status.PauseConditions[0].StartTime = now progressingCondition, _ := newProgressingCondition(conditions.RolloutPausedReason, r2, "") @@ -1966,7 +1997,7 @@ func TestRolloutPrePromotionAnalysisDoNothingOnInconclusiveAnalysis(t *testing.T rs2 := newReplicaSetWithStatus(r2, 1, 1) rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true, false) inconclusivePauseCondition := v1alpha1.PauseCondition{ Reason: v1alpha1.PauseReasonInconclusiveAnalysis, StartTime: timeutil.MetaNow(), @@ -2021,12 +2052,15 @@ func TestAbortRolloutOnErrorPrePromotionAnalysis(t *testing.T) { rs2 := newReplicaSetWithStatus(r2, 1, 1) rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true, false) progressingCondition, _ := newProgressingCondition(conditions.RolloutPausedReason, r2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) + + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) r2.Status.Phase, r2.Status.Message = rolloututil.CalculateRolloutPhase(r2.Spec, r2.Status) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} @@ -2083,7 +2117,7 @@ func TestCreatePostPromotionAnalysisRun(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 1, 1, 2, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 1, 1, 2, 1, false, true, false) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} activeSvc := newService("active", 80, activeSelector, r2) @@ -2136,7 +2170,7 @@ func TestRolloutPostPromotionAnalysisSuccess(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 1, 1, 1, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 1, 1, 1, 1, false, true, false) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} activeSvc := newService("active", 80, activeSelector, r2) @@ -2193,7 +2227,7 @@ func TestPostPromotionAnalysisRunHandleInconclusive(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 1, 1, 2, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 1, 1, 2, 1, false, true, false) r2.Status.PauseConditions = []v1alpha1.PauseCondition{{ Reason: v1alpha1.PauseReasonInconclusiveAnalysis, StartTime: timeutil.MetaNow(), @@ -2204,6 +2238,9 @@ func TestPostPromotionAnalysisRunHandleInconclusive(t *testing.T) { pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) + activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} activeSvc := newService("active", 80, activeSelector, r2) @@ -2254,13 +2291,16 @@ func TestAbortRolloutOnErrorPostPromotionAnalysis(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 1, 1, 2, 1, true, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 1, 1, 2, 1, true, true, false) progressingCondition, _ := newProgressingCondition(conditions.RolloutPausedReason, r2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) + activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} activeSvc := newService("active", 80, activeSelector, r2) diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index f7c3ab9c11..bac0dc468f 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -109,7 +109,7 @@ func TestBlueGreenCreatesReplicaSet(t *testing.T) { rs := newReplicaSet(r, 1) rsPodHash := rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - generatedConditions := generateConditionsPatchWithComplete(false, conditions.ReplicaSetUpdatedReason, rs, false, "", true) + generatedConditions := generateConditionsPatchWithCompleted(false, conditions.ReplicaSetUpdatedReason, rs, false, "", true) f.expectCreateReplicaSetAction(rs) servicePatchIndex := f.expectPatchServiceAction(previewSvc, rsPodHash) @@ -273,7 +273,7 @@ func TestBlueGreenHandlePause(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 2, 2, 4, 2, false, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 2, 2, 4, 2, false, true, false) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} activeSvc := newService("active", 80, activeSelector, r2) @@ -304,7 +304,7 @@ func TestBlueGreenHandlePause(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, false, true, false) previewSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} previewSvc := newService("preview", 80, previewSelector, r2) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} @@ -352,7 +352,7 @@ func TestBlueGreenHandlePause(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true, false) previewSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} previewSvc := newService("preview", 80, previewSelector, r2) @@ -375,7 +375,7 @@ func TestBlueGreenHandlePause(t *testing.T) { "conditions": %s } }` - addedConditions := generateConditionsPatchWithPause(true, conditions.RolloutPausedReason, rs2, true, "", true) + addedConditions := generateConditionsPatchWithPause(true, conditions.RolloutPausedReason, rs2, true, "", true, false) assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, addedConditions)), patch) }) @@ -392,12 +392,15 @@ func TestBlueGreenHandlePause(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true, false) progressingCondition, _ := newProgressingCondition(conditions.RolloutPausedReason, r2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) + + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) r2.Status.Phase, r2.Status.Message = rolloututil.CalculateRolloutPhase(r2.Spec, r2.Status) previewSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} @@ -435,7 +438,7 @@ func TestBlueGreenHandlePause(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, false, true, false) now := timeutil.MetaNow() r2.Status.PauseConditions = append(r2.Status.PauseConditions, v1alpha1.PauseCondition{ Reason: v1alpha1.PauseReasonInconclusiveAnalysis, @@ -481,12 +484,15 @@ func TestBlueGreenHandlePause(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true, false) progressingCondition, _ := newProgressingCondition(conditions.RolloutPausedReason, r2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) + + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) r2.Status.Phase, r2.Status.Message = rolloututil.CalculateRolloutPhase(r2.Spec, r2.Status) previewSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} @@ -519,7 +525,7 @@ func TestBlueGreenHandlePause(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true, false) now := timeutil.MetaNow() before := metav1.NewTime(now.Add(-1 * time.Minute)) r2.Status.PauseConditions[0].StartTime = before @@ -577,7 +583,7 @@ func TestBlueGreenHandlePause(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true, false) now := timeutil.MetaNow() before := metav1.NewTime(now.Add(-1 * time.Minute)) r2.Status.PauseConditions[0].StartTime = before @@ -588,6 +594,9 @@ func TestBlueGreenHandlePause(t *testing.T) { pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) + + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) r2.Status.Phase, r2.Status.Message = rolloututil.CalculateRolloutPhase(r2.Spec, r2.Status) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} @@ -621,7 +630,7 @@ func TestBlueGreenHandlePause(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, false, true, false) r2.Spec.Strategy.BlueGreen.ScaleDownDelaySeconds = pointer.Int32Ptr(10) progressingCondition, _ := newProgressingCondition(conditions.NewReplicaSetReason, rs2, "") @@ -637,7 +646,7 @@ func TestBlueGreenHandlePause(t *testing.T) { servicePatchIndex := f.expectPatchServiceAction(activeSvc, rs2PodHash) - generatedConditions := generateConditionsPatchWithComplete(true, conditions.ReplicaSetUpdatedReason, rs2, true, "", true) + generatedConditions := generateConditionsPatchWithCompleted(true, conditions.ReplicaSetUpdatedReason, rs2, true, "", true) newSelector := metav1.FormatLabelSelector(rs2.Spec.Selector) expectedPatchWithoutSubs := `{ "status": { @@ -672,10 +681,13 @@ func TestBlueGreenHandlePause(t *testing.T) { rs2 := newReplicaSetWithStatus(r2, 1, 1) rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, false, true, false) progressingCondition, _ := newProgressingCondition(conditions.NewReplicaSetReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} activeSvc := newService("active", 80, activeSelector, r2) @@ -715,7 +727,7 @@ func TestBlueGreenHandlePause(t *testing.T) { rs1 := newReplicaSetWithStatus(r1, 1, 1) rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r1 = updateBlueGreenRolloutStatus(r1, "", "", "", 1, 1, 1, 1, false, false) + r1 = updateBlueGreenRolloutStatus(r1, "", "", "", 1, 1, 1, 1, false, false, false) activeSelector := map[string]string{"foo": "bar"} @@ -741,7 +753,7 @@ func TestBlueGreenHandlePause(t *testing.T) { } }` - generateConditions := generateConditionsPatchWithComplete(true, conditions.ReplicaSetUpdatedReason, rs1, false, "", true) + generateConditions := generateConditionsPatchWithCompleted(true, conditions.ReplicaSetUpdatedReason, rs1, false, "", true) newSelector := metav1.FormatLabelSelector(rs1.Spec.Selector) expectedPatch := calculatePatch(r1, fmt.Sprintf(expectedPatchWithoutSubs, rs1PodHash, rs1PodHash, generateConditions, newSelector)) patchRolloutIndex := f.expectPatchRolloutActionWithPatch(r1, expectedPatch) @@ -767,10 +779,12 @@ func TestBlueGreenHandlePause(t *testing.T) { rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r2.Spec.Strategy.BlueGreen.ScaleDownDelaySeconds = pointer.Int32Ptr(10) - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, false, true, false) r2.Status.ControllerPause = true pausedCondition, _ := newProgressingCondition(conditions.RolloutPausedReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, pausedCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} activeSvc := newService("active", 80, activeSelector, r2) @@ -790,7 +804,7 @@ func TestBlueGreenHandlePause(t *testing.T) { f.verifyPatchedService(servicePatchIndex, rs2PodHash, "") unpausePatch := f.getPatchedRollout(unpausePatchIndex) - unpauseConditions := generateConditionsPatch(true, conditions.RolloutResumedReason, rs2, true, "") + unpauseConditions := generateConditionsPatch(true, conditions.RolloutResumedReason, rs2, true, "", false) expectedUnpausePatch := `{ "status": { "conditions": %s @@ -798,7 +812,7 @@ func TestBlueGreenHandlePause(t *testing.T) { }` assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedUnpausePatch, unpauseConditions)), unpausePatch) - generatedConditions := generateConditionsPatchWithComplete(true, conditions.ReplicaSetUpdatedReason, rs2, true, "", true) + generatedConditions := generateConditionsPatchWithCompleted(true, conditions.ReplicaSetUpdatedReason, rs2, true, "", true) expected2ndPatchWithoutSubs := `{ "status": { "blueGreen": { @@ -837,7 +851,7 @@ func TestBlueGreenAddScaleDownDelayToPreviousActiveReplicaSet(t *testing.T) { f.replicaSetLister = append(f.replicaSetLister, rs1, rs2) r2.Spec.Strategy.BlueGreen.ScaleDownDelaySeconds = pointer.Int32Ptr(10) - r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, false, true, false) f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) f.serviceLister = append(f.serviceLister, s) @@ -860,7 +874,7 @@ func TestBlueGreenAddScaleDownDelayToPreviousActiveReplicaSet(t *testing.T) { } }` newSelector := metav1.FormatLabelSelector(rs2.Spec.Selector) - expectedCondition := generateConditionsPatchWithComplete(true, conditions.ReplicaSetUpdatedReason, rs2, true, "", true) + expectedCondition := generateConditionsPatchWithCompleted(true, conditions.ReplicaSetUpdatedReason, rs2, true, "", true) expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, rs2PodHash, rs2PodHash, expectedCondition, newSelector)) assert.Equal(t, expectedPatch, patch) } @@ -881,7 +895,7 @@ func TestBlueGreenRolloutStatusHPAStatusFieldsActiveSelectorSet(t *testing.T) { previewSvc := newService("preview", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash}, r2) activeSvc := newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash}, r2) - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 0, 0, 0, 0, true, false) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 0, 0, 0, 0, true, false, false) r2.Status.Selector = "" progressingCondition, _ := newProgressingCondition(conditions.RolloutPausedReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) @@ -942,6 +956,7 @@ func TestBlueGreenRolloutStatusHPAStatusFieldsNoActiveSelector(t *testing.T) { assert.Len(t, f.client.Actions(), 1) result := f.client.Actions()[0].(core.PatchAction).GetPatch() _, availableStr := newAvailableCondition(false) + _, compCond := newCompletedCondition(true) expectedPatchWithoutSub := `{ "status":{ "HPAReplicas":1, @@ -949,11 +964,11 @@ func TestBlueGreenRolloutStatusHPAStatusFieldsNoActiveSelector(t *testing.T) { "availableReplicas": 1, "updatedReplicas":1, "replicas":1, - "conditions":[%s, %s], + "conditions":[%s, %s, %s], "selector":"foo=bar" } }` - expectedPatch := calculatePatch(ro, fmt.Sprintf(expectedPatchWithoutSub, progressingConditionStr, availableStr)) + expectedPatch := calculatePatch(ro, fmt.Sprintf(expectedPatchWithoutSub, progressingConditionStr, availableStr, compCond)) assert.Equal(t, expectedPatch, string(result)) } @@ -975,7 +990,7 @@ func TestBlueGreenRolloutScaleUpdateActiveRS(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 1, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 1, 1, false, true, false) f.objects = append(f.objects, r2) previewSvc := newService("preview", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash}, r2) activeSvc := newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash}, r2) @@ -1008,7 +1023,7 @@ func TestPreviewReplicaCountHandleScaleUpPreviewCheckPoint(t *testing.T) { activeSvc := newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash}, r2) - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 3, 3, 8, 5, false, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 3, 3, 8, 5, false, true, false) f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) f.kubeobjects = append(f.kubeobjects, activeSvc) @@ -1040,7 +1055,7 @@ func TestPreviewReplicaCountHandleScaleUpPreviewCheckPoint(t *testing.T) { activeSvc := newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash}, r2) - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 5, 5, 8, 5, false, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 5, 5, 8, 5, false, true, false) r2.Status.BlueGreen.ScaleUpPreviewCheckPoint = true f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) @@ -1071,7 +1086,7 @@ func TestPreviewReplicaCountHandleScaleUpPreviewCheckPoint(t *testing.T) { activeSvc := newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash}, r2) - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 5, 5, 8, 5, false, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 5, 5, 8, 5, false, true, false) r2.Status.BlueGreen.ScaleUpPreviewCheckPoint = true f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) @@ -1105,7 +1120,7 @@ func TestBlueGreenRolloutIgnoringScalingUsePreviewRSCount(t *testing.T) { previewSvc := newService("preview", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash}, r2) activeSvc := newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash}, r2) - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 2, 1, 1, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 2, 1, 1, 1, false, true, true) // Scaling up the rollout r2.Spec.Replicas = pointer.Int32Ptr(2) f.rolloutLister = append(f.rolloutLister, r2) @@ -1138,7 +1153,7 @@ func TestBlueGreenRolloutCompleted(t *testing.T) { s := newService("bar", 80, serviceSelector, r2) f.kubeobjects = append(f.kubeobjects, s) - r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 1, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 1, 1, false, true, true) r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) f.rolloutLister = append(f.rolloutLister, r2) @@ -1149,7 +1164,7 @@ func TestBlueGreenRolloutCompleted(t *testing.T) { f.run(getKey(r2, t)) - newConditions := generateConditionsPatchWithHealthy(true, conditions.NewRSAvailableReason, rs2, true, "", true) + newConditions := generateConditionsPatchWithHealthy(true, conditions.NewRSAvailableReason, rs2, true, "", true, true) expectedPatch := fmt.Sprintf(`{ "status":{ "conditions":%s @@ -1184,7 +1199,7 @@ func TestBlueGreenRolloutCompletedFalse(t *testing.T) { s := newService("bar", 80, serviceSelector, r2) f.kubeobjects = append(f.kubeobjects, s) - r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 1, 1, true, false) + r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 1, 1, true, false, false) r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) f.rolloutLister = append(f.rolloutLister, r2) @@ -1199,7 +1214,7 @@ func TestBlueGreenRolloutCompletedFalse(t *testing.T) { err := json.Unmarshal([]byte(patch), &rolloutPatch) assert.NoError(t, err) - index := len(rolloutPatch.Status.Conditions) - 2 + index := len(rolloutPatch.Status.Conditions) - 3 assert.Equal(t, v1alpha1.HealthyAndCompleted, rolloutPatch.Status.Conditions[index].Type) assert.Equal(t, corev1.ConditionFalse, rolloutPatch.Status.Conditions[index].Status) } @@ -1222,7 +1237,7 @@ func TestBlueGreenUnableToReadScaleDownAt(t *testing.T) { f.kubeobjects = append(f.kubeobjects, s, rs1, rs2) f.replicaSetLister = append(f.replicaSetLister, rs1, rs2) - r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 2, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 2, 1, false, true, true) f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) f.serviceLister = append(f.serviceLister, s) @@ -1259,7 +1274,7 @@ func TestBlueGreenNotReadyToScaleDownOldReplica(t *testing.T) { f.kubeobjects = append(f.kubeobjects, s, rs1, rs2) f.replicaSetLister = append(f.replicaSetLister, rs1, rs2) - r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 2, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 2, 1, false, true, true) f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) f.serviceLister = append(f.serviceLister, s) @@ -1292,7 +1307,7 @@ func TestBlueGreenReadyToScaleDownOldReplica(t *testing.T) { f.kubeobjects = append(f.kubeobjects, s, rs1, rs2) f.replicaSetLister = append(f.replicaSetLister, rs1, rs2) - r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 2, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 2, 1, false, true, true) f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) f.serviceLister = append(f.serviceLister, s) @@ -1337,7 +1352,7 @@ func TestFastRollback(t *testing.T) { r2.Status.CurrentPodHash = rs1PodHash rs1.Annotations[annotations.RevisionAnnotation] = "3" - r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, false, true, true) f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) f.serviceLister = append(f.serviceLister, s) @@ -1375,7 +1390,7 @@ func TestBlueGreenScaleDownLimit(t *testing.T) { f.kubeobjects = append(f.kubeobjects, s, rs1, rs2, rs3) f.replicaSetLister = append(f.replicaSetLister, rs1, rs2, rs3) - r3 = updateBlueGreenRolloutStatus(r3, "", rs3PodHash, rs3PodHash, 1, 1, 3, 1, false, true) + r3 = updateBlueGreenRolloutStatus(r3, "", rs3PodHash, rs3PodHash, 1, 1, 3, 1, false, true, true) f.rolloutLister = append(f.rolloutLister, r3) f.objects = append(f.objects, r3) f.serviceLister = append(f.serviceLister, s) @@ -1414,7 +1429,7 @@ func TestBlueGreenAbort(t *testing.T) { f.kubeobjects = append(f.kubeobjects, s, rs1, rs2) f.replicaSetLister = append(f.replicaSetLister, rs1, rs2) - r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 1, 1, 2, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 1, 1, 2, 1, false, true, true) f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) f.serviceLister = append(f.serviceLister, s) @@ -1422,7 +1437,7 @@ func TestBlueGreenAbort(t *testing.T) { f.expectPatchServiceAction(s, rs1PodHash) patchIndex := f.expectPatchRolloutAction(r2) f.run(getKey(r2, t)) - expectedConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, true, "") + expectedConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, true, "", false) expectedPatch := fmt.Sprintf(`{ "status": { "blueGreen": { @@ -1451,7 +1466,7 @@ func TestBlueGreenHandlePauseAutoPromoteWithConditions(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) + r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true, true) now := timeutil.MetaNow() before := metav1.NewTime(now.Add(-1 * time.Minute)) r2.Status.PauseConditions[0].StartTime = before @@ -1465,6 +1480,8 @@ func TestBlueGreenHandlePauseAutoPromoteWithConditions(t *testing.T) { availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + //completedCondition, _ := newCompletedCondition(true) + //conditions.SetRolloutCondition(&r2.Status, completedCondition) r2.Status.Phase, r2.Status.Message = rolloututil.CalculateRolloutPhase(r2.Spec, r2.Status) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} @@ -1497,12 +1514,12 @@ func TestBlueGreenHandlePauseAutoPromoteWithConditions(t *testing.T) { updatedProgressingCond, _ := newProgressingCondition(conditions.ReplicaSetUpdatedReason, rs2, fmt.Sprintf("ReplicaSet \"%s\" is progressing.", rs2.Name)) progressingCondBytes, err := json.Marshal(updatedProgressingCond) assert.Nil(t, err) - pausedCondBytes, err := json.Marshal(r2.Status.Conditions[2]) + pausedCondBytes, err := json.Marshal(r2.Status.Conditions[3]) assert.Nil(t, err) completeCond, _ := newCompletedCondition(true) completeCondBytes, err := json.Marshal(completeCond) assert.Nil(t, err) - expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, rs2PodHash, string(availableCondBytes), string(pausedCondBytes), string(completeCondBytes), string(progressingCondBytes), rs2PodHash, rs2PodHash)) + expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, rs2PodHash, string(availableCondBytes), string(completeCondBytes), string(pausedCondBytes), string(progressingCondBytes), rs2PodHash, rs2PodHash)) f.expectPatchServiceAction(activeSvc, rs2PodHash) patchRolloutIndex := f.expectPatchRolloutActionWithPatch(r2, expectedPatch) f.run(getKey(r2, t)) @@ -1526,7 +1543,7 @@ func TestBlueGreenAddScaleDownDelay(t *testing.T) { rs2 := newReplicaSetWithStatus(r2, 1, 1) rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) - r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 2, 1, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 2, 1, false, true, true) completedCondition, _ := newHealthyCondition(true) conditions.SetRolloutCondition(&r2.Status, completedCondition) progressingCondition, _ := newProgressingCondition(conditions.NewRSAvailableReason, rs2, "") diff --git a/rollout/canary_test.go b/rollout/canary_test.go index de42068963..56445f3978 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -178,7 +178,7 @@ func TestCanaryRolloutEnterPauseState(t *testing.T) { } }` - conditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "") + conditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) now := timeutil.MetaNow().UTC().Format(time.RFC3339) expectedPatchWithoutObservedGen := fmt.Sprintf(expectedPatchTemplate, v1alpha1.PauseReasonCanaryPauseStep, now, conditions, v1alpha1.PauseReasonCanaryPauseStep) expectedPatch := calculatePatch(r2, expectedPatchWithoutObservedGen) @@ -239,6 +239,9 @@ func TestCanaryRolloutUpdatePauseConditionWhilePaused(t *testing.T) { availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) + rs1 := newReplicaSetWithStatus(r1, 10, 10) rs2 := newReplicaSetWithStatus(r2, 0, 0) @@ -337,7 +340,7 @@ func TestCanaryRolloutIncrementStepAfterUnPaused(t *testing.T) { "currentStepIndex": 1 } }` - generatedConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false, "") + generatedConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false, "", false) expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchTemplate, generatedConditions)) assert.Equal(t, expectedPatch, patch) } @@ -379,7 +382,7 @@ func TestCanaryRolloutUpdateStatusWhenAtEndOfSteps(t *testing.T) { } }` - expectedPatch := fmt.Sprintf(expectedPatchWithoutStableRS, expectedStableRS, generateConditionsPatchWithComplete(true, conditions.ReplicaSetUpdatedReason, rs2, false, "", true)) + expectedPatch := fmt.Sprintf(expectedPatchWithoutStableRS, expectedStableRS, generateConditionsPatchWithCompleted(true, conditions.ReplicaSetUpdatedReason, rs2, false, "", true)) assert.Equal(t, calculatePatch(r2, expectedPatch), patch) } @@ -421,7 +424,7 @@ func TestResetCurrentStepIndexOnStepChange(t *testing.T) { "conditions": %s } }` - newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "") + newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) expectedPatch := fmt.Sprintf(expectedPatchWithoutPodHash, expectedCurrentPodHash, expectedCurrentStepHash, newConditions) assert.Equal(t, calculatePatch(r2, expectedPatch), patch) @@ -462,7 +465,7 @@ func TestResetCurrentStepIndexOnPodSpecChange(t *testing.T) { "conditions": %s } }` - newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "") + newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) expectedPatch := fmt.Sprintf(expectedPatchWithoutPodHash, expectedCurrentPodHash, newConditions) assert.Equal(t, calculatePatch(r2, expectedPatch), patch) @@ -502,7 +505,7 @@ func TestCanaryRolloutCreateFirstReplicasetNoSteps(t *testing.T) { } }` - newConditions := generateConditionsPatchWithComplete(false, conditions.ReplicaSetUpdatedReason, rs, false, "", true) + newConditions := generateConditionsPatchWithCompleted(false, conditions.ReplicaSetUpdatedReason, rs, false, "", true) assert.Equal(t, calculatePatch(r, fmt.Sprintf(expectedPatch, newConditions)), patch) } @@ -542,7 +545,7 @@ func TestCanaryRolloutCreateFirstReplicasetWithSteps(t *testing.T) { "conditions": %s } }` - expectedPatch := fmt.Sprintf(expectedPatchWithSub, generateConditionsPatchWithComplete(false, conditions.ReplicaSetUpdatedReason, rs, false, "", true)) + expectedPatch := fmt.Sprintf(expectedPatchWithSub, generateConditionsPatchWithCompleted(false, conditions.ReplicaSetUpdatedReason, rs, false, "", true)) assert.Equal(t, calculatePatch(r, expectedPatch), patch) } @@ -840,7 +843,7 @@ func TestRollBackToStable(t *testing.T) { "conditions": %s } }` - newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs1, false, "") + 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) @@ -883,7 +886,7 @@ func TestGradualShiftToNewStable(t *testing.T) { "conditions": %s } }` - newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "") + newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, newConditions) patch := f.getPatchedRollout(patchIndex) assert.Equal(t, calculatePatch(r2, expectedPatch), patch) @@ -931,7 +934,7 @@ func TestRollBackToStableAndStepChange(t *testing.T) { }` newPodHash := hash.ComputePodTemplateHash(&r2.Spec.Template, r2.Status.CollisionCount) newStepHash := conditions.ComputeStepHash(r2) - newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs1, false, "") + 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) @@ -969,7 +972,7 @@ func TestCanaryRolloutIncrementStepIfSetWeightsAreCorrect(t *testing.T) { "conditions": %s } }` - newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs3, false, "") + newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs3, false, "", false) assert.Equal(t, calculatePatch(r3, fmt.Sprintf(expectedPatch, newConditions)), patch) } @@ -1005,6 +1008,9 @@ func TestSyncRolloutWaitAddToQueue(t *testing.T) { availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) + r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) @@ -1053,6 +1059,9 @@ func TestSyncRolloutIgnoreWaitOutsideOfReconciliationPeriod(t *testing.T) { availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) + f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) @@ -1617,6 +1626,9 @@ func TestHandleNilNewRSOnScaleAndImageChange(t *testing.T) { availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) + f.kubeobjects = append(f.kubeobjects, rs1) f.replicaSetLister = append(f.replicaSetLister, rs1) f.rolloutLister = append(f.rolloutLister, r2) @@ -1672,7 +1684,7 @@ func TestHandleCanaryAbort(t *testing.T) { } }` errmsg := fmt.Sprintf(conditions.RolloutAbortedMessage, 2) - newConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, "") + newConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, "", false) assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, newConditions, conditions.RolloutAbortedReason, errmsg)), patch) }) @@ -1710,7 +1722,7 @@ func TestHandleCanaryAbort(t *testing.T) { } }` errmsg := fmt.Sprintf(conditions.RolloutAbortedMessage, 1) - newConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r1, false, "") + newConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r1, false, "", true) assert.Equal(t, calculatePatch(r1, fmt.Sprintf(expectedPatch, newConditions, conditions.RolloutAbortedReason, errmsg)), patch) }) } diff --git a/rollout/controller_test.go b/rollout/controller_test.go index b87c25958f..1dd5fd25e3 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -337,43 +337,57 @@ func newAvailableCondition(available bool) (v1alpha1.RolloutCondition, string) { return condition, string(conditionBytes) } -func generateConditionsPatch(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string) string { +func generateConditionsPatch(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isCompleted bool) string { _, availableCondition := newAvailableCondition(available) _, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage) + _, completedCondition := newCompletedCondition(isCompleted) if availableConditionFirst { - return fmt.Sprintf("[%s, %s]", availableCondition, progressingCondition) + return fmt.Sprintf("[%s, %s, %s]", availableCondition, progressingCondition, completedCondition) } - return fmt.Sprintf("[%s, %s]", progressingCondition, availableCondition) + return fmt.Sprintf("[%s, %s, %s]", progressingCondition, availableCondition, completedCondition) } -func generateConditionsPatchWithPause(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isPaused bool) string { +func generateConditionsPatchWithPause(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isPaused bool, isCompleted bool) string { _, availableCondition := newAvailableCondition(available) _, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage) _, pauseCondition := newPausedCondition(isPaused) + _, completedCondition := newCompletedCondition(isCompleted) + if availableConditionFirst { + return fmt.Sprintf("[%s, %s, %s, %s]", availableCondition, completedCondition, progressingCondition, pauseCondition) + } + return fmt.Sprintf("[%s, %s, %s, %s]", progressingCondition, pauseCondition, availableCondition, completedCondition) +} + +func generateConditionsPatchWithHealthy(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isHealthy bool, isCompleted bool) string { + _, availableCondition := newAvailableCondition(available) + _, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage) + _, healthyCondition := newHealthyCondition(isHealthy) + _, completedCondition := newCompletedCondition(isCompleted) if availableConditionFirst { - return fmt.Sprintf("[%s, %s, %s]", availableCondition, progressingCondition, pauseCondition) + return fmt.Sprintf("[%s, %s, %s, %s]", availableCondition, completedCondition, healthyCondition, progressingCondition) } - return fmt.Sprintf("[%s, %s, %s]", progressingCondition, pauseCondition, availableCondition) + return fmt.Sprintf("[%s, %s, %s, %s]", completedCondition, healthyCondition, progressingCondition, availableCondition) } -func generateConditionsPatchWithHealthy(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isHealthy bool) string { +func generateConditionsPatchWithHealthyAndCompleted(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isHealthy bool) string { _, availableCondition := newAvailableCondition(available) _, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage) _, healthyCondition := newHealthyCondition(isHealthy) + _, completeCondition := newCompletedCondition(true) if availableConditionFirst { - return fmt.Sprintf("[%s, %s, %s]", availableCondition, healthyCondition, progressingCondition) + return fmt.Sprintf("[%s, %s, %s, %s]", availableCondition, completeCondition, healthyCondition, progressingCondition) } - return fmt.Sprintf("[%s, %s, %s]", healthyCondition, progressingCondition, availableCondition) + return fmt.Sprintf("[%s, %s, %s, %s]", progressingCondition, availableCondition, healthyCondition, completeCondition) } -func generateConditionsPatchWithComplete(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isCompleted bool) string { +func generateConditionsPatchWithCompleted(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isCompleted bool) string { _, availableCondition := newAvailableCondition(available) _, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage) _, completeCondition := newCompletedCondition(isCompleted) if availableConditionFirst { - return fmt.Sprintf("[%s, %s, %s]", availableCondition, completeCondition, progressingCondition) + return fmt.Sprintf("[%s, %s, %s]", availableCondition, progressingCondition, completeCondition) } - return fmt.Sprintf("[%s, %s, %s]", completeCondition, progressingCondition, availableCondition) + return fmt.Sprintf("[%s, %s, %s]", progressingCondition, availableCondition, completeCondition) } func generateConditionsPatchWithCompletedHealthy(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isHealthy bool, isCompleted bool) string { @@ -394,7 +408,7 @@ func updateConditionsPatch(r v1alpha1.Rollout, newCondition v1alpha1.RolloutCond } // func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active string, availableReplicas, updatedReplicas, hpaReplicas int32, pause bool, available bool, progressingStatus string) *v1alpha1.Rollout { -func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active, stable string, availableReplicas, updatedReplicas, totalReplicas, hpaReplicas int32, pause bool, available bool) *v1alpha1.Rollout { +func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active, stable string, availableReplicas, updatedReplicas, totalReplicas, hpaReplicas int32, pause bool, available bool, isCompleted bool) *v1alpha1.Rollout { newRollout := updateBaseRolloutStatus(r, availableReplicas, updatedReplicas, totalReplicas, hpaReplicas) selector := newRollout.Spec.Selector.DeepCopy() if active != "" { @@ -406,8 +420,8 @@ func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active, stable s newRollout.Status.StableRS = stable cond, _ := newAvailableCondition(available) newRollout.Status.Conditions = append(newRollout.Status.Conditions, cond) - //completeCond, _ := newCompletedCondition(isCompleted) - //newRollout.Status.Conditions = append(newRollout.Status.Conditions, completeCond) + completeCond, _ := newCompletedCondition(isCompleted) + newRollout.Status.Conditions = append(newRollout.Status.Conditions, completeCond) if pause { now := timeutil.MetaNow() cond := v1alpha1.PauseCondition{ @@ -1435,6 +1449,8 @@ func TestComputeHashChangeTolerationBlueGreen(t *testing.T) { conditions.SetRolloutCondition(&r.Status, availableCondition) progressingCondition, _ := newProgressingCondition(conditions.ReplicaSetUpdatedReason, rs, "") conditions.SetRolloutCondition(&r.Status, progressingCondition) + completedCondition, _ := newCompletedCondition(true) + conditions.SetRolloutCondition(&r.Status, completedCondition) r.Status.Phase, r.Status.Message = rolloututil.CalculateRolloutPhase(r.Spec, r.Status) podTemplate := corev1.PodTemplate{ @@ -1479,6 +1495,8 @@ func TestComputeHashChangeTolerationCanary(t *testing.T) { conditions.SetRolloutCondition(&r.Status, availableCondition) progressingCondition, _ := newProgressingCondition(conditions.ReplicaSetUpdatedReason, rs, "") conditions.SetRolloutCondition(&r.Status, progressingCondition) + completedCondition, _ := newCompletedCondition(true) + conditions.SetRolloutCondition(&r.Status, completedCondition) podTemplate := corev1.PodTemplate{ Template: rs.Spec.Template, @@ -1506,7 +1524,7 @@ func TestSwitchBlueGreenToCanary(t *testing.T) { activeSvc := newService("active", 80, nil, r) rs := newReplicaSetWithStatus(r, 1, 1) rsPodHash := rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r = updateBlueGreenRolloutStatus(r, "", rsPodHash, rsPodHash, 1, 1, 1, 1, false, true) + r = updateBlueGreenRolloutStatus(r, "", rsPodHash, rsPodHash, 1, 1, 1, 1, false, true, false) // StableRS is set to avoid running the migration code. When .status.canary.stableRS is removed, the line below can be deleted //r.Status.Canary.StableRS = rsPodHash r.Spec.Strategy.BlueGreen = nil @@ -1524,7 +1542,7 @@ func TestSwitchBlueGreenToCanary(t *testing.T) { f.run(getKey(r, t)) patch := f.getPatchedRollout(i) - addedConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs, true, "") + addedConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs, true, "", true) expectedPatch := fmt.Sprintf(`{ "status": { "blueGreen": { diff --git a/rollout/experiment_test.go b/rollout/experiment_test.go index c8b610a303..e2e50c1f28 100644 --- a/rollout/experiment_test.go +++ b/rollout/experiment_test.go @@ -68,7 +68,7 @@ func TestRolloutCreateExperiment(t *testing.T) { "conditions": %s } }` - conds := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "") + conds := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, ex.Name, conds)), patch) } @@ -125,7 +125,7 @@ func TestRolloutCreateClusterTemplateExperiment(t *testing.T) { "conditions": %s } }` - conds := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "") + conds := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, ex.Name, conds)), patch) } @@ -177,7 +177,7 @@ func TestCreateExperimentWithCollision(t *testing.T) { "conditions": %s } }` - conds := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "") + conds := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, createdEx.Name, conds)), patch) } @@ -228,7 +228,7 @@ func TestCreateExperimentWithCollisionAndSemanticEquality(t *testing.T) { "conditions": %s } }` - conds := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "") + conds := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false, "", false) assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, ex.Name, conds)), patch) } @@ -256,6 +256,8 @@ func TestRolloutExperimentProcessingDoNothing(t *testing.T) { conditions.SetRolloutCondition(&r2.Status, progressingCondition) availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) f.rolloutLister = append(f.rolloutLister, r2) f.experimentLister = append(f.experimentLister, ex) @@ -311,7 +313,7 @@ func TestAbortRolloutAfterFailedExperiment(t *testing.T) { } }` now := timeutil.Now().UTC().Format(time.RFC3339) - generatedConditions := generateConditionsPatch(true, conditions.RolloutAbortedReason, r2, false, "") + 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) } @@ -477,7 +479,7 @@ func TestRolloutExperimentFinishedIncrementStep(t *testing.T) { "conditions": %s } }` - generatedConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false, "") + generatedConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false, "", false) assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, generatedConditions)), patch) } diff --git a/rollout/service_test.go b/rollout/service_test.go index 2ebe578dc3..8a402d001c 100644 --- a/rollout/service_test.go +++ b/rollout/service_test.go @@ -302,13 +302,15 @@ func TestBlueGreenAWSVerifyTargetGroupsNotYetReady(t *testing.T) { rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] svc := newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash}, r2) - r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 3, 3, 6, 3, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 3, 3, 6, 3, false, true, false) r2.Status.Message = "" r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) - completedCondition, _ := newHealthyCondition(true) - conditions.SetRolloutCondition(&r2.Status, completedCondition) + completedHealthyCondition, _ := newHealthyCondition(true) + conditions.SetRolloutCondition(&r2.Status, completedHealthyCondition) progressingCondition, _ := newProgressingCondition(conditions.NewRSAvailableReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) + completedCondition, _ := newCompletedCondition(false) + conditions.SetRolloutCondition(&r2.Status, completedCondition) f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2, tgb) @@ -385,7 +387,7 @@ func TestBlueGreenAWSVerifyTargetGroupsReady(t *testing.T) { rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] svc := newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash}, r2) - r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 3, 3, 6, 3, false, true) + r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 3, 3, 6, 3, false, true, false) r2.Status.Message = "waiting for post-promotion verification to complete" r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) completedCondition, _ := newHealthyCondition(true) @@ -491,10 +493,12 @@ func TestCanaryAWSVerifyTargetGroupsNotYetReady(t *testing.T) { r2.Status.StableRS = rs2PodHash availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) - completedCondition, _ := newHealthyCondition(false) - conditions.SetRolloutCondition(&r2.Status, completedCondition) + healthyCondition, _ := newHealthyCondition(false) + conditions.SetRolloutCondition(&r2.Status, healthyCondition) progressingCondition, _ := newProgressingCondition(conditions.NewRSAvailableReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) + completedCondition, _ := newCompletedCondition(true) + conditions.SetRolloutCondition(&r2.Status, completedCondition) _, r2.Status.Canary.Weights = calculateWeightStatus(r2, rs2PodHash, rs2PodHash, 0) f.rolloutLister = append(f.rolloutLister, r2) @@ -587,10 +591,12 @@ func TestCanaryAWSVerifyTargetGroupsReady(t *testing.T) { r2.Status.StableRS = rs2PodHash availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) - completedCondition, _ := newHealthyCondition(false) - conditions.SetRolloutCondition(&r2.Status, completedCondition) + healthyCondition, _ := newHealthyCondition(false) + conditions.SetRolloutCondition(&r2.Status, healthyCondition) progressingCondition, _ := newProgressingCondition(conditions.NewRSAvailableReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) + completedCondition, _ := newCompletedCondition(true) + conditions.SetRolloutCondition(&r2.Status, completedCondition) _, r2.Status.Canary.Weights = calculateWeightStatus(r2, rs2PodHash, rs2PodHash, 0) f.rolloutLister = append(f.rolloutLister, r2) @@ -648,10 +654,12 @@ func TestCanaryAWSVerifyTargetGroupsSkip(t *testing.T) { r2.Status.StableRS = rs2PodHash availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) - completedCondition, _ := newHealthyCondition(false) - conditions.SetRolloutCondition(&r2.Status, completedCondition) + healthyCondition, _ := newHealthyCondition(false) + conditions.SetRolloutCondition(&r2.Status, healthyCondition) progressingCondition, _ := newProgressingCondition(conditions.NewRSAvailableReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) + completedCondition, _ := newCompletedCondition(true) + conditions.SetRolloutCondition(&r2.Status, completedCondition) _, r2.Status.Canary.Weights = calculateWeightStatus(r2, rs2PodHash, rs2PodHash, 0) f.rolloutLister = append(f.rolloutLister, r2) diff --git a/rollout/trafficrouting_test.go b/rollout/trafficrouting_test.go index abb0cc09f6..6c1e49f476 100644 --- a/rollout/trafficrouting_test.go +++ b/rollout/trafficrouting_test.go @@ -739,6 +739,8 @@ func TestCanaryWithTrafficRoutingAddScaleDownDelay(t *testing.T) { r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) + completedCondition, _ := newCompletedCondition(true) + conditions.SetRolloutCondition(&r2.Status, completedCondition) _, r2.Status.Canary.Weights = calculateWeightStatus(r2, rs2PodHash, rs2PodHash, 0) selector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} From 26e39338b1bd735e06847bde9016d348bb498346 Mon Sep 17 00:00:00 2001 From: zachaller Date: Tue, 23 Aug 2022 15:07:24 -0500 Subject: [PATCH 22/27] fix last unit test Signed-off-by: zachaller --- rollout/bluegreen_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index bac0dc468f..d12c11c966 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -781,10 +781,10 @@ func TestBlueGreenHandlePause(t *testing.T) { r2.Spec.Strategy.BlueGreen.ScaleDownDelaySeconds = pointer.Int32Ptr(10) r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, false, true, false) r2.Status.ControllerPause = true - pausedCondition, _ := newProgressingCondition(conditions.RolloutPausedReason, rs2, "") - conditions.SetRolloutCondition(&r2.Status, pausedCondition) completedCondition, _ := newCompletedCondition(false) conditions.SetRolloutCondition(&r2.Status, completedCondition) + pausedCondition, _ := newProgressingCondition(conditions.RolloutPausedReason, rs2, "") + conditions.SetRolloutCondition(&r2.Status, pausedCondition) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} activeSvc := newService("active", 80, activeSelector, r2) @@ -804,7 +804,10 @@ func TestBlueGreenHandlePause(t *testing.T) { f.verifyPatchedService(servicePatchIndex, rs2PodHash, "") unpausePatch := f.getPatchedRollout(unpausePatchIndex) - unpauseConditions := generateConditionsPatch(true, conditions.RolloutResumedReason, rs2, true, "", false) + _, availableCondition := newAvailableCondition(true) + _, progressingCondition := newProgressingCondition(conditions.RolloutResumedReason, rs2, "") + _, compCondition := newCompletedCondition(false) + unpauseConditions := fmt.Sprintf("[%s, %s, %s]", availableCondition, compCondition, progressingCondition) expectedUnpausePatch := `{ "status": { "conditions": %s From 81e7aaca95b98566330b76623c888f000815aacb Mon Sep 17 00:00:00 2001 From: zachaller Date: Tue, 23 Aug 2022 15:48:16 -0500 Subject: [PATCH 23/27] lint Signed-off-by: zachaller --- rollout/controller_test.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/rollout/controller_test.go b/rollout/controller_test.go index 1dd5fd25e3..f4489dc212 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -369,17 +369,6 @@ func generateConditionsPatchWithHealthy(available bool, progressingReason string return fmt.Sprintf("[%s, %s, %s, %s]", completedCondition, healthyCondition, progressingCondition, availableCondition) } -func generateConditionsPatchWithHealthyAndCompleted(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isHealthy bool) string { - _, availableCondition := newAvailableCondition(available) - _, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage) - _, healthyCondition := newHealthyCondition(isHealthy) - _, completeCondition := newCompletedCondition(true) - if availableConditionFirst { - return fmt.Sprintf("[%s, %s, %s, %s]", availableCondition, completeCondition, healthyCondition, progressingCondition) - } - return fmt.Sprintf("[%s, %s, %s, %s]", progressingCondition, availableCondition, healthyCondition, completeCondition) -} - func generateConditionsPatchWithCompleted(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isCompleted bool) string { _, availableCondition := newAvailableCondition(available) _, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage) From 6d75be7f5f9a874cc78897ff2d6cbc57b2246532 Mon Sep 17 00:00:00 2001 From: zachaller Date: Tue, 23 Aug 2022 17:02:25 -0500 Subject: [PATCH 24/27] cleanup Signed-off-by: zachaller --- rollout/sync.go | 13 ++----------- utils/conditions/conditions.go | 2 ++ utils/conditions/rollouts_test.go | 2 +- 3 files changed, 5 insertions(+), 12 deletions(-) diff --git a/rollout/sync.go b/rollout/sync.go index dd9a8e94ca..4759f490a4 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -678,6 +678,7 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt } if conditions.RolloutCompleted(c.rollout, &newStatus) { + // The event gets triggered in function promoteStable updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) conditions.SetRolloutCondition(&newStatus, *updateCompletedCond) @@ -686,7 +687,7 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) if conditions.SetRolloutCondition(&newStatus, *updateCompletedCond) { revision, _ := replicasetutil.Revision(c.rollout) - c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: "RolloutNotCompleted"}, + c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutNotCompletedReason}, conditions.RolloutNotCompletedMessage, revision+1, newStatus.CurrentPodHash) } } @@ -946,16 +947,6 @@ func (c *rolloutContext) promoteStable(newStatus *v1alpha1.RolloutStatus, reason revision, _ := replicasetutil.Revision(c.rollout) c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason) - - //if conditions.RolloutCompleted(c.rollout, newStatus) { - // updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, - // conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) - // if conditions.SetRolloutCondition(newStatus, *updateCompletedCond) { - // revision, _ := replicasetutil.Revision(c.rollout) - // c.recorder.Eventf(c.rollout, record.EventOptions{EventReason: conditions.RolloutCompletedReason}, - // conditions.RolloutCompletedMessage, revision, newStatus.CurrentPodHash, reason) - // } - //} } return nil } diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go index 4c6d8ad91f..66275ff48b 100644 --- a/utils/conditions/conditions.go +++ b/utils/conditions/conditions.go @@ -68,6 +68,8 @@ const ( RolloutCompletedReason = "RolloutCompleted" // RolloutCompletedMessage is added when the rollout is completed RolloutCompletedMessage = "Rollout completed update to revision %d (%s): %s" + // RolloutNotCompletedReason is added in a rollout when it is completed. + RolloutNotCompletedReason = "RolloutNotCompleted" // RolloutNotCompletedMessage is added when the rollout is completed RolloutNotCompletedMessage = "Rollout not completed, started update to revision %d (%s)" diff --git a/utils/conditions/rollouts_test.go b/utils/conditions/rollouts_test.go index 4985b8f728..9a4d49d09e 100644 --- a/utils/conditions/rollouts_test.go +++ b/utils/conditions/rollouts_test.go @@ -350,7 +350,7 @@ func TestRolloutProgressing(t *testing.T) { } -func TestRolloutHealthyAndComplete(t *testing.T) { +func TestRolloutHealthyAndCompleted(t *testing.T) { rollout := func(desired, current, updated, available int32, correctObservedGeneration bool) *v1alpha1.Rollout { r := &v1alpha1.Rollout{ Spec: v1alpha1.RolloutSpec{ From 7ddd59067519fad18f6d201dda863a65230a122d Mon Sep 17 00:00:00 2001 From: zachaller Date: Wed, 24 Aug 2022 12:59:37 -0500 Subject: [PATCH 25/27] Rename Signed-off-by: zachaller --- pkg/apis/rollouts/v1alpha1/types.go | 8 +++++--- rollout/bluegreen_test.go | 4 ++-- rollout/controller_test.go | 8 ++++---- rollout/sync.go | 28 ++++++++++++++-------------- test/e2e/functional_test.go | 22 +++++++++++----------- utils/conditions/conditions.go | 16 ++++++++-------- utils/conditions/rollouts_test.go | 4 ++-- 7 files changed, 46 insertions(+), 44 deletions(-) diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index 826ded984b..3eacc4f830 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -987,10 +987,12 @@ const ( RolloutReplicaFailure RolloutConditionType = "ReplicaFailure" // RolloutPaused means that rollout is in a paused state. It is still progressing at this point. RolloutPaused RolloutConditionType = "Paused" - // RolloutCompleted means that rollout is in a completed state. It is still progressing at this point. + // RolloutCompleted indicates that the rollout completed its update to the desired revision and is not in the middle + // of any update. Note that a Completed rollout could also be considered Progressing or Degraded, if its Pods become + // unavailable sometime after the update completes. RolloutCompleted RolloutConditionType = "Completed" - // HealthyAndCompleted means that rollout is in a completed state and is healthy. - HealthyAndCompleted RolloutConditionType = "HealthyAndCompleted" + // RolloutHealthy means that rollout is in a completed state and is healthy. + RolloutHealthy RolloutConditionType = "Healthy" ) // RolloutCondition describes the state of a rollout at a certain point. diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index d12c11c966..8ad0b1f838 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -45,7 +45,7 @@ func TestBlueGreenCompletedRolloutRestart(t *testing.T) { r := newBlueGreenRollout("foo", 1, nil, "active", "preview") r.Status.Conditions = []v1alpha1.RolloutCondition{} - completedHealthyCond := conditions.NewRolloutCondition(v1alpha1.HealthyAndCompleted, corev1.ConditionFalse, conditions.RolloutHealthyAndCompletedReason, conditions.RolloutNotHealthyAndCompletedMessage) + completedHealthyCond := conditions.NewRolloutCondition(v1alpha1.RolloutHealthy, corev1.ConditionFalse, conditions.RolloutHealthyReason, conditions.RolloutNotHealthyMessage) conditions.SetRolloutCondition(&r.Status, *completedHealthyCond) completedCond, _ := newCompletedCondition(true) conditions.SetRolloutCondition(&r.Status, completedCond) @@ -1218,7 +1218,7 @@ func TestBlueGreenRolloutCompletedFalse(t *testing.T) { assert.NoError(t, err) index := len(rolloutPatch.Status.Conditions) - 3 - assert.Equal(t, v1alpha1.HealthyAndCompleted, rolloutPatch.Status.Conditions[index].Type) + assert.Equal(t, v1alpha1.RolloutHealthy, rolloutPatch.Status.Conditions[index].Type) assert.Equal(t, corev1.ConditionFalse, rolloutPatch.Status.Conditions[index].Status) } diff --git a/rollout/controller_test.go b/rollout/controller_test.go index f4489dc212..0e97a405e9 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -195,18 +195,18 @@ func newPausedCondition(isPaused bool) (v1alpha1.RolloutCondition, string) { func newHealthyCondition(isHealthy bool) (v1alpha1.RolloutCondition, string) { status := corev1.ConditionTrue - msg := conditions.RolloutHealthyAndCompletedMessage + msg := conditions.RolloutHealthyMessage if !isHealthy { status = corev1.ConditionFalse - msg = conditions.RolloutNotHealthyAndCompletedMessage + msg = conditions.RolloutNotHealthyMessage } condition := v1alpha1.RolloutCondition{ LastTransitionTime: timeutil.MetaNow(), LastUpdateTime: timeutil.MetaNow(), Message: msg, - Reason: conditions.RolloutHealthyAndCompletedReason, + Reason: conditions.RolloutHealthyReason, Status: status, - Type: v1alpha1.HealthyAndCompleted, + Type: v1alpha1.RolloutHealthy, } conditionBytes, err := json.Marshal(condition) if err != nil { diff --git a/rollout/sync.go b/rollout/sync.go index 4759f490a4..21ca6ea2b6 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -547,19 +547,19 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt isPaused := len(c.rollout.Status.PauseConditions) > 0 || c.rollout.Spec.Paused isAborted := c.pauseContext.IsAborted() - var becameIncompleteUnhealthy bool // remember if we transitioned from completed - completeCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.HealthyAndCompleted) - if !isPaused && conditions.RolloutHealthyAndCompleted(c.rollout, &newStatus) { - updateHealthyCond := conditions.NewRolloutCondition(v1alpha1.HealthyAndCompleted, corev1.ConditionTrue, conditions.RolloutHealthyAndCompletedReason, conditions.RolloutHealthyAndCompletedMessage) + var becameUnhealthy bool // remember if we transitioned from completed + completeCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutHealthy) + if !isPaused && conditions.RolloutHealthy(c.rollout, &newStatus) { + updateHealthyCond := conditions.NewRolloutCondition(v1alpha1.RolloutHealthy, corev1.ConditionTrue, conditions.RolloutHealthyReason, conditions.RolloutHealthyMessage) conditions.SetRolloutCondition(&newStatus, *updateHealthyCond) // If we ever wanted to emit a healthy event here it would be noisy and somewhat unpredictable for tests and so should probably be skipped // when checking in e2e and unit tests. - //c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.RolloutHealthyAndCompletedReason}, conditions.RolloutHealthyAndCompletedMessage) + //c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.RolloutHealthyReason}, conditions.RolloutHealthyMessage) } else { if completeCond != nil { - updateHealthyCond := conditions.NewRolloutCondition(v1alpha1.HealthyAndCompleted, corev1.ConditionFalse, conditions.RolloutHealthyAndCompletedReason, conditions.RolloutNotHealthyAndCompletedMessage) - becameIncompleteUnhealthy = conditions.SetRolloutCondition(&newStatus, *updateHealthyCond) - //c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.RolloutHealthyAndCompletedReason}, conditions.RolloutNotHealthyAndCompletedMessage) + updateHealthyCond := conditions.NewRolloutCondition(v1alpha1.RolloutHealthy, corev1.ConditionFalse, conditions.RolloutHealthyReason, conditions.RolloutNotHealthyMessage) + becameUnhealthy = conditions.SetRolloutCondition(&newStatus, *updateHealthyCond) + //c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.RolloutHealthyReason}, conditions.RolloutNotHealthyMessage) } } @@ -580,11 +580,11 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt // In such a case, we should simply not estimate any progress for this rollout. currentCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutProgressing) - isHealthyAndCompletedRollout := newStatus.Replicas == newStatus.AvailableReplicas && currentCond != nil && currentCond.Reason == conditions.NewRSAvailableReason && currentCond.Type != v1alpha1.RolloutProgressing + isHealthyRollout := newStatus.Replicas == newStatus.AvailableReplicas && currentCond != nil && currentCond.Reason == conditions.NewRSAvailableReason && currentCond.Type != v1alpha1.RolloutProgressing // Check for progress. Only do this if the latest rollout hasn't completed yet and it is not aborted - if !isHealthyAndCompletedRollout && !isAborted { + if !isHealthyRollout && !isAborted { switch { - case conditions.RolloutHealthyAndCompleted(c.rollout, &newStatus): + case conditions.RolloutHealthy(c.rollout, &newStatus): // Update the rollout conditions with a message for the new replica set that // was successfully deployed. If the condition already exists, we ignore this update. rsName := "" @@ -594,7 +594,7 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt msg := fmt.Sprintf(conditions.ReplicaSetCompletedMessage, rsName) progressingCondition := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionTrue, conditions.NewRSAvailableReason, msg) conditions.SetRolloutCondition(&newStatus, *progressingCondition) - case conditions.RolloutProgressing(c.rollout, &newStatus) || becameIncompleteUnhealthy: + case conditions.RolloutProgressing(c.rollout, &newStatus) || becameUnhealthy: // If there is any progress made, continue by not checking if the rollout failed. This // behavior emulates the rolling updater progressDeadline check. msg := fmt.Sprintf(conditions.RolloutProgressingMessage, c.rollout.Name) @@ -603,7 +603,7 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt } var reason string - if newStatus.StableRS == newStatus.CurrentPodHash && becameIncompleteUnhealthy { + if newStatus.StableRS == newStatus.CurrentPodHash && becameUnhealthy { // When a fully promoted rollout becomes Incomplete, e.g., due to the ReplicaSet status changes like // pod restarts, evicted -> recreated, we'll need to reset the rollout's condition to `PROGRESSING` to // avoid any timeouts. @@ -774,7 +774,7 @@ func (c *rolloutContext) requeueStuckRollout(newStatus v1alpha1.RolloutStatus) t } // No need to estimate progress if the rollout is complete or already timed out. isPaused := len(c.rollout.Status.PauseConditions) > 0 || c.rollout.Spec.Paused - if conditions.RolloutHealthyAndCompleted(c.rollout, &newStatus) || currentCond.Reason == conditions.TimedOutReason || isPaused || c.rollout.Status.Abort || isIndefiniteStep(c.rollout) { + if conditions.RolloutHealthy(c.rollout, &newStatus) || currentCond.Reason == conditions.TimedOutReason || isPaused || c.rollout.Status.Abort || isIndefiniteStep(c.rollout) { return time.Duration(-1) } // If there is no sign of progress at this point then there is a high chance that the diff --git a/test/e2e/functional_test.go b/test/e2e/functional_test.go index 7d24d9655a..8ca5a026ac 100644 --- a/test/e2e/functional_test.go +++ b/test/e2e/functional_test.go @@ -1123,10 +1123,10 @@ func (s *FunctionalSuite) TestKubectlWaitForCompleted() { kind: Service apiVersion: v1 metadata: - name: kubectl-wait-completed + name: kubectl-wait-healthy spec: selector: - app: kubectl-wait-completed + app: kubectl-wait-healthy ports: - protocol: TCP port: 80 @@ -1135,19 +1135,19 @@ spec: apiVersion: argoproj.io/v1alpha1 kind: Rollout metadata: - name: kubectl-wait-completed + name: kubectl-wait-healthy spec: replicas: 1 selector: matchLabels: - app: kubectl-wait-completed + app: kubectl-wait-healthy template: metadata: labels: - app: kubectl-wait-completed + app: kubectl-wait-healthy spec: containers: - - name: kubectl-wait-completed + - name: kubectl-wait-healthy image: nginx:1.19-alpine imagePullPolicy: Always ports: @@ -1161,21 +1161,21 @@ spec: strategy: blueGreen: - activeService: kubectl-wait-completed + activeService: kubectl-wait-healthy autoPromotionEnabled: true `). When(). UpdateSpec(). Then(). - ExpectRollout("HealthyAndCompleted=False", func(r *v1alpha1.Rollout) bool { - cmd := exec.Command("kubectl", "wait", "--for=condition=HealthyAndCompleted=False", fmt.Sprintf("rollout/%s", r.Name)) + ExpectRollout("Healthy=False", func(r *v1alpha1.Rollout) bool { + cmd := exec.Command("kubectl", "wait", "--for=condition=Healthy=False", fmt.Sprintf("rollout/%s", r.Name)) out, err := cmd.CombinedOutput() return err == nil && strings.Contains(string(out), fmt.Sprintf("rollout.argoproj.io/%s condition met", r.Name)) }). ExpectRolloutStatus("Progressing"). ExpectActiveRevision("1"). - ExpectRollout("HealthyAndCompleted=True", func(r *v1alpha1.Rollout) bool { - cmd := exec.Command("kubectl", "wait", "--for=condition=HealthyAndCompleted=True", fmt.Sprintf("rollout/%s", r.Name)) + ExpectRollout("Healthy=True", func(r *v1alpha1.Rollout) bool { + cmd := exec.Command("kubectl", "wait", "--for=condition=Healthy=True", fmt.Sprintf("rollout/%s", r.Name)) out, err := cmd.CombinedOutput() return err == nil && strings.Contains(string(out), fmt.Sprintf("rollout.argoproj.io/%s condition met", r.Name)) }). diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go index 66275ff48b..bdb7a562f9 100644 --- a/utils/conditions/conditions.go +++ b/utils/conditions/conditions.go @@ -73,12 +73,12 @@ const ( // RolloutNotCompletedMessage is added when the rollout is completed RolloutNotCompletedMessage = "Rollout not completed, started update to revision %d (%s)" - // RolloutHealthyAndCompletedReason is added in a rollout when it is healthy. - RolloutHealthyAndCompletedReason = "HealthyAndCompleted" - // RolloutHealthyAndCompletedMessage is added when the rollout is completed and is healthy or not. - RolloutHealthyAndCompletedMessage = "Rollout is healthy and completed" - // RolloutNotHealthyAndCompletedMessage is added when the rollout is completed and is healthy or not. - RolloutNotHealthyAndCompletedMessage = "Rollout is not healthy and completed" + // RolloutHealthyReason is added in a rollout when it is healthy. + RolloutHealthyReason = "RolloutHealthy" + // RolloutHealthyMessage is added when the rollout is completed and is healthy or not. + RolloutHealthyMessage = "Rollout is healthy and completed" + // RolloutNotHealthyMessage is added when the rollout is completed and is healthy or not. + RolloutNotHealthyMessage = "Rollout is not healthy and completed" // RolloutAbortedReason indicates that the rollout was aborted RolloutAbortedReason = "RolloutAborted" @@ -268,9 +268,9 @@ func RolloutProgressing(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutSt strategySpecificProgress } -// RolloutHealthyAndCompleted considers a rollout to be complete once all of its desired replicas +// RolloutHealthy considers a rollout to be healthy once all of its desired replicas // are updated, available, and receiving traffic from the active service, and no old pods are running. -func RolloutHealthyAndCompleted(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) bool { +func RolloutHealthy(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) bool { completedStrategy := true replicas := defaults.GetReplicasOrDefault(rollout.Spec.Replicas) diff --git a/utils/conditions/rollouts_test.go b/utils/conditions/rollouts_test.go index 9a4d49d09e..842f01f594 100644 --- a/utils/conditions/rollouts_test.go +++ b/utils/conditions/rollouts_test.go @@ -350,7 +350,7 @@ func TestRolloutProgressing(t *testing.T) { } -func TestRolloutHealthyAndCompleted(t *testing.T) { +func TestRolloutHealthy(t *testing.T) { rollout := func(desired, current, updated, available int32, correctObservedGeneration bool) *v1alpha1.Rollout { r := &v1alpha1.Rollout{ Spec: v1alpha1.RolloutSpec{ @@ -475,7 +475,7 @@ func TestRolloutHealthyAndCompleted(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - assert.Equal(t, test.expected, RolloutHealthyAndCompleted(test.r, &test.r.Status)) + assert.Equal(t, test.expected, RolloutHealthy(test.r, &test.r.Status)) }) } From d0523f6b7614af797cfd703e012fa2ae8a897b48 Mon Sep 17 00:00:00 2001 From: zachaller Date: Wed, 24 Aug 2022 13:06:06 -0500 Subject: [PATCH 26/27] More renames Signed-off-by: zachaller --- pkg/apis/rollouts/v1alpha1/types.go | 3 ++- utils/conditions/conditions.go | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index 3eacc4f830..746758c60f 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -991,7 +991,8 @@ const ( // of any update. Note that a Completed rollout could also be considered Progressing or Degraded, if its Pods become // unavailable sometime after the update completes. RolloutCompleted RolloutConditionType = "Completed" - // RolloutHealthy means that rollout is in a completed state and is healthy. + // RolloutHealthy means that rollout is in a completed state and is healthy. Which means that all the pods have been updated + // and are passing their health checks and are ready to serve traffic. RolloutHealthy RolloutConditionType = "Healthy" ) diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go index bdb7a562f9..c2aaece787 100644 --- a/utils/conditions/conditions.go +++ b/utils/conditions/conditions.go @@ -76,9 +76,9 @@ const ( // RolloutHealthyReason is added in a rollout when it is healthy. RolloutHealthyReason = "RolloutHealthy" // RolloutHealthyMessage is added when the rollout is completed and is healthy or not. - RolloutHealthyMessage = "Rollout is healthy and completed" + RolloutHealthyMessage = "Rollout is healthy" // RolloutNotHealthyMessage is added when the rollout is completed and is healthy or not. - RolloutNotHealthyMessage = "Rollout is not healthy and completed" + RolloutNotHealthyMessage = "Rollout is not healthy" // RolloutAbortedReason indicates that the rollout was aborted RolloutAbortedReason = "RolloutAborted" From 803114662559eb521f6bdf2c4ed321233018f3f1 Mon Sep 17 00:00:00 2001 From: zachaller Date: Wed, 24 Aug 2022 14:26:05 -0500 Subject: [PATCH 27/27] small comment change Signed-off-by: zachaller --- rollout/sync.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rollout/sync.go b/rollout/sync.go index 21ca6ea2b6..b9bc616552 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -547,7 +547,7 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt isPaused := len(c.rollout.Status.PauseConditions) > 0 || c.rollout.Spec.Paused isAborted := c.pauseContext.IsAborted() - var becameUnhealthy bool // remember if we transitioned from completed + var becameUnhealthy bool // remember if we transitioned from healthy to unhealthy completeCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutHealthy) if !isPaused && conditions.RolloutHealthy(c.rollout, &newStatus) { updateHealthyCond := conditions.NewRolloutCondition(v1alpha1.RolloutHealthy, corev1.ConditionTrue, conditions.RolloutHealthyReason, conditions.RolloutHealthyMessage)