Repository: keikoproj/upgrade-manager
Pull request #152 (2020-11-20 21:36:15)

Abort on strategy failure instead of continuing
Abort on strategy failure instead of continuing.

Previously, when a failure occurred, namely such as when the postDrain helper script fails (#140), the error was sent to the error channel and returned to UpdateInstance (ie in the case of DrainAndTerminate - the node was only drained), however UpdateInstance did nothing to check this error and continued to WaitForTermination, etc.

I could not see why the error channel was passed through rather than directly returning an error so I changed it to do that, allowing UpdateInstance to abort early if it detects an error and pass the error back through the error channel as previously.

## Testing Done

Manually tested this branch on our sandbox cluster and performed two rolling upgrades with a `/bin/false` call in the postDrain script, one with `ignoreDrainFailure: true` which successfully passed without any reported error, and a second which errored out.

```
upgrade-manager-76f8f8c6d8-jhmvv rolling-upgrade-controller {"level":"error","ts":1606769967.175032,"logger":"controllers.RollingUpgrade","msg":"Failed to run postDrain script: ","rollingupgrade":"rollingupgrade-api-us-west-2c","error":"exit status 127","stacktrace":"github.com/go-logr/zapr.(*zapLogger).Error\n\t/go/pkg/mod/github.com/go-logr/zapr@v0.1.0/zapr.go:128\ngithub.com/keikoproj/upgrade-manager/controllers.(*ScriptRunner).error\n\t/workspace/controllers/script_runner.go:101\ngithub.com/keikoproj/upgrade-manager/controllers.(*ScriptRunner).PostDrain\n\t/workspace/controllers/script_runner.go:159\ngithub.com/keikoproj/upgrade-manager/controllers.(*RollingUpgradeReconciler).postDrainHelper\n\t/workspace/controllers/rollingupgrade_controller.go:158\ngithub.com/keikoproj/upgrade-manager/controllers.(*RollingUpgradeReconciler).DrainNode\n\t/workspace/controllers/rollingupgrade_controller.go:212\ngithub.com/keikoproj/upgrade-manager/controllers.(*RollingUpgradeReconciler).DrainTerminate\n\t/workspace/controllers/rollingupgrade_controller.go:967\ngithub.com/keikoproj/upgrade-manager/controllers.(*RollingUpgradeReconciler).UpdateInstanceEager\n\t/workspace/controllers/rollingupgrade_controller.go:957\ngithub.com/keikoproj/upgrade-manager/controllers.(*RollingUpgradeReconciler).UpdateInstance\n\t/workspace/controllers/rollingupgrade_controller.go:1031"}
upgrade-manager-76f8f8c6d8-jhmvv rolling-upgrade-controller {"level":"info","ts":1606769967.1750689,"logger":"controllers.RollingUpgrade","msg":"Uncordoning the node %s since it failed to run postDrain Script","rollingupgrade":"rollingupgrade-api-us-west-2c","nodeName":"ip-172-30-228-52.us-west-2.compute.internal"}
upgrade-manager-76f8f8c6d8-jhmvv rolling-upgrade-controller {"level":"info","ts":1606769967.1750846,"logger":"controllers.RollingUpgrade","msg":"Running script","rollingupgrade":"rollingupgrade-api-us-west-2c","script":"/usr/local/bin/kubectl uncordon ip-172-30-228-52.us-west-2.compute.internal"}
upgrade-manager-76f8f8c6d8-jhmvv rolling-upgrade-controller {"level":"info","ts":1606769967.8605819,"logger":"controllers.RollingUpgrade","msg":"Script finished","rollingupgrade":"rollingupgrade-api-us-west-2c","output":"node/ip-172-30-228-52.us-west-2.compute.internal uncordoned\n"}
upgrade-manager-76f8f8c6d8-jhmvv rolling-upgrade-controller {"level":"error","ts":1606769967.8669488,"logger":"controllers.RollingUpgrade","msg":"Failed to runRestack","rollingupgrade":"rollingupgrade-api-us-west-2c","error":"Error updating instances, ErrorCount: 1, Errors: [Failed to run postDrain script: : exit status 127]","stacktrace":"github.com/go-logr/zapr.(*zapLogger).Error\n\t/go/pkg/mod/github.com/go-logr/zapr@v0.1.0/zapr.go:128\ngithub.com/keikoproj/upgrade-manager/controllers.(*RollingUpgradeReconciler).error\n\t/workspace/controllers/rollingupgrade_controller.go:1139\ngithub.com/keikoproj/upgrade-manager/controllers.(*RollingUpgradeReconciler).Process\n\t/workspace/controllers/rollingupgrade_controller.go:662"}
``` 

shrinandj: Can you add a testing done section to the PR  (see https://github.com/keikoproj/upgrade-manager/pull/105 as an example)? This usually gives reviewers better confidence about the code changes. Also if something obvious was missed, it could be highlighted.
adammw: Seems there were a lot of tests that didn't specify the strategy mode and so were silently failing with 'unknown strategy' and not being fully tested.
codecov[bot]: # [Codecov](https://codecov.io/gh/keikoproj/upgrade-manager/pull/152?src=pr&el=h1) Report
> Merging [#152](https://codecov.io/gh/keikoproj/upgrade-manager/pull/152?src=pr&el=desc) (7104eb1) into [master](https://codecov.io/gh/keikoproj/upgrade-manager/commit/5396be7dad50bd42335796f7b03dcee5ee846def?el=desc) (5396be7) will **increase** coverage by `0.21%`.
> The diff coverage is `72.22%`.

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

```diff
@@            Coverage Diff             @@
##           master     #152      +/-   ##
==========================================
+ Coverage   68.62%   68.83%   +0.21%     
==========================================
  Files          11       11              
  Lines         902      905       +3     
==========================================
+ Hits          619      623       +4     
+ Misses        251      249       -2     
- Partials       32       33       +1     
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `68.83% <72.22%> (+0.21%)` | :arrow_up: |

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/152?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [controllers/rollingupgrade\_controller.go](https://codecov.io/gh/keikoproj/upgrade-manager/pull/152/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcm9sbGluZ3VwZ3JhZGVfY29udHJvbGxlci5nbw==) | `68.61% <68.75%> (+0.34%)` | :arrow_up: |
| [controllers/script\_runner.go](https://codecov.io/gh/keikoproj/upgrade-manager/pull/152/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvc2NyaXB0X3J1bm5lci5nbw==) | `91.66% <100.00%> (ø)` | |

------

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

uthark: `UpdateInstance` is started in a separate goroutine https://github.com/keikoproj/upgrade-manager/blob/5396be7dad50bd42335796f7b03dcee5ee846def/controllers/rollingupgrade_controller.go#L899 and then it collects for the errors in channel. this PR requires extra work.

Patch:
diff --git a/controllers/rollingupgrade_controller.go b/controllers/rollingupgrade_controller.go
index 7e41359..80fdf9e 100644
--- a/controllers/rollingupgrade_controller.go
+++ b/controllers/rollingupgrade_controller.go
@@ -953,56 +953,50 @@ func (r *RollingUpgradeReconciler) UpdateInstances(ctx *context.Context,
 func (r *RollingUpgradeReconciler) UpdateInstanceEager(
 	ruObj *upgrademgrv1alpha1.RollingUpgrade,
 	nodeName,
-	targetInstanceID string,
-	ch chan error) {
+	targetInstanceID string) error {
 
 	// Set instance to standby
 	err := r.SetStandby(ruObj, targetInstanceID)
 	if err != nil {
-		ch <- err
-		return
+		return err
 	}
 
 	// Wait for new instance to be created
 	err = r.WaitForDesiredInstances(ruObj)
 	if err != nil {
-		ch <- err
-		return
+		return err
 	}
 
 	// Wait for in-service nodes to be ready and match desired
 	err = r.WaitForDesiredNodes(ruObj)
 	if err != nil {
-		ch <- err
-		return
+		return err
 	}
 
 	// Drain and wait for draining node.
-	r.DrainTerminate(ruObj, nodeName, targetInstanceID, ch)
-
+	return r.DrainTerminate(ruObj, nodeName, targetInstanceID)
 }
 
 func (r *RollingUpgradeReconciler) DrainTerminate(
 	ruObj *upgrademgrv1alpha1.RollingUpgrade,
 	nodeName,
-	targetInstanceID string,
-	ch chan error) {
+	targetInstanceID string) error {
 
 	// Drain and wait for draining node.
 	if nodeName != "" {
 		err := r.DrainNode(ruObj, nodeName, targetInstanceID, ruObj.Spec.Strategy.DrainTimeout)
 		if err != nil && !ruObj.Spec.IgnoreDrainFailures {
-			ch <- err
-			return
+			return err
 		}
 	}
 
 	// Terminate instance.
 	err := r.TerminateNode(ruObj, targetInstanceID, nodeName)
 	if err != nil {
-		ch <- err
-		return
+		return err
 	}
+
+	return nil
 }
 
 // UpdateInstance runs the rolling upgrade on one instance from an autoscaling group
@@ -1054,10 +1048,17 @@ func (r *RollingUpgradeReconciler) UpdateInstance(ctx *context.Context,
 	mode := ruObj.Spec.Strategy.Mode.String()
 	if strings.ToLower(mode) == upgrademgrv1alpha1.UpdateStrategyModeEager.String() {
 		r.info(ruObj, "starting replacement with eager mode", "mode", mode)
-		r.UpdateInstanceEager(ruObj, nodeName, targetInstanceID, ch)
+		err = r.UpdateInstanceEager(ruObj, nodeName, targetInstanceID)
 	} else if strings.ToLower(mode) == upgrademgrv1alpha1.UpdateStrategyModeLazy.String() {
 		r.info(ruObj, "starting replacement with lazy mode", "mode", mode)
-		r.DrainTerminate(ruObj, nodeName, targetInstanceID, ch)
+		err = r.DrainTerminate(ruObj, nodeName, targetInstanceID)
+	} else {
+		err = errors.Errorf("unhandled strategy mode: %s", mode)
+	}
+
+	if err != nil {
+		ch <- err
+		return
 	}
 
 	unjoined, err := r.WaitForTermination(ruObj, nodeName, r.generatedClient.CoreV1().Nodes())
diff --git a/controllers/script_runner.go b/controllers/script_runner.go
index 9b175d9..089eb18 100644
--- a/controllers/script_runner.go
+++ b/controllers/script_runner.go
@@ -141,7 +141,7 @@ func (r *ScriptRunner) PostWait(instanceID string, nodeName string, ruObj *upgra
 			result := errors.Wrap(err, msg)
 
 			if !ruObj.Spec.IgnoreDrainFailures {
-				r.info(ruObj, "Uncordoning the node %s since it failed to run postDrainWait Script", "nodeName", nodeName)
+				r.info(ruObj, "Uncordoning the node since it failed to run postDrainWait Script", "nodeName", nodeName)
 				_, err = r.uncordonNode(nodeName, ruObj)
 				if err != nil {
 					r.error(ruObj, err, "Failed to uncordon", "nodeName", nodeName)
@@ -163,7 +163,7 @@ func (r *ScriptRunner) PostDrain(instanceID string, nodeName string, ruObj *upgr
 			result := errors.Wrap(err, msg)
 
 			if !ruObj.Spec.IgnoreDrainFailures {
-				r.info(ruObj, "Uncordoning the node %s since it failed to run postDrain Script", "nodeName", nodeName)
+				r.info(ruObj, "Uncordoning the node since it failed to run postDrain Script", "nodeName", nodeName)
 				_, err = r.uncordonNode(nodeName, ruObj)
 				if err != nil {
 					r.error(ruObj, err, "Failed to uncordon", "nodeName", nodeName)


Test patch:
diff --git a/controllers/rollingupgrade_controller_test.go b/controllers/rollingupgrade_controller_test.go
index 83ce3c6..0132a03 100644
--- a/controllers/rollingupgrade_controller_test.go
+++ b/controllers/rollingupgrade_controller_test.go
@@ -997,14 +997,16 @@ func TestRunRestackSuccessOneNode(t *testing.T) {
 	someLaunchConfig := "some-launch-config"
 	diffLaunchConfig := "different-launch-config"
 	az := "az-1"
+
 	mockInstance := autoscaling.Instance{InstanceId: &mockID, LaunchConfigurationName: &diffLaunchConfig, AvailabilityZone: &az}
 	mockAsg := autoscaling.Group{AutoScalingGroupName: &someAsg,
 		LaunchConfigurationName: &someLaunchConfig,
 		Instances:               []*autoscaling.Instance{&mockInstance}}
 
+	strategy := upgrademgrv1alpha1.UpdateStrategy{Mode: upgrademgrv1alpha1.UpdateStrategyModeLazy}
 	ruObj := &upgrademgrv1alpha1.RollingUpgrade{
 		ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
-		Spec:       upgrademgrv1alpha1.RollingUpgradeSpec{AsgName: someAsg},
+		Spec:       upgrademgrv1alpha1.RollingUpgradeSpec{AsgName: someAsg, Strategy: strategy},
 	}
 
 	mgr, err := buildManager()
@@ -1023,6 +1025,7 @@ func TestRunRestackSuccessOneNode(t *testing.T) {
 		ASGClient:       MockAutoscalingGroup{},
 		EC2Client:       MockEC2{},
 		generatedClient: kubernetes.NewForConfigOrDie(mgr.GetConfig()),
+		ScriptRunner:    NewScriptRunner(log2.NullLogger{}),
 		NodeList:        &nodeList,
 		ClusterState:    NewClusterState(),
 		CacheConfig:     cache.NewConfig(0*time.Second, 0, 0),
@@ -1054,8 +1057,9 @@ func TestRunRestackSuccessMultipleNodes(t *testing.T) {
 		LaunchConfigurationName: &someLaunchConfig,
 		Instances:               []*autoscaling.Instance{&mockInstance, &mockInstance2}}
 
+	strategy := upgrademgrv1alpha1.UpdateStrategy{Mode: upgrademgrv1alpha1.UpdateStrategyModeLazy}
 	ruObj := &upgrademgrv1alpha1.RollingUpgrade{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
-		Spec: upgrademgrv1alpha1.RollingUpgradeSpec{AsgName: someAsg}}
+		Spec: upgrademgrv1alpha1.RollingUpgradeSpec{AsgName: someAsg, Strategy: strategy}}
 
 	mgr, err := buildManager()
 	g.Expect(err).NotTo(gomega.HaveOccurred())
@@ -1074,6 +1078,7 @@ func TestRunRestackSuccessMultipleNodes(t *testing.T) {
 		ASGClient:       MockAutoscalingGroup{},
 		EC2Client:       MockEC2{},
 		generatedClient: kubernetes.NewForConfigOrDie(mgr.GetConfig()),
+		ScriptRunner:    NewScriptRunner(log2.NullLogger{}),
 		NodeList:        &nodeList,
 		ClusterState:    NewClusterState(),
 		CacheConfig:     cache.NewConfig(0*time.Second, 0, 0),
@@ -1100,8 +1105,9 @@ func TestRunRestackSameLaunchConfig(t *testing.T) {
 		LaunchConfigurationName: &someLaunchConfig,
 		Instances:               []*autoscaling.Instance{&mockInstance}}
 
+	strategy := upgrademgrv1alpha1.UpdateStrategy{Mode: upgrademgrv1alpha1.UpdateStrategyModeLazy}
 	ruObj := &upgrademgrv1alpha1.RollingUpgrade{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
-		Spec: upgrademgrv1alpha1.RollingUpgradeSpec{AsgName: someAsg}}
+		Spec: upgrademgrv1alpha1.RollingUpgradeSpec{AsgName: someAsg, Strategy: strategy}}
 
 	mgr, err := buildManager()
 	g.Expect(err).NotTo(gomega.HaveOccurred())
@@ -1129,7 +1135,10 @@ func TestRunRestackSameLaunchConfig(t *testing.T) {
 func TestRunRestackRollingUpgradeNotInMap(t *testing.T) {
 	g := gomega.NewGomegaWithT(t)
 
-	ruObj := &upgrademgrv1alpha1.RollingUpgrade{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}}
+	strategy := upgrademgrv1alpha1.UpdateStrategy{Mode: upgrademgrv1alpha1.UpdateStrategyModeLazy}
+	ruObj := &upgrademgrv1alpha1.RollingUpgrade{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
+		Spec: upgrademgrv1alpha1.RollingUpgradeSpec{Strategy: strategy},
+	}
 	rcRollingUpgrade := &RollingUpgradeReconciler{
 		Log:          log2.NullLogger{},
 		ClusterState: NewClusterState(),
@@ -1158,8 +1167,9 @@ func TestRunRestackRollingUpgradeNodeNameNotFound(t *testing.T) {
 		LaunchConfigurationName: &someLaunchConfig,
 		Instances:               []*autoscaling.Instance{&mockInstance}}
 
+	strategy := upgrademgrv1alpha1.UpdateStrategy{Mode: upgrademgrv1alpha1.UpdateStrategyModeLazy}
 	ruObj := &upgrademgrv1alpha1.RollingUpgrade{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
-		Spec: upgrademgrv1alpha1.RollingUpgradeSpec{AsgName: someAsg}}
+		Spec: upgrademgrv1alpha1.RollingUpgradeSpec{AsgName: someAsg, Strategy: strategy}}
 
 	mgr, err := buildManager()
 	g.Expect(err).NotTo(gomega.HaveOccurred())
@@ -1199,8 +1209,9 @@ func TestRunRestackNoNodeName(t *testing.T) {
 		LaunchConfigurationName: &someLaunchConfig,
 		Instances:               []*autoscaling.Instance{&mockInstance}}
 
+	strategy := upgrademgrv1alpha1.UpdateStrategy{Mode: upgrademgrv1alpha1.UpdateStrategyModeLazy}
 	ruObj := &upgrademgrv1alpha1.RollingUpgrade{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
-		Spec: upgrademgrv1alpha1.RollingUpgradeSpec{AsgName: someAsg}}
+		Spec: upgrademgrv1alpha1.RollingUpgradeSpec{AsgName: someAsg, Strategy: strategy}}
 
 	mgr, err := buildManager()
 	g.Expect(err).NotTo(gomega.HaveOccurred())
@@ -1391,6 +1402,7 @@ func TestUniformAcrossAzUpdateSuccessMultipleNodes(t *testing.T) {
 		Spec: upgrademgrv1alpha1.RollingUpgradeSpec{
 			AsgName: someAsg,
 			Strategy: upgrademgrv1alpha1.UpdateStrategy{
+				Mode: "lazy",
 				Type: upgrademgrv1alpha1.UniformAcrossAzUpdateStrategy,
 			},
 		},
@@ -1432,6 +1444,7 @@ func TestUniformAcrossAzUpdateSuccessMultipleNodes(t *testing.T) {
 		ASGClient:       MockAutoscalingGroup{},
 		EC2Client:       MockEC2{},
 		generatedClient: kubernetes.NewForConfigOrDie(mgr.GetConfig()),
+		ScriptRunner:    NewScriptRunner(log2.NullLogger{}),
 		NodeList:        &nodeList,
 		ClusterState:    NewClusterState(),
 		CacheConfig:     cache.NewConfig(0*time.Second, 0, 0),
@@ -2704,30 +2717,12 @@ func TestDrainNodeTerminateTerminatesWhenIgnoreDrainFailuresSet(t *testing.T) {
 		ScriptRunner: NewScriptRunner(log2.NullLogger{}),
 	}
 
-	ch := make(chan error, 1)
-	rcRollingUpgrade.DrainTerminate(ruObj, mockNode, mockNode, ch)
-
-	select {
-	case err, ok := <-ch:
-		fmt.Println(err)
-		g.Expect(ok).To(gomega.BeFalse()) // don't expect errors.
-	default:
-
-		// done.
-	}
+	err := rcRollingUpgrade.DrainTerminate(ruObj, mockNode, mockNode)
+	g.Expect(err).To(gomega.BeNil()) // don't expect errors.
 
 	// nodeName is empty when node isn't part of the cluster. It must skip drain and terminate.
-	ch = make(chan error, 1)
-	rcRollingUpgrade.DrainTerminate(ruObj, "", mockNode, ch)
-
-	select {
-	case err, ok := <-ch:
-		fmt.Println(err)
-		g.Expect(ok).To(gomega.BeFalse()) //don't expect errors.
-	default:
-
-		// done.
-	}
+	err = rcRollingUpgrade.DrainTerminate(ruObj, "", mockNode)
+	g.Expect(err).To(gomega.BeNil()) // don't expect errors.
 
 }