Merge pull request #2147 from benluddy/simpler-deployment-status

Simplify deployment status check to reduce flapping.

Author: openshift-merge-robot

pkg/controller/install/status_viewer.go

@@ -1,7 +1,5 @@
 package install
 
-// See kubernetes/pkg/kubectl/rollout_status.go
-
 import (
 	"fmt"
 
@@ -15,29 +13,26 @@ const TimedOutReason = "ProgressDeadlineExceeded"
 func DeploymentStatus(deployment *appsv1.Deployment) (string, bool, error) {
 	if deployment.Generation <= deployment.Status.ObservedGeneration {
 		// check if deployment has timed out
-		cond := getDeploymentCondition(deployment.Status, appsv1.DeploymentProgressing)
-		if cond != nil && cond.Reason == TimedOutReason {
+		progressing := getDeploymentCondition(deployment.Status, appsv1.DeploymentProgressing)
+		if progressing != nil && progressing.Reason == TimedOutReason {
 			return "", false, fmt.Errorf("deployment %q exceeded its progress deadline", deployment.Name)
 		}
-		// not all replicas are up yet
-		if deployment.Spec.Replicas != nil && deployment.Status.UpdatedReplicas < *deployment.Spec.Replicas {
-			return fmt.Sprintf("Waiting for rollout to finish: %d out of %d new replicas have been updated...\n", deployment.Status.UpdatedReplicas, *deployment.Spec.Replicas), false, nil
-		}
-		// waiting for old replicas to be cleaned up
+
 		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
+			return fmt.Sprintf("deployment %q waiting for %d outdated replica(s) to be terminated", deployment.Name, deployment.Status.Replicas-deployment.Status.UpdatedReplicas), false, nil
 		}
-		if c := getDeploymentCondition(deployment.Status, appsv1.DeploymentAvailable); c == nil || c.Status != corev1.ConditionTrue {
-			msg := fmt.Sprintf("deployment %q missing condition %q", deployment.Name, appsv1.DeploymentAvailable)
-			if c != nil {
-				msg = fmt.Sprintf("deployment %q not available: %s", deployment.Name, c.Message)
+
+		if available := getDeploymentCondition(deployment.Status, appsv1.DeploymentAvailable); available == nil || available.Status != corev1.ConditionTrue {
+			msg := fmt.Sprintf("missing condition %q", appsv1.DeploymentAvailable)
+			if available != nil {
+				msg = available.Message
 			}
-			return fmt.Sprintf("Waiting for rollout to finish: %s\n", msg), false, nil
+			return fmt.Sprintf("deployment %q not available: %s", deployment.Name, msg), false, nil
 		}
-		// deployment is finished
-		return fmt.Sprintf("deployment %q successfully rolled out\n", deployment.Name), true, nil
+
+		return fmt.Sprintf("deployment %q is up-to-date and available", deployment.Name), true, nil
 	}
-	return fmt.Sprintf("Waiting for deployment spec update to be observed...\n"), false, nil
+	return fmt.Sprintf("waiting for spec update of deployment %q to be observed...", deployment.Name), false, nil
 }
 
 func getDeploymentCondition(status appsv1.DeploymentStatus, condType appsv1.DeploymentConditionType) *appsv1.DeploymentCondition {

pkg/controller/install/status_viewer_test.go

@@ -4,140 +4,106 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	apps "k8s.io/api/apps/v1"
 	core "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestDeploymentStatusViewerStatus(t *testing.T) {
 	tests := []struct {
-		generation   int64
-		specReplicas int32
-		status       apps.DeploymentStatus
-		msg          string
-		done         bool
+		generation int64
+		status     apps.DeploymentStatus
+		err        error
+		msg        string
+		done       bool
 	}{
 		{
-			generation:   0,
-			specReplicas: 1,
 			status: apps.DeploymentStatus{
-				ObservedGeneration:  1,
-				Replicas:            1,
-				UpdatedReplicas:     0,
-				AvailableReplicas:   1,
-				UnavailableReplicas: 0,
 				Conditions: []apps.DeploymentCondition{
 					{
 						Type:   apps.DeploymentProgressing,
-						Reason: "NotTimedOut",
+						Reason: TimedOutReason,
 					},
 				},
 			},
-
-			msg:  "Waiting for rollout to finish: 0 out of 1 new replicas have been updated...\n",
+			err:  fmt.Errorf("deployment \"foo\" exceeded its progress deadline"),
 			done: false,
 		},
 		{
-			generation:   0,
-			specReplicas: 1,
 			status: apps.DeploymentStatus{
-				ObservedGeneration:  1,
-				Replicas:            1,
-				UpdatedReplicas:     0,
-				AvailableReplicas:   1,
-				UnavailableReplicas: 0,
+				Conditions: []apps.DeploymentCondition{
+					{
+						Type:   apps.DeploymentProgressing,
+						Reason: "NotTimedOut",
+					},
+					{
+						Type:   apps.DeploymentAvailable,
+						Status: core.ConditionTrue,
+					},
+				},
+			},
+			msg:  "deployment \"foo\" is up-to-date and available",
+			done: true,
+		},
+		{
+			generation: 1,
+			status: apps.DeploymentStatus{
+				ObservedGeneration: 0,
 			},
-
-			msg:  "Waiting for rollout to finish: 0 out of 1 new replicas have been updated...\n",
+			msg:  "waiting for spec update of deployment \"foo\" to be observed...",
 			done: false,
 		},
 		{
-			generation:   1,
-			specReplicas: 1,
 			status: apps.DeploymentStatus{
-				ObservedGeneration:  1,
-				Replicas:            2,
-				UpdatedReplicas:     1,
-				AvailableReplicas:   2,
-				UnavailableReplicas: 0,
+				Replicas:        5,
+				UpdatedReplicas: 3,
 			},
-
-			msg:  "Waiting for rollout to finish: 1 old replicas are pending termination...\n",
+			msg:  "deployment \"foo\" waiting for 2 outdated replica(s) to be terminated",
 			done: false,
 		},
 		{
-			generation:   1,
-			specReplicas: 2,
+			status: apps.DeploymentStatus{},
+			msg:    fmt.Sprintf("deployment \"foo\" not available: missing condition %q", apps.DeploymentAvailable),
+			done:   false,
+		},
+		{
 			status: apps.DeploymentStatus{
-				ObservedGeneration:  1,
-				Replicas:            2,
-				UpdatedReplicas:     2,
-				AvailableReplicas:   1,
-				UnavailableReplicas: 1,
 				Conditions: []apps.DeploymentCondition{
 					{
 						Type:    apps.DeploymentAvailable,
 						Status:  core.ConditionFalse,
-						Message: "Deployment does not have minimum availability.",
+						Message: "test message",
 					},
 				},
 			},
-
-			msg:  "Waiting for rollout to finish: deployment \"foo\" not available: Deployment does not have minimum availability.\n",
+			msg:  "deployment \"foo\" not available: test message",
 			done: false,
 		},
 		{
-			generation:   1,
-			specReplicas: 2,
 			status: apps.DeploymentStatus{
-				ObservedGeneration:  1,
-				Replicas:            2,
-				UpdatedReplicas:     2,
-				AvailableReplicas:   1,
-				UnavailableReplicas: 1,
 				Conditions: []apps.DeploymentCondition{
 					{
-						Type:   apps.DeploymentAvailable,
-						Status: core.ConditionTrue,
+						Type:    apps.DeploymentAvailable,
+						Status:  core.ConditionUnknown,
+						Message: "test message",
 					},
 				},
 			},
-
-			msg:  "deployment \"foo\" successfully rolled out\n",
-			done: true,
+			msg:  "deployment \"foo\" not available: test message",
+			done: false,
 		},
 		{
-			generation:   1,
-			specReplicas: 2,
 			status: apps.DeploymentStatus{
-				ObservedGeneration:  1,
-				Replicas:            2,
-				UpdatedReplicas:     2,
-				AvailableReplicas:   1,
-				UnavailableReplicas: 1,
 				Conditions: []apps.DeploymentCondition{
 					{
-						Type:   "Fooing",
+						Type:   apps.DeploymentAvailable,
 						Status: core.ConditionTrue,
 					},
 				},
 			},
-			msg:  "Waiting for rollout to finish: deployment \"foo\" missing condition \"Available\"\n",
-			done: false,
-		},
-		{
-			generation:   2,
-			specReplicas: 2,
-			status: apps.DeploymentStatus{
-				ObservedGeneration:  1,
-				Replicas:            2,
-				UpdatedReplicas:     2,
-				AvailableReplicas:   2,
-				UnavailableReplicas: 0,
-			},
-
-			msg:  "Waiting for deployment spec update to be observed...\n",
-			done: false,
+			msg:  "deployment \"foo\" is up-to-date and available",
+			done: true,
 		},
 	}
 
@@ -147,29 +113,19 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace:  "bar",
 					Name:       "foo",
-					UID:        "8764ae47-9092-11e4-8393-42010af018ff",
 					Generation: test.generation,
 				},
-				Spec: apps.DeploymentSpec{
-					Replicas: &test.specReplicas,
-				},
 				Status: test.status,
 			}
 			msg, done, err := DeploymentStatus(d)
-			if err != nil {
-				t.Fatalf("DeploymentStatusViewer.Status(): %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",
-					test.generation,
-					test.specReplicas,
-					test.status,
-					msg,
-					done,
-					test.msg,
-					test.done,
-				)
+			assert := assert.New(t)
+			if test.err == nil {
+				assert.NoError(err)
+			} else {
+				assert.EqualError(err, test.err.Error())
 			}
+			assert.Equal(test.done, done)
+			assert.Equal(test.msg, msg)
 		})
 	}
 }