Simplify deployment status check to reduce flapping.

The deployment status logic (originally based on kubectl's rollout
status) can impact its ClusterServiceVersion's status (e.g. by sending
it to Failed/ComponentUnhealthy or triggering reinstallation). If
these ClusterServiceVersion status updates are generated when no
changes have been made to the Deployment spec and when the Deployment
itself has condition Available: True, then the usefulness of
ClusterServiceVersion statuses as a signal is reduced.

This patch changes the deployment status logic so that it is
considered "done" when a) there are no replicas from earlier
generations, and b) the Deployment has condition Available: true.

Signed-off-by: Ben Luddy <[email protected]>

Author: benluddy

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)
 		})
 	}
 }