diff --git a/rollout/sync.go b/rollout/sync.go index f590cec8ee..6f250c075d 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -655,17 +655,23 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt } condition := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionTrue, reason, msg) - // Update the current Progressing condition or add a new one if it doesn't exist. - // If a Progressing condition with status=true already exists, we should update - // everything but lastTransitionTime. SetRolloutCondition already does that but - // it also is not updating conditions when the reason of the new condition is the - // same as the old. The Progressing condition is a special case because we want to - // update with the same reason and change just lastUpdateTime if we notice any - // progress. That's why we handle it here. + // Only reset the progress deadline (LastUpdateTime) when there is structural/scaling + // progress or when the condition doesn't exist yet. ReadyReplicas/AvailableReplicas + // oscillation (e.g., from crash-looping pods) should NOT reset the deadline, as this + // would prevent ProgressDeadlineExceeded from ever firing. + scalingProgress := conditions.RolloutScalingProgress(c.rollout, &newStatus) + readinessOnlyProgress := !scalingProgress && !becameUnhealthy + if currentCond != nil { if currentCond.Status == corev1.ConditionTrue { condition.LastTransitionTime = currentCond.LastTransitionTime } + if readinessOnlyProgress && currentCond.Reason == conditions.ReplicaSetUpdatedReason { + // Readiness-only progress (ReadyReplicas/AvailableReplicas went up) should not + // reset the deadline timer when we're already tracking progress. This prevents + // crash-looping pods from indefinitely deferring the timeout. + condition.LastUpdateTime = currentCond.LastUpdateTime + } conditions.RemoveRolloutCondition(&newStatus, v1alpha1.RolloutProgressing) } conditions.SetRolloutCondition(&newStatus, *condition) diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go index 57d8c0f02f..6e5851cafe 100644 --- a/utils/conditions/conditions.go +++ b/utils/conditions/conditions.go @@ -251,6 +251,13 @@ func filterOutCondition(conditions []v1alpha1.RolloutCondition, condType v1alpha // when new pods are scaled up, become ready or available, old pods are scaled down, or we modify the // services, then we consider the rollout is progressing. func RolloutProgressing(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) bool { + return RolloutScalingProgress(rollout, newStatus) || RolloutReadinessProgress(rollout, newStatus) +} + +// RolloutScalingProgress returns true when there is structural progress: replicas being scaled +// up/down, strategy-specific changes (step advancement, selector switches, stable RS changes). +// This type of progress unconditionally resets the progress deadline. +func RolloutScalingProgress(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) bool { oldStatus := rollout.Status strategySpecificProgress := false @@ -276,11 +283,18 @@ func RolloutProgressing(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutSt return (newStatus.UpdatedReplicas != oldStatus.UpdatedReplicas) || (newStatusOldReplicas < oldStatusOldReplicas) || - newStatus.ReadyReplicas > rollout.Status.ReadyReplicas || - newStatus.AvailableReplicas > rollout.Status.AvailableReplicas || strategySpecificProgress } +// RolloutReadinessProgress returns true when pods have become ready or available compared to +// the last observed status. This type of progress does NOT reset the progress deadline when +// the Progressing condition already exists, to prevent crash-looping pods from indefinitely +// resetting the deadline through ReadyReplicas/AvailableReplicas oscillation. +func RolloutReadinessProgress(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) bool { + return newStatus.ReadyReplicas > rollout.Status.ReadyReplicas || + newStatus.AvailableReplicas > rollout.Status.AvailableReplicas +} + // RolloutHealthy considers a rollout to be healthy once all of its desired replicas // are updated, available, and receiving traffic from the active service, and no old pods are running. func RolloutHealthy(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) bool { diff --git a/utils/conditions/rollouts_test.go b/utils/conditions/rollouts_test.go index 6f42f15640..562944f1ee 100644 --- a/utils/conditions/rollouts_test.go +++ b/utils/conditions/rollouts_test.go @@ -350,6 +350,106 @@ func TestRolloutProgressing(t *testing.T) { } +func TestRolloutProgressSplit(t *testing.T) { + canaryRollout := func(current, updated, ready, available int32, stableRS string, index int32, stepHash string) *v1alpha1.Rollout { + return &v1alpha1.Rollout{ + Spec: v1alpha1.RolloutSpec{ + Strategy: v1alpha1.RolloutStrategy{ + Canary: &v1alpha1.CanaryStrategy{}, + }, + }, + Status: v1alpha1.RolloutStatus{ + Replicas: current, + UpdatedReplicas: updated, + ReadyReplicas: ready, + AvailableReplicas: available, + StableRS: stableRS, + CurrentStepIndex: &index, + CurrentStepHash: stepHash, + }, + } + } + + t.Run("ReadyReplicas oscillation is readiness-only progress", func(t *testing.T) { + // Simulate crash-loop: ReadyReplicas goes from 9 to 10 (pod restarted and briefly ready) + rollout := canaryRollout(10, 1, 9, 9, "stable", 1, "abc") + newStatus := v1alpha1.RolloutStatus{ + Replicas: 10, + UpdatedReplicas: 1, + ReadyReplicas: 10, + AvailableReplicas: 9, + StableRS: "stable", + CurrentStepIndex: ptr.To[int32](1), + CurrentStepHash: "abc", + } + assert.True(t, RolloutProgressing(rollout, &newStatus)) + assert.False(t, RolloutScalingProgress(rollout, &newStatus)) + assert.True(t, RolloutReadinessProgress(rollout, &newStatus)) + }) + + t.Run("UpdatedReplicas change is scaling progress", func(t *testing.T) { + rollout := canaryRollout(10, 1, 10, 10, "stable", 1, "abc") + newStatus := v1alpha1.RolloutStatus{ + Replicas: 10, + UpdatedReplicas: 2, + ReadyReplicas: 10, + AvailableReplicas: 10, + StableRS: "stable", + CurrentStepIndex: ptr.To[int32](1), + CurrentStepHash: "abc", + } + assert.True(t, RolloutProgressing(rollout, &newStatus)) + assert.True(t, RolloutScalingProgress(rollout, &newStatus)) + }) + + t.Run("Step index change is scaling progress", func(t *testing.T) { + rollout := canaryRollout(10, 1, 10, 10, "stable", 1, "abc") + newStatus := v1alpha1.RolloutStatus{ + Replicas: 10, + UpdatedReplicas: 1, + ReadyReplicas: 10, + AvailableReplicas: 10, + StableRS: "stable", + CurrentStepIndex: ptr.To[int32](2), + CurrentStepHash: "abc", + } + assert.True(t, RolloutProgressing(rollout, &newStatus)) + assert.True(t, RolloutScalingProgress(rollout, &newStatus)) + }) + + t.Run("No change means no progress", func(t *testing.T) { + rollout := canaryRollout(10, 1, 10, 10, "stable", 1, "abc") + newStatus := v1alpha1.RolloutStatus{ + Replicas: 10, + UpdatedReplicas: 1, + ReadyReplicas: 10, + AvailableReplicas: 10, + StableRS: "stable", + CurrentStepIndex: ptr.To[int32](1), + CurrentStepHash: "abc", + } + assert.False(t, RolloutProgressing(rollout, &newStatus)) + assert.False(t, RolloutScalingProgress(rollout, &newStatus)) + assert.False(t, RolloutReadinessProgress(rollout, &newStatus)) + }) + + t.Run("AvailableReplicas oscillation is readiness-only progress", func(t *testing.T) { + rollout := canaryRollout(10, 1, 9, 9, "stable", 1, "abc") + newStatus := v1alpha1.RolloutStatus{ + Replicas: 10, + UpdatedReplicas: 1, + ReadyReplicas: 9, + AvailableReplicas: 10, + StableRS: "stable", + CurrentStepIndex: ptr.To[int32](1), + CurrentStepHash: "abc", + } + assert.True(t, RolloutProgressing(rollout, &newStatus)) + assert.False(t, RolloutScalingProgress(rollout, &newStatus)) + assert.True(t, RolloutReadinessProgress(rollout, &newStatus)) + }) +} + func TestRolloutHealthy(t *testing.T) { rollout := func(desired, current, updated, available int32, correctObservedGeneration bool) *v1alpha1.Rollout { r := &v1alpha1.Rollout{