From dbf224475da7d69540e3ca543741ededc99d9349 Mon Sep 17 00:00:00 2001 From: Alex Robinson Date: Wed, 6 May 2015 18:57:13 +0000 Subject: [PATCH] Require DeleteTCPLoadBalancer to be idempotent and change the service controller to rely on that, so that we won't strand partial resources from them anymore (target pools in GCE, pools in OpenStack, etc.). --- pkg/cloudprovider/cloud.go | 9 ++++++-- pkg/cloudprovider/fake/fake.go | 4 ++-- pkg/cloudprovider/gce/gce.go | 4 ++-- pkg/cloudprovider/openstack/openstack.go | 4 ++-- .../servicecontroller/servicecontroller.go | 23 +++---------------- 5 files changed, 16 insertions(+), 28 deletions(-) diff --git a/pkg/cloudprovider/cloud.go b/pkg/cloudprovider/cloud.go index 4075fc2b35d..402d9fbe30a 100644 --- a/pkg/cloudprovider/cloud.go +++ b/pkg/cloudprovider/cloud.go @@ -66,8 +66,13 @@ type TCPLoadBalancer interface { CreateTCPLoadBalancer(name, region string, externalIP net.IP, ports []int, hosts []string, affinityType api.AffinityType) (string, error) // UpdateTCPLoadBalancer updates hosts under the specified load balancer. UpdateTCPLoadBalancer(name, region string, hosts []string) error - // DeleteTCPLoadBalancer deletes a specified load balancer. - DeleteTCPLoadBalancer(name, region string) error + // EnsureTCPLoadBalancerDeleted deletes the specified load balancer if it + // exists, returning nil if the load balancer specified either didn't exist or + // was successfully deleted. + // This construction is useful because many cloud providers' load balancers + // have multiple underlying components, meaning a Get could say that the LB + // doesn't exist even if some part of it is still laying around. + EnsureTCPLoadBalancerDeleted(name, region string) error } // Instances is an abstract, pluggable interface for sets of instances. diff --git a/pkg/cloudprovider/fake/fake.go b/pkg/cloudprovider/fake/fake.go index cdabd2a0365..f102df10934 100644 --- a/pkg/cloudprovider/fake/fake.go +++ b/pkg/cloudprovider/fake/fake.go @@ -115,9 +115,9 @@ func (f *FakeCloud) UpdateTCPLoadBalancer(name, region string, hosts []string) e return f.Err } -// DeleteTCPLoadBalancer is a test-spy implementation of TCPLoadBalancer.DeleteTCPLoadBalancer. +// EnsureTCPLoadBalancerDeleted is a test-spy implementation of TCPLoadBalancer.EnsureTCPLoadBalancerDeleted. // It adds an entry "delete" into the internal method call record. -func (f *FakeCloud) DeleteTCPLoadBalancer(name, region string) error { +func (f *FakeCloud) EnsureTCPLoadBalancerDeleted(name, region string) error { f.addCall("delete") return f.Err } diff --git a/pkg/cloudprovider/gce/gce.go b/pkg/cloudprovider/gce/gce.go index 0b534b3cff2..80599711634 100644 --- a/pkg/cloudprovider/gce/gce.go +++ b/pkg/cloudprovider/gce/gce.go @@ -399,8 +399,8 @@ func (gce *GCECloud) UpdateTCPLoadBalancer(name, region string, hosts []string) return nil } -// DeleteTCPLoadBalancer is an implementation of TCPLoadBalancer.DeleteTCPLoadBalancer. -func (gce *GCECloud) DeleteTCPLoadBalancer(name, region string) error { +// EnsureTCPLoadBalancerDeleted is an implementation of TCPLoadBalancer.EnsureTCPLoadBalancerDeleted. +func (gce *GCECloud) EnsureTCPLoadBalancerDeleted(name, region string) error { op, err := gce.service.ForwardingRules.Delete(gce.projectID, region, name).Do() if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { glog.Infof("Forwarding rule %s already deleted. Continuing to delete target pool.", name) diff --git a/pkg/cloudprovider/openstack/openstack.go b/pkg/cloudprovider/openstack/openstack.go index 05663ae64f4..e9fa59bbf3f 100644 --- a/pkg/cloudprovider/openstack/openstack.go +++ b/pkg/cloudprovider/openstack/openstack.go @@ -631,8 +631,8 @@ func (lb *LoadBalancer) UpdateTCPLoadBalancer(name, region string, hosts []strin return nil } -func (lb *LoadBalancer) DeleteTCPLoadBalancer(name, region string) error { - glog.V(4).Infof("DeleteTCPLoadBalancer(%v, %v)", name, region) +func (lb *LoadBalancer) EnsureTCPLoadBalancerDeleted(name, region string) error { + glog.V(4).Infof("EnsureTCPLoadBalancerDeleted(%v, %v)", name, region) // TODO(#8352): Because we look up the pool using the VIP object, if the VIP // is already gone we can't attempt to delete the pool. We should instead diff --git a/pkg/cloudprovider/servicecontroller/servicecontroller.go b/pkg/cloudprovider/servicecontroller/servicecontroller.go index 1251bf19b52..20b50a2c2e8 100644 --- a/pkg/cloudprovider/servicecontroller/servicecontroller.go +++ b/pkg/cloudprovider/servicecontroller/servicecontroller.go @@ -207,7 +207,7 @@ func (s *ServiceController) processDelta(delta *cache.Delta) (error, bool) { cachedService.service = service s.cache.set(namespacedName.String(), cachedService) case cache.Deleted: - err := s.ensureLBDeleted(service) + err := s.balancer.EnsureTCPLoadBalancerDeleted(s.loadBalancerName(service), s.zone.Region) if err != nil { return err, retryable } @@ -230,7 +230,7 @@ func (s *ServiceController) createLoadBalancerIfNeeded(namespacedName types.Name // we can recreate it cleanly. if cachedService.Spec.CreateExternalLoadBalancer { glog.Infof("Deleting existing load balancer for service %s that needs an updated load balancer.", namespacedName) - if err := s.ensureLBDeleted(cachedService); err != nil { + if err := s.balancer.EnsureTCPLoadBalancerDeleted(s.loadBalancerName(cachedService), s.zone.Region); err != nil { return err, retryable } } @@ -251,7 +251,7 @@ func (s *ServiceController) createLoadBalancerIfNeeded(namespacedName types.Name } 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 { + if err := s.balancer.EnsureTCPLoadBalancerDeleted(s.loadBalancerName(service), s.zone.Region); err != nil { return err, retryable } } @@ -344,23 +344,6 @@ func (s *ServiceController) createExternalLoadBalancer(service *api.Service) err return nil } -// Ensures that the load balancer associated with the given service is deleted, -// doing the deletion if necessary. Should always be retried upon failure. -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.GetTCPLoadBalancer(s.loadBalancerName(service), s.zone.Region); err != nil { - return err - } else if !exists { - return nil - } - - if err := s.balancer.DeleteTCPLoadBalancer(s.loadBalancerName(service), s.zone.Region); err != nil { - return err - } - return nil -} - // ListKeys implements the interface required by DeltaFIFO to list the keys we // already know about. func (s *serviceCache) ListKeys() []string {