Merge pull request #444 from kubernetes-sigs/reconcile-work

Working on TG reconcile logic

Author: bigkraig

pkg/alb/lb/loadbalancer.go

@@ -332,14 +332,15 @@ func (l *LoadBalancer) Reconcile(rOpts *ReconcileOptions) []error {
 		Eventf:            rOpts.Eventf,
 		VpcID:             l.lb.current.VpcId,
 		ManagedSGInstance: l.options.current.managedInstanceSG,
+		IgnoreDeletes:     true,
 	}
 
-	tgs, deletedTG, err := l.targetgroups.Reconcile(tgsOpts)
+	tgs, err := l.targetgroups.Reconcile(tgsOpts)
 	if err != nil {
 		errors = append(errors, err)
-		return errors
+	} else {
+		l.targetgroups = tgs
 	}
-	l.targetgroups = tgs
 
 	lsOpts := &ls.ReconcileOptions{
 		Eventf:          rOpts.Eventf,
@@ -352,33 +353,19 @@ func (l *LoadBalancer) Reconcile(rOpts *ReconcileOptions) []error {
 		l.listeners = ltnrs
 	}
 
-	// REFACTOR!
-	// This chunk of code has some questionable logic and we should probably move
-	// the TG clean up out of here and into tg. I also dont think that lb.listeners < 1 is a valid check
-	//
-	// Return now if listeners are already deleted, signifies has already been destructed and
-	// TG clean-up, based on rules below does not need to occur.
-	if len(l.listeners) < 1 {
-		for _, t := range deletedTG {
-			if err := albelbv2.ELBV2svc.RemoveTargetGroup(t.CurrentARN()); err != nil {
-				errors = append(errors, err)
-				return errors
-			}
-			index, _ := l.targetgroups.FindById(t.ID)
-			l.targetgroups = append(l.targetgroups[:index], l.targetgroups[index+1:]...)
-		}
-		return errors
+	// Decide: Is this still needed?
+	for _, listener := range l.listeners {
+		unusedTGs := listener.GetRules().FindUnusedTGs(l.targetgroups)
+		unusedTGs.StripDesiredState()
 	}
-	unusedTGs := l.listeners[0].GetRules().FindUnusedTGs(l.targetgroups)
-	for _, t := range unusedTGs {
-		if err := albelbv2.ELBV2svc.RemoveTargetGroup(t.CurrentARN()); err != nil {
-			errors = append(errors, err)
-			return errors
-		}
-		index, _ := l.targetgroups.FindById(t.ID)
-		l.targetgroups = append(l.targetgroups[:index], l.targetgroups[index+1:]...)
+
+	tgsOpts.IgnoreDeletes = false
+	tgs, err = l.targetgroups.Reconcile(tgsOpts)
+	if err != nil {
+		errors = append(errors, err)
+	} else {
+		l.targetgroups = tgs
 	}
-	// END REFACTOR
 
 	return errors
 }
@@ -564,7 +551,7 @@ func (l *LoadBalancer) modify(rOpts *ReconcileOptions) error {
 
 		// Modify Tags
 		if needsMod&tagsModified != 0 {
-			l.logger.Infof("Modifying ELBV2 tags to %v.", log.Prettify(l.tags.current))
+			l.logger.Infof("Modifying ELBV2 tags to %v.", log.Prettify(l.tags.desired))
 			if err := albelbv2.ELBV2svc.UpdateTags(l.lb.current.LoadBalancerArn, l.tags.current, l.tags.desired); err != nil {
 				rOpts.Eventf(api.EventTypeWarning, "ERROR", "%s tag modification failed: %s", *l.lb.current.LoadBalancerName, err.Error())
 				return fmt.Errorf("Failed ELBV2 tag modification: %s", err.Error())

pkg/alb/tg/targetgroup.go

@@ -144,9 +144,13 @@ func (t *TargetGroup) Reconcile(rOpts *ReconcileOptions) error {
 	// No DesiredState means target group may not be needed.
 	// However, target groups aren't deleted until after rules are created
 	// Ensuring we know what target groups are truly no longer in use.
-	case t.tg.desired == nil:
-		t.deleted = true
-		return nil
+	case t.tg.desired == nil && !rOpts.IgnoreDeletes:
+		t.logger.Infof("Start TargetGroup deletion.")
+		if err := t.delete(rOpts); err != nil {
+			return err
+		}
+		rOpts.Eventf(api.EventTypeNormal, "DELETE", "%v target group deleted", t.ID)
+		t.logger.Infof("Completed TargetGroup deletion.")
 
 		// No CurrentState means target group doesn't exist in AWS and should be created.
 	case t.tg.current == nil:
@@ -302,6 +306,16 @@ func (t *TargetGroup) modify(mods tgChange, rOpts *ReconcileOptions) error {
 	return nil
 }
 
+// delete a TargetGroup.
+func (t *TargetGroup) delete(rOpts *ReconcileOptions) error {
+	if err := albelbv2.ELBV2svc.RemoveTargetGroup(t.CurrentARN()); err != nil {
+		rOpts.Eventf(api.EventTypeWarning, "ERROR", "Error deleting %v target group: %s", t.ID, err.Error())
+		return err
+	}
+	t.deleted = true
+	return nil
+}
+
 func (t *TargetGroup) needsModification() tgChange {
 	var changes tgChange
 

pkg/alb/tg/targetgroups.go

@@ -48,20 +48,20 @@ func (t TargetGroups) FindCurrentByARN(id string) (int, *TargetGroup) {
 // Reconcile kicks off the state synchronization for every target group inside this TargetGroups
 // instance. It returns the new TargetGroups its created and a list of TargetGroups it believes
 // should be cleaned up.
-func (t TargetGroups) Reconcile(rOpts *ReconcileOptions) (TargetGroups, TargetGroups, error) {
+func (t TargetGroups) Reconcile(rOpts *ReconcileOptions) (TargetGroups, error) {
 	var output TargetGroups
-	var deleted TargetGroups
+
 	for _, tg := range t {
 		if err := tg.Reconcile(rOpts); err != nil {
-			return nil, nil, err
+			return nil, err
 		}
-		if tg.deleted {
-			deleted = append(deleted, tg)
+
+		if !tg.deleted {
+			output = append(output, tg)
 		}
-		output = append(output, tg)
 	}
 
-	return output, deleted, nil
+	return output, nil
 }
 
 // StripDesiredState removes the Tags.Desired, DesiredTargetGroup, and Targets.Desired from all TargetGroups

pkg/alb/tg/types.go

@@ -48,6 +48,7 @@ type ReconcileOptions struct {
 	Eventf            func(string, string, string, ...interface{})
 	VpcID             *string
 	ManagedSGInstance *string
+	IgnoreDeletes     bool
 }
 
 type tgChange uint