From 63e848456710b9bb47c3cbf1aa360ef9eeb2322f Mon Sep 17 00:00:00 2001
From: Michail Kargakis <mkargaki@redhat.com>
Date: Tue, 18 Apr 2017 17:32:06 +0200
Subject: [PATCH] controller: fix saturation check for Deployments

Signed-off-by: Michail Kargakis <mkargaki@redhat.com>
---
 pkg/controller/deployment/sync_test.go        | 29 ++++++++++++++-----
 .../deployment/util/deployment_util.go        |  7 +++--
 2 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/pkg/controller/deployment/sync_test.go b/pkg/controller/deployment/sync_test.go
index 68f9c900a1a..8f889697853 100644
--- a/pkg/controller/deployment/sync_test.go
+++ b/pkg/controller/deployment/sync_test.go
@@ -31,9 +31,9 @@ import (
 	deploymentutil "k8s.io/kubernetes/pkg/controller/deployment/util"
 )
 
-func maxSurge(val int) *intstr.IntOrString {
-	surge := intstr.FromInt(val)
-	return &surge
+func intOrStrP(val int) *intstr.IntOrString {
+	intOrStr := intstr.FromInt(val)
+	return &intOrStr
 }
 
 func TestScale(t *testing.T) {
@@ -218,8 +218,8 @@ func TestScale(t *testing.T) {
 		},
 		{
 			name:          "deployment with surge pods",
-			deployment:    newDeployment("foo", 20, nil, maxSurge(2), nil, nil),
-			oldDeployment: newDeployment("foo", 10, nil, maxSurge(2), nil, nil),
+			deployment:    newDeployment("foo", 20, nil, intOrStrP(2), nil, nil),
+			oldDeployment: newDeployment("foo", 10, nil, intOrStrP(2), nil, nil),
 
 			newRS:  rs("foo-v2", 6, nil, newTimestamp),
 			oldRSs: []*extensions.ReplicaSet{rs("foo-v1", 6, nil, oldTimestamp)},
@@ -229,8 +229,8 @@ func TestScale(t *testing.T) {
 		},
 		{
 			name:          "change both surge and size",
-			deployment:    newDeployment("foo", 50, nil, maxSurge(6), nil, nil),
-			oldDeployment: newDeployment("foo", 10, nil, maxSurge(3), nil, nil),
+			deployment:    newDeployment("foo", 50, nil, intOrStrP(6), nil, nil),
+			oldDeployment: newDeployment("foo", 10, nil, intOrStrP(3), nil, nil),
 
 			newRS:  rs("foo-v2", 5, nil, newTimestamp),
 			oldRSs: []*extensions.ReplicaSet{rs("foo-v1", 8, nil, oldTimestamp)},
@@ -249,6 +249,21 @@ func TestScale(t *testing.T) {
 			expectedNew: nil,
 			expectedOld: []*extensions.ReplicaSet{rs("foo-v2", 10, nil, newTimestamp), rs("foo-v1", 4, nil, oldTimestamp)},
 		},
+		{
+			name:          "saturated but broken new replica set does not affect old pods",
+			deployment:    newDeployment("foo", 2, nil, intOrStrP(1), intOrStrP(1), nil),
+			oldDeployment: newDeployment("foo", 2, nil, intOrStrP(1), intOrStrP(1), nil),
+
+			newRS: func() *extensions.ReplicaSet {
+				rs := rs("foo-v2", 2, nil, newTimestamp)
+				rs.Status.AvailableReplicas = 0
+				return rs
+			}(),
+			oldRSs: []*extensions.ReplicaSet{rs("foo-v1", 1, nil, oldTimestamp)},
+
+			expectedNew: rs("foo-v2", 2, nil, newTimestamp),
+			expectedOld: []*extensions.ReplicaSet{rs("foo-v1", 1, nil, oldTimestamp)},
+		},
 	}
 
 	for _, test := range tests {
diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go
index cf038b80466..a56e50160b9 100644
--- a/pkg/controller/deployment/util/deployment_util.go
+++ b/pkg/controller/deployment/util/deployment_util.go
@@ -931,7 +931,8 @@ func NewRSNewReplicas(deployment *extensions.Deployment, allRSs []*extensions.Re
 
 // IsSaturated checks if the new replica set is saturated by comparing its size with its deployment size.
 // Both the deployment and the replica set have to believe this replica set can own all of the desired
-// replicas in the deployment and the annotation helps in achieving that.
+// replicas in the deployment and the annotation helps in achieving that. All pods of the ReplicaSet
+// need to be available.
 func IsSaturated(deployment *extensions.Deployment, rs *extensions.ReplicaSet) bool {
 	if rs == nil {
 		return false
@@ -941,7 +942,9 @@ func IsSaturated(deployment *extensions.Deployment, rs *extensions.ReplicaSet) b
 	if err != nil {
 		return false
 	}
-	return *(rs.Spec.Replicas) == *(deployment.Spec.Replicas) && int32(desired) == *(deployment.Spec.Replicas)
+	return *(rs.Spec.Replicas) == *(deployment.Spec.Replicas) &&
+		int32(desired) == *(deployment.Spec.Replicas) &&
+		rs.Status.AvailableReplicas == *(deployment.Spec.Replicas)
 }
 
 // WaitForObservedDeployment polls for deployment to be updated so that deployment.Status.ObservedGeneration >= desiredGeneration.