Changes as per PR comments

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go

@@ -224,14 +224,11 @@ func (s *ForwardingRuleSyncer) RemoveClustersFromStatus(clusters []string) error
 	return fmt.Errorf("errors in updating both http and https forwarding rules, http rule error: %s, https rule error: %s", httpErr, httpsErr)
 }
 
-// canIgnoreStatusUpdateError returns true iff either of the following is true:
+// canIgnoreStatusUpdateError returns true if and only if either of the following is true:
 // * err is nil
 // * err has http.StatusNotFound code
 func canIgnoreStatusUpdateError(err error) bool {
-	if err == nil || utils.IsHTTPErrorCode(err, http.StatusNotFound) {
-		return true
-	}
-	return false
+	return (err == nil || utils.IsHTTPErrorCode(err, http.StatusNotFound))
 }
 
 // removeClustersFromStatus removes the given list of clusters from the existing forwarding rule with the given name.
@@ -336,9 +333,9 @@ func (s *ForwardingRuleSyncer) desiredForwardingRule(lbName, ipAddress, targetPr
 	}, nil
 }
 
-func (s *ForwardingRuleSyncer) desiredForwardingRuleWithoutClusters(existingFR *compute.ForwardingRule, removeClusters []string) (*compute.ForwardingRule, error) {
+func (s *ForwardingRuleSyncer) desiredForwardingRuleWithoutClusters(existingFR *compute.ForwardingRule, clustersToRemove []string) (*compute.ForwardingRule, error) {
 	existingStatusStr := existingFR.Description
-	newStatusStr, err := status.RemoveClusters(existingStatusStr, removeClusters)
+	newStatusStr, err := status.RemoveClusters(existingStatusStr, clustersToRemove)
 	if err != nil {
 		return nil, fmt.Errorf("unexpected error in updating status to remove clusters on forwarding rule %s: %s", existingFR.Name, err)
 	}

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go

@@ -34,6 +34,7 @@ import (
 	ingressbe "k8s.io/ingress-gce/pkg/backends"
 	ingressig "k8s.io/ingress-gce/pkg/instances"
 	ingresslb "k8s.io/ingress-gce/pkg/loadbalancers"
+	ingressutils "k8s.io/ingress-gce/pkg/utils"
 	"k8s.io/kubernetes/pkg/cloudprovider/providers/gce"
 	"k8s.io/kubernetes/pkg/printers"
 
@@ -261,7 +262,7 @@ func (l *LoadBalancerSyncer) DeleteLoadBalancer(ing *v1beta1.Ingress) error {
 	return err
 }
 
-// DeleteLoadBalancer deletes the GCP resources associated with the L7 GCP load balancer for the given ingress.
+// RemoveFromClusters removes the given ingress from the clusters corresponding to the given clients.
 // TODO(nikhiljindal): Do not require the ingress yaml from users. Just the name should be enough. We can fetch ingress YAML from one of the clusters.
 func (l *LoadBalancerSyncer) RemoveFromClusters(ing *v1beta1.Ingress, removeClients map[string]kubeclient.Interface, forceUpdate bool) error {
 	client, cErr := getAnyClient(l.clients)
@@ -300,24 +301,27 @@ func (l *LoadBalancerSyncer) RemoveFromClusters(ing *v1beta1.Ingress, removeClie
 	}
 
 	// Convert the client map to an array of cluster names.
-	removeClusters := make([]string, len(removeClients))
+	clustersToRemove := make([]string, len(removeClients))
 	removeIndex := 0
 	for k := range removeClients {
-		removeClusters[removeIndex] = k
+		clustersToRemove[removeIndex] = k
 		removeIndex++
 	}
 	// Update the status at the end.
-	if statusErr := l.removeClustersFromStatus(l.lbName, removeClusters); statusErr != nil {
+	if statusErr := l.removeClustersFromStatus(l.lbName, clustersToRemove); statusErr != nil {
 		// Aggregate errors and return all at the end.
 		err = multierror.Append(err, statusErr)
 	}
 	return err
 }
 
-func (l *LoadBalancerSyncer) removeClustersFromStatus(lbName string, removeClusters []string) error {
+// removeClustersFromStatus updates the status of the given load balancer to remove the given list of clusters from it.
+// Since the status could be stored on either the url map or the forwarding rule, it tries updating both and returns an error if that fails.
+// Returns an http.StatusNotFound error if either the url map or the forwarding rule does not exist.
+func (l *LoadBalancerSyncer) removeClustersFromStatus(lbName string, clustersToRemove []string) error {
 	// Try updating status in both url map and forwarding rule.
-	frErr := l.frs.RemoveClustersFromStatus(removeClusters)
-	umErr := l.ums.RemoveClustersFromStatus(removeClusters)
+	frErr := l.frs.RemoveClustersFromStatus(clustersToRemove)
+	umErr := l.ums.RemoveClustersFromStatus(clustersToRemove)
 	if ingressutils.IsHTTPErrorCode(frErr, http.StatusNotFound) || ingressutils.IsHTTPErrorCode(umErr, http.StatusNotFound) {
 		return fmt.Errorf("load balancer %s does not exist", lbName)
 	}

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go

@@ -475,8 +475,8 @@ func TestRemoveClustersFromStatus(t *testing.T) {
 		}
 
 		// Remove the load balancer from the first cluster and verify that the load balancer status is updated appropriately.
-		removeClusters := []string{"cluster1"}
-		if err := lbc.removeClustersFromStatus(lbName, removeClusters); err != nil {
+		clustersToRemove := []string{"cluster1"}
+		if err := lbc.removeClustersFromStatus(lbName, clustersToRemove); err != nil {
 			t.Errorf("unexpected error in removing existing load balancer from clusters: %s", err)
 		}
 		// TODO: Remove this if once we update GetLoadBalancerStatus to read from urlmap as well.

app/kubemci/pkg/gcp/status/loadbalancerstatus.go

@@ -40,14 +40,14 @@ func FromString(str string) (*LoadBalancerStatus, error) {
 
 // RemoveClusters removes the given list of clusters from the given status string.
 // Returns NotFoundError if the given statusStr is not a valid LoadBalancerStatus.
-func RemoveClusters(statusStr string, removeClusters []string) (string, error) {
+func RemoveClusters(statusStr string, clustersToRemove []string) (string, error) {
 	status, err := FromString(statusStr)
 	if err != nil {
 		glog.V(2).Infof("error in parsing description %s: %s. Ignoring the error by assuming that this is because status is on some other resource and continuing", statusStr, err)
 		return statusStr, nil
 	}
 	removeMap := map[string]bool{}
-	for _, v := range removeClusters {
+	for _, v := range clustersToRemove {
 		removeMap[v] = true
 	}
 	var newClusters []string

app/kubemci/pkg/gcp/urlmap/urlmapsyncer.go

@@ -240,9 +240,9 @@ func (s *URLMapSyncer) desiredURLMap(lbName string, ing *v1beta1.Ingress, beMap
 }
 
 // desiredUrlMapWithoutClusters returns a desired url map based on the given existing map such that the given list of clusters is removed from the status.
-func (s *URLMapSyncer) desiredUrlMapWithoutClusters(existingUM *compute.UrlMap, removeClusters []string) (*compute.UrlMap, error) {
+func (s *URLMapSyncer) desiredUrlMapWithoutClusters(existingUM *compute.UrlMap, clustersToRemove []string) (*compute.UrlMap, error) {
 	existingStatusStr := existingUM.Description
-	newStatusStr, err := status.RemoveClusters(existingStatusStr, removeClusters)
+	newStatusStr, err := status.RemoveClusters(existingStatusStr, clustersToRemove)
 	if err != nil {
 		return nil, fmt.Errorf("unexpected error in updating status to remove clusters on url map %s: %s", existingUM.Name, err)
 	}

app/kubemci/pkg/gcp/urlmap/urlmapsyncer_test.go

@@ -170,13 +170,13 @@ func TestRemoveClustersFromStatus(t *testing.T) {
 		svcName: &compute.BackendService{},
 	}
 	clusters := []string{"cluster1", "cluster2"}
-	removeClusters := []string{"cluster1"}
+	clustersToRemove := []string{"cluster1"}
 	ump := ingresslb.NewFakeLoadBalancers("" /*name*/, nil /*namer*/)
 	namer := utilsnamer.NewNamer("mci1", lbName)
 	ums := NewURLMapSyncer(namer, ump)
 
 	// Should return an error when url map does not exist.
-	if err := ums.RemoveClustersFromStatus(removeClusters); err == nil {
+	if err := ums.RemoveClustersFromStatus(clustersToRemove); err == nil {
 		t.Errorf("expected NotFound error when url map does not exist, got nil err")
 	}
 
@@ -185,15 +185,15 @@ func TestRemoveClustersFromStatus(t *testing.T) {
 	}
 
 	// Should run silently when url map does not have a status (old url map)
-	if err := ums.RemoveClustersFromStatus(removeClusters); err != nil {
+	if err := ums.RemoveClustersFromStatus(clustersToRemove); err != nil {
 		t.Errorf("expected no error when url map does not have load balancer status, got: %s", err)
 	}
 
 	typedUMS := ums.(*URLMapSyncer)
 	if err := addStatus(typedUMS, lbName, clusters); err != nil {
 		t.Errorf("unexpected error in adding status to url map: %s", err)
 	}
-	if err := ums.RemoveClustersFromStatus(removeClusters); err != nil {
+	if err := ums.RemoveClustersFromStatus(clustersToRemove); err != nil {
 		t.Errorf("expected no error in updating load balancer status, got: %s", err)
 	}