fix: loop when paused and completed (#4134)

* fix: loop when paused and completed Signed-off-by: Zach Aller <[email protected]> * fix: loop when paused and completed Signed-off-by: Zach Aller <[email protected]> * fix: loop when paused and completed Signed-off-by: Zach Aller <[email protected]> * cleanup Signed-off-by: Zach Aller <[email protected]> * add e2e tests Signed-off-by: Zach Aller <[email protected]> * fix: loop when paused and completed Signed-off-by: Zach Aller <[email protected]> * fix: loop when paused and completed Signed-off-by: Zach Aller <[email protected]> * fix: loop when paused and completed Signed-off-by: Zach Aller <[email protected]> * cleanup Signed-off-by: Zach Aller <[email protected]> * add e2e tests Signed-off-by: Zach Aller <[email protected]> * add comments Signed-off-by: Zach Aller <[email protected]> * use function for completed Signed-off-by: Zach Aller <[email protected]> * remove un-used variable Signed-off-by: Zach Aller <[email protected]> --------- Signed-off-by: Zach Aller <[email protected]>
argoproj · Feb 21, 2025 · 04c62f6 · 04c62f6
1 parent abeaca1
commit 04c62f6
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 7 deletions.
diff --git a/rollout/sync.go b/rollout/sync.go
@@ -492,7 +492,7 @@ func (c *rolloutContext) checkPausedConditions() error {
 
 	var updatedConditions []*v1alpha1.RolloutCondition
 
-	if (isPaused != progCondPaused) && !abortCondExists {
+	if (isPaused != progCondPaused) && !abortCondExists && !conditions.RolloutCompleted(&c.rollout.Status) {
 		if isPaused {
 			updatedConditions = append(updatedConditions, conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.RolloutPausedReason, conditions.RolloutPausedMessage))
 		} else {
@@ -719,7 +719,7 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt
 		conditions.RemoveRolloutCondition(&newStatus, v1alpha1.RolloutReplicaFailure)
 	}
 
-	if conditions.RolloutCompleted(c.rollout, &newStatus) {
+	if conditions.RolloutCompleted(&newStatus) {
 		// The event gets triggered in function promoteStable
 		updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue,
 			conditions.RolloutCompletedReason, conditions.RolloutCompletedReason)

diff --git a/test/e2e/canary_test.go b/test/e2e/canary_test.go
@@ -4,6 +4,7 @@
 package e2e
 
 import (
+	"github.com/argoproj/argo-rollouts/utils/conditions"
 	"log"
 	"testing"
 	"time"
@@ -632,7 +633,7 @@ func (s *CanarySuite) TestCanaryDynamicStableScale() {
 		ApplyManifests().
 		MarkPodsReady("1", 4). // mark all 4 pods ready
 		WaitForRolloutStatus("Healthy").
-		UpdateSpec().          // update to revision 2
+		UpdateSpec(). // update to revision 2
 		MarkPodsReady("2", 1). // mark 1 of 1 canary pods ready
 		WaitForRolloutStatus("Paused").
 		Sleep(2*time.Second).
@@ -708,7 +709,7 @@ func (s *CanarySuite) TestCanaryDynamicStableScaleRollbackToStable() {
 			assert.Equal(s.T(), int32(75), ro.Status.Canary.Weights.Stable.Weight)
 		}).
 		When().
-		MarkPodsReady("3", 1).           // marks the 4th pod of stableRS/newRS (revision 3) ready
+		MarkPodsReady("3", 1). // marks the 4th pod of stableRS/newRS (revision 3) ready
 		WaitForRevisionPodCount("2", 0). // make sure we scale down the previous desired (revision 2)
 		Then().
 		Assert(func(t *fixtures.Then) {
@@ -727,3 +728,56 @@ func (s *CanarySuite) TestCanaryDynamicStableScaleRollbackToStable() {
 
 		})
 }
+
+// TestCanaryWithSpecPause This tests that we do not go into a update loop where we flap back updating condition patches.
+func (s *CanarySuite) TestCanaryWithSpecPause() {
+	s.Given().
+		RolloutObjects(`
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+metadata:
+  name: rollout-canary-with-pause
+spec:
+  replicas: 3
+  revisionHistoryLimit: 2
+  progressDeadlineSeconds: 5
+  selector:
+    matchLabels:
+      app: rollout-canary-with-pause
+  template:
+    metadata:
+      labels:
+        app: rollout-canary-with-pause
+    spec:
+      containers:
+      - name: rollouts-demo
+        image: nginx:1.19-alpine
+        ports:
+        - containerPort: 80
+        readinessProbe:
+          initialDelaySeconds: 10
+          httpGet:
+            path: /
+            port: 80
+          periodSeconds: 30
+  strategy:
+    canary:
+      steps:
+      - setWeight: 20
+`).
+		When().
+		ApplyManifests().
+		WaitForRolloutStatus("Degraded").
+		WaitForRolloutStatus("Healthy").
+		WaitForRolloutReplicas(3).
+		UpdateSpec(`{"spec":{"paused": true}}`).
+		WaitForRolloutConditionToNotExist(func(ro *v1alpha1.Rollout) bool {
+			for _, c := range ro.Status.Conditions {
+				if c.Type == v1alpha1.RolloutProgressing && c.Reason == conditions.RolloutPausedReason && c.Status == corev1.ConditionUnknown {
+					return true
+				}
+			}
+			return false
+		}, "ProgressingPausedUnknown", 5*time.Second).
+		WaitForRolloutStatus("Paused")
+}
diff --git a/test/fixtures/when.go b/test/fixtures/when.go
@@ -469,6 +469,45 @@ func (w *When) WaitForRolloutCondition(test func(ro *rov1.Rollout) bool, conditi
 	}
 }
 
+// WaitForRolloutConditionToNotExist this function will check for the condition to exist for the given duration, if it is found
+// the test fails.
+func (w *When) WaitForRolloutConditionToNotExist(test func(ro *rov1.Rollout) bool, condition string, timeout time.Duration) *When {
+	start := time.Now()
+	w.log.Infof("Waiting for condition to not exist: %s", condition)
+	rolloutIf := w.dynamicClient.Resource(rov1.RolloutGVR).Namespace(w.namespace)
+	ro, err := rolloutIf.Get(w.Context, w.rollout.GetName(), metav1.GetOptions{})
+	w.CheckError(err)
+	retryWatcher, err := watchutil.NewRetryWatcher(ro.GetResourceVersion(), &cache.ListWatch{
+		WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) {
+			opts := metav1.ListOptions{FieldSelector: fields.ParseSelectorOrDie(fmt.Sprintf("metadata.name=%s", w.rollout.GetName())).String()}
+			return w.rolloutClient.ArgoprojV1alpha1().Rollouts(w.namespace).Watch(w.Context, opts)
+		},
+	})
+	w.CheckError(err)
+	defer retryWatcher.Stop()
+	timeoutCh := make(chan bool, 1)
+	go func() {
+		time.Sleep(timeout)
+		timeoutCh <- true
+	}()
+	for {
+		select {
+		case event := <-retryWatcher.ResultChan():
+			ro, ok := event.Object.(*rov1.Rollout)
+			if ok {
+				if test(ro) {
+					//w.PrintRollout(ro)
+					w.log.Infof("Condition '%s' met after %v", condition, time.Since(start).Truncate(time.Second))
+					w.t.Fatal("not ok")
+				}
+			}
+		case <-timeoutCh:
+			w.t.Logf("Condition %s not found after %v", condition, timeout)
+			return w
+		}
+	}
+}
+
 func (w *When) DeleteRollout() *When {
 	w.log.Info("Deleting")
 	err := w.rolloutClient.ArgoprojV1alpha1().Rollouts(w.namespace).Delete(w.Context, w.rollout.GetName(), metav1.DeleteOptions{})

diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go
@@ -313,7 +313,7 @@ func RolloutHealthy(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus
 }
 
 // RolloutCompleted considers a rollout to be complete once StableRS == CurrentPodHash
-func RolloutCompleted(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) bool {
+func RolloutCompleted(newStatus *v1alpha1.RolloutStatus) bool {
 	return newStatus.StableRS != "" && newStatus.StableRS == newStatus.CurrentPodHash
 }
 

diff --git a/utils/conditions/rollouts_test.go b/utils/conditions/rollouts_test.go
@@ -499,10 +499,10 @@ func TestRolloutComplete(t *testing.T) {
 		return r
 	}
 	r := rollout(5, 5, 5, 5)
-	assert.Equal(t, true, RolloutCompleted(r, &r.Status))
+	assert.Equal(t, true, RolloutCompleted(&r.Status))
 
 	r.Status.StableRS = "not-current-pod-hash"
-	assert.Equal(t, false, RolloutCompleted(r, &r.Status))
+	assert.Equal(t, false, RolloutCompleted(&r.Status))
 }
 
 func TestRolloutTimedOut(t *testing.T) {