Repository: pravega/zookeeper-operator (go)

Issue:
[BUG] Reading stale ZookeeperCluster spec/status can lead to undesired pod and PVC deletion
### Description

We find that zookeeper-operator could accidentally delete zookeeper pod and its PVC when the controller reads stale information about ZookeeperCluster.

More concretely, we observe that if a ZookeeperCluster goes through scale down and then scale up, and one of the apisevers gets slow (or partitioned from others), the (restarted) controller may read the stale status/spec information of the ZookeeperCluster. The stale information can trigger the PVC deletion in`reconcileFinalizers` and statefulset scale down (zookeeper pod deletion) in `reconcileStatefulSet`.

We list concrete reproduction steps as below:
1. Run the controller in a HA k8s cluster and create a ZookeeperCluster `zkc` with `replicas=2`. The controller is talking to apiserver1 (which is not stale). There will be two zookeeper pods `zk1` and `zk2` and two PVC in the cluster.
2. Scale `zkc` down (by setting `replicas=1`). After getting stable, there will only be one zookeeper pod `zk1` and one PVC.  Meanwhile, apiserver2 gets partitioned so its watch cache stops at the moment that `zkc` has `replicas=1`.
3. Scale `zkc` up (by setting `replicas=2`). Now a new `zk2` and its PVC get back.
4. After experiencing a crash, the restarted controller talks to the stale apiserver2. From apiserver2's watch cache, the controller finds that `zkc`'s `Status.ReadyReplicas` and `Spec.Replicas` are both 1, which is lower than the PVC count (i.e., 2). Inside `cleanupOrphanPVCs` the controller will treat the PVC of `zk2` as an orphan PVC and delete it. Later in `Updating StatefulSet`, the controller will also set the `Replicas` of the statefulset back to 1, which will trigger deletion of `zk2`.

### Importance

blocker: The unexpected PVC and pod deletion caused by reading stale data from the apiserver can further lead to data loss or availability issues.

### Location

zookeepercluster_controller.go

### Suggestions for an improvement

We are willing to help alleviate this problem by issuing a PR.
It is hard to fully avoid this issue because the controller has no clue whether it is reading the stale information of the ZookeeperCluster or not, given the controller is supposed to be stateless. However, some sanity check can help prevent this issue in certain cases and ensure the controller is not going to perform undesired deletion before the updated information is populated to the controller:
1. Before issuing PVC deletion in `cleanupOrphanPVCs`, we can first check the number of zookeeper pods and see whether the number is the same as `Spec.Replicas` of the ZookeeperCluster. If not, we know at least one of them is wrong and the PVC may not be an orphan. The sanity check is helpful if the pod information is fresh (especially when the pod information is retrieved from a different apiserver/etcd than the  ZookeeperCluster information).
2. Before updating the statefulset in `reconcileStatefulSet`, we may want to check `ZookeeperCluster.Spec.Replicas`, `ZookeeperCluster.Status.Replicas` and `StatefulSet.Spec.Replicas`. For example, it is a sign of abnormality if `ZookeeperCluster.Spec.Replicas != StatefulSet.Spec.Replicas` but  `ZookeeperCluster.Spec.Replicas == ZookeeperCluster.Status.Replicas`. Logging a warning and returning from reconcile() with error in such case can be helpful.

We are willing to issue a PR to add the above checks if they are not considered too expensive.


Pull request description:
fix issue-314 by checking zookeeperCluster resource version before updating sts ADDENDUM-2
### Change log description

When upgrading the operator and the ZookeeperCluster and StatefulSet are already
created, the sts owner-rv is not set (it is set once when the sts is created)
Handle this upgrade scenario and set the owner-rv label on sts once when this is not set

### Purpose of the change

Fixes #314 


Patch:
diff --git a/pkg/controller/zookeepercluster/zookeepercluster_controller.go b/pkg/controller/zookeepercluster/zookeepercluster_controller.go
index ed78a26..0b53c43 100644
--- a/pkg/controller/zookeepercluster/zookeepercluster_controller.go
+++ b/pkg/controller/zookeepercluster/zookeepercluster_controller.go
@@ -164,26 +164,39 @@ func (r *ReconcileZookeeperCluster) Reconcile(request reconcile.Request) (reconc
 	return reconcile.Result{RequeueAfter: ReconcileTime}, nil
 }
 
