Skip to content

Commit b7f948b

Browse files
committed
fix(controller): honor scaleUpPreviewCheckPoint reset on blueGreen pod template change
When a new pod template is rolled out while the previous revision had already completed prePromotionAnalysis (and thus set status.blueGreen.scaleUpPreviewCheckPoint=true), the new revision inherits that value and Argo Rollouts skips creating an AnalysisRun for it entirely -- including the prePromotionAnalysis gate. syncRolloutStatusBlueGreen calls resetRolloutStatus(&newStatus) on pod template change, which clears newStatus.BlueGreen.ScaleUpPreviewCheckPoint. The subsequent call to calculateScaleUpPreviewCheckPoint read prevValue from c.rollout.Status (the pre-reset value) rather than the newStatus parameter that was just reset, immediately clobbering the reset. Read prevValue from newStatus so the reset applied earlier in the same reconcile is honored, matching the documented behavior: // ... gets reset to false when the pod template changes, or the // rollout fully promotes (stableRS == newRS) Add a regression test that fails on the old behavior and passes with the fix. Signed-off-by: Abizer Lokhandwala <abizer@abizer.me>
1 parent 2ccdae8 commit b7f948b

2 files changed

Lines changed: 46 additions & 2 deletions

File tree

rollout/bluegreen.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -326,8 +326,11 @@ func (c *rolloutContext) calculateScaleUpPreviewCheckPoint(newStatus v1alpha1.Ro
326326
}
327327

328328
// Once the ScaleUpPreviewCheckPoint is set to true, the rollout should keep that value until
329-
// the newRS becomes the new stableRS or there is a template change.
330-
prevValue := c.rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint
329+
// the newRS becomes the new stableRS or there is a template change. Read from newStatus rather
330+
// than c.rollout.Status so that a reset applied earlier in this reconcile (e.g. by
331+
// resetRolloutStatus on a pod template change) is honored instead of being clobbered by the
332+
// pre-reset value.
333+
prevValue := newStatus.BlueGreen.ScaleUpPreviewCheckPoint
331334
if prevValue {
332335
return true
333336
}

rollout/bluegreen_test.go

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"github.com/argoproj/argo-rollouts/utils/annotations"
2121
"github.com/argoproj/argo-rollouts/utils/conditions"
2222
"github.com/argoproj/argo-rollouts/utils/hash"
23+
logutil "github.com/argoproj/argo-rollouts/utils/log"
2324
rolloututil "github.com/argoproj/argo-rollouts/utils/rollout"
2425
timeutil "github.com/argoproj/argo-rollouts/utils/time"
2526
)
@@ -1193,6 +1194,46 @@ func TestPreviewReplicaCountHandleScaleUpPreviewCheckPoint(t *testing.T) {
11931194
})
11941195
}
11951196

1197+
// TestCalculateScaleUpPreviewCheckPointHonorsReset is a regression test for
1198+
// https://github.com/argoproj/argo-rollouts/issues/<TBD>.
1199+
//
1200+
// When a new pod template arrives while the previous revision had already completed
1201+
// prePromotionAnalysis, syncRolloutStatusBlueGreen calls resetRolloutStatus(&newStatus),
1202+
// which sets newStatus.BlueGreen.ScaleUpPreviewCheckPoint=false. The subsequent call to
1203+
// calculateScaleUpPreviewCheckPoint must honor that reset — i.e. read prevValue from
1204+
// newStatus, not from c.rollout.Status — otherwise the new revision inherits the
1205+
// previous revision's scaleUpPreviewCheckPoint=true and skips its own prePromotionAnalysis.
1206+
func TestCalculateScaleUpPreviewCheckPointHonorsReset(t *testing.T) {
1207+
r := newBlueGreenRollout("foo", 5, nil, "active", "")
1208+
r.Spec.Strategy.BlueGreen.PreviewReplicaCount = ptr.To[int32](3)
1209+
r.Spec.Strategy.BlueGreen.PrePromotionAnalysis = &v1alpha1.RolloutAnalysis{
1210+
Templates: []v1alpha1.AnalysisTemplateRef{{TemplateName: "test"}},
1211+
}
1212+
// The previous revision's status carried ScaleUpPreviewCheckPoint=true (its
1213+
// prePromotionAnalysis had succeeded). c.rollout.Status still reflects that value
1214+
// at the start of this reconcile.
1215+
r.Status.BlueGreen.ScaleUpPreviewCheckPoint = true
1216+
newRS := newReplicaSetWithStatus(r, 0, 0)
1217+
1218+
c := &rolloutContext{
1219+
rollout: r,
1220+
newRS: newRS,
1221+
log: logutil.WithRollout(r),
1222+
pauseContext: &pauseContext{rollout: r},
1223+
}
1224+
1225+
// resetRolloutStatus has already cleared the checkpoint on newStatus for this
1226+
// reconcile. With no AnalysisRun yet for the new revision (currentArs is empty),
1227+
// completedPrePromotionAnalysis() is false, so prePromotion must run before the
1228+
// checkpoint can flip. The function must read prevValue from newStatus rather than
1229+
// the stale c.rollout.Status, then fall through and return false.
1230+
newStatus := v1alpha1.RolloutStatus{}
1231+
newStatus.BlueGreen.ScaleUpPreviewCheckPoint = false
1232+
1233+
assert.False(t, c.calculateScaleUpPreviewCheckPoint(newStatus),
1234+
"scaleUpPreviewCheckPoint must be reset on pod template change so the new revision's prePromotionAnalysis runs")
1235+
}
1236+
11961237
func TestBlueGreenRolloutIgnoringScalingUsePreviewRSCount(t *testing.T) {
11971238
f := newFixture(t)
11981239
defer f.Close()

0 commit comments

Comments
 (0)