[GLBC] Specify balancing mode for backends being added to existing backend service (#652)

Add backends of proper type to backend service

controllers/gce/backends/backends.go

@@ -173,55 +173,18 @@ func (b *Backends) ensureHealthCheck(sp ServicePort) (string, error) {
 	return b.healthChecker.Sync(hc)
 }
 
-func (b *Backends) create(igs []*compute.InstanceGroup, namedPort *compute.NamedPort, hcLink string, protocol utils.AppProtocol, name string) (*compute.BackendService, error) {
+func (b *Backends) create(namedPort *compute.NamedPort, hcLink string, protocol utils.AppProtocol, name string) (*compute.BackendService, error) {
-	var errs []string
+	bs := &compute.BackendService{
-	// We first try to create the backend with balancingMode=RATE.  If this
-	// fails, it's mostly likely because there are existing backends with
-	// balancingMode=UTILIZATION. This failure mode throws a googleapi error
-	// which wraps a HTTP 400 status code. We handle it in the loop below
-	// and come around to retry with the right balancing mode. The goal is to
-	// switch everyone to using RATE.
-	for _, bm := range []BalancingMode{Rate, Utilization} {
-		// Create a new backend
-		bs := newBackendService(igs, bm, namedPort, []string{hcLink}, protocol, name)
-		if err := b.cloud.CreateBackendService(bs); err != nil {
-			// This is probably a failure because we tried to create the backend
-			// with balancingMode=RATE when there are already backends with
-			// balancingMode=UTILIZATION. Just ignore it and retry setting
-			// balancingMode=UTILIZATION (b/35102911).
-			if utils.IsHTTPErrorCode(err, http.StatusBadRequest) {
-				glog.Infof("Error creating backend service with balancing mode %v:%v", bm, err)
-				errs = append(errs, fmt.Sprintf("%v", err))
-				continue
-			}
-			return nil, err
-		}
-		return b.Get(namedPort.Port)
-	}
-	return nil, fmt.Errorf("%v", strings.Join(errs, "\n"))
-}
-
-func newBackendService(igs []*compute.InstanceGroup, bm BalancingMode, namedPort *compute.NamedPort, healthCheckLinks []string, protocol utils.AppProtocol, name string) *compute.BackendService {
-	backends := getBackendsForIGs(igs)
-	for _, b := range backends {
-		switch bm {
-		case Rate:
-			b.MaxRatePerInstance = maxRPS
-		default:
-			// TODO: Set utilization and connection limits when we accept them
-			// as valid fields.
-		}
-		b.BalancingMode = string(bm)
-	}
-
-	return &compute.BackendService{
 		Name:         name,
 		Protocol:     string(protocol),
-		Backends:     backends,
+		HealthChecks: []string{hcLink},
-		HealthChecks: healthCheckLinks,
 		Port:         namedPort.Port,
 		PortName:     namedPort.Name,
 	}
+	if err := b.cloud.CreateBackendService(bs); err != nil {
+		return nil, err
+	}
+	return b.Get(namedPort.Port)
 }
 
 // Add will get or create a Backend for the given port.
@@ -242,21 +205,22 @@ func (b *Backends) Add(p ServicePort) error {
 		return err
 	}
 
+	// Verify existance of a backend service for the proper port, but do not specify any backends/igs
 	pName := b.namer.BeName(p.Port)
 	be, _ = b.Get(p.Port)
 	if be == nil {
-		glog.V(2).Infof("Creating backend for %d instance groups, port %v named port %v", len(igs), p.Port, namedPort)
+		glog.V(2).Infof("Creating backend service for port %v named port %v", p.Port, namedPort)
-		be, err = b.create(igs, namedPort, hcLink, p.Protocol, pName)
+		be, err = b.create(namedPort, hcLink, p.Protocol, pName)
 		if err != nil {
 			return err
 		}
 	}
 
+	// Check that the backend service has the correct protocol and health check link
 	existingHCLink := ""
 	if len(be.HealthChecks) == 1 {
 		existingHCLink = be.HealthChecks[0]
 	}
-
 	if be.Protocol != string(p.Protocol) || existingHCLink != hcLink {
 		glog.V(2).Infof("Updating backend protocol %v (%v) for change in protocol (%v) or health check", pName, be.Protocol, string(p.Protocol))
 		be.Protocol = string(p.Protocol)
@@ -277,6 +241,8 @@ func (b *Backends) Add(p ServicePort) error {
 	if len(igs) == 0 {
 		return nil
 	}
+
+	// Verify that backend service contains links to all backends/instance-groups
 	return b.edgeHop(be, igs)
 }
 
@@ -315,10 +281,22 @@ func (b *Backends) List() ([]interface{}, error) {
 	return interList, nil
 }
 
-func getBackendsForIGs(igs []*compute.InstanceGroup) []*compute.Backend {
+func getBackendsForIGs(igs []*compute.InstanceGroup, bm BalancingMode) []*compute.Backend {
-	backends := []*compute.Backend{}
+	var backends []*compute.Backend
 	for _, ig := range igs {
-		backends = append(backends, &compute.Backend{Group: ig.SelfLink})
+		b := &compute.Backend{
+			Group:         ig.SelfLink,
+			BalancingMode: string(bm),
+		}
+		switch bm {
+		case Rate:
+			b.MaxRatePerInstance = maxRPS
+		default:
+			// TODO: Set utilization and connection limits when we accept them
+			// as valid fields.
+		}
+
+		backends = append(backends, b)
 	}
 	return backends
 }
@@ -337,20 +315,45 @@ func (b *Backends) edgeHop(be *compute.BackendService, igs []*compute.InstanceGr
 	if beIGs.IsSuperset(igLinks) {
 		return nil
 	}
-	glog.Infof("Backend %v has a broken edge, expected igs %+v, current igs %+v",
+	glog.V(2).Infof("Updating backend service %v with %d backends: expected igs %+v, current igs %+v",
-		be.Name, igLinks.List(), beIGs.List())
+		be.Name, igLinks.Len(), igLinks.List(), beIGs.List())
 
-	newBackends := []*compute.Backend{}
+	originalBackends := be.Backends
-	for _, b := range getBackendsForIGs(igs) {
+	var addIGs []*compute.InstanceGroup
-		if !beIGs.Has(b.Group) {
+	for _, ig := range igs {
-			newBackends = append(newBackends, b)
+		if !beIGs.Has(ig.SelfLink) {
+			addIGs = append(addIGs, ig)
 		}
 	}
-	be.Backends = append(be.Backends, newBackends...)
+
-	if err := b.cloud.UpdateBackendService(be); err != nil {
+	// We first try to create the backend with balancingMode=RATE.  If this
-		return err
+	// fails, it's mostly likely because there are existing backends with
+	// balancingMode=UTILIZATION. This failure mode throws a googleapi error
+	// which wraps a HTTP 400 status code. We handle it in the loop below
+	// and come around to retry with the right balancing mode. The goal is to
+	// switch everyone to using RATE.
+	var errs []string
+	for _, bm := range []BalancingMode{Rate, Utilization} {
+		// Generate backends with given instance groups with a specific mode
+		newBackends := getBackendsForIGs(addIGs, bm)
+		be.Backends = append(originalBackends, newBackends...)
+
+		if err := b.cloud.UpdateBackendService(be); err != nil {
+			if utils.IsHTTPErrorCode(err, http.StatusBadRequest) {
+				glog.V(2).Infof("Updating backend service backends with balancing mode %v failed, will try another mode. err:%v", bm, err)
+				errs = append(errs, err.Error())
+				// This is probably a failure because we tried to create the backend
+				// with balancingMode=RATE when there are already backends with
+				// balancingMode=UTILIZATION. Just ignore it and retry setting
+				// balancingMode=UTILIZATION (b/35102911).
+				continue
+			}
+			glog.V(2).Infof("Error updating backend service backends with balancing mode %v:%v", bm, err)
+			return err
+		}
+		return nil
 	}
-	return nil
+	return fmt.Errorf("received errors when updating backend service: %v", strings.Join(errs, "\n"))
 }
 
 // Sync syncs backend services corresponding to ports in the given list.

controllers/gce/backends/fakes.go

@@ -99,6 +99,11 @@ func (f *FakeBackendServices) ListBackendServices() (*compute.BackendServiceList
 
 // UpdateBackendService fakes updating a backend service.
 func (f *FakeBackendServices) UpdateBackendService(be *compute.BackendService) error {
+	if f.errFunc != nil {
+		if err := f.errFunc(utils.Update, be); err != nil {
+			return err
+		}
+	}
 	f.calls = append(f.calls, utils.Update)
 	return f.backendServices.Update(be)
 }