diff --git a/controller/bluegreen.go b/controller/bluegreen.go index eb83cd1afe..8f4afadeff 100644 --- a/controller/bluegreen.go +++ b/controller/bluegreen.go @@ -5,7 +5,6 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - patchtypes "k8s.io/apimachinery/pkg/types" "k8s.io/kubernetes/pkg/controller" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" @@ -16,14 +15,6 @@ import ( replicasetutil "github.com/argoproj/argo-rollouts/utils/replicaset" ) -const ( - verifyingPreviewPatch = `{ - "status": { - "verifyingPreview": true - } -}` -) - // rolloutBlueGreen implements the logic for rolling a new replica set. func (c *Controller) rolloutBlueGreen(r *v1alpha1.Rollout, rsList []*appsv1.ReplicaSet) error { logCtx := logutil.WithRollout(r) @@ -45,7 +36,7 @@ func (c *Controller) rolloutBlueGreen(r *v1alpha1.Rollout, rsList []*appsv1.Repl } if scaledUp { logCtx.Infof("Not finished reconciling new ReplicaSet '%s'", newRS.Name) - return c.syncRolloutStatusBlueGreen(allRSs, newRS, previewSvc, activeSvc, r) + return c.syncRolloutStatusBlueGreen(allRSs, newRS, previewSvc, activeSvc, r, false) } if previewSvc != nil { @@ -56,15 +47,17 @@ func (c *Controller) rolloutBlueGreen(r *v1alpha1.Rollout, rsList []*appsv1.Repl } if switchPreviewSvc { logCtx.Infof("Not finished reconciling preview service' %s'", previewSvc.Name) - return c.syncRolloutStatusBlueGreen(allRSs, newRS, previewSvc, activeSvc, r) - } - logCtx.Info("Reconciling verifying preview service") - verfyingPreview := c.reconcileVerifyingPreview(activeSvc, r) - if verfyingPreview { - logCtx.Info("Not finished reconciling verifying preview service") - return c.syncRolloutStatusBlueGreen(allRSs, newRS, previewSvc, activeSvc, r) + return c.syncRolloutStatusBlueGreen(allRSs, newRS, previewSvc, activeSvc, r, true) } } + + logCtx.Info("Reconciling pause before switching active service") + pauseBeforeSwitchActive := c.reconcileBlueGreenPause(activeSvc, r) + if pauseBeforeSwitchActive { + logCtx.Info("Not finished reconciling pause before switching active service") + return c.syncRolloutStatusBlueGreen(allRSs, newRS, previewSvc, activeSvc, r, true) + } + logCtx.Infof("Reconciling active service '%s'", activeSvc.Name) switchActiveSvc, err := c.reconcileActiveService(r, newRS, previewSvc, activeSvc) if err != nil { @@ -72,8 +65,9 @@ func (c *Controller) rolloutBlueGreen(r *v1alpha1.Rollout, rsList []*appsv1.Repl } if switchActiveSvc { logCtx.Infof("Not Finished reconciling active service '%s'", activeSvc.Name) - return c.syncRolloutStatusBlueGreen(allRSs, newRS, previewSvc, activeSvc, r) + return c.syncRolloutStatusBlueGreen(allRSs, newRS, previewSvc, activeSvc, r, false) } + // Scale down, if we can. logCtx.Info("Reconciling old replica sets") scaledDown, err := c.reconcileOldReplicaSets(allRSs, controller.FilterActiveReplicaSets(oldRSs), newRS, r) @@ -82,8 +76,9 @@ func (c *Controller) rolloutBlueGreen(r *v1alpha1.Rollout, rsList []*appsv1.Repl } if scaledDown { logCtx.Info("Not finished reconciling old replica sets") - return c.syncRolloutStatusBlueGreen(allRSs, newRS, previewSvc, activeSvc, r) + return c.syncRolloutStatusBlueGreen(allRSs, newRS, previewSvc, activeSvc, r, false) } + logCtx.Infof("Confirming rollout is complete") if conditions.RolloutComplete(r, &r.Status) { logCtx.Info("Cleaning up old replicasets") @@ -91,10 +86,10 @@ func (c *Controller) rolloutBlueGreen(r *v1alpha1.Rollout, rsList []*appsv1.Repl return err } } - return c.syncRolloutStatusBlueGreen(allRSs, newRS, previewSvc, activeSvc, r) + return c.syncRolloutStatusBlueGreen(allRSs, newRS, previewSvc, activeSvc, r, false) } -func (c *Controller) reconcileVerifyingPreview(activeSvc *corev1.Service, rollout *v1alpha1.Rollout) bool { +func (c *Controller) reconcileBlueGreenPause(activeSvc *corev1.Service, rollout *v1alpha1.Rollout) bool { if rollout.Spec.Strategy.BlueGreenStrategy.PreviewService == "" { return false } @@ -102,11 +97,7 @@ func (c *Controller) reconcileVerifyingPreview(activeSvc *corev1.Service, rollou return false } - if rollout.Status.VerifyingPreview == nil { - return false - } - - return *rollout.Status.VerifyingPreview + return rollout.Spec.Paused && rollout.Status.PauseStartTime != nil } // scaleDownOldReplicaSetsForBlueGreen scales down old replica sets when rollout strategy is "Blue Green". @@ -140,13 +131,7 @@ func (c *Controller) scaleDownOldReplicaSetsForBlueGreen(allRSs []*appsv1.Replic return totalScaledDown, nil } -func (c *Controller) setVerifyingPreview(r *v1alpha1.Rollout) error { - logutil.WithRollout(r).Infof("Patching setVerifyingPreview to true") - _, err := c.rolloutsclientset.ArgoprojV1alpha1().Rollouts(r.Namespace).Patch(r.Name, patchtypes.MergePatchType, []byte(verifyingPreviewPatch)) - return err -} - -func (c *Controller) syncRolloutStatusBlueGreen(allRSs []*appsv1.ReplicaSet, newRS *appsv1.ReplicaSet, previewSvc *corev1.Service, activeSvc *corev1.Service, r *v1alpha1.Rollout) error { +func (c *Controller) syncRolloutStatusBlueGreen(allRSs []*appsv1.ReplicaSet, newRS *appsv1.ReplicaSet, previewSvc *corev1.Service, activeSvc *corev1.Service, r *v1alpha1.Rollout, addPause bool) error { newStatus := c.calculateBaseStatus(allRSs, newRS, r) previewSelector, ok := c.getRolloutSelectorLabel(previewSvc) if !ok { @@ -170,9 +155,10 @@ func (c *Controller) syncRolloutStatusBlueGreen(allRSs []*appsv1.ReplicaSet, new conditions.SetRolloutCondition(&prevStatus, *noAvailability) } newStatus.Conditions = prevStatus.Conditions - newStatus.VerifyingPreview = r.Status.VerifyingPreview - return c.persistRolloutStatus(r, &newStatus, nil) + pauseStartTime, paused := calculatePauseStatus(r, addPause) + newStatus.PauseStartTime = pauseStartTime + return c.persistRolloutStatus(r, &newStatus, &paused) } // Should run only on scaling events and not during the normal rollout process. diff --git a/controller/bluegreen_test.go b/controller/bluegreen_test.go index 3491cfd896..11f3d62a12 100644 --- a/controller/bluegreen_test.go +++ b/controller/bluegreen_test.go @@ -2,18 +2,14 @@ package controller import ( "encoding/json" + "fmt" "testing" - "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - k8sfake "k8s.io/client-go/kubernetes/fake" - "k8s.io/client-go/tools/record" "k8s.io/kubernetes/pkg/controller" - "k8s.io/utils/pointer" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" - "github.com/argoproj/argo-rollouts/pkg/client/clientset/versioned/fake" "github.com/argoproj/argo-rollouts/utils/annotations" "github.com/argoproj/argo-rollouts/utils/conditions" ) @@ -22,13 +18,12 @@ var ( noTimestamp = metav1.Time{} ) -func newBlueGreenRollout(name string, replicas int, revisionHistoryLimit *int32, stepIndex *int32, activeSvc string, previewSvc string) *v1alpha1.Rollout { +func newBlueGreenRollout(name string, replicas int, revisionHistoryLimit *int32, activeSvc string, previewSvc string) *v1alpha1.Rollout { rollout := newRollout(name, replicas, revisionHistoryLimit, map[string]string{"foo": "bar"}) rollout.Spec.Strategy.BlueGreenStrategy = &v1alpha1.BlueGreenStrategy{ ActiveService: activeSvc, PreviewService: previewSvc, } - rollout.Status.CurrentStepIndex = stepIndex rollout.Status.CurrentStepHash = conditions.ComputeStepHash(rollout) rollout.Status.CurrentPodHash = controller.ComputeHash(&rollout.Spec.Template, rollout.Status.CollisionCount) return rollout @@ -54,73 +49,10 @@ func newAvailableCondition(available bool) ([]v1alpha1.RolloutCondition, string) return rc, string(rcStr) } -func TestBlueGreenReconcileVerifyingPreview(t *testing.T) { - boolPtr := func(boolean bool) *bool { return &boolean } - tests := []struct { - name string - activeSvc *corev1.Service - previewSvcName string - verifyingPreviewFlag *bool - notFinishedVerifying bool - }{ - { - name: "Continue if preview Service isn't specificed", - activeSvc: newService("active", 80, nil), - verifyingPreviewFlag: boolPtr(true), - notFinishedVerifying: false, - }, - { - name: "Continue if active service doesn't have a selector from the rollout", - previewSvcName: "previewSvc", - activeSvc: newService("active", 80, nil), - verifyingPreviewFlag: boolPtr(true), - notFinishedVerifying: false, - }, - { - name: "Do not continue if verifyingPreview flag is true", - previewSvcName: "previewSvc", - activeSvc: newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: "test"}), - verifyingPreviewFlag: boolPtr(true), - notFinishedVerifying: true, - }, - { - name: "Continue if verifyingPreview flag is false", - previewSvcName: "previewSvc", - activeSvc: newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: "test"}), - verifyingPreviewFlag: boolPtr(false), - notFinishedVerifying: false, - }, - { - name: "Continue if verifyingPreview flag is not set", - previewSvcName: "previewSvc", - activeSvc: newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: "test"}), - notFinishedVerifying: false, - }, - } - for i := range tests { - test := tests[i] - t.Run(test.name, func(t *testing.T) { - rollout := newBlueGreenRollout("foo", 1, nil, nil, "", test.previewSvcName) - rollout.Status = v1alpha1.RolloutStatus{ - VerifyingPreview: test.verifyingPreviewFlag, - } - fake := fake.Clientset{} - k8sfake := k8sfake.Clientset{} - controller := &Controller{ - rolloutsclientset: &fake, - kubeclientset: &k8sfake, - recorder: &record.FakeRecorder{}, - } - finishedVerifying := controller.reconcileVerifyingPreview(test.activeSvc, rollout) - assert.Equal(t, test.notFinishedVerifying, finishedVerifying) - }) - } -} - func TestBlueGreenHandlePreviewWhenActiveSet(t *testing.T) { f := newFixture(t) - r1 := newBlueGreenRollout("foo", 1, nil, nil, "preview", "active") + r1 := newBlueGreenRollout("foo", 1, nil, "preview", "active") r2 := r1.DeepCopy() annotations.SetRolloutRevision(r2, "2") @@ -149,44 +81,10 @@ func TestBlueGreenHandlePreviewWhenActiveSet(t *testing.T) { f.run(getKey(r2, t)) } -func TestBlueGreenHandleVerifyingPreviewSetButNotPreviewSvc(t *testing.T) { - f := newFixture(t) - - r1 := newBlueGreenRollout("foo", 1, nil, nil, "active", "preview") - r2 := r1.DeepCopy() - annotations.SetRolloutRevision(r2, "2") - r2.Spec.Template.Spec.Containers[0].Image = "foo/bar2.0" - f.rolloutLister = append(f.rolloutLister, r2) - f.objects = append(f.objects, r2) - - rs1 := newReplicaSetWithStatus(r1, "foo-895c6c4f9", 1, 1) - f.kubeobjects = append(f.kubeobjects, rs1) - f.replicaSetLister = append(f.replicaSetLister, rs1) - - rs2 := newReplicaSetWithStatus(r2, "foo-6479c8f85c", 1, 1) - f.kubeobjects = append(f.kubeobjects, rs2) - f.replicaSetLister = append(f.replicaSetLister, rs2) - - previewSvc := newService("preview", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: ""}) - f.kubeobjects = append(f.kubeobjects, previewSvc) - - activeSvc := newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: "895c6c4f9"}) - f.kubeobjects = append(f.kubeobjects, activeSvc) - - r2.Status.VerifyingPreview = func(boolean bool) *bool { return &boolean }(true) - - f.expectGetServiceAction(previewSvc) - f.expectGetServiceAction(activeSvc) - f.expectPatchRolloutAction(r2) - f.expectPatchServiceAction(previewSvc, "") - f.expectPatchRolloutAction(r2) - f.run(getKey(r2, t)) -} - func TestBlueGreenCreatesReplicaSet(t *testing.T) { f := newFixture(t) - r := newBlueGreenRollout("foo", 1, nil, nil, "bar", "") + r := newBlueGreenRollout("foo", 1, nil, "bar", "") f.rolloutLister = append(f.rolloutLister, r) f.objects = append(f.objects, r) s := newService("bar", 80, nil) @@ -203,7 +101,7 @@ func TestBlueGreenCreatesReplicaSet(t *testing.T) { func TestBlueGreenSetPreviewService(t *testing.T) { f := newFixture(t) - r := newBlueGreenRollout("foo", 1, nil, pointer.Int32Ptr(0), "active", "preview") + r := newBlueGreenRollout("foo", 1, nil, "active", "preview") f.rolloutLister = append(f.rolloutLister, r) f.objects = append(f.objects, r) @@ -220,39 +118,174 @@ func TestBlueGreenSetPreviewService(t *testing.T) { f.expectGetServiceAction(previewSvc) f.expectPatchServiceAction(previewSvc, "") f.expectPatchRolloutAction(r) - f.expectPatchRolloutAction(r) f.run(getKey(r, t)) } -func TestBlueGreenVerifyPreviewNoActions(t *testing.T) { - f := newFixture(t) - - r := newBlueGreenRollout("foo", 1, nil, pointer.Int32Ptr(1), "active", "preview") - r.Status.VerifyingPreview = func(boolean bool) *bool { return &boolean }(true) - f.rolloutLister = append(f.rolloutLister, r) - f.objects = append(f.objects, r) - - rs := newReplicaSetWithStatus(r, "foo-895c6c4f9", 1, 1) - rs2 := newImage(rs, "foo/bar2.0") - f.kubeobjects = append(f.kubeobjects, rs, rs2) - f.replicaSetLister = append(f.replicaSetLister, rs, rs2) - - previewSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: "895c6c4f9"} - previewSvc := newService("preview", 80, previewSelector) - activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2.Name} - activeSvc := newService("active", 80, activeSelector) - f.kubeobjects = append(f.kubeobjects, previewSvc, activeSvc) - - f.expectGetServiceAction(activeSvc) - f.expectGetServiceAction(previewSvc) - f.expectPatchRolloutAction(r) - f.run(getKey(r, t)) +func TestBlueGreenHandlePause(t *testing.T) { + t.Run("NoActionsWhilePaused", func(t *testing.T) { + f := newFixture(t) + f.checkObjects = true + + r1 := newBlueGreenRollout("foo", 1, nil, "active", "preview") + r2 := bumpVersion(r1) + + rs1 := newReplicaSetWithStatus(r1, "foo-895c6c4f9", 1, 1) + rs2 := newReplicaSetWithStatus(r2, "foo-5f79b78d7f", 1, 1) + rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + + availableConditions, _ := newAvailableCondition(true) + now := metav1.Now() + r2.Spec.Paused = true + r2.Status.PauseStartTime = &now + r2.Status.Conditions = availableConditions + r2.Status.AvailableReplicas = 2 + r2.Status.BlueGreen.ActiveSelector = rs1PodHash + r2.Status.BlueGreen.PreviewSelector = rs2PodHash + + previewSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} + previewSvc := newService("preview", 80, previewSelector) + activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} + activeSvc := newService("active", 80, activeSelector) + + f.objects = append(f.objects, r2) + f.kubeobjects = append(f.kubeobjects, previewSvc, activeSvc, rs1, rs2) + f.rolloutLister = append(f.rolloutLister, r2) + f.replicaSetLister = append(f.replicaSetLister, rs1, rs2) + + f.expectGetServiceAction(activeSvc) + f.expectGetServiceAction(previewSvc) + f.expectPatchRolloutActionWithPatch(r2, OnlyObservedGenerationPatch) + f.run(getKey(r2, t)) + }) + + t.Run("SkipWhenNoPreviewSpecified", func(t *testing.T) { + f := newFixture(t) + f.checkObjects = true + + r1 := newBlueGreenRollout("foo", 1, nil, "active", "") + r2 := bumpVersion(r1) + + rs1 := newReplicaSetWithStatus(r1, "foo-895c6c4f9", 1, 1) + rs2 := newReplicaSetWithStatus(r2, "foo-5f79b78d7f", 1, 1) + rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + + availableConditions, _ := newAvailableCondition(true) + r2.Status.Conditions = availableConditions + r2.Status.AvailableReplicas = 2 + r2.Status.BlueGreen.ActiveSelector = rs1PodHash + + activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} + activeSvc := newService("active", 80, activeSelector) + + f.objects = append(f.objects, r2) + f.kubeobjects = append(f.kubeobjects, activeSvc, rs1, rs2) + f.rolloutLister = append(f.rolloutLister, r2) + f.replicaSetLister = append(f.replicaSetLister, rs1, rs2) + + f.expectGetServiceAction(activeSvc) + f.expectPatchServiceAction(activeSvc, rs2PodHash) + expectedPatchWithoutSubs := `{ + "status": { + "blueGreen": { + "activeSelector": "%s" + } + } + }` + expectedPatch := fmt.Sprintf(expectedPatchWithoutSubs, rs2PodHash) + f.expectPatchRolloutActionWithPatch(r2, expectedPatch) + f.run(getKey(r2, t)) + }) + + t.Run("SkipNoActiveSelector", func(t *testing.T) { + f := newFixture(t) + f.checkObjects = true + + r1 := newBlueGreenRollout("foo", 1, nil, "active", "preview") + + rs1 := newReplicaSetWithStatus(r1, "foo-895c6c4f9", 1, 1) + rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + + availableConditions, _ := newAvailableCondition(false) + r1.Status.Conditions = availableConditions + r1.Status.AvailableReplicas = 1 + + activeSvc := newService("active", 80, nil) + previewSvc := newService("preview", 80, nil) + + f.objects = append(f.objects, r1) + f.kubeobjects = append(f.kubeobjects, activeSvc, previewSvc, rs1) + f.rolloutLister = append(f.rolloutLister, r1) + f.replicaSetLister = append(f.replicaSetLister, rs1) + + f.expectGetServiceAction(activeSvc) + f.expectGetServiceAction(previewSvc) + f.expectPatchServiceAction(activeSvc, rs1PodHash) + expectedPatchWithoutSubs := `{ + "status": { + "blueGreen": { + "activeSelector": "%s" + } + } + }` + expectedPatch := fmt.Sprintf(expectedPatchWithoutSubs, rs1PodHash) + f.expectPatchRolloutActionWithPatch(r1, expectedPatch) + f.run(getKey(r1, t)) + }) + + t.Run("ContinueAfterUnpaused", func(t *testing.T) { + f := newFixture(t) + f.checkObjects = true + + r1 := newBlueGreenRollout("foo", 1, nil, "active", "preview") + r2 := bumpVersion(r1) + + rs1 := newReplicaSetWithStatus(r1, "foo-895c6c4f9", 1, 1) + rs2 := newReplicaSetWithStatus(r2, "foo-5f79b78d7f", 1, 1) + rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + + availableConditions, _ := newAvailableCondition(true) + now := metav1.Now() + r2.Spec.Paused = false + r2.Status.PauseStartTime = &now + r2.Status.Conditions = availableConditions + r2.Status.AvailableReplicas = 2 + r2.Status.BlueGreen.ActiveSelector = rs1PodHash + r2.Status.BlueGreen.PreviewSelector = rs2PodHash + + activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} + activeSvc := newService("active", 80, activeSelector) + previewSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} + previewSvc := newService("preview", 80, previewSelector) + + f.objects = append(f.objects, r2) + f.kubeobjects = append(f.kubeobjects, activeSvc, previewSvc, rs1, rs2) + f.rolloutLister = append(f.rolloutLister, r2) + f.replicaSetLister = append(f.replicaSetLister, rs1, rs2) + + f.expectGetServiceAction(activeSvc) + f.expectGetServiceAction(previewSvc) + f.expectPatchServiceAction(activeSvc, rs2PodHash) + expectedPatchWithoutSubs := `{ + "status": { + "blueGreen": { + "activeSelector": "%s" + }, + "pauseStartTime": null + } + }` + expectedPatch := fmt.Sprintf(expectedPatchWithoutSubs, rs2PodHash) + f.expectPatchRolloutActionWithPatch(r2, expectedPatch) + f.run(getKey(r2, t)) + }) } func TestBlueGreenSkipPreviewUpdateActive(t *testing.T) { f := newFixture(t) - r := newBlueGreenRollout("foo", 1, nil, nil, "active", "preview") + r := newBlueGreenRollout("foo", 1, nil, "active", "preview") r.Status.AvailableReplicas = 1 f.rolloutLister = append(f.rolloutLister, r) f.objects = append(f.objects, r) @@ -275,7 +308,7 @@ func TestBlueGreenSkipPreviewUpdateActive(t *testing.T) { func TestBlueGreenScaleDownOldRS(t *testing.T) { f := newFixture(t) - r1 := newBlueGreenRollout("foo", 1, nil, pointer.Int32Ptr(3), "bar", "") + r1 := newBlueGreenRollout("foo", 1, nil, "bar", "") r2 := bumpVersion(r1) f.rolloutLister = append(f.rolloutLister, r2) diff --git a/controller/canary.go b/controller/canary.go index f6c1b8a44b..ca731d258e 100644 --- a/controller/canary.go +++ b/controller/canary.go @@ -2,11 +2,9 @@ package controller import ( "sort" - "time" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/kubernetes/pkg/controller" "k8s.io/utils/pointer" @@ -79,7 +77,7 @@ func (c *Controller) rolloutCanary(rollout *v1alpha1.Rollout, rsList []*appsv1.R } logCtx.Info("Reconciling Canary Pause") - stillReconciling, err := c.reconcilePause(oldRSs, newRS, stableRS, rollout) + stillReconciling, err := c.reconcileCanaryPause(rollout) if err != nil { return c.syncRolloutStatusCanary(oldRSs, newRS, stableRS, rollout) } @@ -101,7 +99,7 @@ func (c *Controller) reconcileStableRS(olderRSs []*appsv1.ReplicaSet, newRS *app return scaled, err } -func (c *Controller) reconcilePause(oldRSs []*appsv1.ReplicaSet, newRS *appsv1.ReplicaSet, stableRS *appsv1.ReplicaSet, rollout *v1alpha1.Rollout) (bool, error) { +func (c *Controller) reconcileCanaryPause(rollout *v1alpha1.Rollout) (bool, error) { logCtx := logutil.WithRollout(rollout) if len(rollout.Spec.Strategy.CanaryStrategy.Steps) == 0 { logCtx.Info("Rollout does not have any steps") @@ -118,20 +116,7 @@ func (c *Controller) reconcilePause(oldRSs []*appsv1.ReplicaSet, newRS *appsv1.R return false, nil } - if currentStep.Pause.Duration == nil { - return true, nil - } - - if rollout.Status.PauseStartTime != nil { - now := metav1.Now() - expiredTime := rollout.Status.PauseStartTime.Add(time.Duration(*currentStep.Pause.Duration) * time.Second) - nextResync := now.Add(c.resyncPeriod) - if nextResync.After(expiredTime) && expiredTime.After(now.Time) { - timeRemaining := expiredTime.Sub(now.Time) - logCtx.Infof("Enqueueing Rollout in %s seconds", timeRemaining.String()) - c.enqueueRolloutAfter(rollout, timeRemaining) - } - } + c.checkEnqueueRolloutDuringPause(rollout, *currentStep.Pause) return true, nil } @@ -207,19 +192,8 @@ func completedCurrentCanaryStep(olderRSs []*appsv1.ReplicaSet, newRS *appsv1.Rep if currentStep == nil { return false } - if currentStep.Pause != nil && currentStep.Pause.Duration != nil { - now := metav1.Now() - if r.Status.PauseStartTime != nil { - expiredTime := r.Status.PauseStartTime.Add(time.Duration(*currentStep.Pause.Duration) * time.Second) - if now.After(expiredTime) { - logCtx.Info("Rollout has waited the duration of the pause step") - return true - } - } - } - if currentStep.Pause != nil && currentStep.Pause.Duration == nil && r.Status.PauseStartTime != nil && !r.Spec.Paused { - logCtx.Info("Rollout has been unpaused") - return true + if currentStep.Pause != nil { + return completedPauseStep(r, currentStep.Pause) } if currentStep.SetWeight != nil && replicasetutil.AtDesiredReplicaCountsForCanary(r, newRS, stableRS, olderRSs) { logCtx.Info("Rollout has reached the desired state for the correct weight") @@ -304,18 +278,10 @@ func (c *Controller) syncRolloutStatusCanary(olderRSs []*appsv1.ReplicaSet, newR return c.persistRolloutStatus(r, &newStatus, pointer.BoolPtr(false)) } - pauseStartTime := r.Status.PauseStartTime - paused := r.Spec.Paused - if currentStep != nil && currentStep.Pause != nil { - now := metav1.Now() - if r.Status.PauseStartTime == nil && replicasetutil.AtDesiredReplicaCountsForCanary(r, newRS, stableRS, olderRSs) { - logCtx.Infof("Setting PauseStartTime to %s", now.UTC().Format(time.RFC3339)) - pauseStartTime = &now - paused = true - } - } - + addPause := currentStep.Pause != nil + pauseStartTime, paused := calculatePauseStatus(r, addPause) newStatus.PauseStartTime = pauseStartTime + newStatus.CurrentStepIndex = currentStepIndex return c.persistRolloutStatus(r, &newStatus, &paused) } diff --git a/controller/canary_test.go b/controller/canary_test.go index 9041dc881c..229433d849 100644 --- a/controller/canary_test.go +++ b/controller/canary_test.go @@ -66,14 +66,14 @@ func TestReconcileCanaryStepsHandleBaseCases(t *testing.T) { // Handle case with no steps r := newCanaryRollout("test", 1, nil, nil, nil, intstr.FromInt(0), intstr.FromInt(1)) - stepResult, err := controller.reconcilePause(nil, nil, nil, r) + stepResult, err := controller.reconcileCanaryPause(r) assert.Nil(t, err) assert.False(t, stepResult) assert.Len(t, fake.Actions(), 0) r2 := newCanaryRollout("test", 1, nil, []v1alpha1.CanaryStep{{SetWeight: int32Ptr(10)}}, nil, intstr.FromInt(0), intstr.FromInt(1)) r2.Status.CurrentStepIndex = int32Ptr(1) - stepResult, err = controller.reconcilePause(nil, nil, nil, r2) + stepResult, err = controller.reconcileCanaryPause(r2) assert.Nil(t, err) assert.False(t, stepResult) assert.Len(t, fake.Actions(), 0) @@ -180,17 +180,18 @@ func TestCanaryRolloutIncrementStepAfterUnPaused(t *testing.T) { f.expectPatchRolloutAction(r2) f.run(getKey(r2, t)) - + patchBytes := filterInformerActions(f.client.Actions())[0].(core.PatchAction).GetPatch() expectedPatchTemplate := `{ "status":{ "canary": { - "pauseStartTime": null + "stableRS":"5f79b78d7f" }, + "pauseStartTime": null, "currentStepIndex": 1 } }` expectedPatch := calculatePatch(r2, expectedPatchTemplate) - assert.Equal(t, expectedPatch, expectedPatch) + assert.Equal(t, expectedPatch, string(patchBytes)) } func TestCanaryRolloutUpdateStatusWhenAtEndOfSteps(t *testing.T) { @@ -682,6 +683,7 @@ func TestSyncRolloutWaitAddToQueue(t *testing.T) { r2 := bumpVersion(r1) r2.Status.AvailableReplicas = 10 + r2.Spec.Paused = true r2.Status.ObservedGeneration = conditions.ComputeGenerationHash(r2.Spec) now := metav1.Now() @@ -725,6 +727,7 @@ func TestSyncRolloutIgnoreWaitOutsideOfReconciliationPeriod(t *testing.T) { r2 := bumpVersion(r1) now := metav1.Now() r2.Status.PauseStartTime = &now + r2.Spec.Paused = true r2.Status.ObservedGeneration = conditions.ComputeGenerationHash(r2.Spec) r2.Status.AvailableReplicas = 10 f.rolloutLister = append(f.rolloutLister, r2) diff --git a/controller/controller_test.go b/controller/controller_test.go index 8153eff888..26d33da92d 100644 --- a/controller/controller_test.go +++ b/controller/controller_test.go @@ -411,7 +411,7 @@ func (f *fixture) expectPatchRolloutActionWithPatch(rollout *v1alpha1.Rollout, p func TestDontSyncRolloutsWithEmptyPodSelector(t *testing.T) { f := newFixture(t) - r := newBlueGreenRollout("foo", 1, nil, nil, "", "") + r := newBlueGreenRollout("foo", 1, nil, "", "") f.rolloutLister = append(f.rolloutLister, r) f.objects = append(f.objects, r) diff --git a/controller/pause.go b/controller/pause.go new file mode 100644 index 0000000000..cb2811d3c4 --- /dev/null +++ b/controller/pause.go @@ -0,0 +1,68 @@ +package controller + +import ( + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" + logutil "github.com/argoproj/argo-rollouts/utils/log" +) + +func completedPauseStep(rollout *v1alpha1.Rollout, pause *v1alpha1.RolloutPause) bool { + logCtx := logutil.WithRollout(rollout) + + if pause != nil && pause.Duration != nil { + now := metav1.Now() + if rollout.Status.PauseStartTime != nil { + expiredTime := rollout.Status.PauseStartTime.Add(time.Duration(*pause.Duration) * time.Second) + if now.After(expiredTime) { + logCtx.Info("Rollout has waited the duration of the pause step") + return true + } + } + } + if pause != nil && pause.Duration == nil && rollout.Status.PauseStartTime != nil && !rollout.Spec.Paused { + logCtx.Info("Rollout has been unpaused") + return true + } + return false +} + +func (c *Controller) checkEnqueueRolloutDuringPause(rollout *v1alpha1.Rollout, pause v1alpha1.RolloutPause) { + logCtx := logutil.WithRollout(rollout) + if pause.Duration == nil { + return + } + if rollout.Status.PauseStartTime == nil { + return + } + now := metav1.Now() + expiredTime := rollout.Status.PauseStartTime.Add(time.Duration(*pause.Duration) * time.Second) + nextResync := now.Add(c.resyncPeriod) + if nextResync.After(expiredTime) && expiredTime.After(now.Time) { + timeRemaining := expiredTime.Sub(now.Time) + logCtx.Infof("Enqueueing Rollout in %s seconds", timeRemaining.String()) + c.enqueueRolloutAfter(rollout, timeRemaining) + } +} + +// calculatePauseStatus finds the fields related to a pause step for a rollout. If the pause is nil, +// the rollout will use the previous values +func calculatePauseStatus(rollout *v1alpha1.Rollout, addPause bool) (*metav1.Time, bool) { + logCtx := logutil.WithRollout(rollout) + pauseStartTime := rollout.Status.PauseStartTime + paused := rollout.Spec.Paused + if !paused { + pauseStartTime = nil + } + if addPause { + if pauseStartTime == nil { + now := metav1.Now() + logCtx.Infof("Setting PauseStartTime to %s", now.UTC().Format(time.RFC3339)) + pauseStartTime = &now + paused = true + } + } + return pauseStartTime, paused +} diff --git a/controller/replicaset_test.go b/controller/replicaset_test.go index 5ba47dea48..98555d6bb8 100644 --- a/controller/replicaset_test.go +++ b/controller/replicaset_test.go @@ -152,7 +152,7 @@ func TestReconcileNewReplicaSet(t *testing.T) { test := tests[i] newRS := rs("foo-v2", test.newReplicas, nil, noTimestamp, nil) allRSs := []*appsv1.ReplicaSet{newRS} - rollout := newBlueGreenRollout("foo", test.rolloutReplicas, nil, nil, "", "") + rollout := newBlueGreenRollout("foo", test.rolloutReplicas, nil, "", "") fake := fake.Clientset{} k8sfake := k8sfake.Clientset{} controller := &Controller{ @@ -250,7 +250,7 @@ func TestReconcileOldReplicaSet(t *testing.T) { oldRS.Status.AvailableReplicas = int32(test.readyPodsFromOldRS) oldRSs := []*appsv1.ReplicaSet{oldRS} allRSs := []*appsv1.ReplicaSet{oldRS, newRS} - rollout := newBlueGreenRollout("foo", test.rolloutReplicas, nil, nil, "", "") + rollout := newBlueGreenRollout("foo", test.rolloutReplicas, nil, "", "") rollout.Spec.Selector = &metav1.LabelSelector{MatchLabels: newSelector} fake := fake.Clientset{} k8sfake := k8sfake.Clientset{} diff --git a/controller/service.go b/controller/service.go index c6d6bc4ffe..49b1189325 100644 --- a/controller/service.go +++ b/controller/service.go @@ -73,12 +73,7 @@ func (c *Controller) reconcilePreviewService(r *v1alpha1.Rollout, newRS *appsv1. } } - err := c.setVerifyingPreview(r) - if err != nil { - return false, err - } - - err = c.switchServiceSelector(previewSvc, newRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], r) + err := c.switchServiceSelector(previewSvc, newRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], r) if err != nil { return false, err } diff --git a/controller/service_test.go b/controller/service_test.go index 8c82eef4ed..f80cb204d0 100644 --- a/controller/service_test.go +++ b/controller/service_test.go @@ -157,9 +157,9 @@ func TestGetRolloutsForService(t *testing.T) { f := newFixture(t) s := newService("foo", 80, nil) - ro1 := newBlueGreenRollout("bar", 0, nil, nil, "active", "") + ro1 := newBlueGreenRollout("bar", 0, nil, "active", "") ro1.Spec.Strategy.BlueGreenStrategy.ActiveService = "foo" - ro2 := newBlueGreenRollout("baz", 0, nil, nil, "active", "") + ro2 := newBlueGreenRollout("baz", 0, nil, "active", "") ro2.Spec.Strategy.BlueGreenStrategy.ActiveService = "foo2" f.rolloutLister = append(f.rolloutLister, ro1, ro2) f.objects = append(f.objects, ro1, ro2) @@ -179,9 +179,9 @@ func TestHandleServiceEnqueueRollout(t *testing.T) { f := newFixture(t) s := newService("foo", 80, nil) - ro1 := newBlueGreenRollout("bar", 0, nil, nil, "active", "") + ro1 := newBlueGreenRollout("bar", 0, nil, "active", "") ro1.Spec.Strategy.BlueGreenStrategy.ActiveService = "foo" - ro2 := newBlueGreenRollout("baz", 0, nil, nil, "active", "") + ro2 := newBlueGreenRollout("baz", 0, nil, "active", "") ro2.Spec.Strategy.BlueGreenStrategy.ActiveService = "foo2" f.objects = append(f.objects, ro1, ro2) @@ -202,7 +202,7 @@ func TestHandleServiceNoAdditions(t *testing.T) { f := newFixture(t) s := newService("foo", 80, nil) - ro1 := newBlueGreenRollout("bar", 0, nil, nil, "active", "") + ro1 := newBlueGreenRollout("bar", 0, nil, "active", "") ro1.Spec.Strategy.BlueGreenStrategy.ActiveService = "notFoo" f.objects = append(f.objects, ro1) @@ -232,7 +232,7 @@ func TestUpdateServiceEnqueueRollout(t *testing.T) { oldSvc := newService("foo", 80, nil) newSvc := oldSvc.DeepCopy() newSvc.ResourceVersion = "2" - ro1 := newBlueGreenRollout("bar", 0, nil, nil, "active", "") + ro1 := newBlueGreenRollout("bar", 0, nil, "active", "") ro1.Spec.Strategy.BlueGreenStrategy.ActiveService = "foo" f.objects = append(f.objects, ro1) // Create the fixture but don't start it, diff --git a/controller/sync.go b/controller/sync.go index 53cd64d7e9..d39734108d 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -199,7 +199,7 @@ func (c *Controller) sync(r *v1alpha1.Rollout, rsList []*appsv1.ReplicaSet) erro return err } allRSs := append([]*appsv1.ReplicaSet{newRS}, oldRSs...) - return c.syncRolloutStatusBlueGreen(allRSs, newRS, previewSvc, activeSvc, r) + return c.syncRolloutStatusBlueGreen(allRSs, newRS, previewSvc, activeSvc, r, r.Spec.Paused) } return fmt.Errorf("no rollout strategy provided") } diff --git a/controller/sync_test.go b/controller/sync_test.go index 914faafbef..5e0b75c4f1 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -77,8 +77,8 @@ func TestScaleBlueGreen(t *testing.T) { }{ { name: "normal scaling event: 10 -> 12", - rollout: newBlueGreenRollout("foo", 12, nil, nil, "", ""), - oldRollout: newBlueGreenRollout("foo", 10, nil, nil, "", ""), + rollout: newBlueGreenRollout("foo", 12, nil, "", ""), + oldRollout: newBlueGreenRollout("foo", 10, nil, "", ""), newRS: rs("foo-v1", 10, nil, newTimestamp, nil), oldRSs: []*appsv1.ReplicaSet{}, @@ -90,8 +90,8 @@ func TestScaleBlueGreen(t *testing.T) { }, { name: "normal scaling event: 10 -> 5", - rollout: newBlueGreenRollout("foo", 5, nil, nil, "", ""), - oldRollout: newBlueGreenRollout("foo", 10, nil, nil, "", ""), + rollout: newBlueGreenRollout("foo", 5, nil, "", ""), + oldRollout: newBlueGreenRollout("foo", 10, nil, "", ""), newRS: rs("foo-v1", 10, nil, newTimestamp, nil), oldRSs: []*appsv1.ReplicaSet{}, @@ -103,8 +103,8 @@ func TestScaleBlueGreen(t *testing.T) { }, { name: "Scale up non-active latest Replicaset", - rollout: newBlueGreenRollout("foo", 5, nil, nil, "", ""), - oldRollout: newBlueGreenRollout("foo", 5, nil, nil, "", ""), + rollout: newBlueGreenRollout("foo", 5, nil, "", ""), + oldRollout: newBlueGreenRollout("foo", 5, nil, "", ""), newRS: rs("foo-v2", 0, nil, newTimestamp, nil), oldRSs: []*appsv1.ReplicaSet{rs("foo-v1", 0, nil, oldTimestamp, nil)}, @@ -337,7 +337,7 @@ func TestCleanupRollouts(t *testing.T) { for i := range tests { test := tests[i] t.Run(test.name, func(t *testing.T) { - r := newBlueGreenRollout("baz", 1, test.revisionHistoryLimit, nil, "", "") + r := newBlueGreenRollout("baz", 1, test.revisionHistoryLimit, "", "") fake := fake.Clientset{} k8sfake := k8sfake.Clientset{} c := &Controller{ diff --git a/manifests/crds/rollout-crd.yaml b/manifests/crds/rollout-crd.yaml index 2b022a685c..bda4c2135f 100644 --- a/manifests/crds/rollout-crd.yaml +++ b/manifests/crds/rollout-crd.yaml @@ -196,9 +196,4 @@ spec: that have the desired template spec. format: int32 type: integer - verifyingPreview: - description: VerifyingPreview indicates the rollout is verifying the - replicas set being served traffic from the preview service. User will - need to edit this field to continue the rollout. - type: boolean version: v1alpha1 diff --git a/manifests/install.yaml b/manifests/install.yaml index 8abcac67f1..60806baf85 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -197,11 +197,6 @@ spec: that have the desired template spec. format: int32 type: integer - verifyingPreview: - description: VerifyingPreview indicates the rollout is verifying the - replicas set being served traffic from the preview service. User will - need to edit this field to continue the rollout. - type: boolean version: v1alpha1 --- apiVersion: v1 diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index 70f97bd03e..050249ea87 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -197,11 +197,6 @@ spec: that have the desired template spec. format: int32 type: integer - verifyingPreview: - description: VerifyingPreview indicates the rollout is verifying the - replicas set being served traffic from the preview service. User will - need to edit this field to continue the rollout. - type: boolean version: v1alpha1 --- apiVersion: v1 diff --git a/pkg/apis/rollouts/v1alpha1/openapi_generated.go b/pkg/apis/rollouts/v1alpha1/openapi_generated.go index ca92e34644..8e236b1395 100644 --- a/pkg/apis/rollouts/v1alpha1/openapi_generated.go +++ b/pkg/apis/rollouts/v1alpha1/openapi_generated.go @@ -431,13 +431,6 @@ func schema_pkg_apis_rollouts_v1alpha1_RolloutStatus(ref common.ReferenceCallbac Format: "", }, }, - "verifyingPreview": { - SchemaProps: spec.SchemaProps{ - Description: "VerifyingPreview indicates the rollout is verifying the replicas set being served traffic from the preview service. User will need to edit this field to continue the rollout.", - Type: []string{"boolean"}, - Format: "", - }, - }, "replicas": { SchemaProps: spec.SchemaProps{ Description: "Total number of non-terminated pods targeted by this rollout (their labels match the selector).", diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index 628d0cb127..71dcfc5318 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -134,11 +134,8 @@ type RolloutStatus struct { CurrentPodHash string `json:"currentPodHash"` // CurrentStepHash the hash of the current list of steps for the current strategy. This is used to detect when the // list of current steps change - CurrentStepHash string `json:"currentStepHash,omitempty"` - // VerifyingPreview indicates the rollout is verifying the replicas set being served - // traffic from the preview service. User will need to edit this field to continue the rollout. // +optional - VerifyingPreview *bool `json:"verifyingPreview,omitempty"` + CurrentStepHash string `json:"currentStepHash,omitempty"` // Total number of non-terminated pods targeted by this rollout (their labels match the selector). // +optional Replicas int32 `json:"replicas"` diff --git a/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go index 07e613bcae..292bc7b1d8 100644 --- a/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go @@ -269,11 +269,6 @@ func (in *RolloutSpec) DeepCopy() *RolloutSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RolloutStatus) DeepCopyInto(out *RolloutStatus) { *out = *in - if in.VerifyingPreview != nil { - in, out := &in.VerifyingPreview, &out.VerifyingPreview - *out = new(bool) - **out = **in - } if in.CurrentStepIndex != nil { in, out := &in.CurrentStepIndex, &out.CurrentStepIndex *out = new(int32)