Merge pull request #7669 from a-robinson/lb

pkg/cloudprovider/cloud.go

@@ -58,11 +58,10 @@ func GetLoadBalancerName(service *api.Service) string {
 
 // TCPLoadBalancer is an abstract, pluggable interface for TCP load balancers.
 type TCPLoadBalancer interface {
-	// TCPLoadBalancerExists returns whether the specified load balancer exists.
 	// TODO: Break this up into different interfaces (LB, etc) when we have more than one type of service
-	// TODO: This should really return the details of the load balancer so we can
-	// determine if it matches the needs of a service rather than if it exists.
-	TCPLoadBalancerExists(name, region string) (bool, error)
+	// GetTCPLoadBalancer returns whether the specified load balancer exists, and
+	// if so, what its IP address or hostname is.
+	GetTCPLoadBalancer(name, region string) (endpoint string, exists bool, err error)
 	// CreateTCPLoadBalancer creates a new tcp load balancer. Returns the IP address or hostname of the balancer
 	CreateTCPLoadBalancer(name, region string, externalIP net.IP, ports []int, hosts []string, affinityType api.AffinityType) (string, error)
 	// UpdateTCPLoadBalancer updates hosts under the specified load balancer.

pkg/cloudprovider/fake/fake.go

@@ -94,9 +94,9 @@ func (f *FakeCloud) Zones() (cloudprovider.Zones, bool) {
 	return f, true
 }
 
-// TCPLoadBalancerExists is a stub implementation of TCPLoadBalancer.TCPLoadBalancerExists.
-func (f *FakeCloud) TCPLoadBalancerExists(name, region string) (bool, error) {
-	return f.Exists, f.Err
+// GetTCPLoadBalancer is a stub implementation of TCPLoadBalancer.GetTCPLoadBalancer.
+func (f *FakeCloud) GetTCPLoadBalancer(name, region string) (endpoint string, exists bool, err error) {
+	return f.ExternalIP.String(), f.Exists, f.Err
 }
 
 // CreateTCPLoadBalancer is a test-spy implementation of TCPLoadBalancer.CreateTCPLoadBalancer.

pkg/cloudprovider/gce/gce.go

@@ -239,16 +239,16 @@ func (gce *GCECloud) waitForRegionOp(op *compute.Operation, region string) error
 	return nil
 }
 
-// TCPLoadBalancerExists is an implementation of TCPLoadBalancer.TCPLoadBalancerExists.
-func (gce *GCECloud) TCPLoadBalancerExists(name, region string) (bool, error) {
-	_, err := gce.service.ForwardingRules.Get(gce.projectID, region, name).Do()
+// GetTCPLoadBalancer is an implementation of TCPLoadBalancer.GetTCPLoadBalancer
+func (gce *GCECloud) GetTCPLoadBalancer(name, region string) (endpoint string, exists bool, err error) {
+	fw, err := gce.service.ForwardingRules.Get(gce.projectID, region, name).Do()
 	if err == nil {
-		return true, nil
+		return fw.IPAddress, true, nil
 	}
 	if isHTTPErrorCode(err, http.StatusNotFound) {
-		return false, nil
+		return "", false, nil
 	}
-	return false, err
+	return "", false, err
 }
 
 func isHTTPErrorCode(err error, code int) bool {

pkg/cloudprovider/openstack/openstack.go

@@ -456,12 +456,15 @@ func getVipByName(client *gophercloud.ServiceClient, name string) (*vips.Virtual
 	return &vipList[0], nil
 }
 
-func (lb *LoadBalancer) TCPLoadBalancerExists(name, region string) (bool, error) {
+func (lb *LoadBalancer) GetTCPLoadBalancer(name, region string) (endpoint string, exists bool, err error) {
 	vip, err := getVipByName(lb.network, name)
 	if err == ErrNotFound {
-		return false, nil
+		return "", false, nil
 	}
-	return vip != nil, err
+	if vip == nil {
+		return "", false, err
+	}
+	return vip.Address, true, err
 }
 
 // TODO: This code currently ignores 'region' and always creates a

pkg/cloudprovider/openstack/openstack_test.go

@@ -173,12 +173,12 @@ func TestTCPLoadBalancer(t *testing.T) {
 		t.Fatalf("TCPLoadBalancer() returned false - perhaps your stack doesn't support Neutron?")
 	}
 
-	exists, err := lb.TCPLoadBalancerExists("noexist", "region")
+	_, exists, err := lb.GetTCPLoadBalancer("noexist", "region")
 	if err != nil {
-		t.Fatalf("TCPLoadBalancerExists(\"noexist\") returned error: %s", err)
+		t.Fatalf("GetTCPLoadBalancer(\"noexist\") returned error: %s", err)
 	}
 	if exists {
-		t.Fatalf("TCPLoadBalancerExists(\"noexist\") returned true")
+		t.Fatalf("GetTCPLoadBalancer(\"noexist\") returned exists")
 	}
 }
 

pkg/cloudprovider/servicecontroller/servicecontroller.go

@@ -235,26 +235,25 @@ func (s *ServiceController) createLoadBalancerIfNeeded(namespacedName types.Name
 			}
 		}
 	} else {
-		// If we don't have any cached memory of the load balancer and it already
-		// exists, optimistically consider our work done.
-		// TODO: If we could read the spec of the existing load balancer, we could
-		// determine if an update is necessary.
-		exists, err := s.balancer.TCPLoadBalancerExists(s.loadBalancerName(service), s.zone.Region)
+		// If we don't have any cached memory of the load balancer, we have to ask
+		// the cloud provider for what it knows about it.
+		endpoint, exists, err := s.balancer.GetTCPLoadBalancer(s.loadBalancerName(service), s.zone.Region)
 		if err != nil {
 			return fmt.Errorf("Error getting LB for service %s", namespacedName), retryable
 		}
-		if exists && len(service.Spec.PublicIPs) == 0 {
-			// The load balancer exists, but we apparently don't know about its public
-			// IPs, so just delete it and recreate it to get back to a sane state.
-			// TODO: Ideally the cloud provider interface would return the IP for us.
-			glog.Infof("Deleting old LB for service with no public IPs %s", namespacedName)
+		if exists && stringSlicesEqual(service.Spec.PublicIPs, []string{endpoint}) {
+			// TODO: If we could read more of the spec (ports, affinityType) of the
+			// existing load balancer, we could better determine if an update is
+			// necessary in more cases. For now, we optimistically assume that a
+			// matching IP suffices.
+			glog.Infof("LB already exists with endpoint %s for previously uncached service %s", endpoint, namespacedName)
+			return nil, notRetryable
+		} else if exists {
+			glog.Infof("Deleting old LB for previously uncached service %s whose endpoint %s doesn't match the service's desired IPs %v",
+				namespacedName, endpoint, service.Spec.PublicIPs)
 			if err := s.ensureLBDeleted(service); err != nil {
 				return err, retryable
 			}
-		} else if exists {
-			// TODO: Better handle updates for non-cached services, this is optimistic.
-			glog.Infof("LB already exists for service %s", namespacedName)
-			return nil, notRetryable
 		}
 	}
 
@@ -350,7 +349,7 @@ func (s *ServiceController) createExternalLoadBalancer(service *api.Service) err
 func (s *ServiceController) ensureLBDeleted(service *api.Service) error {
 	// This is only needed because not all delete load balancer implementations
 	// are currently idempotent to the LB not existing.
-	if exists, err := s.balancer.TCPLoadBalancerExists(s.loadBalancerName(service), s.zone.Region); err != nil {
+	if _, exists, err := s.balancer.GetTCPLoadBalancer(s.loadBalancerName(service), s.zone.Region); err != nil {
 		return err
 	} else if !exists {
 		return nil
@@ -578,7 +577,7 @@ func (s *ServiceController) lockedUpdateLoadBalancerHosts(service *api.Service,
 	}
 
 	// It's only an actual error if the load balancer still exists.
-	if exists, err := s.balancer.TCPLoadBalancerExists(name, s.zone.Region); err != nil {
+	if _, exists, err := s.balancer.GetTCPLoadBalancer(name, s.zone.Region); err != nil {
 		glog.Errorf("External error while checking if TCP load balancer %q exists: name, %v")
 	} else if !exists {
 		return nil