Repository: keikoproj/instance-manager
Pull request #249 (2021-02-11 06:46:51)

feature: Add MaxRetries to CRD Strategy
Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>

This PR continues work done in #243 
Since deletion of inactive resources is now removed, behavior has changed so that if the upgrade fails it does not retry and enters an error state - previous to this change, the upgrade would be retried to infinity (or until success).

This PR adds a MaxRetries field to CRD Strategy:
```
spec:
  provisioner: eks
  strategy:
    type: crd
    crd:
      maxRetries: 2
```

By default, if not set, maxRetries is set to `3`.
Controller will cache current retry count in IG status, and reset to 0 when:
- Rotation no longer needed (all nodes have rotated)
- New custom resource is submitted (e.g. the failed / timed out resource is manually deleted)
- Resource is completed successfully



codecov[bot]: # [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/249?src=pr&el=h1) Report
> Merging [#249](https://codecov.io/gh/keikoproj/instance-manager/pull/249?src=pr&el=desc) (8638fc4) into [master](https://codecov.io/gh/keikoproj/instance-manager/commit/b218110dea3ba4a5b3b0f900c8ddfcc0bd8fa9ec?el=desc) (b218110) will **decrease** coverage by `0.07%`.
> The diff coverage is `33.33%`.

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

```diff
@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
- Coverage   88.19%   88.12%   -0.08%     
==========================================
  Files          17       17              
  Lines        2237     2240       +3     
==========================================
+ Hits         1973     1974       +1     
- Misses        161      162       +1     
- Partials      103      104       +1     
```


| [Impacted Files](https://codecov.io/gh/keikoproj/instance-manager/pull/249?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [controllers/provisioners/eks/update.go](https://codecov.io/gh/keikoproj/instance-manager/pull/249/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcHJvdmlzaW9uZXJzL2Vrcy91cGRhdGUuZ28=) | `85.27% <33.33%> (-0.81%)` | :arrow_down: |

------

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


Patch:
diff --git a/api/v1alpha1/instancegroup_types.go b/api/v1alpha1/instancegroup_types.go
index 8478791..9318bf7 100644
--- a/api/v1alpha1/instancegroup_types.go
+++ b/api/v1alpha1/instancegroup_types.go
@@ -103,6 +103,7 @@ var (
 			IntVal: 1,
 		},
 	}
+	DefaultCRDStrategyMaxRetries = 3
 
 	AllowedFileSystemTypes              = []string{FileSystemTypeXFS, FileSystemTypeEXT4}
 	AllowedMixedPolicyStrategies        = []string{LaunchTemplateStrategyCapacityOptimized, LaunchTemplateStrategyLowestPrice}
@@ -164,6 +165,7 @@ type CRDUpdateStrategy struct {
 	Spec                string `json:"spec,omitempty"`
 	CRDName             string `json:"crdName,omitempty"`
 	ConcurrencyPolicy   string `json:"concurrencyPolicy,omitempty"`
+	MaxRetries          *int   `json:"maxRetries,omitempty"`
 	StatusJSONPath      string `json:"statusJSONPath,omitempty"`
 	StatusSuccessString string `json:"statusSuccessString,omitempty"`
 	StatusFailureString string `json:"statusFailureString,omitempty"`
@@ -319,8 +321,8 @@ type EKSFargateSelectors struct {
 // InstanceGroupStatus defines the schema of resource Status
 type InstanceGroupStatus struct {
 	CurrentState                  string                   `json:"currentState,omitempty"`
-	CurrentMin                    int                      `json:"currentMin,omitempty"`
-	CurrentMax                    int                      `json:"currentMax,omitempty"`
+	CurrentMin                    int                      `json:"currentMin"`
+	CurrentMax                    int                      `json:"currentMax"`
 	ActiveLaunchConfigurationName string                   `json:"activeLaunchConfigurationName,omitempty"`
 	ActiveLaunchTemplateName      string                   `json:"activeLaunchTemplateName,omitempty"`
 	LatestTemplateVersion         string                   `json:"latestTemplateVersion,omitempty"`
@@ -328,6 +330,7 @@ type InstanceGroupStatus struct {
 	NodesArn                      string                   `json:"nodesInstanceRoleArn,omitempty"`
 	StrategyResourceName          string                   `json:"strategyResourceName,omitempty"`
 	StrategyResourceNamespace     string                   `json:"strategyResourceNamespace,omitempty"`
+	StrategyRetryCount            int                      `json:"strategyRetryCount,omitempty"`
 	UsingSpotRecommendation       bool                     `json:"usingSpotRecommendation,omitempty"`
 	Lifecycle                     string                   `json:"lifecycle,omitempty"`
 	ConfigHash                    string                   `json:"configMD5,omitempty"`
@@ -842,6 +845,10 @@ func (c *CRDUpdateStrategy) Validate() error {
 		c.SetConcurrencyPolicy(ForbidConcurrencyPolicy)
 	}
 
+	if c.MaxRetries == nil {
+		c.MaxRetries = &DefaultCRDStrategyMaxRetries
+	}
+
 	if c.GetCRDName() == "" {
 		return errors.New("crdName is empty")
 	}
@@ -908,6 +915,14 @@ func (c *CRDUpdateStrategy) SetStatusFailureString(str string) {
 	c.StatusFailureString = str
 }
 
+func (status *InstanceGroupStatus) IncrementStrategyRetryCount() {
+	status.StrategyRetryCount++
+}
+
+func (status *InstanceGroupStatus) SetStrategyRetryCount(n int) {
+	status.StrategyRetryCount = n
+}
+
 func (status *InstanceGroupStatus) GetActiveLaunchConfigurationName() string {
 	return status.ActiveLaunchConfigurationName
 }
@@ -980,6 +995,10 @@ func (status *InstanceGroupStatus) GetStrategyResourceName() string {
 	return status.StrategyResourceName
 }
 
+func (status *InstanceGroupStatus) GetStrategyRetryCount() int {
+	return status.StrategyRetryCount
+}
+
 func (status *InstanceGroupStatus) GetStrategyResourceNamespace() string {
 	return status.StrategyResourceNamespace
 }
diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go
index 1fb88b4..02afd23 100644
--- a/api/v1alpha1/zz_generated.deepcopy.go
+++ b/api/v1alpha1/zz_generated.deepcopy.go
@@ -31,7 +31,7 @@ func (in *AwsUpgradeStrategy) DeepCopyInto(out *AwsUpgradeStrategy) {
 	if in.CRDType != nil {
 		in, out := &in.CRDType, &out.CRDType
 		*out = new(CRDUpdateStrategy)
-		**out = **in
+		(*in).DeepCopyInto(*out)
 	}
 	if in.RollingUpdateType != nil {
 		in, out := &in.RollingUpdateType, &out.RollingUpdateType
@@ -68,6 +68,11 @@ func (in *BootstrapOptions) DeepCopy() *BootstrapOptions {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *CRDUpdateStrategy) DeepCopyInto(out *CRDUpdateStrategy) {
 	*out = *in
+	if in.MaxRetries != nil {
+		in, out := &in.MaxRetries, &out.MaxRetries
+		*out = new(int)
+		**out = **in
+	}
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CRDUpdateStrategy.
diff --git a/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml b/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml
index 3fabf74..358b8b8 100644
--- a/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml
+++ b/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml
@@ -366,6 +366,8 @@ spec:
                         type: string
                       crdName:
                         type: string
+                      maxRetries:
+                        type: integer
                       spec:
                         type: string
                       statusFailureString:
@@ -428,8 +430,13 @@ spec:
                 type: string
               strategyResourceNamespace:
                 type: string
+              strategyRetryCount:
+                type: integer
               usingSpotRecommendation:
                 type: boolean
+            required:
+            - currentMax
+            - currentMin
             type: object
         required:
         - metadata
diff --git a/controllers/providers/kubernetes/crd.go b/controllers/providers/kubernetes/crd.go
index cae4e26..7cffef9 100644
--- a/controllers/providers/kubernetes/crd.go
+++ b/controllers/providers/kubernetes/crd.go
@@ -117,6 +117,7 @@ func ProcessCRDStrategy(kube dynamic.Interface, instanceGroup *v1alpha1.Instance
 			var isRunning bool
 			for _, resource := range activeResources {
 				resourceName := resource.GetName()
+				resourceNamespace := resource.GetNamespace()
 				// if active resource exist with same launch id, it's not replaceable
 				if strings.HasSuffix(resourceName, launchID) {
 					isRunning = true
@@ -125,7 +126,7 @@ func ProcessCRDStrategy(kube dynamic.Interface, instanceGroup *v1alpha1.Instance
 
 				// if active resource exist with a different launch id, it is replaceable
 				log.Info("active custom resource/s exists, will replace", "instancegroup", instanceGroupNamespacedName)
-				err = kube.Resource(GVR).Namespace(resource.GetNamespace()).Delete(context.Background(), resourceName, metav1.DeleteOptions{})
+				err = kube.Resource(GVR).Namespace(resourceNamespace).Delete(context.Background(), resourceName, metav1.DeleteOptions{})
 				if err != nil {
 					if !kerr.IsNotFound(err) {
 						return false, errors.Wrap(err, "failed to delete custom resource")
@@ -151,6 +152,7 @@ func ProcessCRDStrategy(kube dynamic.Interface, instanceGroup *v1alpha1.Instance
 		}
 	} else {
 		log.Info("submitted custom resource", "instancegroup", instanceGroupNamespacedName)
+		status.SetStrategyRetryCount(0)
 	}
 
 	// get created resource
@@ -170,11 +172,33 @@ func ProcessCRDStrategy(kube dynamic.Interface, instanceGroup *v1alpha1.Instance
 
 	if strings.EqualFold(resourceStatus, successString) {
 		log.Info("custom resource succeeded", "instancegroup", instanceGroupNamespacedName, "resource", customResourceName, "status", resourceStatus)
+		status.SetStrategyRetryCount(0)
 		return true, nil
 	}
 
 	if strings.EqualFold(resourceStatus, failureString) {
 		log.Info("custom resource failed", "instancegroup", instanceGroupNamespacedName, "resource", customResourceName, "status", resourceStatus)
+		maxRetries := *strategy.MaxRetries
+		if maxRetries > status.GetStrategyRetryCount() {
+
+			if maxRetries == -1 {
+				// if maxRetries is set to -1, retry forever
+				status.SetStrategyRetryCount(-1)
+			} else {
+				// otherwise increment retry counter
+				status.IncrementStrategyRetryCount()
+			}
+
+			log.Info("max retries not met, will resubmit", "instancegroup", instanceGroupNamespacedName, "maxRetries", maxRetries, "retryNumber", status.StrategyRetryCount)
+			err = kube.Resource(GVR).Namespace(customResourceNamespace).Delete(context.Background(), customResourceName, metav1.DeleteOptions{})
+			if err != nil {
+				if !kerr.IsNotFound(err) {
+					return false, errors.Wrap(err, "failed to delete custom resource")
+				}
+			}
+			return false, nil
+		}
+
 		return false, errors.Errorf("custom resource failed to converge, %v status is %v", statusPath, resourceStatus)
 	}
 
diff --git a/controllers/provisioners/eks/update.go b/controllers/provisioners/eks/update.go
index 7be4b41..d4ec123 100644
--- a/controllers/provisioners/eks/update.go
+++ b/controllers/provisioners/eks/update.go
@@ -37,6 +37,7 @@ func (ctx *EksInstanceGroupContext) Update() error {
 		rotationNeeded  bool
 		instanceGroup   = ctx.GetInstanceGroup()
 		state           = ctx.GetDiscoveredState()
+		status          = instanceGroup.GetStatus()
 		scalingConfig   = state.GetScalingConfiguration()
 		configuration   = instanceGroup.GetEKSConfiguration()
 		spec            = instanceGroup.GetEKSSpec()
@@ -123,6 +124,8 @@ func (ctx *EksInstanceGroupContext) Update() error {
 	}
 	if rotationNeeded {
 		instanceGroup.SetState(v1alpha1.ReconcileInitUpgrade)
+	} else {
+		status.SetStrategyRetryCount(0)
 	}
 
 	return nil
diff --git a/controllers/provisioners/eksfargate/eksfargate.go b/controllers/provisioners/eksfargate/eksfargate.go
index b275721..0e2f22a 100644
--- a/controllers/provisioners/eksfargate/eksfargate.go
+++ b/controllers/provisioners/eksfargate/eksfargate.go
@@ -207,12 +207,22 @@ func (ctx *FargateInstanceGroupContext) Create() error {
 	return nil
 }
 func (ctx *FargateInstanceGroupContext) CloudDiscovery() error {
+	var (
+		instanceGroup = ctx.GetInstanceGroup()
+		status        = instanceGroup.GetStatus()
+	)
+
 	profile, err := ctx.AwsWorker.DescribeFargateProfile()
 	if err != nil {
 		profile = &eks.FargateProfile{
 			Status: nil,
 		}
 	}
+
+	// set required status fields
+	status.SetCurrentMax(1)
+	status.SetCurrentMin(1)
+
 	if profile.Status == nil {
 		ctx.DiscoveredState.ProfileStatus = aws.StringValue(nil)
 	} else {
diff --git a/docs/EKS.md b/docs/EKS.md
index 152a99e..1a9ac2b 100644
--- a/docs/EKS.md
+++ b/docs/EKS.md
@@ -313,6 +313,12 @@ spec:
               args: ["echo", "{{ .InstanceGroup.Status.ActiveScalingGroupName }}"]
 ```
 
+#### Recovering from Upgrade Failures
+
+You can configure whether you want the controller to retry by setting the `spec.strategy.crd.maxRetries` field - if left unset it will default to 3, max retries can also be set to -1 which would retry until success.
+When the submitted resource fails, the controller will delete/recreate the resource up to configured amount of times, once the max retries are met, the instance-group will enter an error state and requeue with exponential backoff.
+In order to manually retry, you must delete the failed custom resource and either wait for the next reconcile, or trigger a reconcile by making a modifications to the instance group or restarting the controller.
+
 ## Spot instances
 
 You can switch to spot instances in two ways:


Test patch:
diff --git a/controllers/provisioners/eks/eks_test.go b/controllers/provisioners/eks/eks_test.go
index c6ef5fb..3f81cc4 100644
--- a/controllers/provisioners/eks/eks_test.go
+++ b/controllers/provisioners/eks/eks_test.go
@@ -141,7 +141,9 @@ func MockInstanceGroup() *v1alpha1.InstanceGroup {
 				},
 			},
 			AwsUpgradeStrategy: v1alpha1.AwsUpgradeStrategy{
-				CRDType:           &v1alpha1.CRDUpdateStrategy{},
+				CRDType: &v1alpha1.CRDUpdateStrategy{
+					MaxRetries: &v1alpha1.DefaultCRDStrategyMaxRetries,
+				},
 				RollingUpdateType: &v1alpha1.RollingUpdateStrategy{},
 			},
 		},
@@ -363,6 +365,7 @@ func MockTypeInfo(types ...MockInstanceTypeInfo) []*ec2.InstanceTypeInfo {
 }
 
 func MockAwsCRDStrategy(spec string) v1alpha1.AwsUpgradeStrategy {
+	infiniteRetries := -1
 	return v1alpha1.AwsUpgradeStrategy{
 		Type: kubeprovider.CRDStrategyName,
 		CRDType: &v1alpha1.CRDUpdateStrategy{
@@ -371,6 +374,7 @@ func MockAwsCRDStrategy(spec string) v1alpha1.AwsUpgradeStrategy {
 			StatusJSONPath:      ".status.dogStatus",
 			StatusSuccessString: "woof",
 			StatusFailureString: "grr",
+			MaxRetries:          &infiniteRetries,
 		},
 	}
 }