Repository: keikoproj/upgrade-manager
Pull request #148 (2020-11-18 17:55:49)

Move cluster state from package level to a cluster state impl
Found out that cluster state was public at package level, move it to a ClusterState and make it private.
Other changes: improve code readability.

uthark: @keikoproj/upgrade-manager-approvers Please take a look. 
codecov[bot]: # [Codecov](https://codecov.io/gh/keikoproj/upgrade-manager/pull/148?src=pr&el=h1) Report
> Merging [#148](https://codecov.io/gh/keikoproj/upgrade-manager/pull/148?src=pr&el=desc) (8b6abd6) into [master](https://codecov.io/gh/keikoproj/upgrade-manager/commit/2e14a2ca98529c16abb67efd1295808ca5301ed5?el=desc) (2e14a2c) will **decrease** coverage by `0.13%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/keikoproj/upgrade-manager/pull/148/graphs/tree.svg?width=650&height=150&src=pr&token=PbUYkNX6wz)](https://codecov.io/gh/keikoproj/upgrade-manager/pull/148?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #148      +/-   ##
==========================================
- Coverage   69.31%   69.18%   -0.14%     
==========================================
  Files          11       11              
  Lines         919      915       -4     
==========================================
- Hits          637      633       -4     
  Misses        248      248              
  Partials       34       34              
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `69.18% <100.00%> (-0.14%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/keikoproj/upgrade-manager/pull/148?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [controllers/rollingupgrade\_controller.go](https://codecov.io/gh/keikoproj/upgrade-manager/pull/148/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcm9sbGluZ3VwZ3JhZGVfY29udHJvbGxlci5nbw==) | `68.83% <100.00%> (ø)` | |
| [controllers/rollup\_cluster\_state.go](https://codecov.io/gh/keikoproj/upgrade-manager/pull/148/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcm9sbHVwX2NsdXN0ZXJfc3RhdGUuZ28=) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/keikoproj/upgrade-manager/pull/148?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/keikoproj/upgrade-manager/pull/148?src=pr&el=footer). Last update [2e14a2c...8b6abd6](https://codecov.io/gh/keikoproj/upgrade-manager/pull/148?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/controllers/rollingupgrade_controller.go b/controllers/rollingupgrade_controller.go
index 453a662..c6e29a8 100644
--- a/controllers/rollingupgrade_controller.go
+++ b/controllers/rollingupgrade_controller.go
@@ -501,7 +501,7 @@ func (r *RollingUpgradeReconciler) runRestack(ctx *context.Context, ruObj *upgra
 
 	r.inProcessASGs.Store(*asg.AutoScalingGroupName, "running")
 	r.ClusterState.initializeAsg(*asg.AutoScalingGroupName, asg.Instances)
-	defer r.ClusterState.deleteEntryOfAsg(*asg.AutoScalingGroupName)
+	defer r.ClusterState.deleteAllInstancesInAsg(*asg.AutoScalingGroupName)
 
 	launchDefinition := NewLaunchDefinition(asg)
 
@@ -589,7 +589,7 @@ func (r *RollingUpgradeReconciler) finishExecution(finalStatus string, nodesProc
 		}
 	}
 
-	r.ClusterState.deleteEntryOfAsg(ruObj.Spec.AsgName)
+	r.ClusterState.deleteAllInstancesInAsg(ruObj.Spec.AsgName)
 	r.info(ruObj, "Deleted the entries of ASG in the cluster store", "asgName", ruObj.Spec.AsgName)
 	r.inProcessASGs.Delete(ruObj.Spec.AsgName)
 	r.admissionMap.Delete(ruObj.Name)
diff --git a/controllers/rollup_cluster_state.go b/controllers/rollup_cluster_state.go
index 142101b..64167f5 100644
--- a/controllers/rollup_cluster_state.go
+++ b/controllers/rollup_cluster_state.go
@@ -39,7 +39,7 @@ type ClusterState interface {
 	instanceUpdateInitialized(instanceId string) bool
 	instanceUpdateInProgress(instanceId string) bool
 	instanceUpdateCompleted(instanceId string) bool
-	deleteEntryOfAsg(asgName string) bool
+	deleteAllInstancesInAsg(asgName string) bool
 	getNextAvailableInstanceIdInAz(asgName string, azName string) string
 	initializeAsg(asgName string, instances []*autoscaling.Instance)
 	addInstanceState(instanceData *InstanceData)
@@ -58,17 +58,17 @@ type InstanceData struct {
 // ClusterStateImpl implements the ClusterState interface
 type ClusterStateImpl struct {
 	mu sync.RWMutex
-}
 
-// ClusterStateStore stores the state of the instances running in different Azs for multiple ASGs
-var ClusterStateStore sync.Map
+	// store stores the state of the instances running in different Azs for multiple ASGs
+	store sync.Map
+}
 
 // newClusterState returns the object the struct implementing the ClusterState interface
 func NewClusterState() ClusterState {
 	return &ClusterStateImpl{}
 }
 
-// markUpdateInProgress updates the instance state to in-progresss
+// markUpdateInProgress updates the instance state to in-progress
 func (c *ClusterStateImpl) markUpdateInProgress(instanceId string) {
 	c.updateInstanceState(instanceId, updateInProgress)
 }
@@ -94,13 +94,13 @@ func (c *ClusterStateImpl) instanceUpdateCompleted(instanceId string) bool {
 }
 
 // deleteEntryOfAsg deletes the entry for an ASG in the cluster state map
-func (c *ClusterStateImpl) deleteEntryOfAsg(asgName string) bool {
+func (c *ClusterStateImpl) deleteAllInstancesInAsg(asgName string) bool {
 	deleted := false
-	ClusterStateStore.Range(func(key interface{}, value interface{}) bool {
-		keyName, _ := key.(string)
+	c.store.Range(func(key interface{}, value interface{}) bool {
+		instanceID, _ := key.(string)
 		instanceData, _ := value.(*InstanceData)
 		if instanceData.AsgName == asgName {
-			ClusterStateStore.Delete(keyName)
+			c.store.Delete(instanceID)
 			deleted = true
 		}
 		return true
@@ -138,7 +138,7 @@ func (c *ClusterStateImpl) getNextAvailableInstanceIdInAz(asgName string, azName
 	defer c.mu.Unlock()
 
 	instanceId := ""
-	ClusterStateStore.Range(func(key interface{}, value interface{}) bool {
+	c.store.Range(func(key interface{}, value interface{}) bool {
 		state, _ := value.(*InstanceData)
 		if state.AsgName == asgName &&
 			(azName == "" || state.AzName == azName) &&
@@ -154,23 +154,21 @@ func (c *ClusterStateImpl) getNextAvailableInstanceIdInAz(asgName string, azName
 
 // updateInstanceState updates the state of the instance in cluster store
 func (c *ClusterStateImpl) addInstanceState(instanceData *InstanceData) {
-	ClusterStateStore.Store(instanceData.Id, instanceData)
+	c.store.Store(instanceData.Id, instanceData)
 }
 
 // updateInstanceState updates the state of the instance in cluster store
 func (c *ClusterStateImpl) updateInstanceState(instanceId, instanceState string) {
-	val, ok := ClusterStateStore.Load(instanceId)
-	if ok {
+	if val, ok := c.store.Load(instanceId); ok {
 		instanceData, _ := val.(*InstanceData)
 		instanceData.InstanceState = instanceState
-		ClusterStateStore.Store(instanceId, instanceData)
+		c.store.Store(instanceId, instanceData)
 	}
 }
 
 // getInstanceState returns the state of the instance from cluster store
 func (c *ClusterStateImpl) getInstanceState(instanceId string) string {
-	val, ok := ClusterStateStore.Load(instanceId)
-	if ok {
+	if val, ok := c.store.Load(instanceId); ok {
 		state, _ := val.(*InstanceData)
 		return state.InstanceState
 	}


Test patch:
diff --git a/controllers/helpers_test.go b/controllers/helpers_test.go
index 235e0fd..cf18e30 100644
--- a/controllers/helpers_test.go
+++ b/controllers/helpers_test.go
@@ -281,7 +281,7 @@ func TestGetNextAvailableInstance(t *testing.T) {
 	available := getNextAvailableInstances(mockAsgName, 1, instancesList, rcRollingUpgrade.ClusterState)
 
 	g.Expect(1).Should(gomega.Equal(len(available)))
-	g.Expect(rcRollingUpgrade.ClusterState.deleteEntryOfAsg(mockAsgName)).To(gomega.BeTrue())
+	g.Expect(rcRollingUpgrade.ClusterState.deleteAllInstancesInAsg(mockAsgName)).To(gomega.BeTrue())
 
 }
 
@@ -302,7 +302,7 @@ func TestGetNextAvailableInstanceNoInstanceFound(t *testing.T) {
 	available := getNextAvailableInstances("asg2", 1, instancesList, rcRollingUpgrade.ClusterState)
 
 	g.Expect(0).Should(gomega.Equal(len(available)))
-	g.Expect(rcRollingUpgrade.ClusterState.deleteEntryOfAsg(mockAsgName)).To(gomega.BeTrue())
+	g.Expect(rcRollingUpgrade.ClusterState.deleteAllInstancesInAsg(mockAsgName)).To(gomega.BeTrue())
 
 }
 
@@ -333,7 +333,7 @@ func TestGetNextAvailableInstanceInAz(t *testing.T) {
 	instances = getNextSetOfAvailableInstancesInAz(mockAsgName, "az3", 1, instancesList, rcRollingUpgrade.ClusterState)
 	g.Expect(0).Should(gomega.Equal(len(instances)))
 
-	g.Expect(rcRollingUpgrade.ClusterState.deleteEntryOfAsg(mockAsgName)).To(gomega.BeTrue())
+	g.Expect(rcRollingUpgrade.ClusterState.deleteAllInstancesInAsg(mockAsgName)).To(gomega.BeTrue())
 
 }
 
@@ -359,5 +359,5 @@ func TestGetNextAvailableInstanceInAzGetMultipleInstances(t *testing.T) {
 	instanceIds := []string{*instances[0].InstanceId, *instances[1].InstanceId}
 	g.Expect(instanceIds).Should(gomega.ConsistOf(mockInstanceName1, mockInstanceName2))
 
-	g.Expect(rcRollingUpgrade.ClusterState.deleteEntryOfAsg(mockAsgName)).To(gomega.BeTrue())
+	g.Expect(rcRollingUpgrade.ClusterState.deleteAllInstancesInAsg(mockAsgName)).To(gomega.BeTrue())
 }
diff --git a/controllers/rollingupgrade_controller_test.go b/controllers/rollingupgrade_controller_test.go
index 884b302..19c6553 100644
--- a/controllers/rollingupgrade_controller_test.go
+++ b/controllers/rollingupgrade_controller_test.go
@@ -2359,7 +2359,7 @@ func TestRunRestackWithNodesLessThanMaxUnavailable(t *testing.T) {
 		CacheConfig:     cache.NewConfig(0*time.Second, 0, 0),
 	}
 	rcRollingUpgrade.ruObjNameToASG.Store(ruObj.Name, &mockAsg)
-	rcRollingUpgrade.ClusterState.deleteEntryOfAsg(someAsg)
+	rcRollingUpgrade.ClusterState.deleteAllInstancesInAsg(someAsg)
 	ctx := context.TODO()
 
 	// This execution should not perform drain or termination, but should pass
diff --git a/controllers/rollup_cluster_state_test.go b/controllers/rollup_cluster_state_test.go
index 1dc1ae5..6e3f4c0 100644
--- a/controllers/rollup_cluster_state_test.go
+++ b/controllers/rollup_cluster_state_test.go
@@ -109,8 +109,8 @@ func TestDeleteEntryOfAsg(t *testing.T) {
 	populateClusterState()
 	mockAsgName := "asg-1"
 
-	g.Expect(clusterState.deleteEntryOfAsg(mockAsgName)).To(gomega.BeTrue())
-	g.Expect(clusterState.deleteEntryOfAsg(mockAsgName)).To(gomega.BeFalse())
+	g.Expect(clusterState.deleteAllInstancesInAsg(mockAsgName)).To(gomega.BeTrue())
+	g.Expect(clusterState.deleteAllInstancesInAsg(mockAsgName)).To(gomega.BeFalse())
 }
 
 func TestInstanceStateUpdateSequence(t *testing.T) {
@@ -126,8 +126,8 @@ func TestInstanceStateUpdateSequence(t *testing.T) {
 	clusterState.markUpdateCompleted(mockNodeName)
 	g.Expect(clusterState.instanceUpdateCompleted(mockNodeName)).To(gomega.BeTrue())
 
-	g.Expect(clusterState.deleteEntryOfAsg(mockAsgName)).To(gomega.BeTrue())
-	g.Expect(clusterState.deleteEntryOfAsg(mockAsgName)).To(gomega.BeFalse())
+	g.Expect(clusterState.deleteAllInstancesInAsg(mockAsgName)).To(gomega.BeTrue())
+	g.Expect(clusterState.deleteAllInstancesInAsg(mockAsgName)).To(gomega.BeFalse())
 }
 
 func TestGetNextAvailableInstanceIdInAz(t *testing.T) {