Merge pull request #41809 from kargakis/rollout-status-fix

Automatic merge from submit-queue

kubectl: respect deployment strategy parameters for rollout status

Fixes https://github.com/kubernetes/kubernetes/issues/40496

`rollout status` now respects the strategy parameters for a RollingUpdate Deployment. This means that it will exit as soon as minimum availability is reached for a rollout (note that if you allow maximum availability, `rollout status` will succeed as soon as the new pods are created)

@janetkuo @AdoHe ptal

pkg/controller/deployment/util/deployment_util.go

@@ -415,7 +415,7 @@ func SetReplicasAnnotations(rs *extensions.ReplicaSet, desiredReplicas, maxRepli
 
 // MaxUnavailable returns the maximum unavailable pods a rolling deployment can take.
 func MaxUnavailable(deployment extensions.Deployment) int32 {
-	if !IsRollingUpdate(&deployment) {
+	if !IsRollingUpdate(&deployment) || *(deployment.Spec.Replicas) == 0 {
 		return int32(0)
 	}
 	// Error caught by validation
@@ -423,6 +423,17 @@ func MaxUnavailable(deployment extensions.Deployment) int32 {
 	return maxUnavailable
 }
 
+// MaxUnavailableInternal returns the maximum unavailable pods a rolling deployment can take.
+// TODO: remove the duplicate
+func MaxUnavailableInternal(deployment internalextensions.Deployment) int32 {
+	if !(deployment.Spec.Strategy.Type == internalextensions.RollingUpdateDeploymentStrategyType) || deployment.Spec.Replicas == 0 {
+		return int32(0)
+	}
+	// Error caught by validation
+	_, maxUnavailable, _ := ResolveFenceposts(&deployment.Spec.Strategy.RollingUpdate.MaxSurge, &deployment.Spec.Strategy.RollingUpdate.MaxUnavailable, deployment.Spec.Replicas)
+	return maxUnavailable
+}
+
 // MinAvailable returns the minimum available pods of a given deployment
 func MinAvailable(deployment *extensions.Deployment) int32 {
 	if !IsRollingUpdate(deployment) {

pkg/kubectl/rollout_status.go

@@ -77,8 +77,9 @@ func (s *DeploymentStatusViewer) Status(namespace, name string, revision int64)
 		if deployment.Status.Replicas > deployment.Status.UpdatedReplicas {
 			return fmt.Sprintf("Waiting for rollout to finish: %d old replicas are pending termination...\n", deployment.Status.Replicas-deployment.Status.UpdatedReplicas), false, nil
 		}
-		if deployment.Status.AvailableReplicas < deployment.Status.UpdatedReplicas {
-			return fmt.Sprintf("Waiting for rollout to finish: %d of %d updated replicas are available...\n", deployment.Status.AvailableReplicas, deployment.Status.UpdatedReplicas), false, nil
+		minRequired := deployment.Spec.Replicas - util.MaxUnavailableInternal(*deployment)
+		if deployment.Status.AvailableReplicas < minRequired {
+			return fmt.Sprintf("Waiting for rollout to finish: %d of %d updated replicas are available (minimum required: %d)...\n", deployment.Status.AvailableReplicas, deployment.Status.UpdatedReplicas, minRequired), false, nil
 		}
 		return fmt.Sprintf("deployment %q successfully rolled out\n", name), true, nil
 	}

pkg/kubectl/rollout_status_test.go

@@ -20,17 +20,24 @@ import (
 	"testing"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	intstrutil "k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/kubernetes/pkg/apis/extensions"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
 )
 
+func intOrStringP(i int) *intstrutil.IntOrString {
+	intstr := intstrutil.FromInt(i)
+	return &intstr
+}
+
 func TestDeploymentStatusViewerStatus(t *testing.T) {
 	tests := []struct {
-		generation   int64
-		specReplicas int32
-		status       extensions.DeploymentStatus
-		msg          string
-		done         bool
+		generation     int64
+		specReplicas   int32
+		maxUnavailable *intstrutil.IntOrString
+		status         extensions.DeploymentStatus
+		msg            string
+		done           bool
 	}{
 		{
 			generation:   0,
@@ -61,8 +68,9 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
 			done: false,
 		},
 		{
-			generation:   1,
-			specReplicas: 2,
+			generation:     1,
+			specReplicas:   2,
+			maxUnavailable: intOrStringP(0),
 			status: extensions.DeploymentStatus{
 				ObservedGeneration:  1,
 				Replicas:            2,
@@ -71,7 +79,7 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
 				UnavailableReplicas: 1,
 			},
 
-			msg:  "Waiting for rollout to finish: 1 of 2 updated replicas are available...\n",
+			msg:  "Waiting for rollout to finish: 1 of 2 updated replicas are available (minimum required: 2)...\n",
 			done: false,
 		},
 		{
@@ -102,9 +110,26 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
 			msg:  "Waiting for deployment spec update to be observed...\n",
 			done: false,
 		},
+		{
+			generation:     1,
+			specReplicas:   2,
+			maxUnavailable: intOrStringP(1),
+			status: extensions.DeploymentStatus{
+				ObservedGeneration:  1,
+				Replicas:            2,
+				UpdatedReplicas:     2,
+				AvailableReplicas:   1,
+				UnavailableReplicas: 0,
+			},
+
+			msg:  "deployment \"foo\" successfully rolled out\n",
+			done: true,
+		},
 	}
 
-	for _, test := range tests {
+	for i := range tests {
+		test := tests[i]
+		t.Logf("testing scenario %d", i)
 		d := &extensions.Deployment{
 			ObjectMeta: metav1.ObjectMeta{
 				Namespace:  "bar",
@@ -113,18 +138,27 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
 				Generation: test.generation,
 			},
 			Spec: extensions.DeploymentSpec{
+				Strategy: extensions.DeploymentStrategy{
+					Type: extensions.RollingUpdateDeploymentStrategyType,
+					RollingUpdate: &extensions.RollingUpdateDeployment{
+						MaxSurge: *intOrStringP(1),
+					},
+				},
 				Replicas: test.specReplicas,
 			},
 			Status: test.status,
 		}
+		if test.maxUnavailable != nil {
+			d.Spec.Strategy.RollingUpdate.MaxUnavailable = *test.maxUnavailable
+		}
 		client := fake.NewSimpleClientset(d).Extensions()
 		dsv := &DeploymentStatusViewer{c: client}
 		msg, done, err := dsv.Status("bar", "foo", 0)
 		if err != nil {
-			t.Fatalf("DeploymentStatusViewer.Status(): %v", err)
+			t.Fatalf("unexpected error: %v", err)
 		}
 		if done != test.done || msg != test.msg {
-			t.Errorf("DeploymentStatusViewer.Status() for deployment with generation %d, %d replicas specified, and status %+v returned %q, %t, want %q, %t",
+			t.Errorf("deployment with generation %d, %d replicas specified, and status:\n%+v\nreturned:\n%q, %t\nwant:\n%q, %t",
 				test.generation,
 				test.specReplicas,
 				test.status,