add tests for getReplicaSetFraction in the deployment controller (kubernetes#128535)

* better name variables in deployment_util * add tests for getReplicaSetFraction in the deployment controller - make validation more robust and make sure we do not divide by 0

Author: atiratree

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

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)
 }
 

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)
+}
+})
+}
+}