-func zookeeperClusterFresherThanSts(instance *zookeeperv1beta1.ZookeeperCluster, sts *appsv1.StatefulSet) bool {
-	labeledRV, ok := sts.Labels["owner-rv"]
-	if !ok {
-		log.Info("Fail to find owner-rv in statefulset's labels; skip checking resource version")
-		return true
-	}
-	largerRV, err := strconv.Atoi(instance.ResourceVersion)
-	if err != nil {
-		log.Info("Fail to convert %s to integer; skip checking resource version", instance.ResourceVersion)
-		return true
+// compareResourceVersion compare resoure versions for the supplied ZookeeperCluster and StatefulSet
+// resources
+// Returns:
+//  0 if versions are equal
+// -1 if ZookeeperCluster version is less than StatefulSet version
+//  1 if ZookeeperCluster version is greater than StatefulSet version
+func compareResourceVersion(zk *zookeeperv1beta1.ZookeeperCluster, sts *appsv1.StatefulSet) int {
+
+	zkResourceVersion, zkErr := strconv.Atoi(zk.ResourceVersion)
+	stsVersion, stsVersionFound := sts.Labels["owner-rv"]
+
+	if !stsVersionFound {
+		if zkErr != nil {
+			log.Info("Fail to parse ZookeeperCluster version. Cannot decide zookeeper StatefulSet version")
+			return 0
+		}
+		return 1
 	}
-	smallerRV, err := strconv.Atoi(labeledRV)
+	stsResourceVersion, err := strconv.Atoi(stsVersion)
 	if err != nil {
-		log.Info("Fail to convert %s to integer; skip checking resource version", labeledRV)
-		return true
+		if zkErr != nil {
+			log.Info("Fail to parse ZookeeperCluster version. Cannot decide zookeeper StatefulSet version")
+			return 0
+		}
+		log.Info("Fail to convert StatefulSet version %s to integer; setting it to ZookeeperCluster version", stsVersion)
+		return 1
 	}
-	if largerRV < smallerRV {
-		return false
+	if zkResourceVersion < stsResourceVersion {
+		return -1
+	} else if zkResourceVersion > stsResourceVersion {
+		return 1
 	}
-	return true
+	return 0
 }
 
 func (r *ReconcileZookeeperCluster) reconcileStatefulSet(instance *zookeeperv1beta1.ZookeeperCluster) (err error) {
@@ -234,8 +247,12 @@ func (r *ReconcileZookeeperCluster) reconcileStatefulSet(instance *zookeeperv1be
 		return err
 	} else {
 		// check whether zookeeperCluster is updated before updating the sts
-		if !zookeeperClusterFresherThanSts(instance, foundSts) {
+		cmp := compareResourceVersion(instance, foundSts)
+		if cmp < 0 {
 			return fmt.Errorf("Staleness: cr.ResourceVersion %s is smaller than labeledRV %s", instance.ResourceVersion, foundSts.Labels["owner-rv"])
+		} else if cmp > 0 {
+			// Zookeeper StatefulSet version inherits ZookeeperCluster resource version
+			foundSts.Labels["owner-rv"] = instance.ResourceVersion
 		}
 		foundSTSSize := *foundSts.Spec.Replicas
 		newSTSSize := *sts.Spec.Replicas


Test patch:
diff --git a/pkg/controller/zookeepercluster/zookeepercluster_controller_test.go b/pkg/controller/zookeepercluster/zookeepercluster_controller_test.go
index cea195d..caf297f 100644
--- a/pkg/controller/zookeepercluster/zookeepercluster_controller_test.go
+++ b/pkg/controller/zookeepercluster/zookeepercluster_controller_test.go
@@ -576,22 +576,35 @@ var _ = Describe("ZookeeperCluster Controller", func() {
 				sts.Labels = make(map[string]string)
 			})
 
-			It("should return true as 99 < 100", func() {
+			It("should return 1 as 100 > 99", func() {
 				sts.Labels["owner-rv"] = "99"
-				updated := zookeeperClusterFresherThanSts(z, sts)
-				Ω(updated).To(BeTrue())
+				updated := compareResourceVersion(z, sts)
+				Ω(updated).To(Equal(1))
 			})
 
-			It("should return true as 100 == 100", func() {
+			It("should return 0 as 100 == 100", func() {
 				sts.Labels["owner-rv"] = "100"
-				updated := zookeeperClusterFresherThanSts(z, sts)
-				Ω(updated).To(BeTrue())
+				updated := compareResourceVersion(z, sts)
+				Ω(updated).To(Equal(0))
 			})
 
-			It("should return false as 101 > 100", func() {
+			It("should return -1 as 100 < 101", func() {
 				sts.Labels["owner-rv"] = "101"
-				updated := zookeeperClusterFresherThanSts(z, sts)
-				Ω(updated).To(BeFalse())
+				updated := compareResourceVersion(z, sts)
+				Ω(updated).To(Equal(-1))
+			})
+
+			It("should return 1 as 100 > invalid numeric version 'xoxo'", func() {
+				sts.Labels["owner-rv"] = "xoxo"
+				updated := compareResourceVersion(z, sts)
+				Ω(updated).To(Equal(1))
+			})
+
+			It("should return 0 as invalid zk version z0z0 == invalid numeric version 'x0x0'", func() {
+				sts.Labels["owner-rv"] = "x0x0"
+				z.ResourceVersion = "z0z0"
+				updated := compareResourceVersion(z, sts)
+				Ω(updated).To(Equal(0))
 			})
 		})
 	})