Simplify error handling

- Most of the error handling in these batch update funcs can be removed. The caller only logs the errors anyway. - Fix a few rebase issues.
nginx · Sep 5, 2018 · 120adcd · 120adcd
1 parent ebfd452
commit 120adcd
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 48 deletions.
diff --git a/nginx-controller/controller/controller.go b/nginx-controller/controller/controller.go
@@ -244,14 +244,8 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc
 					lbc.syncQueue.enqueue(curSvc)
 					return
 				}
-				// attempt to avoid enqueuing the ingress for this service
-				oldSvc := old.(*api_v1.Service)
-				if !reflect.DeepEqual(curSvc.Spec.Ports, oldSvc.Spec.Ports) ||
-					!reflect.DeepEqual(curSvc.Spec.Type, oldSvc.Spec.Type) {
-					glog.V(3).Infof("Service %v changed, syncing", curSvc.Name)
-					lbc.enqueueIngressForService(curSvc)
-				}
-
+				glog.V(3).Infof("Service %v changed, syncing", curSvc.Name)
+				lbc.enqueueIngressForService(curSvc)
 			}
 		},
 	}
@@ -444,7 +438,7 @@ func (lbc *LoadBalancerController) syncEndp(task Task) {
 		ings := lbc.getIngressForEndpoints(obj)
 
 		var ingExes []*nginx.IngressEx
-		var mergableIngressesBatch []*nginx.MergeableIngresses
+		var mergableIngressesSlice []*nginx.MergeableIngresses
 
 		for i := range ings {
 			if !lbc.isNginxIngress(&ings[i]) {
@@ -456,7 +450,7 @@ func (lbc *LoadBalancerController) syncEndp(task Task) {
 					glog.Errorf("Ignoring Ingress %v(Minion): %v", ings[i].Name, err)
 					continue
 				}
-				if !lbc.cnf.HasIngress(master) {
+				if !lbc.cnf.HasMinion(master, &ings[i]) {
 					continue
 				}
 				mergeableIngresses, err := lbc.createMergableIngresses(master)
@@ -465,7 +459,7 @@ func (lbc *LoadBalancerController) syncEndp(task Task) {
 					continue
 				}
 
-				mergableIngressesBatch = append(mergableIngressesBatch, mergeableIngresses)
+				mergableIngressesSlice = append(mergableIngressesSlice, mergeableIngresses)
 				continue
 			}
 			if !lbc.cnf.HasIngress(&ings[i]) {
@@ -480,19 +474,20 @@ func (lbc *LoadBalancerController) syncEndp(task Task) {
 			ingExes = append(ingExes, ingEx)
 		}
 
-		if len(ingExes) == 0 {
-			return
+		if len(ingExes) > 0 {
+			glog.V(3).Infof("Updating Endpoints for %v", ingExes)
+			err = lbc.cnf.UpdateEndpoints(ingExes)
+			if err != nil {
+				glog.Errorf("Error updating endpoints for %v: %v", ingExes, err)
+			}
 		}
 
-		glog.V(3).Infof("Updating Endpoints for %v", ingExes)
-		lbc.cnf.UpdateEndpoints(ingExes)
-		if err != nil {
-			glog.Errorf("Error updating endpoints for %v: %v", ingExes, err)
-		}
-		glog.V(3).Infof("Updating Endpoints for %v", mergableIngressesBatch)
-		err = lbc.cnf.UpdateEndpointsMergeableIngress(mergableIngressesBatch)
-		if err != nil {
-			glog.Errorf("Error updating endpoints for %v: %v", mergableIngressesBatch, err)
+		if len(mergableIngressesSlice) > 0 {
+			glog.V(3).Infof("Updating Endpoints for %v", mergableIngressesSlice)
+			err = lbc.cnf.UpdateEndpointsMergeableIngress(mergableIngressesSlice)
+			if err != nil {
+				glog.Errorf("Error updating endpoints for %v: %v", mergableIngressesSlice, err)
+			}
 		}
 	}
 }
@@ -1315,9 +1310,8 @@ func (lbc *LoadBalancerController) isNginxIngress(ing *extensions.Ingress) bool
 			return class == lbc.ingressClass
 		}
 		return class == lbc.ingressClass || class == ""
-	} else {
-		return !lbc.useIngressClassOnly
 	}
+	return !lbc.useIngressClassOnly
 }
 
 // isHealthCheckEnabled checks if health checks are enabled so we can only query pods if enabled.

diff --git a/nginx-controller/nginx/configurator.go b/nginx-controller/nginx/configurator.go
@@ -994,48 +994,33 @@ func (cnf *Configurator) UpdateEndpoints(ingExes []*IngressEx) error {
 }
 
 // UpdateEndpointsMergeableIngress updates endpoints in NGINX configuration for a mergeable Ingress resource
-func (cnf *Configurator) UpdateEndpointsMergeableIngress(mergableIngressesBatch []*MergeableIngresses) error {
+func (cnf *Configurator) UpdateEndpointsMergeableIngress(mergableIngressesSlice []*MergeableIngresses) error {
 	reloadPlus := false
-	errors := []error{}
-	for i := range mergableIngressesBatch {
-		err := cnf.addOrUpdateMergeableIngress(mergableIngressesBatch[i])
+	for i := range mergableIngressesSlice {
+		err := cnf.addOrUpdateMergeableIngress(mergableIngressesSlice[i])
 		if err != nil {
-			glog.V(3).Infof("Error adding or updating ingress %v/%v: %v", mergableIngressesBatch[i].Master.Ingress.Namespace, mergableIngressesBatch[i].Master.Ingress.Name, err)
-			errors = append(errors, err)
-			continue
+			return fmt.Errorf("Error adding or updating mergeableIngress %v/%v: %v", mergableIngressesSlice[i].Master.Ingress.Namespace, mergableIngressesSlice[i].Master.Ingress.Name, err)
 		}
 
 		if cnf.isPlus() {
-			for _, ing := range mergableIngressesBatch[i].Minions {
+			for _, ing := range mergableIngressesSlice[i].Minions {
 				err = cnf.updatePlusEndpoints(ing)
 				if err != nil {
 					glog.Warningf("Couldn't update the endpoints via the API: %v; reloading configuration instead", err)
-					errors = append(errors, err)
 					reloadPlus = true
-					break
 				}
 			}
 		}
 	}
 	if cnf.isPlus() && !reloadPlus {
-		return consolodiateErrors(errors)
+		glog.V(3).Info("No need to reload nginx")
+		return nil
 	}
 
 	if err := cnf.nginx.Reload(); err != nil {
-		return fmt.Errorf("Error reloading NGINX: %v. additional errors: %v", err, consolodiateErrors(errors))
-	}
-	return consolodiateErrors(errors)
-}
-
-func consolodiateErrors(errors []error) error {
-	errorString := ""
-	if len(errors) == 0 {
-		return nil
+		return fmt.Errorf("Error reloading NGINX when updating endpoints for %v: %v", mergableIngressesSlice, err)
 	}
-	for _, e := range errors {
-		errorString = errorString + e.Error()
-	}
-	return fmt.Errorf(errorString)
+	return nil
 }
 
 func (cnf *Configurator) updatePlusEndpoints(ingEx *IngressEx) error {