diff --git a/pkg/controllers/clusteroperator_controller.go b/pkg/controllers/clusteroperator_controller.go index c7d8768de..b796be37e 100644 --- a/pkg/controllers/clusteroperator_controller.go +++ b/pkg/controllers/clusteroperator_controller.go @@ -237,9 +237,16 @@ func isDeploymentRolloutStalled(deploy *appsv1.Deployment) bool { // reboots pods are evicted and rescheduled without a spec change, which bumps // NumberUnavailable transiently. Treating that as incomplete would cause // Progressing=True flapping while MCO rolls nodes at a later run level. +// +// We compare UpdatedNumberScheduled against CurrentNumberScheduled rather than +// DesiredNumberScheduled. DesiredNumberScheduled includes nodes that match the +// selector but don't yet have a pod (e.g. during MachineSet scale-up). Comparing +// against it would violate the API convention that operators must not report +// Progressing due to DaemonSets adjusting to new nodes from cluster scale-up. func isDaemonSetRolloutComplete(ds *appsv1.DaemonSet) bool { return ds.Status.ObservedGeneration >= ds.Generation && - ds.Status.UpdatedNumberScheduled >= ds.Status.DesiredNumberScheduled + ds.Status.CurrentNumberScheduled > 0 && + ds.Status.UpdatedNumberScheduled >= ds.Status.CurrentNumberScheduled } func (r *CloudOperatorReconciler) operandsConverged(ctx context.Context, resources []client.Object) (bool, error) { diff --git a/pkg/controllers/clusteroperator_controller_test.go b/pkg/controllers/clusteroperator_controller_test.go index e33be174f..db6af7e37 100644 --- a/pkg/controllers/clusteroperator_controller_test.go +++ b/pkg/controllers/clusteroperator_controller_test.go @@ -1307,12 +1307,13 @@ var _ = Describe("Rollout completion checks", func() { }) Context("isDaemonSetRolloutComplete", func() { - It("is not complete when UpdatedNumberScheduled < DesiredNumberScheduled", func() { + It("is not complete when UpdatedNumberScheduled < CurrentNumberScheduled", func() { ds := &appsv1.DaemonSet{} ds.Generation = 1 ds.Status = appsv1.DaemonSetStatus{ ObservedGeneration: 1, DesiredNumberScheduled: 3, + CurrentNumberScheduled: 3, UpdatedNumberScheduled: 1, } Expect(isDaemonSetRolloutComplete(ds)).To(BeFalse()) @@ -1324,6 +1325,7 @@ var _ = Describe("Rollout completion checks", func() { ds.Status = appsv1.DaemonSetStatus{ ObservedGeneration: 1, DesiredNumberScheduled: 3, + CurrentNumberScheduled: 3, UpdatedNumberScheduled: 3, } Expect(isDaemonSetRolloutComplete(ds)).To(BeFalse()) @@ -1335,6 +1337,7 @@ var _ = Describe("Rollout completion checks", func() { ds.Status = appsv1.DaemonSetStatus{ ObservedGeneration: 2, DesiredNumberScheduled: 3, + CurrentNumberScheduled: 3, UpdatedNumberScheduled: 3, } Expect(isDaemonSetRolloutComplete(ds)).To(BeTrue()) @@ -1343,20 +1346,18 @@ var _ = Describe("Rollout completion checks", func() { // NumberUnavailable and NumberMisscheduled are deliberately not checked. // During MCO-driven node reboots, DaemonSet pods are evicted and // rescheduled without a spec change. The pod hash is unchanged so - // UpdatedNumberScheduled stays at DesiredNumberScheduled and + // UpdatedNumberScheduled stays at CurrentNumberScheduled and // ObservedGeneration == Generation. Only NumberUnavailable bumps // transiently. Treating that as "not complete" would cause // Progressing=True flapping during upgrades (run level ~29 CCCMO // re-entering Progressing while run level ~90 MCO rolls nodes). - // See also: CNO uses an observed-stable-generation annotation for - // finer-grained tracking; our simpler check is sufficient because - // CCCMO's DaemonSet has already converged before CVO advances. It("is complete despite NumberUnavailable > 0 when generation and updated counts match (node reboot)", func() { ds := &appsv1.DaemonSet{} ds.Generation = 1 ds.Status = appsv1.DaemonSetStatus{ ObservedGeneration: 1, DesiredNumberScheduled: 3, + CurrentNumberScheduled: 3, UpdatedNumberScheduled: 3, NumberUnavailable: 1, } @@ -1369,10 +1370,42 @@ var _ = Describe("Rollout completion checks", func() { ds.Status = appsv1.DaemonSetStatus{ ObservedGeneration: 1, DesiredNumberScheduled: 3, + CurrentNumberScheduled: 3, UpdatedNumberScheduled: 3, NumberMisscheduled: 1, } Expect(isDaemonSetRolloutComplete(ds)).To(BeTrue(), "misscheduled pods are a scheduling concern, not a rollout concern") }) + + // OCPBUGS-98617: MachineSet scale-up increases DesiredNumberScheduled + // before pods are created on new nodes. Comparing against + // DesiredNumberScheduled caused Progressing=True during scaling. + // Per API convention, operators must not report Progressing due to + // DaemonSets adjusting to new nodes from cluster scale-up. + It("is complete during node scale-up when all existing pods are updated (OCPBUGS-98617)", func() { + ds := &appsv1.DaemonSet{} + ds.Generation = 1 + ds.Status = appsv1.DaemonSetStatus{ + ObservedGeneration: 1, + DesiredNumberScheduled: 5, + CurrentNumberScheduled: 3, + UpdatedNumberScheduled: 3, + } + Expect(isDaemonSetRolloutComplete(ds)).To(BeTrue(), + "new nodes without pods yet should not cause Progressing=True") + }) + + It("is not complete when no pods have been scheduled yet (initial deploy)", func() { + ds := &appsv1.DaemonSet{} + ds.Generation = 1 + ds.Status = appsv1.DaemonSetStatus{ + ObservedGeneration: 1, + DesiredNumberScheduled: 3, + CurrentNumberScheduled: 0, + UpdatedNumberScheduled: 0, + } + Expect(isDaemonSetRolloutComplete(ds)).To(BeFalse(), + "zero pods scheduled means rollout has not started") + }) }) })