Merge pull request #15257 from nikhiljindal/deploymentScaleDown

Auto commit by PR queue bot

pkg/controller/deployment/deployment_controller.go

@@ -102,7 +102,7 @@ func (d *DeploymentController) reconcileRollingUpdateDeployment(deployment exper
 	allRCs := append(oldRCs, newRC)
 
 	// Scale up, if we can.
-	scaledUp, err := d.scaleUp(allRCs, newRC, deployment)
+	scaledUp, err := d.reconcileNewRC(allRCs, newRC, deployment)
 	if err != nil {
 		return err
 	}
@@ -112,7 +112,7 @@ func (d *DeploymentController) reconcileRollingUpdateDeployment(deployment exper
 	}
 
 	// Scale down, if we can.
-	scaledDown, err := d.scaleDown(allRCs, oldRCs, newRC, deployment)
+	scaledDown, err := d.reconcileOldRCs(allRCs, oldRCs, newRC, deployment)
 	if err != nil {
 		return err
 	}
@@ -158,11 +158,17 @@ func (d *DeploymentController) getNewRC(deployment experimental.Deployment) (*ap
 	return createdRC, nil
 }
 
-func (d *DeploymentController) scaleUp(allRCs []*api.ReplicationController, newRC *api.ReplicationController, deployment experimental.Deployment) (bool, error) {
+func (d *DeploymentController) reconcileNewRC(allRCs []*api.ReplicationController, newRC *api.ReplicationController, deployment experimental.Deployment) (bool, error) {
 	if newRC.Spec.Replicas == deployment.Spec.Replicas {
-		// Scaling up not required.
+		// Scaling not required.
 		return false, nil
 	}
+	if newRC.Spec.Replicas > deployment.Spec.Replicas {
+		// Scale down.
+		_, err := d.scaleRCAndRecordEvent(newRC, deployment.Spec.Replicas, deployment)
+		return true, err
+	}
+	// Check if we can scale up.
 	maxSurge, isPercent, err := util.GetIntOrPercentValue(&deployment.Spec.Strategy.RollingUpdate.MaxSurge)
 	if err != nil {
 		return false, fmt.Errorf("invalid value for MaxSurge: %v", err)
@@ -172,7 +178,6 @@ func (d *DeploymentController) scaleUp(allRCs []*api.ReplicationController, newR
 	}
 	// Find the total number of pods
 	currentPodCount := deploymentUtil.GetReplicaCountForRCs(allRCs)
-	// Check if we can scale up.
 	maxTotalPods := deployment.Spec.Replicas + maxSurge
 	if currentPodCount >= maxTotalPods {
 		// Cannot scale up.
@@ -180,16 +185,14 @@ func (d *DeploymentController) scaleUp(allRCs []*api.ReplicationController, newR
 	}
 	// Scale up.
 	scaleUpCount := maxTotalPods - currentPodCount
+	// Do not exceed the number of desired replicas.
 	scaleUpCount = int(math.Min(float64(scaleUpCount), float64(deployment.Spec.Replicas-newRC.Spec.Replicas)))
 	newReplicasCount := newRC.Spec.Replicas + scaleUpCount
-	_, err = d.scaleRC(newRC, newReplicasCount)
+	_, err = d.scaleRCAndRecordEvent(newRC, newReplicasCount, deployment)
-	if err == nil {
-		d.eventRecorder.Eventf(&deployment, "ScalingRC", "Scaled up rc %s to %d", newRC.Name, newReplicasCount)
-	}
 	return true, err
 }
 
-func (d *DeploymentController) scaleDown(allRCs []*api.ReplicationController, oldRCs []*api.ReplicationController, newRC *api.ReplicationController, deployment experimental.Deployment) (bool, error) {
+func (d *DeploymentController) reconcileOldRCs(allRCs []*api.ReplicationController, oldRCs []*api.ReplicationController, newRC *api.ReplicationController, deployment experimental.Deployment) (bool, error) {
 	oldPodsCount := deploymentUtil.GetReplicaCountForRCs(oldRCs)
 	if oldPodsCount == 0 {
 		// Cant scale down further
@@ -227,11 +230,10 @@ func (d *DeploymentController) scaleDown(allRCs []*api.ReplicationController, ol
 		// Scale down.
 		scaleDownCount := int(math.Min(float64(targetRC.Spec.Replicas), float64(totalScaleDownCount)))
 		newReplicasCount := targetRC.Spec.Replicas - scaleDownCount
-		_, err = d.scaleRC(targetRC, newReplicasCount)
+		_, err = d.scaleRCAndRecordEvent(targetRC, newReplicasCount, deployment)
 		if err != nil {
 			return false, err
 		}
-		d.eventRecorder.Eventf(&deployment, "ScalingRC", "Scaled down rc %s to %d", targetRC.Name, newReplicasCount)
 		totalScaleDownCount -= scaleDownCount
 	}
 	return true, err
@@ -250,6 +252,18 @@ func (d *DeploymentController) updateDeploymentStatus(allRCs []*api.ReplicationC
 	return err
 }
 
+func (d *DeploymentController) scaleRCAndRecordEvent(rc *api.ReplicationController, newScale int, deployment experimental.Deployment) (*api.ReplicationController, error) {
+	scalingOperation := "down"
+	if rc.Spec.Replicas < newScale {
+		scalingOperation = "up"
+	}
+	newRC, err := d.scaleRC(rc, newScale)
+	if err == nil {
+		d.eventRecorder.Eventf(&deployment, "ScalingRC", "Scaled %s rc %s to %d", scalingOperation, rc.Name, newScale)
+	}
+	return newRC, err
+}
+
 func (d *DeploymentController) scaleRC(rc *api.ReplicationController, newScale int) (*api.ReplicationController, error) {
 	// TODO: Using client for now, update to use store when it is ready.
 	rc.Spec.Replicas = newScale

pkg/controller/deployment/deployment_controller_test.go

@@ -28,7 +28,7 @@ import (
 	"k8s.io/kubernetes/pkg/util"
 )
 
-func TestDeploymentController_scaleUp(t *testing.T) {
+func TestDeploymentController_reconcileNewRC(t *testing.T) {
 	tests := []struct {
 		deploymentReplicas  int
 		maxSurge            util.IntOrString
@@ -38,6 +38,7 @@ func TestDeploymentController_scaleUp(t *testing.T) {
 		expectedNewReplicas int
 	}{
 		{
+			// Should not scale up.
 			deploymentReplicas: 10,
 			maxSurge:           util.NewIntOrStringFromInt(0),
 			oldReplicas:        10,
@@ -67,6 +68,15 @@ func TestDeploymentController_scaleUp(t *testing.T) {
 			newReplicas:        2,
 			scaleExpected:      false,
 		},
+		{
+			// Should scale down.
+			deploymentReplicas:  10,
+			maxSurge:            util.NewIntOrStringFromInt(2),
+			oldReplicas:         2,
+			newReplicas:         11,
+			scaleExpected:       true,
+			expectedNewReplicas: 10,
+		},
 	}
 
 	for i, test := range tests {
@@ -80,7 +90,7 @@ func TestDeploymentController_scaleUp(t *testing.T) {
 			client:        fake,
 			eventRecorder: &record.FakeRecorder{},
 		}
-		scaled, err := controller.scaleUp(allRcs, newRc, deployment)
+		scaled, err := controller.reconcileNewRC(allRcs, newRc, deployment)
 		if err != nil {
 			t.Errorf("unexpected error: %v", err)
 			continue
@@ -106,7 +116,7 @@ func TestDeploymentController_scaleUp(t *testing.T) {
 	}
 }
 
-func TestDeploymentController_scaleDown(t *testing.T) {
+func TestDeploymentController_reconcileOldRCs(t *testing.T) {
 	tests := []struct {
 		deploymentReplicas  int
 		maxUnavailable      util.IntOrString
@@ -180,7 +190,7 @@ func TestDeploymentController_scaleDown(t *testing.T) {
 			client:        fake,
 			eventRecorder: &record.FakeRecorder{},
 		}
-		scaled, err := controller.scaleDown(allRcs, oldRcs, nil, deployment)
+		scaled, err := controller.reconcileOldRCs(allRcs, oldRcs, nil, deployment)
 		if err != nil {
 			t.Errorf("unexpected error: %v", err)
 			continue