diff --git a/pkg/cloudprovider/cloud.go b/pkg/cloudprovider/cloud.go index 57f9d110cf5..1aff0212c5b 100644 --- a/pkg/cloudprovider/cloud.go +++ b/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. diff --git a/pkg/cloudprovider/fake/fake.go b/pkg/cloudprovider/fake/fake.go index 664fd175be9..2c7a8f1bfdf 100644 --- a/pkg/cloudprovider/fake/fake.go +++ b/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. diff --git a/pkg/cloudprovider/gce/gce.go b/pkg/cloudprovider/gce/gce.go index f3ac5b1736c..a56ff67eb99 100644 --- a/pkg/cloudprovider/gce/gce.go +++ b/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 { diff --git a/pkg/cloudprovider/openstack/openstack.go b/pkg/cloudprovider/openstack/openstack.go index 367d49bac11..1a2699b270c 100644 --- a/pkg/cloudprovider/openstack/openstack.go +++ b/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 diff --git a/pkg/cloudprovider/openstack/openstack_test.go b/pkg/cloudprovider/openstack/openstack_test.go index 8b3c0850841..3c7b50c4682 100644 --- a/pkg/cloudprovider/openstack/openstack_test.go +++ b/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") } } diff --git a/pkg/cloudprovider/servicecontroller/servicecontroller.go b/pkg/cloudprovider/servicecontroller/servicecontroller.go index 18d7b359423..24cd98c111e 100644 --- a/pkg/cloudprovider/servicecontroller/servicecontroller.go +++ b/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