From 87df1d6fb687ca36cdae0a7738b83beaa7e5d9e6 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Sat, 13 Jun 2015 11:58:39 -0400 Subject: [PATCH] Change CreateTCPLoadBalancer -> EnsureTCPLoadBalancer; implementations auto-delete if already exists Previously the servicecontroller would do the delete, but by having the cloudprovider take that task on, we can later remove it from the servicecontroller, and the cloudprovider can do something more efficient. --- pkg/cloudprovider/cloud.go | 4 ++-- pkg/cloudprovider/providers/aws/aws.go | 19 ++++++++++++++-- pkg/cloudprovider/providers/fake/fake.go | 11 ++++++---- pkg/cloudprovider/providers/gce/gce.go | 21 +++++++++++++++--- .../providers/openstack/openstack.go | 19 ++++++++++++++-- pkg/controller/service/servicecontroller.go | 6 ++--- .../service/servicecontroller_test.go | 22 ++++++++++++++----- 7 files changed, 80 insertions(+), 22 deletions(-) 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 {