Repository: keikoproj/instance-manager (go)

Issue:
Upgrade keeps replacing resource
Looks like we messed up the iteration back in #179 where we did:
```
for _, r := range r.Items {
```

There are also other issues leading to activeResource detection having the wrong resources in the list when many instance groups are upgrading simultaneously - the end result of this is that we keep deleting the upgrade resource and replacing it with a new one.

In addition, there is a problem when we go to upgrade straight after updating the ASG with a new launch config - since the cache still has a reference to old ASG with old launch config - this causes recreation of the upgrade resource after the initial submission. - we should requeue right after / if we make an update to the scaling group.

Pull request:
fix: upgrade resource replacement loop
Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>

Fixes #251 

- [x] BDD passing
- [x] Manual testing of all policies w/ template/config

Patch:
diff --git a/api/v1alpha1/instancegroup_types.go b/api/v1alpha1/instancegroup_types.go
index 9318bf7..8acb552 100644
--- a/api/v1alpha1/instancegroup_types.go
+++ b/api/v1alpha1/instancegroup_types.go
@@ -321,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"`
-	CurrentMax                    int                      `json:"currentMax"`
+	CurrentMin                    int                      `json:"currentMin,omitempty"`
+	CurrentMax                    int                      `json:"currentMax,omitempty"`
 	ActiveLaunchConfigurationName string                   `json:"activeLaunchConfigurationName,omitempty"`
 	ActiveLaunchTemplateName      string                   `json:"activeLaunchTemplateName,omitempty"`
 	LatestTemplateVersion         string                   `json:"latestTemplateVersion,omitempty"`
diff --git a/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml b/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml
index 358b8b8..32347cf 100644
--- a/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml
+++ b/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml
@@ -434,9 +434,6 @@ spec:
                 type: integer
               usingSpotRecommendation:
                 type: boolean
-            required:
-            - currentMax
-            - currentMin
             type: object
         required:
         - metadata
