diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index 126bebe4bed..b03c3be8f11 100644 --- a/pkg/controller/deployment/sync.go +++ b/pkg/controller/deployment/sync.go @@ -360,7 +360,7 @@ func (dc *DeploymentController) scale(ctx context.Context, deployment *apps.Depl // Estimate proportions if we have replicas to add, otherwise simply populate // nameToSize with the current sizes for each replica set. if deploymentReplicasToAdd != 0 { - proportion := deploymentutil.GetProportion(logger, rs, *deployment, deploymentReplicasToAdd, deploymentReplicasAdded) + proportion := deploymentutil.GetReplicaSetProportion(logger, rs, *deployment, deploymentReplicasToAdd, deploymentReplicasAdded) nameToSize[rs.Name] = *(rs.Spec.Replicas) + proportion deploymentReplicasAdded += proportion diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index 7324d336770..341d0a9e4dc 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -474,10 +474,10 @@ func MaxSurge(deployment apps.Deployment) int32 { return maxSurge } -// GetProportion will estimate the proportion for the provided replica set using 1. the current size +// GetReplicaSetProportion will estimate the proportion for the provided replica set using 1. the current size // of the parent deployment, 2. the replica count that needs be added on the replica sets of the // deployment, and 3. the total replicas added in the replica sets of the deployment so far. -func GetProportion(logger klog.Logger, rs *apps.ReplicaSet, d apps.Deployment, deploymentReplicasToAdd, deploymentReplicasAdded int32) int32 { +func GetReplicaSetProportion(logger klog.Logger, rs *apps.ReplicaSet, d apps.Deployment, deploymentReplicasToAdd, deploymentReplicasAdded int32) int32 { if rs == nil || *(rs.Spec.Replicas) == 0 || deploymentReplicasToAdd == 0 || deploymentReplicasToAdd == deploymentReplicasAdded { return int32(0) } @@ -505,19 +505,25 @@ func getReplicaSetFraction(logger klog.Logger, rs apps.ReplicaSet, d apps.Deploy return -*(rs.Spec.Replicas) } - deploymentReplicas := *(d.Spec.Replicas) + MaxSurge(d) - annotatedReplicas, ok := getMaxReplicasAnnotation(logger, &rs) - if !ok { - // If we cannot find the annotation then fallback to the current deployment size. Note that this - // will not be an accurate proportion estimation in case other replica sets have different values + deploymentMaxReplicas := *(d.Spec.Replicas) + MaxSurge(d) + deploymentMaxReplicasBeforeScale, ok := getMaxReplicasAnnotation(logger, &rs) + if !ok || deploymentMaxReplicasBeforeScale == 0 { + // If we cannot find the annotation then fallback to the current deployment size. + // This can occur if someone tampers with the annotation (removes it, sets it to an invalid value, or to 0). + // Note that this will not be an accurate proportion estimation in case other replica sets have different values // which means that the deployment was scaled at some point but we at least will stay in limits - // due to the min-max comparisons in getProportion. - annotatedReplicas = d.Status.Replicas + // due to the min-max comparisons in GetReplicaSetProportion. + deploymentMaxReplicasBeforeScale = d.Status.Replicas + if deploymentMaxReplicasBeforeScale == 0 { + // Rare situation: missing annotation; some actor has removed it and pods are failing to be created. + return 0 + } } - // We should never proportionally scale up from zero which means rs.spec.replicas and annotatedReplicas - // will never be zero here. - newRSsize := (float64(*(rs.Spec.Replicas) * deploymentReplicas)) / float64(annotatedReplicas) + // We should never proportionally scale up from zero (see GetReplicaSetProportion) which means rs.spec.replicas will never be zero here. + scaleBase := *(rs.Spec.Replicas) + // deploymentMaxReplicasBeforeScale should normally be a positive value, and we have made sure that it is not a zero. + newRSsize := (float64(scaleBase * deploymentMaxReplicas)) / float64(deploymentMaxReplicasBeforeScale) return integer.RoundToInt32(newRSsize) - *(rs.Spec.Replicas) } diff --git a/pkg/controller/deployment/util/deployment_util_test.go b/pkg/controller/deployment/util/deployment_util_test.go index 486c738f99d..17554644eb9 100644 --- a/pkg/controller/deployment/util/deployment_util_test.go +++ b/pkg/controller/deployment/util/deployment_util_test.go @@ -1401,3 +1401,121 @@ func TestMinAvailable(t *testing.T) { }) } } + +func TestGetReplicaSetFraction(t *testing.T) { + tests := []struct { + name string + enableDeploymentPodReplacementPolicy bool + deploymentReplicas int32 + deploymentStatusReplicas int32 + deploymentMaxSurge int32 + rsReplicas int32 + rsAnnotations map[string]string + expectedFraction int32 + }{ + { + name: "empty deployment always scales to 0", + deploymentReplicas: 0, + rsReplicas: 10, + expectedFraction: -10, + }, + { + name: "unsynced deployment does not scale when max-replicas annotation is missing (removed by a 3rd party)", + deploymentReplicas: 10, + rsReplicas: 5, + expectedFraction: 0, + }, + { + name: "unsynced deployment does not scale when max-replicas annotation is incorrectly set to 0 (by a 3rd party)", + deploymentReplicas: 10, + rsReplicas: 5, + rsAnnotations: map[string]string{ + MaxReplicasAnnotation: "0", + }, + expectedFraction: 0, + }, + { + name: "scale up by 1/5 should increase RS replicas by 1/5 when max-replicas annotation is missing (removed by a 3rd party)", + deploymentReplicas: 120, + deploymentStatusReplicas: 100, + rsReplicas: 50, + expectedFraction: 10, + }, + { + name: "scale up by 1/5 should increase RS replicas by 1/5 when max-replicas annotation is incorrectly set to 0 (by a 3rd party)", + deploymentReplicas: 120, + deploymentStatusReplicas: 100, + rsReplicas: 50, + rsAnnotations: map[string]string{ + MaxReplicasAnnotation: "0", + }, + expectedFraction: 10, + }, + { + name: "scale up by 1/5 should increase RS replicas by 1/5", + deploymentReplicas: 120, + rsReplicas: 50, + rsAnnotations: map[string]string{ + MaxReplicasAnnotation: "100", + }, + expectedFraction: 10, + }, + { + name: "scale up with maxSurge by 1/5 should increase RS replicas approximately by 1/5", + deploymentReplicas: 120, + deploymentMaxSurge: 10, + rsReplicas: 50, + rsAnnotations: map[string]string{ + MaxReplicasAnnotation: "110", + }, + // expectedFraction is not the whole 1/5 (10) since maxSurge pods have to be taken into account + // and replica sets with these surge pods should proportionally scale as well during a rollout + expectedFraction: 9, + }, + { + name: "scale down by 1/6 should decrease RS replicas by 1/6", + deploymentReplicas: 10, + rsReplicas: 6, + rsAnnotations: map[string]string{ + MaxReplicasAnnotation: "12", + }, + expectedFraction: -1, + }, + { + name: "scale down with maxSurge by 1/6 should decrease RS replicas approximately by 1/6", + deploymentReplicas: 100, + deploymentMaxSurge: 10, + rsReplicas: 50, + rsAnnotations: map[string]string{ + MaxReplicasAnnotation: "130", + }, + expectedFraction: -8, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + logger, _ := ktesting.NewTestContext(t) + + tDeployment := generateDeployment("nginx") + tDeployment.Status.Replicas = test.deploymentStatusReplicas + tDeployment.Spec.Replicas = ptr.To(test.deploymentReplicas) + tDeployment.Spec.Strategy = apps.DeploymentStrategy{ + Type: apps.RollingUpdateDeploymentStrategyType, + RollingUpdate: &apps.RollingUpdateDeployment{ + MaxSurge: ptr.To(intstr.FromInt32(test.deploymentMaxSurge)), + MaxUnavailable: ptr.To(intstr.FromInt32(1)), + }, + } + + tRS := generateRS(tDeployment) + tRS.Annotations = test.rsAnnotations + tRS.Spec.Replicas = ptr.To(test.rsReplicas) + + fraction := getReplicaSetFraction(logger, tRS, tDeployment) + if test.expectedFraction != fraction { + t.Fatalf("expected fraction: %v, got:%v", test.expectedFraction, fraction) + } + }) + } +}