From 9c28396e8173117b73de34d54740d376bb835a52 Mon Sep 17 00:00:00 2001 From: khhirani Date: Wed, 31 Mar 2021 01:21:43 -0700 Subject: [PATCH 01/11] feat: Create RolloutPaused condition --- manifests/install.yaml | 6 ++---- manifests/namespace-install.yaml | 6 ++---- pkg/apis/rollouts/v1alpha1/types.go | 2 ++ rollout/sync.go | 29 +++++++++++++++++++++-------- 4 files changed, 27 insertions(+), 16 deletions(-) diff --git a/manifests/install.yaml b/manifests/install.yaml index 6fc4614db5..65ac574f58 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -11541,13 +11541,11 @@ spec: name: Current type: integer - JSONPath: .status.updatedReplicas - description: Total number of non-terminated pods targeted by this rollout that - have the desired template spec + description: Total number of non-terminated pods targeted by this rollout that have the desired template spec name: Up-to-date type: integer - JSONPath: .status.availableReplicas - description: Total number of available pods (ready for at least minReadySeconds) - targeted by this rollout + description: Total number of available pods (ready for at least minReadySeconds) targeted by this rollout name: Available type: integer group: argoproj.io diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index 6e625972a7..04187220a5 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -11541,13 +11541,11 @@ spec: name: Current type: integer - JSONPath: .status.updatedReplicas - description: Total number of non-terminated pods targeted by this rollout that - have the desired template spec + description: Total number of non-terminated pods targeted by this rollout that have the desired template spec name: Up-to-date type: integer - JSONPath: .status.availableReplicas - description: Total number of available pods (ready for at least minReadySeconds) - targeted by this rollout + description: Total number of available pods (ready for at least minReadySeconds) targeted by this rollout name: Available type: integer group: argoproj.io diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index 3dc68e930c..aec5ac377a 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -639,6 +639,8 @@ const ( // RolloutReplicaFailure ReplicaFailure is added in a deployment when one of its pods // fails to be created or deleted. RolloutReplicaFailure RolloutConditionType = "ReplicaFailure" + // RolloutPaused means that rollout is in a paused state. It is still progressing at this point. + RolloutPaused RolloutConditionType = "Paused" ) // RolloutCondition describes the state of a rollout at a certain point. diff --git a/rollout/sync.go b/rollout/sync.go index 3b3d20e522..568fce91b0 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -484,33 +484,46 @@ func (c *rolloutContext) cleanupRollouts(oldRSs []*appsv1.ReplicaSet) error { // that were paused for longer than progressDeadlineSeconds. func (c *rolloutContext) checkPausedConditions() error { cond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutProgressing) + pauseCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutPaused) pausedCondExists := cond != nil && cond.Reason == conditions.PausedRolloutReason isPaused := len(c.rollout.Status.PauseConditions) > 0 || c.rollout.Spec.Paused - var updatedCondition *v1alpha1.RolloutCondition + var updatedConditions []*v1alpha1.RolloutCondition if isPaused && !pausedCondExists { - updatedCondition = conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.PausedRolloutReason, conditions.PausedRolloutMessage) + // Add extra pause condition + updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutPaused, corev1.ConditionTrue, conditions.PausedRolloutReason, conditions.PausedRolloutMessage)) + updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.PausedRolloutReason, conditions.PausedRolloutMessage)) } else if !isPaused && pausedCondExists { - updatedCondition = conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.ResumedRolloutReason, conditions.ResumeRolloutMessage) + if pauseCond != nil { + // If pause condition exists in conditionList, set to False + updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutPaused, corev1.ConditionFalse, conditions.PausedRolloutReason, conditions.PausedRolloutMessage)) + } + updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.ResumedRolloutReason, conditions.ResumeRolloutMessage)) } abortCondExists := cond != nil && cond.Reason == conditions.RolloutAbortedReason if !c.rollout.Status.Abort && abortCondExists { - updatedCondition = conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.RolloutRetryReason, conditions.RolloutRetryMessage) + //if pauseCond != nil { + // // If pause condition exists in conditionList, set to False + // updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutPaused, corev1.ConditionFalse, conditions.PausedRolloutReason, conditions.PausedRolloutMessage)) + //} + updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.RolloutRetryReason, conditions.RolloutRetryMessage)) } - if updatedCondition == nil { + if len(updatedConditions) == 0 { return nil } newStatus := c.rollout.Status.DeepCopy() - err := c.patchCondition(c.rollout, newStatus, updatedCondition) + err := c.patchCondition(c.rollout, newStatus, updatedConditions...) return err } -func (c *rolloutContext) patchCondition(r *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus, condition *v1alpha1.RolloutCondition) error { +func (c *rolloutContext) patchCondition(r *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus, conditionList ...*v1alpha1.RolloutCondition) error { ctx := context.TODO() - conditions.SetRolloutCondition(newStatus, *condition) + for _, condition := range conditionList { + conditions.SetRolloutCondition(newStatus, *condition) + } newStatus.ObservedGeneration = strconv.Itoa(int(c.rollout.Generation)) logCtx := logutil.WithVersionFields(c.log, r) patch, modified, err := diff.CreateTwoWayMergePatch( From 0083317269aa15b79641d77d89a99ed9df5f1610 Mon Sep 17 00:00:00 2001 From: khhirani Date: Wed, 31 Mar 2021 12:14:50 -0700 Subject: [PATCH 02/11] Modify sync.go and tests Signed-off-by: khhirani --- rollout/analysis_test.go | 1 + rollout/sync.go | 35 ++++++++++++++++++----------------- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index 576e3e2712..5f297f59ba 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -1445,6 +1445,7 @@ func TestDoNotCreateBackgroundAnalysisRunAfterInconclusiveRun(t *testing.T) { }} pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") conditions.SetRolloutCondition(&r2.Status, pausedCondition) + // TODO: Add Pause Condition f.rolloutLister = append(f.rolloutLister, r2) f.analysisTemplateLister = append(f.analysisTemplateLister, at) diff --git a/rollout/sync.go b/rollout/sync.go index 568fce91b0..670dd1c43a 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -483,33 +483,34 @@ func (c *rolloutContext) cleanupRollouts(oldRSs []*appsv1.ReplicaSet) error { // These conditions are needed so that we won't accidentally report lack of progress for resumed rollouts // that were paused for longer than progressDeadlineSeconds. func (c *rolloutContext) checkPausedConditions() error { - cond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutProgressing) - pauseCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutPaused) - pausedCondExists := cond != nil && cond.Reason == conditions.PausedRolloutReason - + // Progressing condition + progCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutProgressing) + progCondPaused := progCond != nil && progCond.Reason == conditions.PausedRolloutReason isPaused := len(c.rollout.Status.PauseConditions) > 0 || c.rollout.Spec.Paused var updatedConditions []*v1alpha1.RolloutCondition - if isPaused && !pausedCondExists { - // Add extra pause condition - updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutPaused, corev1.ConditionTrue, conditions.PausedRolloutReason, conditions.PausedRolloutMessage)) + if isPaused && !progCondPaused { updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.PausedRolloutReason, conditions.PausedRolloutMessage)) - } else if !isPaused && pausedCondExists { - if pauseCond != nil { - // If pause condition exists in conditionList, set to False - updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutPaused, corev1.ConditionFalse, conditions.PausedRolloutReason, conditions.PausedRolloutMessage)) - } + } else if !isPaused && progCondPaused { updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.ResumedRolloutReason, conditions.ResumeRolloutMessage)) } - abortCondExists := cond != nil && cond.Reason == conditions.RolloutAbortedReason + abortCondExists := progCond != nil && progCond.Reason == conditions.RolloutAbortedReason if !c.rollout.Status.Abort && abortCondExists { - //if pauseCond != nil { - // // If pause condition exists in conditionList, set to False - // updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutPaused, corev1.ConditionFalse, conditions.PausedRolloutReason, conditions.PausedRolloutMessage)) - //} updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.RolloutRetryReason, conditions.RolloutRetryMessage)) } + pauseCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutPaused) + pausedCondTrue := pauseCond != nil && pauseCond.Status == corev1.ConditionTrue + + if (isPaused && !abortCondExists) != pausedCondTrue { + condStatus := corev1.ConditionFalse + // Add extra pause condition + if isPaused { + condStatus = corev1.ConditionTrue + } + updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutPaused, condStatus, conditions.PausedRolloutReason, conditions.PausedRolloutMessage)) + } + if len(updatedConditions) == 0 { return nil } From cc505708019121a49c7fe8c5e85947e6e7dd2c75 Mon Sep 17 00:00:00 2001 From: khhirani Date: Thu, 1 Apr 2021 00:54:16 -0700 Subject: [PATCH 03/11] Modify failing tests --- rollout/analysis_test.go | 57 +++++++++++++++++++++++++++------- rollout/bluegreen_test.go | 30 ++++++++++++++---- rollout/canary_test.go | 43 ++++++++++++++++++++----- rollout/controller_test.go | 20 ++++++++++++ rollout/sync.go | 18 ++++++----- rollout/trafficrouting_test.go | 3 ++ 6 files changed, 139 insertions(+), 32 deletions(-) diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index 5f297f59ba..3dfd330d14 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -1443,9 +1443,12 @@ func TestDoNotCreateBackgroundAnalysisRunAfterInconclusiveRun(t *testing.T) { Reason: v1alpha1.PauseReasonInconclusiveAnalysis, StartTime: metav1.Now(), }} - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) - // TODO: Add Pause Condition f.rolloutLister = append(f.rolloutLister, r2) f.analysisTemplateLister = append(f.analysisTemplateLister, at) @@ -1455,6 +1458,13 @@ func TestDoNotCreateBackgroundAnalysisRunAfterInconclusiveRun(t *testing.T) { f.run(getKey(r2, t)) patch := f.getPatchedRollout(patchIndex) + + //expectedPatch := fmt.Sprintf(`{ + // "status":{ + // "conditions":[%s, %s], + // "observedGeneration": "" + // } + //}`, progressingConditionString, pausedConditionString) assert.Equal(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) } @@ -1544,7 +1554,10 @@ func TestCreatePrePromotionAnalysisRun(t *testing.T) { rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) previewSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} @@ -1724,8 +1737,12 @@ func TestRolloutPrePromotionAnalysisBecomesInconclusive(t *testing.T) { Name: ar.Name, Status: v1alpha1.AnalysisPhaseRunning, } - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") - conditions.SetRolloutCondition(&r2.Status, pausedCondition) + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) + conditions.SetRolloutCondition(&r2.Status, pausedCondition) + activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} activeSvc := newService("active", 80, activeSelector, r2) @@ -1789,7 +1806,10 @@ func TestRolloutPrePromotionAnalysisSwitchServiceAfterSuccess(t *testing.T) { Name: ar.Name, Status: v1alpha1.AnalysisPhaseRunning, } - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} @@ -1852,7 +1872,10 @@ func TestRolloutPrePromotionAnalysisHonorAutoPromotionSeconds(t *testing.T) { r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) now := metav1.NewTime(metav1.Now().Add(-10 * time.Second)) r2.Status.PauseConditions[0].StartTime = now - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} @@ -1915,7 +1938,10 @@ func TestRolloutPrePromotionAnalysisDoNothingOnInconclusiveAnalysis(t *testing.T } r2.Status.PauseConditions = append(r2.Status.PauseConditions, inconclusivePauseCondition) r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} @@ -1958,7 +1984,10 @@ func TestAbortRolloutOnErrorPrePromotionAnalysis(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r2 = updateBlueGreenRolloutStatus(r2, "", rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} @@ -2123,7 +2152,10 @@ func TestPostPromotionAnalysisRunHandleInconclusive(t *testing.T) { Reason: v1alpha1.PauseReasonInconclusiveAnalysis, StartTime: metav1.Now(), }} - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} @@ -2176,7 +2208,10 @@ func TestAbortRolloutOnErrorPostPromotionAnalysis(t *testing.T) { rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs1PodHash, 1, 1, 2, 1, true, true) - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 0976a75438..464244e0aa 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -266,7 +266,10 @@ func TestBlueGreenHandlePause(t *testing.T) { rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) previewSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} @@ -310,7 +313,10 @@ func TestBlueGreenHandlePause(t *testing.T) { Reason: v1alpha1.PauseReasonInconclusiveAnalysis, StartTime: now, }) - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) previewSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} @@ -347,7 +353,10 @@ func TestBlueGreenHandlePause(t *testing.T) { rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true) - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) previewSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} @@ -385,7 +394,10 @@ func TestBlueGreenHandlePause(t *testing.T) { before := metav1.NewTime(now.Add(-1 * time.Minute)) r2.Status.PauseConditions[0].StartTime = before r2.Status.ControllerPause = true - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} @@ -439,7 +451,10 @@ func TestBlueGreenHandlePause(t *testing.T) { r2.Status.PauseConditions[0].StartTime = before r2.Status.ControllerPause = true r2.Spec.Paused = true - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} @@ -731,7 +746,10 @@ func TestBlueGreenRolloutStatusHPAStatusFieldsActiveSelectorSet(t *testing.T) { r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 0, 0, 0, 0, true, false) r2.Status.Selector = "" - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) f.rolloutLister = append(f.rolloutLister, r2) diff --git a/rollout/canary_test.go b/rollout/canary_test.go index 4e4e37bc46..fbd8063f17 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -189,6 +189,9 @@ func TestCanaryRolloutNoProgressWhilePaused(t *testing.T) { progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) + pausedCondition, _ := newPausedCondition(true) + conditions.SetRolloutCondition(&r2.Status, pausedCondition) + rs1 := newReplicaSetWithStatus(r1, 10, 10) rs2 := newReplicaSetWithStatus(r2, 0, 0) @@ -216,9 +219,12 @@ func TestCanaryRolloutUpdatePauseConditionWhilePaused(t *testing.T) { r1 := newCanaryRollout("foo", 10, nil, steps, pointer.Int32Ptr(0), intstr.FromInt(1), intstr.FromInt(0)) r2 := bumpVersion(r1) - progressingCondition, _ := newProgressingCondition(conditions.ReplicaSetUpdatedReason, r2, "") + progressingCondition, progressingConditionString := newProgressingCondition(conditions.PausedRolloutMessage, r2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) + pausedCondition, pausedConditionString := newPausedCondition(true) + conditions.SetRolloutCondition(&r2.Status, pausedCondition) + rs1 := newReplicaSetWithStatus(r1, 10, 10) rs2 := newReplicaSetWithStatus(r2, 0, 0) @@ -235,12 +241,12 @@ func TestCanaryRolloutUpdatePauseConditionWhilePaused(t *testing.T) { f.run(getKey(r2, t)) patch := f.getPatchedRollout(addPausedConditionPatch) - _, pausedCondition := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") + expectedPatch := fmt.Sprintf(`{ "status": { - "conditions": [%s] + "conditions": [%s, %s] } - }`, pausedCondition) + }`, pausedConditionString, progressingConditionString) assert.Equal(t, calculatePatch(r2, expectedPatch), patch) } @@ -885,6 +891,9 @@ func TestSyncRolloutWaitAddToQueue(t *testing.T) { progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) + pausedCondition, _ := newPausedCondition(true) + conditions.SetRolloutCondition(&r2.Status, pausedCondition) + r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) @@ -927,6 +936,10 @@ func TestSyncRolloutIgnoreWaitOutsideOfReconciliationPeriod(t *testing.T) { progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) + pausedCondition, _ := newPausedCondition(true) + conditions.SetRolloutCondition(&r2.Status, pausedCondition) + + f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) @@ -964,7 +977,10 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) { f.replicaSetLister = append(f.replicaSetLister, rs1, rs2) r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 10, 1, 10, false) - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) earlier := metav1.Now() @@ -1008,6 +1024,9 @@ func TestCanaryRolloutStatusHPAStatusFields(t *testing.T) { progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) + pausedCondition, _ := newPausedCondition(true) + conditions.SetRolloutCondition(&r2.Status, pausedCondition) + rs1 := newReplicaSetWithStatus(r1, 4, 4) rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] rs2 := newReplicaSetWithStatus(r2, 1, 1) @@ -1249,7 +1268,10 @@ func TestCanaryRolloutScaleWhilePaused(t *testing.T) { r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 5, 0, 5, true) r2.Spec.Replicas = pointer.Int32Ptr(10) - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) f.rolloutLister = append(f.rolloutLister, r2) @@ -1343,7 +1365,9 @@ func TestNoResumeAfterPauseDurationIfUserPaused(t *testing.T) { Reason: v1alpha1.PauseReasonCanaryPauseStep, StartTime: overAMinuteAgo, }} - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs1, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs1, "") + conditions.SetRolloutCondition(&r1.Status, progressingCondition) + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r1.Status, pausedCondition) r1.Spec.Paused = true f.kubeobjects = append(f.kubeobjects, rs1) @@ -1380,7 +1404,10 @@ func TestHandleNilNewRSOnScaleAndImageChange(t *testing.T) { r2 := bumpVersion(r1) r2.Spec.Replicas = pointer.Int32Ptr(3) r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 3, 0, 3, true) - pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs1, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs1, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) f.kubeobjects = append(f.kubeobjects, rs1) diff --git a/rollout/controller_test.go b/rollout/controller_test.go index e3b4bedc96..78c4e35ee5 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -165,6 +165,26 @@ func newReplicaSetWithStatus(r *v1alpha1.Rollout, replicas int, availableReplica return rs } +func newPausedCondition(isPaused bool) (v1alpha1.RolloutCondition, string){ + status := corev1.ConditionTrue + if !isPaused { + status = corev1.ConditionFalse + } + condition := v1alpha1.RolloutCondition{ + LastTransitionTime: metav1.Now(), + LastUpdateTime: metav1.Now(), + Message: conditions.PausedRolloutMessage, + Reason: conditions.PausedRolloutReason, + Status: status, + Type: v1alpha1.RolloutPaused, + } + 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 := "" diff --git a/rollout/sync.go b/rollout/sync.go index 670dd1c43a..7f2a8337aa 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -486,15 +486,20 @@ func (c *rolloutContext) checkPausedConditions() error { // Progressing condition progCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutProgressing) progCondPaused := progCond != nil && progCond.Reason == conditions.PausedRolloutReason + isPaused := len(c.rollout.Status.PauseConditions) > 0 || c.rollout.Spec.Paused + abortCondExists := progCond != nil && progCond.Reason == conditions.RolloutAbortedReason + var updatedConditions []*v1alpha1.RolloutCondition - if isPaused && !progCondPaused { - updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.PausedRolloutReason, conditions.PausedRolloutMessage)) - } else if !isPaused && progCondPaused { - updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.ResumedRolloutReason, conditions.ResumeRolloutMessage)) + + if (isPaused != progCondPaused) && !abortCondExists { + if isPaused { + updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.PausedRolloutReason, conditions.PausedRolloutMessage)) + } else { + updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.ResumedRolloutReason, conditions.ResumeRolloutMessage)) + } } - abortCondExists := progCond != nil && progCond.Reason == conditions.RolloutAbortedReason if !c.rollout.Status.Abort && abortCondExists { updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.RolloutRetryReason, conditions.RolloutRetryMessage)) } @@ -502,9 +507,8 @@ func (c *rolloutContext) checkPausedConditions() error { pauseCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutPaused) pausedCondTrue := pauseCond != nil && pauseCond.Status == corev1.ConditionTrue - if (isPaused && !abortCondExists) != pausedCondTrue { + if (isPaused != pausedCondTrue) && !abortCondExists { condStatus := corev1.ConditionFalse - // Add extra pause condition if isPaused { condStatus = corev1.ConditionTrue } diff --git a/rollout/trafficrouting_test.go b/rollout/trafficrouting_test.go index 26894a5ebe..167450e940 100644 --- a/rollout/trafficrouting_test.go +++ b/rollout/trafficrouting_test.go @@ -135,6 +135,9 @@ func TestRolloutUseDesiredWeight(t *testing.T) { progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) + pausedCondition, _ := newPausedCondition(true) + conditions.SetRolloutCondition(&r2.Status, pausedCondition) + rs1 := newReplicaSetWithStatus(r1, 10, 10) rs2 := newReplicaSetWithStatus(r2, 1, 1) From 8b2227b9f538cc33bd3f6f7fe2b418a38dac7d21 Mon Sep 17 00:00:00 2001 From: khhirani Date: Thu, 1 Apr 2021 01:13:49 -0700 Subject: [PATCH 04/11] All tests passing Signed-off-by: khhirani --- rollout/analysis_test.go | 5 ++--- rollout/bluegreen_test.go | 2 +- rollout/canary_test.go | 14 +++----------- rollout/controller_test.go | 12 +++++++++++- 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index 3dfd330d14..ff6b01b0d7 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -1738,11 +1738,10 @@ func TestRolloutPrePromotionAnalysisBecomesInconclusive(t *testing.T) { Status: v1alpha1.AnalysisPhaseRunning, } progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") - conditions.SetRolloutCondition(&r2.Status, progressingCondition) + conditions.SetRolloutCondition(&r2.Status, progressingCondition) pausedCondition, _ := newPausedCondition(true) - conditions.SetRolloutCondition(&r2.Status, pausedCondition) - + conditions.SetRolloutCondition(&r2.Status, pausedCondition) activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} activeSvc := newService("active", 80, activeSelector, r2) diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 464244e0aa..64a8c8d02b 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -248,7 +248,7 @@ func TestBlueGreenHandlePause(t *testing.T) { "conditions": %s } }` - addedConditions := generateConditionsPatch(true, conditions.PausedRolloutReason, rs2, true, "") + addedConditions := generateConditionsPatchWithPause(true, conditions.PausedRolloutReason, rs2, true, "", true) assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, addedConditions)), patch) }) diff --git a/rollout/canary_test.go b/rollout/canary_test.go index fbd8063f17..6ea1fdda1a 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -219,10 +219,10 @@ func TestCanaryRolloutUpdatePauseConditionWhilePaused(t *testing.T) { r1 := newCanaryRollout("foo", 10, nil, steps, pointer.Int32Ptr(0), intstr.FromInt(1), intstr.FromInt(0)) r2 := bumpVersion(r1) - progressingCondition, progressingConditionString := newProgressingCondition(conditions.PausedRolloutMessage, r2, "") + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) - pausedCondition, pausedConditionString := newPausedCondition(true) + pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) rs1 := newReplicaSetWithStatus(r1, 10, 10) @@ -237,17 +237,10 @@ func TestCanaryRolloutUpdatePauseConditionWhilePaused(t *testing.T) { f.objects = append(f.objects, r2) addPausedConditionPatch := f.expectPatchRolloutAction(r2) - f.expectPatchRolloutAction(r2) f.run(getKey(r2, t)) patch := f.getPatchedRollout(addPausedConditionPatch) - - expectedPatch := fmt.Sprintf(`{ - "status": { - "conditions": [%s, %s] - } - }`, pausedConditionString, progressingConditionString) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) + assert.Equal(t, calculatePatch(r2, OnlyObservedGenerationPatch), patch) } func TestCanaryRolloutResetProgressDeadlineOnRetry(t *testing.T) { @@ -939,7 +932,6 @@ func TestSyncRolloutIgnoreWaitOutsideOfReconciliationPeriod(t *testing.T) { pausedCondition, _ := newPausedCondition(true) conditions.SetRolloutCondition(&r2.Status, pausedCondition) - f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) diff --git a/rollout/controller_test.go b/rollout/controller_test.go index 78c4e35ee5..c3c7157db0 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -165,7 +165,7 @@ func newReplicaSetWithStatus(r *v1alpha1.Rollout, replicas int, availableReplica return rs } -func newPausedCondition(isPaused bool) (v1alpha1.RolloutCondition, string){ +func newPausedCondition(isPaused bool) (v1alpha1.RolloutCondition, string) { status := corev1.ConditionTrue if !isPaused { status = corev1.ConditionFalse @@ -297,6 +297,16 @@ func generateConditionsPatch(available bool, progressingReason string, progressi return fmt.Sprintf("[%s, %s]", progressingCondition, availableCondition) } +func generateConditionsPatchWithPause(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isPaused bool) string { + _, availableCondition := newAvailableCondition(available) + _, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage) + _, pauseCondition := newPausedCondition(isPaused) + if availableConditionFirst { + return fmt.Sprintf("[%s, %s, %s]", availableCondition, progressingCondition, pauseCondition) + } + return fmt.Sprintf("[%s, %s, %s]", progressingCondition, pauseCondition, availableCondition) +} + // 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 { newRollout := updateBaseRolloutStatus(r, availableReplicas, updatedReplicas, totalReplicas, hpaReplicas) From 95a44e77e542c5013f86aa38e4ab4b21b1470db1 Mon Sep 17 00:00:00 2001 From: khhirani Date: Thu, 1 Apr 2021 01:22:14 -0700 Subject: [PATCH 05/11] make manifests --- manifests/install.yaml | 6 ++++-- manifests/namespace-install.yaml | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/manifests/install.yaml b/manifests/install.yaml index 65ac574f58..6fc4614db5 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -11541,11 +11541,13 @@ spec: name: Current type: integer - JSONPath: .status.updatedReplicas - description: Total number of non-terminated pods targeted by this rollout that have the desired template spec + description: Total number of non-terminated pods targeted by this rollout that + have the desired template spec name: Up-to-date type: integer - JSONPath: .status.availableReplicas - description: Total number of available pods (ready for at least minReadySeconds) targeted by this rollout + description: Total number of available pods (ready for at least minReadySeconds) + targeted by this rollout name: Available type: integer group: argoproj.io diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index 04187220a5..6e625972a7 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -11541,11 +11541,13 @@ spec: name: Current type: integer - JSONPath: .status.updatedReplicas - description: Total number of non-terminated pods targeted by this rollout that have the desired template spec + description: Total number of non-terminated pods targeted by this rollout that + have the desired template spec name: Up-to-date type: integer - JSONPath: .status.availableReplicas - description: Total number of available pods (ready for at least minReadySeconds) targeted by this rollout + description: Total number of available pods (ready for at least minReadySeconds) + targeted by this rollout name: Available type: integer group: argoproj.io From 734272e41d2b1f6adab98c12a873ea9ef61d17ec Mon Sep 17 00:00:00 2001 From: khhirani Date: Tue, 6 Apr 2021 15:43:57 -0700 Subject: [PATCH 06/11] e2e test Signed-off-by: khhirani --- test/e2e/functional_test.go | 66 +++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/test/e2e/functional_test.go b/test/e2e/functional_test.go index a3caa642bb..f9ef9c2a73 100644 --- a/test/e2e/functional_test.go +++ b/test/e2e/functional_test.go @@ -3,6 +3,9 @@ package e2e import ( + "fmt" + "os/exec" + "strings" "testing" "time" @@ -11,6 +14,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" "github.com/argoproj/argo-rollouts/test/fixtures" ) @@ -763,3 +767,65 @@ spec: ExpectRevisionPodCount("1", 0). ExpectReplicaCounts(1, 2, 1, 1, 1) } + +func (s *FunctionalSuite) TestKubectlWaitForPaused() { + s.Given(). + RolloutObjects(` +kind: Service +apiVersion: v1 +metadata: + name: rollout-bluegreen-active +spec: + selector: + app: rollout-bluegreen + ports: + - protocol: TCP + port: 80 + targetPort: 8080 +--- +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +metadata: + name: rollout-bluegreen +spec: + replicas: 1 + revisionHistoryLimit: 2 + selector: + matchLabels: + app: rollout-bluegreen + template: + metadata: + labels: + app: rollout-bluegreen + spec: + containers: + - name: rollouts-demo + image: argoproj/rollouts-demo:blue + imagePullPolicy: Always + ports: + - containerPort: 8080 + strategy: + blueGreen: + activeService: rollout-bluegreen-active + autoPromotionEnabled: false +`). + When(). + ApplyManifests(). + WaitForRolloutReplicas(1). + WaitForRolloutStatus("Healthy"). + UpdateSpec(). + Then(). + ExpectRollout("Paused", func(r *v1alpha1.Rollout) bool { + cmd := exec.Command("kubectl", "wait", "--for=condition=Paused", fmt.Sprintf("rollout/%s", r.Name)) + out, err := cmd.CombinedOutput() + return err == nil && strings.Contains(string(out), "rollout.argoproj.io/rollout-bluegreen condition met") + }). + When(). + PromoteRollout(). + Then(). + ExpectRollout("UnPaused", func(r *v1alpha1.Rollout) bool { + cmd := exec.Command("kubectl", "wait", "--for=condition=Paused=False", fmt.Sprintf("rollout/%s", r.Name)) + return cmd.Run() == nil + }). + ExpectActiveRevision("2") +} From edf6edebd1d98cb6b7b84449a7b507fd8835e17d Mon Sep 17 00:00:00 2001 From: khhirani Date: Wed, 7 Apr 2021 22:38:50 -0700 Subject: [PATCH 07/11] feat: Add RolloutCompleted condition Signed-off-by: khhirani --- pkg/apis/rollouts/v1alpha1/types.go | 2 ++ rollout/sync.go | 6 ++++-- utils/conditions/conditions.go | 2 ++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index aec5ac377a..e40e681514 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -641,6 +641,8 @@ 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 RolloutConditionType = "Completed" ) // RolloutCondition describes the state of a rollout at a certain point. diff --git a/rollout/sync.go b/rollout/sync.go index 7f2a8337aa..15784ef19f 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -590,8 +590,10 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt if c.newRS != nil { msg = fmt.Sprintf(conditions.ReplicaSetCompletedMessage, c.newRS.Name) } - condition := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionTrue, conditions.NewRSAvailableReason, msg) - conditions.SetRolloutCondition(&newStatus, *condition) + progressingCondition := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionTrue, conditions.NewRSAvailableReason, msg) + conditions.SetRolloutCondition(&newStatus, *progressingCondition) + completedCondition := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, conditions.RolloutCompletedReason, msg) + conditions.SetRolloutCondition(&newStatus, *completedCondition) case conditions.RolloutProgressing(c.rollout, &newStatus): // If there is any progress made, continue by not checking if the rollout failed. This // behavior emulates the rolling updater progressDeadline check. diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go index 871ab9f866..4ac20c9e52 100644 --- a/utils/conditions/conditions.go +++ b/utils/conditions/conditions.go @@ -87,6 +87,8 @@ const ( // within the given deadline (progressDeadlineSeconds). ReplicaSetTimeOutMessage = "ReplicaSet %q has timed out progressing." + // RolloutCompletedReason is added in a rollout when it is completed. + RolloutCompletedReason = "RolloutCompleted" // RolloutCompletedMessage is added when the rollout is completed RolloutCompletedMessage = "Rollout %q has successfully progressed." // ReplicaSetCompletedMessage is added when the rollout is completed From 3649ba52df0bdb176ee9959c66820783f85e1c49 Mon Sep 17 00:00:00 2001 From: khhirani Date: Thu, 8 Apr 2021 15:19:21 -0700 Subject: [PATCH 08/11] Modify sync.go Signed-off-by: khhirani --- rollout/sync.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/rollout/sync.go b/rollout/sync.go index 15784ef19f..88a3947301 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -571,6 +571,18 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt // a new rollout and this is a resync where we don't need to estimate any progress. // In such a case, we should simply not estimate any progress for this rollout. currentCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutProgressing) + + completeCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutCompleted) + if conditions.RolloutComplete(c.rollout, &newStatus) { + completedCondition := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) + conditions.SetRolloutCondition(&newStatus, *completedCondition) + } else { + if completeCond != nil { + completedCondition := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionFalse, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) + conditions.SetRolloutCondition(&newStatus, *completedCondition) + } + } + isCompleteRollout := newStatus.Replicas == newStatus.AvailableReplicas && currentCond != nil && currentCond.Reason == conditions.NewRSAvailableReason // Check for progress only if the latest rollout hasn't completed yet. if !isCompleteRollout { @@ -592,8 +604,6 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt } progressingCondition := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionTrue, conditions.NewRSAvailableReason, msg) conditions.SetRolloutCondition(&newStatus, *progressingCondition) - completedCondition := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, conditions.RolloutCompletedReason, msg) - conditions.SetRolloutCondition(&newStatus, *completedCondition) case conditions.RolloutProgressing(c.rollout, &newStatus): // If there is any progress made, continue by not checking if the rollout failed. This // behavior emulates the rolling updater progressDeadline check. From 96eacc21bb875488b946929b0f0fadd59bdd1077 Mon Sep 17 00:00:00 2001 From: khhirani Date: Thu, 8 Apr 2021 23:49:04 -0700 Subject: [PATCH 09/11] Create tests Signed-off-by: khhirani --- rollout/analysis_test.go | 2 +- rollout/bluegreen_test.go | 2 +- rollout/controller_test.go | 30 ++++++++++++++++++ test/e2e/functional_test.go | 61 +++++++++++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 2 deletions(-) diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index ff6b01b0d7..b1bf69bc28 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -1628,7 +1628,7 @@ func TestDoNotCreatePrePromotionAnalysisAfterPromotionRollout(t *testing.T) { f.run(getKey(r2, t)) - newConditions := generateConditionsPatch(true, conditions.NewRSAvailableReason, rs2, true, "") + newConditions := generateConditionsPatchWithComplete(true, conditions.NewRSAvailableReason, rs2, true, "", true) expectedPatch := fmt.Sprintf(`{ "status":{ "conditions":%s diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 64a8c8d02b..b93d8aa6a3 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -1014,7 +1014,7 @@ func TestBlueGreenRolloutCompleted(t *testing.T) { f.run(getKey(r2, t)) - newConditions := generateConditionsPatch(true, conditions.NewRSAvailableReason, rs2, true, "") + newConditions := generateConditionsPatchWithComplete(true, conditions.NewRSAvailableReason, rs2, true, "", true) expectedPatch := fmt.Sprintf(`{ "status":{ "conditions":%s diff --git a/rollout/controller_test.go b/rollout/controller_test.go index c3c7157db0..80ef5af626 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -185,6 +185,26 @@ func newPausedCondition(isPaused bool) (v1alpha1.RolloutCondition, string) { return condition, string(conditionBytes) } +func newCompletedCondition(isPaused bool) (v1alpha1.RolloutCondition, string) { + status := corev1.ConditionTrue + if !isPaused { + status = corev1.ConditionFalse + } + condition := v1alpha1.RolloutCondition{ + LastTransitionTime: metav1.Now(), + LastUpdateTime: metav1.Now(), + 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 := "" @@ -307,6 +327,16 @@ func generateConditionsPatchWithPause(available bool, progressingReason string, return fmt.Sprintf("[%s, %s, %s]", progressingCondition, pauseCondition, 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 := newCompletedCondition(isCompleted) + if availableConditionFirst { + return fmt.Sprintf("[%s, %s, %s]", availableCondition, completeCondition, progressingCondition) + } + return fmt.Sprintf("[%s, %s, %s]", completeCondition, progressingCondition, availableCondition) +} + // 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 { newRollout := updateBaseRolloutStatus(r, availableReplicas, updatedReplicas, totalReplicas, hpaReplicas) diff --git a/test/e2e/functional_test.go b/test/e2e/functional_test.go index 4bab71b3d6..81ded0cbbf 100644 --- a/test/e2e/functional_test.go +++ b/test/e2e/functional_test.go @@ -904,3 +904,64 @@ spec: }). ExpectActiveRevision("2") } + +func (s *FunctionalSuite) TestKubectlWaitForCompleted() { + s.Given(). + RolloutObjects(` +kind: Service +apiVersion: v1 +metadata: + name: rollout-bluegreen-active +spec: + selector: + app: rollout-bluegreen + ports: + - protocol: TCP + port: 80 + targetPort: 8080 +--- +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +metadata: + name: rollout-bluegreen +spec: + replicas: 1 + revisionHistoryLimit: 2 + selector: + matchLabels: + app: rollout-bluegreen + template: + metadata: + labels: + app: rollout-bluegreen + spec: + containers: + - name: rollouts-demo + image: argoproj/rollouts-demo:blue + imagePullPolicy: Always + ports: + - containerPort: 8080 + strategy: + blueGreen: + activeService: rollout-bluegreen-active + autoPromotionEnabled: true +`). + When(). + ApplyManifests(). + WaitForRolloutReplicas(1). + WaitForRolloutStatus("Healthy"). + UpdateSpec(). + Then(). + ExpectRollout("Completed", func(r *v1alpha1.Rollout) bool { + cmd := exec.Command("kubectl", "wait", "--for=condition=Completed=False", fmt.Sprintf("rollout/%s", r.Name)) + return cmd.Run() == nil + }). + When(). + PromoteRollout(). + Then(). + ExpectRollout("Completed", func(r *v1alpha1.Rollout) bool { + cmd := exec.Command("kubectl", "wait", "--for=condition=Completed=True", fmt.Sprintf("rollout/%s", r.Name)) + return cmd.Run() == nil + }). + ExpectActiveRevision("2") +} From 1998e40e6bd711821a03ebdbabb1dd547ebf32ce Mon Sep 17 00:00:00 2001 From: khhirani Date: Tue, 13 Apr 2021 20:53:16 -0700 Subject: [PATCH 10/11] Fix/add unit tests --- rollout/bluegreen_test.go | 44 ++++++++++++++++++++++++++++++++++++++ rollout/controller_test.go | 4 ++-- rollout/sync.go | 26 +++++++++++----------- 3 files changed, 60 insertions(+), 14 deletions(-) diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index b93d8aa6a3..42cfe06579 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -1,6 +1,7 @@ package rollout import ( + "encoding/json" "fmt" "strconv" "testing" @@ -1024,6 +1025,49 @@ func TestBlueGreenRolloutCompleted(t *testing.T) { assert.Equal(t, cleanPatch(expectedPatch), patch) } +func TestBlueGreenRolloutCompletedFalse(t *testing.T) { + f := newFixture(t) + defer f.Close() + + r1 := newBlueGreenRollout("foo", 1, nil, "bar", "") + completedCondition, _ := newCompletedCondition(true) + conditions.SetRolloutCondition(&r1.Status, completedCondition) + + r2 := bumpVersion(r1) + rs2 := newReplicaSetWithStatus(r2, 1, 1) + progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") + conditions.SetRolloutCondition(&r2.Status, progressingCondition) + pausedCondition, _ := newPausedCondition(true) + conditions.SetRolloutCondition(&r2.Status, pausedCondition) + + f.kubeobjects = append(f.kubeobjects, rs2) + f.replicaSetLister = append(f.replicaSetLister, rs2) + rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + + serviceSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} + s := newService("bar", 80, serviceSelector, r2) + f.kubeobjects = append(f.kubeobjects, s) + + r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 1, 1, true, false) + r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) + + f.rolloutLister = append(f.rolloutLister, r2) + f.objects = append(f.objects, r2) + f.serviceLister = append(f.serviceLister, s) + + patchIndex := f.expectPatchRolloutAction(r1) + f.run(getKey(r2, t)) + + patch := f.getPatchedRollout(patchIndex) + rolloutPatch := v1alpha1.Rollout{} + err := json.Unmarshal([]byte(patch), &rolloutPatch) + assert.NoError(t, err) + + index := len(rolloutPatch.Status.Conditions)-1 + assert.Equal(t, v1alpha1.RolloutCompleted, rolloutPatch.Status.Conditions[index].Type) + assert.Equal(t, corev1.ConditionFalse, rolloutPatch.Status.Conditions[index].Status) +} + func TestBlueGreenUnableToReadScaleDownAt(t *testing.T) { f := newFixture(t) defer f.Close() diff --git a/rollout/controller_test.go b/rollout/controller_test.go index 80ef5af626..315e6b3919 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -185,9 +185,9 @@ func newPausedCondition(isPaused bool) (v1alpha1.RolloutCondition, string) { return condition, string(conditionBytes) } -func newCompletedCondition(isPaused bool) (v1alpha1.RolloutCondition, string) { +func newCompletedCondition(isCompleted bool) (v1alpha1.RolloutCondition, string) { status := corev1.ConditionTrue - if !isPaused { + if !isCompleted { status = corev1.ConditionFalse } condition := v1alpha1.RolloutCondition{ diff --git a/rollout/sync.go b/rollout/sync.go index 88a3947301..afe34de84b 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -563,7 +563,20 @@ func isIndefiniteStep(r *v1alpha1.Rollout) bool { } func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutStatus) v1alpha1.RolloutStatus { - if len(c.rollout.Status.PauseConditions) > 0 || c.rollout.Spec.Paused { + isPaused := len(c.rollout.Status.PauseConditions) > 0 || c.rollout.Spec.Paused + + 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) + } else { + if completeCond != nil { + updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionFalse, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) + conditions.SetRolloutCondition(&newStatus, *updateCompletedCond) + } + } + + if isPaused { return newStatus } @@ -572,17 +585,6 @@ 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) - completeCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutCompleted) - if conditions.RolloutComplete(c.rollout, &newStatus) { - completedCondition := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) - conditions.SetRolloutCondition(&newStatus, *completedCondition) - } else { - if completeCond != nil { - completedCondition := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionFalse, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason) - conditions.SetRolloutCondition(&newStatus, *completedCondition) - } - } - isCompleteRollout := newStatus.Replicas == newStatus.AvailableReplicas && currentCond != nil && currentCond.Reason == conditions.NewRSAvailableReason // Check for progress only if the latest rollout hasn't completed yet. if !isCompleteRollout { From c85bf710b6f439df3ee612df7d4d0157b8855ed4 Mon Sep 17 00:00:00 2001 From: khhirani Date: Tue, 13 Apr 2021 20:53:52 -0700 Subject: [PATCH 11/11] Make lint Signed-off-by: khhirani --- rollout/bluegreen_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 42cfe06579..438d7e67c5 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -1063,7 +1063,7 @@ func TestBlueGreenRolloutCompletedFalse(t *testing.T) { err := json.Unmarshal([]byte(patch), &rolloutPatch) assert.NoError(t, err) - index := len(rolloutPatch.Status.Conditions)-1 + index := len(rolloutPatch.Status.Conditions) - 1 assert.Equal(t, v1alpha1.RolloutCompleted, rolloutPatch.Status.Conditions[index].Type) assert.Equal(t, corev1.ConditionFalse, rolloutPatch.Status.Conditions[index].Status) }