From 5f21c6c709dcb6bf19e6ba5612469fcc6b1e2bdd Mon Sep 17 00:00:00 2001 From: Maciej Szulik Date: Thu, 15 Jan 2026 13:50:30 +0100 Subject: [PATCH] Eliminate duplicate scale methods deployment controller in favor of a parameter This initially was attempted in 5f083e3b9f59e27029c08f8f79f70fa809909160 but it caused e2e failing heavily (see https://github.com/kubernetes/kubernetes/issues/135222 for more details). The changes proposed in e6641cd290b9331394ba5189bbd8c82d6fb7dbd1 clarified the naming of the two existing scale methods but they still leave both of them as is. This change goes a step further by combining both into a single method with a parameter `forceUpdate` which is responsible for driving the mode of execution between lazy update and a forced update. Signed-off-by: Maciej Szulik --- pkg/controller/deployment/recreate.go | 4 ++-- pkg/controller/deployment/rolling.go | 8 ++++---- pkg/controller/deployment/sync.go | 27 +++++++++++---------------- 3 files changed, 17 insertions(+), 22 deletions(-) diff --git a/pkg/controller/deployment/recreate.go b/pkg/controller/deployment/recreate.go index f6225249418..8b8b80a0905 100644 --- a/pkg/controller/deployment/recreate.go +++ b/pkg/controller/deployment/recreate.go @@ -83,7 +83,7 @@ func (dc *DeploymentController) scaleDownOldReplicaSetsForRecreate(ctx context.C if *(rs.Spec.Replicas) == 0 { continue } - scaledRS, updatedRS, err := dc.scaleReplicaSetWithLazyAnnotationUpdate(ctx, rs, 0, deployment) + scaledRS, updatedRS, err := dc.scaleReplicaSet(ctx, rs, 0, deployment, false) if err != nil { return false, err } @@ -127,6 +127,6 @@ func oldPodsRunning(newRS *apps.ReplicaSet, oldRSs []*apps.ReplicaSet, podMap ma // scaleUpNewReplicaSetForRecreate scales up new replica set when deployment strategy is "Recreate". func (dc *DeploymentController) scaleUpNewReplicaSetForRecreate(ctx context.Context, newRS *apps.ReplicaSet, deployment *apps.Deployment) (bool, error) { - scaled, _, err := dc.scaleReplicaSetWithLazyAnnotationUpdate(ctx, newRS, *(deployment.Spec.Replicas), deployment) + scaled, _, err := dc.scaleReplicaSet(ctx, newRS, *(deployment.Spec.Replicas), deployment, false) return scaled, err } diff --git a/pkg/controller/deployment/rolling.go b/pkg/controller/deployment/rolling.go index b0b833bf0ed..d32584881b1 100644 --- a/pkg/controller/deployment/rolling.go +++ b/pkg/controller/deployment/rolling.go @@ -72,14 +72,14 @@ func (dc *DeploymentController) reconcileNewReplicaSet(ctx context.Context, allR } if *(newRS.Spec.Replicas) > *(deployment.Spec.Replicas) { // Scale down. - scaled, _, err := dc.scaleReplicaSetWithLazyAnnotationUpdate(ctx, newRS, *(deployment.Spec.Replicas), deployment) + scaled, _, err := dc.scaleReplicaSet(ctx, newRS, *(deployment.Spec.Replicas), deployment, false) return scaled, err } newReplicasCount, err := deploymentutil.NewRSNewReplicas(deployment, allRSs, newRS) if err != nil { return false, err } - scaled, _, err := dc.scaleReplicaSetWithLazyAnnotationUpdate(ctx, newRS, newReplicasCount, deployment) + scaled, _, err := dc.scaleReplicaSet(ctx, newRS, newReplicasCount, deployment, false) return scaled, err } @@ -178,7 +178,7 @@ func (dc *DeploymentController) cleanupUnhealthyReplicas(ctx context.Context, ol if newReplicasCount > *(targetRS.Spec.Replicas) { return nil, 0, fmt.Errorf("when cleaning up unhealthy replicas, got invalid request to scale down %s/%s %d -> %d", targetRS.Namespace, targetRS.Name, *(targetRS.Spec.Replicas), newReplicasCount) } - _, updatedOldRS, err := dc.scaleReplicaSetWithLazyAnnotationUpdate(ctx, targetRS, newReplicasCount, deployment) + _, updatedOldRS, err := dc.scaleReplicaSet(ctx, targetRS, newReplicasCount, deployment, false) if err != nil { return nil, totalScaledDown, err } @@ -223,7 +223,7 @@ func (dc *DeploymentController) scaleDownOldReplicaSetsForRollingUpdate(ctx cont if newReplicasCount > *(targetRS.Spec.Replicas) { return 0, fmt.Errorf("when scaling down old RS, got invalid request to scale down %s/%s %d -> %d", targetRS.Namespace, targetRS.Name, *(targetRS.Spec.Replicas), newReplicasCount) } - _, _, err := dc.scaleReplicaSetWithLazyAnnotationUpdate(ctx, targetRS, newReplicasCount, deployment) + _, _, err := dc.scaleReplicaSet(ctx, targetRS, newReplicasCount, deployment, false) if err != nil { return totalScaledDown, err } diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index 2112f2b8467..d2700981b51 100644 --- a/pkg/controller/deployment/sync.go +++ b/pkg/controller/deployment/sync.go @@ -311,7 +311,7 @@ func (dc *DeploymentController) scale(ctx context.Context, deployment *apps.Depl if *(activeOrLatest.Spec.Replicas) == *(deployment.Spec.Replicas) { return nil } - _, _, err := dc.scaleReplicaSetWithLazyAnnotationUpdate(ctx, activeOrLatest, *(deployment.Spec.Replicas), deployment) + _, _, err := dc.scaleReplicaSet(ctx, activeOrLatest, *(deployment.Spec.Replicas), deployment, false) return err } @@ -319,7 +319,7 @@ func (dc *DeploymentController) scale(ctx context.Context, deployment *apps.Depl // This case handles replica set adoption during a saturated new replica set. if deploymentutil.IsSaturated(deployment, newRS) { for _, old := range controller.FilterActiveReplicaSets(oldRSs) { - if _, _, err := dc.scaleReplicaSetWithLazyAnnotationUpdate(ctx, old, 0, deployment); err != nil { + if _, _, err := dc.scaleReplicaSet(ctx, old, 0, deployment, false); err != nil { return err } } @@ -391,7 +391,7 @@ func (dc *DeploymentController) scale(ctx context.Context, deployment *apps.Depl } // TODO: Use transactions when we have them. - if _, _, err := dc.scaleReplicaSet(ctx, rs, nameToSize[rs.Name], deployment); err != nil { + if _, _, err := dc.scaleReplicaSet(ctx, rs, nameToSize[rs.Name], deployment, true); err != nil { // Return as soon as we fail, the deployment is requeued return err } @@ -400,23 +400,18 @@ func (dc *DeploymentController) scale(ctx context.Context, deployment *apps.Depl return nil } -// scaleReplicaSetWithLazyAnnotationUpdate does not update the replica set annotations (DesiredReplicasAnnotation and -// MaxReplicasAnnotation) if no replica set scaling is requested. -// IMPORTANT: This method should not be called when an annotation update is necessary (e.g. scale during a rolling update) -// and scaleReplicaSet should be used instead. -func (dc *DeploymentController) scaleReplicaSetWithLazyAnnotationUpdate(ctx context.Context, rs *apps.ReplicaSet, newScale int32, deployment *apps.Deployment) (bool, *apps.ReplicaSet, error) { - // No need to scale - if *(rs.Spec.Replicas) == newScale { +// scaleReplicaSet acts in two modes: +// 1. lazy update - when forceUpdate is false, it will only act when the replicas +// where changed; +// 2. normal - either replicaset annotations (DesiredReplicasAnnotation and +// MaxReplicasAnnotation) require an update or the replicas have changed. +func (dc *DeploymentController) scaleReplicaSet(ctx context.Context, rs *apps.ReplicaSet, newScale int32, deployment *apps.Deployment, forceUpdate bool) (bool, *apps.ReplicaSet, error) { + // Don't scale, unless it's a forced update or the replicas actually differ + if !forceUpdate && *(rs.Spec.Replicas) == newScale { return false, rs, nil } - scaled, newRS, err := dc.scaleReplicaSet(ctx, rs, newScale, deployment) - return scaled, newRS, err -} - -func (dc *DeploymentController) scaleReplicaSet(ctx context.Context, rs *apps.ReplicaSet, newScale int32, deployment *apps.Deployment) (bool, *apps.ReplicaSet, error) { sizeNeedsUpdate := *(rs.Spec.Replicas) != newScale - annotationsNeedUpdate := deploymentutil.ReplicasAnnotationsNeedUpdate(rs, *(deployment.Spec.Replicas), *(deployment.Spec.Replicas)+deploymentutil.MaxSurge(*deployment)) scaled := false