Repository: m3db/m3db-operator
Pull request #235 (2020-09-24 23:44:45)

[controller] Fix bug in TestHandleUpdateClusterCreatesStatefulSets
This commit fixes a bug in the logic of the `TestHandleUpdateClusterCreatesStatefulSets` test. The test checks that the controller creates all the `StatefulSet`'s in a new cluster. Previously, the test checked that all the expected `StatefulSet`'s were created by checking the length of the `map` that the test uses to track `StatefulSet`'s that were created:

```go

expectedMu.Lock()
created := len(expectedSetsCreated)
expectedMu.Unlock()
if created != len(test.expCreateStatefulSets) {
    time.Sleep(100 * time.Millisecond)
    continue
}
done = true
```

However, when the test creates `expectedSetsCreated` it also inserts into it all the `StatefulSet`'s that it expects so the previous `if` condition will always be true:

```go
expectedSetsCreated := make(map[string]bool)
for _, name := range test.expCreateStatefulSets {
    expectedSetsCreated[name] = false
}
```

This commit changes the test so it no longer adds the expected `StatefulSet`'s to `expectedSetsCreated` after it's created, so the subsequent check on the length of `expectedSetsCreated` will be valid because only the `Reactor` function will insert into `expectedSetsCreated`.

After making this change, however, I noticed that the tests were starting to fail. After some debugging, I realized this was because the `Reaction` function was `true`. The [documentation for it states](url):

>  If "handled" is false, then the test client will ignore the results and continue to the next ReactionFunc. 

Since we were returning true, the next function in the chain, which would add it to the "fake" Kubernetes client we're using, never got called, so it would never be returned from subsequent calls to list the `StatefulSet`'s and, as a result, we would continue retrying to create it.

After changing the function to return false, I began encountering one final problem. Occasionally, the call to create a missing `StatefulSet` would return an error because the `StatefulSet` we were trying to create already existed. It seemed this was a caching problem caused when the call to list the `StatefulSet`'s in the cluster didn't return the new `StatefulSet`. To address this problem I updated the error handing after calling `Create` to check if the error was because the `StatefulSet` already exists and to log and return nil if so.

codecov[bot]: # [Codecov](https://codecov.io/gh/m3db/m3db-operator/pull/235?src=pr&el=h1) Report
> Merging [#235](https://codecov.io/gh/m3db/m3db-operator/pull/235?src=pr&el=desc) into [master](https://codecov.io/gh/m3db/m3db-operator/commit/8cf55a084f56e9153ee93bb81343dd4d76dc28b8?el=desc) will **decrease** coverage by `0.10%`.
> The diff coverage is `n/a`.


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

```diff
@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
- Coverage   75.69%   75.59%   -0.11%     
==========================================
  Files          30       30              
  Lines        2115     2118       +3     
==========================================
  Hits         1601     1601              
- Misses        391      392       +1     
- Partials      123      125       +2     
```


------

[Continue to review full report at Codecov](https://codecov.io/gh/m3db/m3db-operator/pull/235?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/235?src=pr&el=footer). Last update [8cf55a0...6b26b61](https://codecov.io/gh/m3db/m3db-operator/pull/235?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/go.sum b/go.sum
index a932539..b4ef31c 100644
--- a/go.sum
+++ b/go.sum
@@ -839,7 +839,6 @@ k8s.io/apiextensions-apiserver v0.17.2 h1:cP579D2hSZNuO/rZj9XFRzwJNYb41DbNANJb6K
 k8s.io/apiextensions-apiserver v0.17.2/go.mod h1:4KdMpjkEjjDI2pPfBA15OscyNldHWdBCfsWMDWAmSTs=
 k8s.io/apimachinery v0.17.2 h1:hwDQQFbdRlpnnsR64Asdi55GyCaIP/3WQpMmbNBeWr4=
 k8s.io/apimachinery v0.17.2/go.mod h1:b9qmWdKlLuU9EBh+06BtLcSf/Mu89rWL33naRxs1uZg=
-k8s.io/apimachinery v0.18.5 h1:Lh6tgsM9FMkC12K5T5QjRm7rDs6aQN5JHkA0JomULDM=
 k8s.io/apiserver v0.17.2/go.mod h1:lBmw/TtQdtxvrTk0e2cgtOxHizXI+d0mmGQURIHQZlo=
 k8s.io/client-go v0.17.2 h1:ndIfkfXEGrNhLIgkr0+qhRguSD3u6DCmonepn1O6NYc=
 k8s.io/client-go v0.17.2/go.mod h1:QAzRgsa0C2xl4/eVpeVAZMvikCn8Nm81yqVx3Kk9XYI=
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
index f47d38c..a0a1dab 100644
--- a/pkg/controller/controller.go
+++ b/pkg/controller/controller.go
@@ -450,6 +450,14 @@ func (c *M3DBController) handleClusterUpdate(cluster *myspec.M3DBCluster) error
 
 			_, err = c.kubeClient.AppsV1().StatefulSets(cluster.Namespace).Create(sts)
 			if err != nil {
+				if kerrors.IsAlreadyExists(err) {
+					// There may be a delay between when a StatefulSet is created and when it is
+					// returned in calls to List because of the caching that the k8s client does.
+					// So if we receive an error because the StatefulSet already exists that's not
+					// indicative of a real problem.
+					c.logger.Info("statefulset already exists", zap.String("name", name))
+					return nil
+				}
 				c.logger.Error(err.Error())
 				return err
 			}


Test patch:
diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go
index 3ba5ad7..d2deb09 100644
--- a/pkg/controller/controller_test.go
+++ b/pkg/controller/controller_test.go
@@ -529,22 +529,23 @@ func TestHandleUpdateClusterCreatesStatefulSets(t *testing.T) {
 
 			// Keep running cluster updates until no more stateful sets required
 			var expectedMu sync.Mutex
-			expectedSetsCreated := make(map[string]bool)
-			for _, name := range test.expCreateStatefulSets {
-				expectedSetsCreated[name] = false
-			}
+			expectedSetsCreated := make(map[string]struct{})
 
 			c.kubeClient.(*kubefake.Clientset).PrependReactor("create", "statefulsets", func(action ktesting.Action) (bool, runtime.Object, error) {
 				cluster := action.(kubetesting.CreateActionImpl).GetObject().(*appsv1.StatefulSet)
 				name := cluster.Name
 				expectedMu.Lock()
-				expectedSetsCreated[name] = true
+				expectedSetsCreated[name] = struct{}{}
 				expectedMu.Unlock()
-				return true, cluster, nil
+				// Return false to indicate that the next reactor in the chain should process this
+				// object as well.
+				return false, nil, nil
 			})
 
+			// Iterate through the expected stateful sets twice to make sure we see all stateful
+			// sets that we expect and also be able to catch any extra stateful sets that we don't.
 			var done bool
-			for i := 0; i < 5; i++ {
+			for i := 0; i < 2*len(test.expCreateStatefulSets); i++ {
 				err := c.handleClusterUpdate(cluster)
 				require.NoError(t, err)