From 1923cc60c95943543f036a1c9633f3c4219c917f Mon Sep 17 00:00:00 2001 From: Michail Kargakis Date: Wed, 19 Apr 2017 14:29:39 +0200 Subject: [PATCH 1/3] Revert "kubectl: respect deployment strategy parameters for rollout status" This reverts commit d20ac8766eddde3fdb814aadf7338b0209c66436. --- .../deployment/util/deployment_util.go | 13 +---- pkg/kubectl/rollout_status.go | 5 +- pkg/kubectl/rollout_status_test.go | 56 ++++--------------- 3 files changed, 14 insertions(+), 60 deletions(-) diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index cf038b80466..bdb69886ff2 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -410,7 +410,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) || *(deployment.Spec.Replicas) == 0 { + if !IsRollingUpdate(&deployment) { return int32(0) } // Error caught by validation @@ -418,17 +418,6 @@ 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 67f4f7f84cb..a0227d55213 100644 --- a/pkg/kubectl/rollout_status.go +++ b/pkg/kubectl/rollout_status.go @@ -78,9 +78,8 @@ 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 } - 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 + 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 } 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 acf308156cf..53cad4b1ea0 100644 --- a/pkg/kubectl/rollout_status_test.go +++ b/pkg/kubectl/rollout_status_test.go @@ -20,24 +20,17 @@ 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 - maxUnavailable *intstrutil.IntOrString - status extensions.DeploymentStatus - msg string - done bool + generation int64 + specReplicas int32 + status extensions.DeploymentStatus + msg string + done bool }{ { generation: 0, @@ -68,9 +61,8 @@ func TestDeploymentStatusViewerStatus(t *testing.T) { done: false, }, { - generation: 1, - specReplicas: 2, - maxUnavailable: intOrStringP(0), + generation: 1, + specReplicas: 2, status: extensions.DeploymentStatus{ ObservedGeneration: 1, Replicas: 2, @@ -79,7 +71,7 @@ func TestDeploymentStatusViewerStatus(t *testing.T) { UnavailableReplicas: 1, }, - msg: "Waiting for rollout to finish: 1 of 2 updated replicas are available (minimum required: 2)...\n", + msg: "Waiting for rollout to finish: 1 of 2 updated replicas are available...\n", done: false, }, { @@ -110,26 +102,9 @@ 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 i := range tests { - test := tests[i] - t.Logf("testing scenario %d", i) + for _, test := range tests { d := &extensions.Deployment{ ObjectMeta: metav1.ObjectMeta{ Namespace: "bar", @@ -138,27 +113,18 @@ 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("unexpected error: %v", err) + t.Fatalf("DeploymentStatusViewer.Status(): %v", err) } if done != test.done || msg != test.msg { - t.Errorf("deployment with generation %d, %d replicas specified, and status:\n%+v\nreturned:\n%q, %t\nwant:\n%q, %t", + t.Errorf("DeploymentStatusViewer.Status() for deployment with generation %d, %d replicas specified, and status %+v returned %q, %t, want %q, %t", test.generation, test.specReplicas, test.status, From 0a59f6c4877475c0f38915fa91894b25a0f31bd9 Mon Sep 17 00:00:00 2001 From: Michail Kargakis Date: Wed, 19 Apr 2017 14:33:34 +0200 Subject: [PATCH 2/3] Revert respecting maxUnavailable for DaemonSets Signed-off-by: Michail Kargakis --- pkg/kubectl/rollout_status.go | 8 ++------ pkg/kubectl/rollout_status_test.go | 33 +++++++----------------------- 2 files changed, 9 insertions(+), 32 deletions(-) diff --git a/pkg/kubectl/rollout_status.go b/pkg/kubectl/rollout_status.go index a0227d55213..bb466075a88 100644 --- a/pkg/kubectl/rollout_status.go +++ b/pkg/kubectl/rollout_status.go @@ -21,7 +21,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" - intstrutil "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/kubernetes/pkg/apis/apps" "k8s.io/kubernetes/pkg/apis/extensions" "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset" @@ -101,11 +100,8 @@ func (s *DaemonSetStatusViewer) Status(namespace, name string, revision int64) ( if daemon.Status.UpdatedNumberScheduled < daemon.Status.DesiredNumberScheduled { return fmt.Sprintf("Waiting for rollout to finish: %d out of %d new pods have been updated...\n", daemon.Status.UpdatedNumberScheduled, daemon.Status.DesiredNumberScheduled), false, nil } - - maxUnavailable, _ := intstrutil.GetValueFromIntOrPercent(&daemon.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable, int(daemon.Status.DesiredNumberScheduled), true) - minRequired := daemon.Status.DesiredNumberScheduled - int32(maxUnavailable) - if daemon.Status.NumberAvailable < minRequired { - return fmt.Sprintf("Waiting for rollout to finish: %d of %d updated pods are available (minimum required: %d)...\n", daemon.Status.NumberAvailable, daemon.Status.DesiredNumberScheduled, minRequired), false, nil + if daemon.Status.NumberAvailable < daemon.Status.DesiredNumberScheduled { + return fmt.Sprintf("Waiting for rollout to finish: %d of %d updated pods are available...\n", daemon.Status.NumberAvailable, daemon.Status.DesiredNumberScheduled), false, nil } return fmt.Sprintf("daemon set %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 53cad4b1ea0..1c3682de005 100644 --- a/pkg/kubectl/rollout_status_test.go +++ b/pkg/kubectl/rollout_status_test.go @@ -139,11 +139,10 @@ func TestDeploymentStatusViewerStatus(t *testing.T) { func TestDaemonSetStatusViewerStatus(t *testing.T) { tests := []struct { - generation int64 - maxUnavailable *intstrutil.IntOrString - status extensions.DaemonSetStatus - msg string - done bool + generation int64 + status extensions.DaemonSetStatus + msg string + done bool }{ { generation: 0, @@ -158,8 +157,7 @@ func TestDaemonSetStatusViewerStatus(t *testing.T) { done: false, }, { - generation: 1, - maxUnavailable: intOrStringP(0), + generation: 1, status: extensions.DaemonSetStatus{ ObservedGeneration: 1, UpdatedNumberScheduled: 2, @@ -167,7 +165,7 @@ func TestDaemonSetStatusViewerStatus(t *testing.T) { NumberAvailable: 1, }, - msg: "Waiting for rollout to finish: 1 of 2 updated pods are available (minimum required: 2)...\n", + msg: "Waiting for rollout to finish: 1 of 2 updated pods are available...\n", done: false, }, { @@ -194,19 +192,6 @@ func TestDaemonSetStatusViewerStatus(t *testing.T) { msg: "Waiting for daemon set spec update to be observed...\n", done: false, }, - { - generation: 1, - maxUnavailable: intOrStringP(1), - status: extensions.DaemonSetStatus{ - ObservedGeneration: 1, - UpdatedNumberScheduled: 2, - DesiredNumberScheduled: 2, - NumberAvailable: 1, - }, - - msg: "daemon set \"foo\" successfully rolled out\n", - done: true, - }, } for i := range tests { @@ -221,15 +206,11 @@ func TestDaemonSetStatusViewerStatus(t *testing.T) { }, Spec: extensions.DaemonSetSpec{ UpdateStrategy: extensions.DaemonSetUpdateStrategy{ - Type: extensions.RollingUpdateDaemonSetStrategyType, - RollingUpdate: &extensions.RollingUpdateDaemonSet{}, + Type: extensions.RollingUpdateDaemonSetStrategyType, }, }, Status: test.status, } - if test.maxUnavailable != nil { - d.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable = *test.maxUnavailable - } client := fake.NewSimpleClientset(d).Extensions() dsv := &DaemonSetStatusViewer{c: client} msg, done, err := dsv.Status("bar", "foo", 0) From f1e8356265186a2a8c1cef184e74bb75d1058fa9 Mon Sep 17 00:00:00 2001 From: Michail Kargakis Date: Wed, 19 Apr 2017 14:40:31 +0200 Subject: [PATCH 3/3] Deployments complete only when all desired pods are available Signed-off-by: Michail Kargakis --- .../deployment/util/deployment_util.go | 8 ++++---- .../deployment/util/deployment_util_test.go | 18 ++++++++++++------ 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index bdb69886ff2..9b0ba5ebffc 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -410,7 +410,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 @@ -828,12 +828,12 @@ func IsRollingUpdate(deployment *extensions.Deployment) bool { return deployment.Spec.Strategy.Type == extensions.RollingUpdateDeploymentStrategyType } -// DeploymentComplete considers a deployment to be complete once its desired replicas equals its -// updatedReplicas, no old pods are running, and it doesn't violate minimum availability. +// DeploymentComplete considers a deployment to be complete once all of its desired replicas +// are updated and available, and no old pods are running. func DeploymentComplete(deployment *extensions.Deployment, newStatus *extensions.DeploymentStatus) bool { return newStatus.UpdatedReplicas == *(deployment.Spec.Replicas) && newStatus.Replicas == *(deployment.Spec.Replicas) && - newStatus.AvailableReplicas >= *(deployment.Spec.Replicas)-MaxUnavailable(*deployment) && + newStatus.AvailableReplicas == *(deployment.Spec.Replicas) && newStatus.ObservedGeneration >= deployment.Generation } diff --git a/pkg/controller/deployment/util/deployment_util_test.go b/pkg/controller/deployment/util/deployment_util_test.go index 18869302880..9e5615b3d92 100644 --- a/pkg/controller/deployment/util/deployment_util_test.go +++ b/pkg/controller/deployment/util/deployment_util_test.go @@ -960,37 +960,43 @@ func TestDeploymentComplete(t *testing.T) { expected bool }{ { - name: "complete", + name: "not complete: min but not all pods become available", d: deployment(5, 5, 5, 4, 1, 0), - expected: true, + expected: false, }, { - name: "not complete", + name: "not complete: min availability is not honored", d: deployment(5, 5, 5, 3, 1, 0), expected: false, }, { - name: "complete #2", + name: "complete", d: deployment(5, 5, 5, 5, 0, 0), expected: true, }, { - name: "not complete #2", + name: "not complete: all pods are available but not updated", d: deployment(5, 5, 4, 5, 0, 0), expected: false, }, { - name: "not complete #3", + name: "not complete: still running old pods", // old replica set: spec.replicas=1, status.replicas=1, status.availableReplicas=1 // new replica set: spec.replicas=1, status.replicas=1, status.availableReplicas=0 d: deployment(1, 2, 1, 1, 0, 1), expected: false, }, + { + name: "not complete: one replica deployment never comes up", + + d: deployment(1, 1, 1, 0, 1, 1), + expected: false, + }, } for _, test := range tests {