From 7c74f1dd027709156c3345e1965693f04b8dd9ac Mon Sep 17 00:00:00 2001
From: Dominic Evans <dominic.evans@uk.ibm.com>
Date: Fri, 28 Jan 2022 10:25:43 +0000
Subject: [PATCH] fix: improve logging & safety of statefulSetReady

Confirm that the current and updated revision numbers also match as part
of the readiness check. Add coverage for readiness scenarios where
StatefulSet status does not reflect the most recent generation of the
StatefulSet yet.

Also add additional logging around the sts transitions from non-ready to
ready.

Fixes: #10163

Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com>
---
 pkg/action/upgrade.go  |  3 +++
 pkg/kube/ready.go      | 14 ++++++++++++++
 pkg/kube/ready_test.go | 38 ++++++++++++++++++++++++++++++++++----
 3 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go
index c98b524bc..690397d4a 100644
--- a/pkg/action/upgrade.go
+++ b/pkg/action/upgrade.go
@@ -391,6 +391,9 @@ func (u *Upgrade) releasingUpgrade(c chan<- resultMessage, upgradedRelease *rele
 	}
 
 	if u.Wait {
+		u.cfg.Log(
+			"waiting for release %s resources (created: %d updated: %d  deleted: %d)",
+			upgradedRelease.Name, len(results.Created), len(results.Updated), len(results.Deleted))
 		if u.WaitForJobs {
 			if err := u.cfg.KubeClient.WaitWithJobs(target, u.Timeout); err != nil {
 				u.cfg.recordRelease(originalRelease)
diff --git a/pkg/kube/ready.go b/pkg/kube/ready.go
index 5d080d9bf..0554c1729 100644
--- a/pkg/kube/ready.go
+++ b/pkg/kube/ready.go
@@ -353,9 +353,16 @@ func (c *ReadyChecker) crdReady(crd apiextv1.CustomResourceDefinition) bool {
 func (c *ReadyChecker) statefulSetReady(sts *appsv1.StatefulSet) bool {
 	// If the update strategy is not a rolling update, there will be nothing to wait for
 	if sts.Spec.UpdateStrategy.Type != appsv1.RollingUpdateStatefulSetStrategyType {
+		c.log("StatefulSet skipped ready check: %s/%s. updateStrategy is %v", sts.Namespace, sts.Name, sts.Spec.UpdateStrategy.Type)
 		return true
 	}
 
+	// Make sure the status is up-to-date with the StatefulSet changes
+	if sts.Status.ObservedGeneration < sts.Generation {
+		c.log("StatefulSet is not ready: %s/%s. update has not yet been observed", sts.Namespace, sts.Name)
+		return false
+	}
+
 	// Dereference all the pointers because StatefulSets like them
 	var partition int
 	// 1 is the default for replicas if not set
@@ -386,6 +393,13 @@ func (c *ReadyChecker) statefulSetReady(sts *appsv1.StatefulSet) bool {
 		c.log("StatefulSet is not ready: %s/%s. %d out of %d expected pods are ready", sts.Namespace, sts.Name, sts.Status.ReadyReplicas, replicas)
 		return false
 	}
+
+	if sts.Status.CurrentRevision != sts.Status.UpdateRevision {
+		c.log("StatefulSet is not ready: %s/%s. currentRevision %s does not yet match updateRevision %s", sts.Namespace, sts.Name, sts.Status.CurrentRevision, sts.Status.UpdateRevision)
+		return false
+	}
+
+	c.log("StatefulSet is ready: %s/%s. %d out of %d expected pods are ready", sts.Namespace, sts.Name, sts.Status.ReadyReplicas, replicas)
 	return true
 }
 
diff --git a/pkg/kube/ready_test.go b/pkg/kube/ready_test.go
index 931b8fa19..9fe20d8cb 100644
--- a/pkg/kube/ready_test.go
+++ b/pkg/kube/ready_test.go
@@ -175,6 +175,20 @@ func Test_ReadyChecker_statefulSetReady(t *testing.T) {
 			},
 			want: true,
 		},
+		{
+			name: "statefulset is not ready when status of latest generation has not yet been observed",
+			args: args{
+				sts: newStatefulSetWithNewGeneration("foo", 1, 0, 1, 1),
+			},
+			want: false,
+		},
+		{
+			name: "statefulset is not ready when current revision for current replicas does not match update revision for updated replicas",
+			args: args{
+				sts: newStatefulSetWithUpdateRevision("foo", 1, 0, 1, 1, "foo-bbbbbbb"),
+			},
+			want: false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -377,8 +391,9 @@ func newDaemonSet(name string, maxUnavailable, numberReady, desiredNumberSchedul
 func newStatefulSet(name string, replicas, partition, readyReplicas, updatedReplicas int) *appsv1.StatefulSet {
 	return &appsv1.StatefulSet{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      name,
-			Namespace: defaultNamespace,
+			Name:       name,
+			Namespace:  defaultNamespace,
+			Generation: int64(1),
 		},
 		Spec: appsv1.StatefulSetSpec{
 			UpdateStrategy: appsv1.StatefulSetUpdateStrategy{
@@ -404,12 +419,27 @@ func newStatefulSet(name string, replicas, partition, readyReplicas, updatedRepl
 			},
 		},
 		Status: appsv1.StatefulSetStatus{
-			UpdatedReplicas: int32(updatedReplicas),
-			ReadyReplicas:   int32(readyReplicas),
+			ObservedGeneration: int64(1),
+			CurrentRevision:    name + "-aaaaaaa",
+			UpdateRevision:     name + "-aaaaaaa",
+			UpdatedReplicas:    int32(updatedReplicas),
+			ReadyReplicas:      int32(readyReplicas),
 		},
 	}
 }
 
+func newStatefulSetWithNewGeneration(name string, replicas, partition, readyReplicas, updatedReplicas int) *appsv1.StatefulSet {
+	ss := newStatefulSet(name, replicas, partition, readyReplicas, updatedReplicas)
+	ss.Generation++
+	return ss
+}
+
+func newStatefulSetWithUpdateRevision(name string, replicas, partition, readyReplicas, updatedReplicas int, updateRevision string) *appsv1.StatefulSet {
+	ss := newStatefulSet(name, replicas, partition, readyReplicas, updatedReplicas)
+	ss.Status.UpdateRevision = updateRevision
+	return ss
+}
+
 func newDeployment(name string, replicas, maxSurge, maxUnavailable int) *appsv1.Deployment {
 	return &appsv1.Deployment{
 		ObjectMeta: metav1.ObjectMeta{