diff --git a/pkg/cloudprovider/cloud.go b/pkg/cloudprovider/cloud.go index 39ca7493606..50241fd28de 100644 --- a/pkg/cloudprovider/cloud.go +++ b/pkg/cloudprovider/cloud.go @@ -80,8 +80,8 @@ type TCPLoadBalancer interface { // GetTCPLoadBalancer returns whether the specified load balancer exists, and // if so, what its status is. GetTCPLoadBalancer(name, region string) (status *api.LoadBalancerStatus, exists bool, err error) - // CreateTCPLoadBalancer creates a new tcp load balancer. Returns the status of the balancer - CreateTCPLoadBalancer(name, region string, externalIP net.IP, ports []*api.ServicePort, hosts []string, affinityType api.ServiceAffinity) (*api.LoadBalancerStatus, error) + // EnsureTCPLoadBalancer creates a new tcp load balancer, or updates an existing one. Returns the status of the balancer + EnsureTCPLoadBalancer(name, region string, externalIP net.IP, ports []*api.ServicePort, hosts []string, affinityType api.ServiceAffinity) (*api.LoadBalancerStatus, error) // UpdateTCPLoadBalancer updates hosts under the specified load balancer. UpdateTCPLoadBalancer(name, region string, hosts []string) error // EnsureTCPLoadBalancerDeleted deletes the specified load balancer if it diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 3a049718e56..cf8381fe552 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -1584,8 +1584,23 @@ func (s *AWSCloud) createTags(request *ec2.CreateTagsInput) (*ec2.CreateTagsOutp // CreateTCPLoadBalancer implements TCPLoadBalancer.CreateTCPLoadBalancer // TODO(justinsb): This must be idempotent // TODO(justinsb) It is weird that these take a region. I suspect it won't work cross-region anwyay. -func (s *AWSCloud) CreateTCPLoadBalancer(name, region string, publicIP net.IP, ports []*api.ServicePort, hosts []string, affinity api.ServiceAffinity) (*api.LoadBalancerStatus, error) { - glog.V(2).Infof("CreateTCPLoadBalancer(%v, %v, %v, %v, %v)", name, region, publicIP, ports, hosts) +func (s *AWSCloud) EnsureTCPLoadBalancer(name, region string, publicIP net.IP, ports []*api.ServicePort, hosts []string, affinity api.ServiceAffinity) (*api.LoadBalancerStatus, error) { + glog.V(2).Infof("EnsureTCPLoadBalancer(%v, %v, %v, %v, %v)", name, region, publicIP, ports, hosts) + + glog.V(2).Info("Checking if load balancer already exists: %s", name) + _, exists, err := s.GetTCPLoadBalancer(name, region) + if err != nil { + return nil, fmt.Errorf("error checking if AWS load balancer already exists: %v", err) + } + + // TODO: Implement a more efficient update strategy for common changes than delete & create + // In particular, if we implement hosts update, we can get rid of UpdateHosts + if exists { + err := s.EnsureTCPLoadBalancerDeleted(name, region) + if err != nil { + return nil, fmt.Errorf("error deleting existing AWS load balancer: %v", err) + } + } elbClient, err := s.getELBClient(region) if err != nil { diff --git a/pkg/cloudprovider/providers/fake/fake.go b/pkg/cloudprovider/providers/fake/fake.go index c78bf3d5f50..f66eee0c96c 100644 --- a/pkg/cloudprovider/providers/fake/fake.go +++ b/pkg/cloudprovider/providers/fake/fake.go @@ -56,7 +56,7 @@ type FakeCloud struct { ClusterList []string MasterName string ExternalIP net.IP - Balancers []FakeBalancer + Balancers map[string]FakeBalancer UpdateCalls []FakeUpdateBalancerCall RouteMap map[string]*FakeRoute Lock sync.Mutex @@ -123,11 +123,14 @@ func (f *FakeCloud) GetTCPLoadBalancer(name, region string) (*api.LoadBalancerSt return status, f.Exists, f.Err } -// CreateTCPLoadBalancer is a test-spy implementation of TCPLoadBalancer.CreateTCPLoadBalancer. +// EnsureTCPLoadBalancer is a test-spy implementation of TCPLoadBalancer.EnsureTCPLoadBalancer. // It adds an entry "create" into the internal method call record. -func (f *FakeCloud) CreateTCPLoadBalancer(name, region string, externalIP net.IP, ports []*api.ServicePort, hosts []string, affinityType api.ServiceAffinity) (*api.LoadBalancerStatus, error) { +func (f *FakeCloud) EnsureTCPLoadBalancer(name, region string, externalIP net.IP, ports []*api.ServicePort, hosts []string, affinityType api.ServiceAffinity) (*api.LoadBalancerStatus, error) { f.addCall("create") - f.Balancers = append(f.Balancers, FakeBalancer{name, region, externalIP, ports, hosts}) + if f.Balancers == nil { + f.Balancers = make(map[string]FakeBalancer) + } + f.Balancers[name] = FakeBalancer{name, region, externalIP, ports, hosts} status := &api.LoadBalancerStatus{} status.Ingress = []api.LoadBalancerIngress{{IP: f.ExternalIP.String()}} diff --git a/pkg/cloudprovider/providers/gce/gce.go b/pkg/cloudprovider/providers/gce/gce.go index fdbb8c595c0..8d9d0b42987 100644 --- a/pkg/cloudprovider/providers/gce/gce.go +++ b/pkg/cloudprovider/providers/gce/gce.go @@ -348,11 +348,26 @@ func makeFirewallName(name string) string { return fmt.Sprintf("k8s-fw-%s", name) } -// CreateTCPLoadBalancer is an implementation of TCPLoadBalancer.CreateTCPLoadBalancer. +// EnsureTCPLoadBalancer is an implementation of TCPLoadBalancer.EnsureTCPLoadBalancer. // TODO(a-robinson): Don't just ignore specified IP addresses. Check if they're // owned by the project and available to be used, and use them if they are. -func (gce *GCECloud) CreateTCPLoadBalancer(name, region string, externalIP net.IP, ports []*api.ServicePort, hosts []string, affinityType api.ServiceAffinity) (*api.LoadBalancerStatus, error) { - err := gce.makeTargetPool(name, region, hosts, translateAffinityType(affinityType)) +func (gce *GCECloud) EnsureTCPLoadBalancer(name, region string, externalIP net.IP, ports []*api.ServicePort, hosts []string, affinityType api.ServiceAffinity) (*api.LoadBalancerStatus, error) { + glog.V(2).Info("Checking if load balancer already exists: %s", name) + _, exists, err := gce.GetTCPLoadBalancer(name, region) + if err != nil { + return nil, fmt.Errorf("error checking if GCE load balancer already exists: %v", err) + } + + // TODO: Implement a more efficient update strategy for common changes than delete & create + // In particular, if we implement hosts update, we can get rid of UpdateHosts + if exists { + err := gce.EnsureTCPLoadBalancerDeleted(name, region) + if err != nil { + return nil, fmt.Errorf("error deleting existing GCE load balancer: %v", err) + } + } + + err = gce.makeTargetPool(name, region, hosts, translateAffinityType(affinityType)) if err != nil { if !isHTTPErrorCode(err, http.StatusConflict) { return nil, err diff --git a/pkg/cloudprovider/providers/openstack/openstack.go b/pkg/cloudprovider/providers/openstack/openstack.go index 49fdb1882ea..0f42f38b52f 100644 --- a/pkg/cloudprovider/providers/openstack/openstack.go +++ b/pkg/cloudprovider/providers/openstack/openstack.go @@ -521,8 +521,8 @@ func (lb *LoadBalancer) GetTCPLoadBalancer(name, region string) (*api.LoadBalanc // a list of regions (from config) and query/create loadbalancers in // each region. -func (lb *LoadBalancer) CreateTCPLoadBalancer(name, region string, externalIP net.IP, ports []*api.ServicePort, hosts []string, affinity api.ServiceAffinity) (*api.LoadBalancerStatus, error) { - glog.V(4).Infof("CreateTCPLoadBalancer(%v, %v, %v, %v, %v, %v)", name, region, externalIP, ports, hosts, affinity) +func (lb *LoadBalancer) EnsureTCPLoadBalancer(name, region string, externalIP net.IP, ports []*api.ServicePort, hosts []string, affinity api.ServiceAffinity) (*api.LoadBalancerStatus, error) { + glog.V(4).Infof("EnsureTCPLoadBalancer(%v, %v, %v, %v, %v, %v)", name, region, externalIP, ports, hosts, affinity) if len(ports) > 1 { return nil, fmt.Errorf("multiple ports are not yet supported in openstack load balancers") @@ -538,6 +538,21 @@ func (lb *LoadBalancer) CreateTCPLoadBalancer(name, region string, externalIP ne return nil, fmt.Errorf("unsupported load balancer affinity: %v", affinity) } + glog.V(2).Info("Checking if openstack load balancer already exists: %s", name) + _, exists, err := lb.GetTCPLoadBalancer(name, region) + if err != nil { + return nil, fmt.Errorf("error checking if openstack load balancer already exists: %v", err) + } + + // TODO: Implement a more efficient update strategy for common changes than delete & create + // In particular, if we implement hosts update, we can get rid of UpdateHosts + if exists { + err := lb.EnsureTCPLoadBalancerDeleted(name, region) + if err != nil { + return nil, fmt.Errorf("error deleting existing openstack load balancer: %v", err) + } + } + lbmethod := lb.opts.LBMethod if lbmethod == "" { lbmethod = pools.LBMethodRoundRobin diff --git a/pkg/controller/service/servicecontroller.go b/pkg/controller/service/servicecontroller.go index d8022dbd84b..9382109f812 100644 --- a/pkg/controller/service/servicecontroller.go +++ b/pkg/controller/service/servicecontroller.go @@ -335,7 +335,7 @@ func (s *ServiceController) createLoadBalancerIfNeeded(namespacedName types.Name return fmt.Errorf("Failed to persist updated status to apiserver, even after retries. Giving up: %v", err), notRetryable } } else { - glog.Infof("Not persisting unchanged LoadBalancerStatus to registry.") + glog.V(2).Infof("Not persisting unchanged LoadBalancerStatus to registry.") } return nil, notRetryable @@ -383,7 +383,7 @@ func (s *ServiceController) createExternalLoadBalancer(service *api.Service) err for _, publicIP := range service.Spec.DeprecatedPublicIPs { // TODO: Make this actually work for multiple IPs by using different // names for each. For now, we'll just create the first and break. - status, err := s.balancer.CreateTCPLoadBalancer(name, s.zone.Region, net.ParseIP(publicIP), + status, err := s.balancer.EnsureTCPLoadBalancer(name, s.zone.Region, net.ParseIP(publicIP), ports, hostsFromNodeList(&nodes), service.Spec.SessionAffinity) if err != nil { return err @@ -393,7 +393,7 @@ func (s *ServiceController) createExternalLoadBalancer(service *api.Service) err break } } else { - status, err := s.balancer.CreateTCPLoadBalancer(name, s.zone.Region, nil, + status, err := s.balancer.EnsureTCPLoadBalancer(name, s.zone.Region, nil, ports, hostsFromNodeList(&nodes), service.Spec.SessionAffinity) if err != nil { return err diff --git a/pkg/controller/service/servicecontroller_test.go b/pkg/controller/service/servicecontroller_test.go index 28e55cd50ff..2d443494fce 100644 --- a/pkg/controller/service/servicecontroller_test.go +++ b/pkg/controller/service/servicecontroller_test.go @@ -110,12 +110,22 @@ func TestCreateExternalLoadBalancer(t *testing.T) { t.Errorf("unexpected client actions: %v", actions) } } else { - if len(cloud.Balancers) != 1 { - t.Errorf("expected one load balancer to be created, got %v", cloud.Balancers) - } else if cloud.Balancers[0].Name != controller.loadBalancerName(item.service) || - cloud.Balancers[0].Region != region || - cloud.Balancers[0].Ports[0].Port != item.service.Spec.Ports[0].Port { - t.Errorf("created load balancer has incorrect parameters: %v", cloud.Balancers[0]) + var balancer *fake_cloud.FakeBalancer + for k := range cloud.Balancers { + if balancer == nil { + b := cloud.Balancers[k] + balancer = &b + } else { + t.Errorf("expected one load balancer to be created, got %v", cloud.Balancers) + break + } + } + if balancer == nil { + t.Errorf("expected one load balancer to be created, got none") + } else if balancer.Name != controller.loadBalancerName(item.service) || + balancer.Region != region || + balancer.Ports[0].Port != item.service.Spec.Ports[0].Port { + t.Errorf("created load balancer has incorrect parameters: %v", balancer) } actionFound := false for _, action := range actions {