diff --git a/controllers/instancegroup_controller.go b/controllers/instancegroup_controller.go
index 0fb355c..0650f97 100644
--- a/controllers/instancegroup_controller.go
+++ b/controllers/instancegroup_controller.go
@@ -225,7 +225,9 @@ func (r *InstanceGroupReconciler) IsNamespaceAnnotated(namespace, key, value str
 		unstructuredNamespace := &unstructured.Unstructured{
 			Object: nsObject,
 		}
-		if kubeprovider.HasAnnotation(unstructuredNamespace, key, value) {
+
+		annotations := unstructuredNamespace.GetAnnotations()
+		if kubeprovider.HasAnnotation(annotations, key, value) {
 			return true
 		}
 	}
diff --git a/controllers/providers/aws/aws.go b/controllers/providers/aws/aws.go
index ebdbc2f..fe7329a 100644
--- a/controllers/providers/aws/aws.go
+++ b/controllers/providers/aws/aws.go
@@ -115,6 +115,7 @@ var (
 const (
 	LaunchTemplateStrategyCapacityOptimized = "capacity-optimized"
 	LaunchTemplateStrategyLowestPrice       = "lowest-price"
+	LaunchTemplateAllocationStrategy        = "prioritized"
 	LaunchTemplateLatestVersionKey          = "$Latest"
 	IAMPolicyPrefix                         = "arn:aws:iam::aws:policy"
 	IAMARNPrefix                            = "arn:aws:iam::"
diff --git a/controllers/providers/kubernetes/crd.go b/controllers/providers/kubernetes/crd.go
index 7cffef9..dcb4f5a 100644
--- a/controllers/providers/kubernetes/crd.go
+++ b/controllers/providers/kubernetes/crd.go
@@ -307,15 +307,15 @@ func IsResourceActive(kube dynamic.Interface, instanceGroup *v1alpha1.InstanceGr
 	return true
 }
 
-func GetResources(kube dynamic.Interface, instanceGroup *v1alpha1.InstanceGroup, resource *unstructured.Unstructured) ([]*unstructured.Unstructured, []*unstructured.Unstructured, error) {
+func GetResources(kube dynamic.Interface, instanceGroup *v1alpha1.InstanceGroup, resource *unstructured.Unstructured) ([]unstructured.Unstructured, []unstructured.Unstructured, error) {
 	var (
 		status            = instanceGroup.GetStatus()
 		strategy          = instanceGroup.GetUpgradeStrategy().GetCRDType()
 		statusJSONPath    = strategy.GetStatusJSONPath()
 		completedStatus   = strategy.GetStatusSuccessString()
 		errorStatus       = strategy.GetStatusFailureString()
-		activeResources   = make([]*unstructured.Unstructured, 0)
-		inactiveResources = make([]*unstructured.Unstructured, 0)
+		activeResources   = make([]unstructured.Unstructured, 0)
+		inactiveResources = make([]unstructured.Unstructured, 0)
 		GVR               = GetGVR(resource, strategy.GetCRDName())
 		resourceNamespace = resource.GetNamespace()
 	)
@@ -325,18 +325,17 @@ func GetResources(kube dynamic.Interface, instanceGroup *v1alpha1.InstanceGroup,
 		return inactiveResources, activeResources, err
 	}
 
-	for _, r := range r.Items {
+	for _, ru := range r.Items {
 
-		if !HasAnnotation(&r, OwnershipAnnotationKey, OwnershipAnnotationValue) || !HasAnnotation(&r, ScopeAnnotationKey, status.GetActiveScalingGroupName()) {
-			// skip resources not owned by controller
-			continue
-		}
+		annotations := ru.GetAnnotations()
 
-		if IsPathValue(r, statusJSONPath, completedStatus) || IsPathValue(r, statusJSONPath, errorStatus) {
-			// if resource is not completed and not failed, it must be still active
-			inactiveResources = append(inactiveResources, &r)
-		} else {
-			activeResources = append(activeResources, &r)
+		if HasAnnotation(annotations, OwnershipAnnotationKey, OwnershipAnnotationValue) && HasAnnotation(annotations, ScopeAnnotationKey, status.GetActiveScalingGroupName()) {
+			if IsPathValue(ru, statusJSONPath, completedStatus) || IsPathValue(ru, statusJSONPath, errorStatus) {
+				// if resource is not completed and not failed, it must be still active
+				inactiveResources = append(inactiveResources, ru)
+			} else {
+				activeResources = append(activeResources, ru)
+			}
 		}
 
 	}
diff --git a/controllers/providers/kubernetes/utils.go b/controllers/providers/kubernetes/utils.go
index 7b9ae55..7756938 100644
--- a/controllers/providers/kubernetes/utils.go
+++ b/controllers/providers/kubernetes/utils.go
@@ -119,10 +119,9 @@ func AddAnnotation(u *unstructured.Unstructured, key, value string) {
 	u.SetAnnotations(annotations)
 }
 
-func HasAnnotation(u *unstructured.Unstructured, key, value string) bool {
-	annotations := u.GetAnnotations()
+func HasAnnotation(annotations map[string]string, key, value string) bool {
 	if val, ok := annotations[key]; ok {
-		if val == value {
+		if strings.EqualFold(val, value) {
 			return true
 		}
 	}
diff --git a/controllers/provisioners/eks/helpers.go b/controllers/provisioners/eks/helpers.go
index e781d8c..f764441 100644
--- a/controllers/provisioners/eks/helpers.go
+++ b/controllers/provisioners/eks/helpers.go
@@ -1001,11 +1001,19 @@ func (ctx *EksInstanceGroupContext) GetDesiredMixedInstancesPolicy(name string)
 		allocationStrategy = awsprovider.LaunchTemplateStrategyLowestPrice
 	}
 
+	var baseCapacity *int64
+	if mixedPolicy.BaseCapacity == nil {
+		baseCapacity = aws.Int64(0)
+	} else {
+		baseCapacity = mixedPolicy.BaseCapacity
+	}
+
 	spotRatio := common.IntOrStrValue(mixedPolicy.SpotRatio)
 
 	policy := &autoscaling.MixedInstancesPolicy{
 		InstancesDistribution: &autoscaling.InstancesDistribution{
-			OnDemandBaseCapacity:                mixedPolicy.BaseCapacity,
+			OnDemandAllocationStrategy:          aws.String(awsprovider.LaunchTemplateAllocationStrategy),
+			OnDemandBaseCapacity:                baseCapacity,
 			SpotAllocationStrategy:              aws.String(allocationStrategy),
 			SpotInstancePools:                   mixedPolicy.SpotPools,
 			OnDemandPercentageAboveBaseCapacity: aws.Int64(int64(100 - spotRatio)),
diff --git a/controllers/provisioners/eks/update.go b/controllers/provisioners/eks/update.go
index d4ec123..471f768 100644
--- a/controllers/provisioners/eks/update.go
+++ b/controllers/provisioners/eks/update.go
@@ -100,7 +100,7 @@ func (ctx *EksInstanceGroupContext) Update() error {
 	}
 
 	// update scaling group
-	err = ctx.UpdateScalingGroup(config.Name, &scalingConfig)
+	updated, err := ctx.UpdateScalingGroup(config.Name, &scalingConfig)
 	if err != nil {
 		if aerr, ok := err.(awserr.Error); ok {
 			if aerr.Code() == autoscaling.ErrCodeScalingActivityInProgressFault {
@@ -111,6 +111,11 @@ func (ctx *EksInstanceGroupContext) Update() error {
 		return errors.Wrap(err, "failed to update scaling group")
 	}
 
+	if updated {
+		// requeue after scaling group update occurs to refresh cache
+		return nil
+	}
+
 	// we should try to bootstrap the role before we wait for nodes to be ready
 	// to avoid getting locked if someone made a manual change to aws-auth
 	if err = ctx.BootstrapNodes(); err != nil {
@@ -131,8 +136,9 @@ func (ctx *EksInstanceGroupContext) Update() error {
 	return nil
 }
 
-func (ctx *EksInstanceGroupContext) UpdateScalingGroup(configName string, scalingConfig *scaling.Configuration) error {
+func (ctx *EksInstanceGroupContext) UpdateScalingGroup(configName string, scalingConfig *scaling.Configuration) (bool, error) {
 	var (
+		asgUpdated    bool
 		instanceGroup = ctx.GetInstanceGroup()
 		spec          = instanceGroup.GetEKSSpec()
 		configuration = instanceGroup.GetEKSConfiguration()
@@ -174,9 +180,9 @@ func (ctx *EksInstanceGroupContext) UpdateScalingGroup(configName string, scalin
 	if ctx.ScalingGroupUpdateNeeded(configName) {
 		err := ctx.AwsWorker.UpdateScalingGroup(input)
 		if err != nil {
-			return err
+			return asgUpdated, err
 		}
-
+		asgUpdated = true
 		ctx.Log.Info("updated scaling group", "instancegroup", instanceGroup.GetName(), "scalinggroup", asgName)
 	}
 
@@ -186,23 +192,23 @@ func (ctx *EksInstanceGroupContext) UpdateScalingGroup(configName string, scalin
 	if ctx.TagsUpdateNeeded() {
 		err := ctx.AwsWorker.UpdateScalingGroupTags(tags, rmTags)
 		if err != nil {
-			return err
+			return asgUpdated, err
 		}
 		ctx.Log.Info("updated scaling group tags", "instancegroup", instanceGroup.GetName(), "scalinggroup", asgName)
 	}
 
 	if err := ctx.UpdateScalingProcesses(asgName); err != nil {
-		return err
+		return asgUpdated, err
 	}
 
 	if err := ctx.UpdateMetricsCollection(asgName); err != nil {
-		return err
+		return asgUpdated, err
 	}
 	if err := ctx.UpdateLifecycleHooks(asgName); err != nil {
-		return err
+		return asgUpdated, err
 	}
 
-	return nil
+	return asgUpdated, nil
 }
 
 func (ctx *EksInstanceGroupContext) TagsUpdateNeeded() bool {
@@ -269,6 +275,7 @@ func (ctx *EksInstanceGroupContext) ScalingGroupUpdateNeeded(configName string)
 		}
 	case scalingGroup.MixedInstancesPolicy != nil:
 		name = aws.StringValue(scalingGroup.MixedInstancesPolicy.LaunchTemplate.LaunchTemplateSpecification.LaunchTemplateName)
+		scalingGroup.MixedInstancesPolicy.LaunchTemplate.LaunchTemplateSpecification.LaunchTemplateId = nil
 		if desiredPolicy == nil {
 			return true
 		}
diff --git a/controllers/provisioners/eksfargate/eksfargate.go b/controllers/provisioners/eksfargate/eksfargate.go
index 0e2f22a..9607a71 100644
--- a/controllers/provisioners/eksfargate/eksfargate.go
+++ b/controllers/provisioners/eksfargate/eksfargate.go
@@ -207,11 +207,6 @@ 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{
@@ -219,10 +214,6 @@ func (ctx *FargateInstanceGroupContext) CloudDiscovery() error {
 		}
 	}
 
-	// set required status fields
-	status.SetCurrentMax(1)
-	status.SetCurrentMin(1)
-
 	if profile.Status == nil {
 		ctx.DiscoveredState.ProfileStatus = aws.StringValue(nil)
 	} else {


Test patch:
diff --git a/controllers/provisioners/eks/update_test.go b/controllers/provisioners/eks/update_test.go
index 079f4d2..b2c3c13 100644
--- a/controllers/provisioners/eks/update_test.go
+++ b/controllers/provisioners/eks/update_test.go
@@ -117,7 +117,7 @@ func TestUpdateWithDriftRotationPositive(t *testing.T) {
 	err = ctx.Update()
 	g.Expect(err).NotTo(gomega.HaveOccurred())
 	g.Expect(ctx.TagsUpdateNeeded()).To(gomega.BeTrue())
-	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileInitUpgrade))
+	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileModifying))
 }
 
 func TestUpdateWithLaunchTemplate(t *testing.T) {
@@ -214,7 +214,7 @@ func TestUpdateWithLaunchTemplate(t *testing.T) {
 	g.Expect(err).NotTo(gomega.HaveOccurred())
 	g.Expect(ec2Mock.CreateLaunchTemplateVersionCallCount).To(gomega.Equal(1))
 	g.Expect(ec2Mock.ModifyLaunchTemplateCallCount).To(gomega.Equal(1))
-	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileInitUpgrade))
+	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileModifying))
 
 	state := ctx.GetDiscoveredState()
 	ec2Mock.CreateLaunchTemplateVersionCallCount = 0
@@ -264,7 +264,7 @@ func TestUpdateWithLaunchTemplate(t *testing.T) {
 	g.Expect(err).NotTo(gomega.HaveOccurred())
 	g.Expect(ec2Mock.CreateLaunchTemplateVersionCallCount).To(gomega.Equal(1))
 	g.Expect(ec2Mock.ModifyLaunchTemplateCallCount).To(gomega.Equal(1))
-	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileInitUpgrade))
+	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileModifying))
 
 	ec2Mock.CreateLaunchTemplateVersionCallCount = 0
 	ec2Mock.ModifyLaunchTemplateCallCount = 0
@@ -292,7 +292,7 @@ func TestUpdateWithLaunchTemplate(t *testing.T) {
 	g.Expect(err).NotTo(gomega.HaveOccurred())
 	g.Expect(ec2Mock.CreateLaunchTemplateVersionCallCount).To(gomega.Equal(1))
 	g.Expect(ec2Mock.ModifyLaunchTemplateCallCount).To(gomega.Equal(1))
-	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileInitUpgrade))
+	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileModifying))
 }
 
 func TestUpdateWithRotationPositive(t *testing.T) {
@@ -369,7 +369,7 @@ func TestUpdateWithRotationPositive(t *testing.T) {
 
 	err = ctx.Update()
 	g.Expect(err).NotTo(gomega.HaveOccurred())
-	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileInitUpgrade))
+	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileModifying))
 }
 
 func TestLaunchConfigurationDrifted(t *testing.T) {