Repository: m3db/m3db-operator
Pull request #237 (2020-09-29 21:48:16)

[controller] Always remove update annotation after processing StatefulSet
This commit updates the controller to always remove the update annotation after processing a StatefulSet, even if the controller doesn't need to change the StatefulSet's spec. This will ensure that a user, after setting the annotation, can always know if the operator has processed a StatefulSet by checking if the annotation has been removed or not.

codecov[bot]: # [Codecov](https://codecov.io/gh/m3db/m3db-operator/pull/237?src=pr&el=h1) Report
> Merging [#237](https://codecov.io/gh/m3db/m3db-operator/pull/237?src=pr&el=desc) into [master](https://codecov.io/gh/m3db/m3db-operator/commit/6d59f5f7bc3ee5dc5d0bf9bea5fc06f6812116b8?el=desc) will **not change** coverage.
> The diff coverage is `n/a`.


[![Impacted file tree graph](https://codecov.io/gh/m3db/m3db-operator/pull/237/graphs/tree.svg?width=650&height=150&src=pr&token=jtHDyWhsto)](https://codecov.io/gh/m3db/m3db-operator/pull/237?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master     #237   +/-   ##
=======================================
  Coverage   76.08%   76.08%           
=======================================
  Files          30       30           
  Lines        2170     2170           
=======================================
  Hits         1651     1651           
  Misses        385      385           
  Partials      134      134           
```


------

[Continue to review full report at Codecov](https://codecov.io/gh/m3db/m3db-operator/pull/237?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/m3db/m3db-operator/pull/237?src=pr&el=footer). Last update [6d59f5f...bc1d77a](https://codecov.io/gh/m3db/m3db-operator/pull/237?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
index dc50392..3cc177a 100644
--- a/pkg/controller/controller.go
+++ b/pkg/controller/controller.go
@@ -987,10 +987,20 @@ func updatedStatefulSet(
 		update = true
 	}
 
+	// If we don't need to perform an update to the StatefulSet's spec, but the StatefulSet
+	// has the update annotation, we'll still update the StatefulSet to remove the update
+	// annotation. This ensures that users can always set the update annotation and then
+	// wait for it to be removed to know if the operator has processed a StatefulSet.
 	if !update {
-		return nil, false, nil
+		delete(actual.Annotations, annotations.Update)
+		return actual, true, nil
 	}
 
+	copyAnnotations(expected, actual)
+	return expected, true, nil
+}
+
+func copyAnnotations(expected, actual *appsv1.StatefulSet) {
 	// It's okay for users to add annotations to a StatefulSet after it has been created so
 	// we'll want to copy over any that exist on the actual StatefulSet but not the expected
 	// one. The only exception is we don't want to copy over the update annotation so users
@@ -1006,6 +1016,4 @@ func updatedStatefulSet(
 			expected.Annotations[k] = v
 		}
 	}
-
-	return expected, true, nil
 }


Test patch:
diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go
index 2b501eb..dc7adea 100644
--- a/pkg/controller/controller_test.go
+++ b/pkg/controller/controller_test.go
@@ -80,6 +80,7 @@ func setupTestCluster(
 	sets []*metav1.ObjectMeta,
 	replicationFactor int,
 ) (*myspec.M3DBCluster, *testDeps) {
+	const numInstances = 1
 	cfgMapName := defaultConfigMapName
 	cluster := &myspec.M3DBCluster{
 		ObjectMeta: clusterMeta,
@@ -90,38 +91,32 @@ func setupTestCluster(
 		},
 	}
 	cluster.ObjectMeta.UID = "abcd"
+	groups := make([]myspec.IsolationGroup, 0, replicationFactor)
 	for i := 0; i < replicationFactor; i++ {
 		group := myspec.IsolationGroup{
 			Name:         fmt.Sprintf("group%d", i),
-			NumInstances: 1,
+			NumInstances: numInstances,
 		}
-		cluster.Spec.IsolationGroups = append(cluster.Spec.IsolationGroups, group)
+		groups = append(groups, group)
 	}
+	cluster.Spec.IsolationGroups = groups
 	cluster.ObjectMeta.Finalizers = []string{labels.EtcdDeletionFinalizer}
 
 	objects := make([]runtime.Object, len(sets))
 	statefulSets := make([]*appsv1.StatefulSet, len(sets))
-	for i, s := range sets {
-		set := &appsv1.StatefulSet{
-			ObjectMeta: *s,
-		}
-		// Apply base labels
-		if set.ObjectMeta.Labels == nil {
-			set.ObjectMeta.Labels = make(map[string]string)
+	for i, meta := range sets {
+		set, err := m3db.GenerateStatefulSet(cluster, groups[i].Name, numInstances)
+		require.NoError(t, err)
+
+		set.Namespace = meta.Namespace
+		set.Status.ReadyReplicas = numInstances
+		for k, v := range meta.Labels {
+			set.Labels[k] = v
 		}
-		for k, v := range labels.BaseLabels(cluster) {
-			set.ObjectMeta.Labels[k] = v
+		for k, v := range meta.Annotations {
+			set.Annotations[k] = v
 		}
-		set.Spec.Template.Spec.Containers = append(set.Spec.Template.Spec.Containers, corev1.Container{
-			Image: defaultTestImage,
-		})
-		set.Spec.Template.Spec.Volumes = append(set.Spec.Template.Spec.Volumes, corev1.Volume{
-			VolumeSource: corev1.VolumeSource{
-				ConfigMap: &corev1.ConfigMapVolumeSource{
-					LocalObjectReference: corev1.LocalObjectReference{Name: defaultConfigMapName},
-				},
-			},
-		})
+
 		statefulSets[i] = set
 		objects[i] = set
 		set.OwnerReferences = []metav1.OwnerReference{
@@ -702,6 +697,22 @@ func TestHandleUpdateClusterUpdatesStatefulSets(t *testing.T) {
 			newImage:              "m3db:v2.0.0",
 			expUpdateStatefulSets: []string{"cluster1-rep2"},
 		},
+		{
+			name: "removes update annotation even if stateful set doesn't change",
+			cluster: newMeta("cluster1", map[string]string{
+				"foo":                      "bar",
+				"operator.m3db.io/app":     "m3db",
+				"operator.m3db.io/cluster": "cluster1",
+			}, nil),
+			sets: []*metav1.ObjectMeta{
+				newMeta("cluster1-rep0", nil, nil),
+				newMeta("cluster1-rep1", nil, map[string]string{
+					annotations.Update: "enabled",
+				}),
+				newMeta("cluster1-rep2", nil, nil),
+			},
+			expUpdateStatefulSets: []string{"cluster1-rep1"},
+		},
 		{
 			name: "doesn't call update for replica changes",
 			cluster: newMeta("cluster1", map[string]string{