From d20ac8766eddde3fdb814aadf7338b0209c66436 Mon Sep 17 00:00:00 2001 From: Michail Kargakis Date: Tue, 21 Feb 2017 15:31:20 +0100 Subject: [PATCH] kubectl: respect deployment strategy parameters for rollout status --- .../deployment/util/deployment_util.go | 13 ++++- pkg/kubectl/rollout_status.go | 5 +- pkg/kubectl/rollout_status_test.go | 56 +++++++++++++++---- 3 files changed, 60 insertions(+), 14 deletions(-) diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index ee43cbbf41f..df926b9b80e 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -416,7 +416,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 @@ -424,6 +424,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) { diff --git a/pkg/kubectl/rollout_status.go b/pkg/kubectl/rollout_status.go index 2dabc264420..91855f7ed2d 100644 --- a/pkg/kubectl/rollout_status.go +++ b/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 } diff --git a/pkg/kubectl/rollout_status_test.go b/pkg/kubectl/rollout_status_test.go index 5d104e85e10..2f659f5ea86 100644 --- a/pkg/kubectl/rollout_status_test.go +++ b/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,