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 {