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

@@ -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.
+	// EnsureTCPLoadBalancerDeleted deletes the specified load balancer if it
-	DeleteTCPLoadBalancer(name, region string) error
+	// 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.

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
 }

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.
+// EnsureTCPLoadBalancerDeleted is an implementation of TCPLoadBalancer.EnsureTCPLoadBalancerDeleted.
-func (gce *GCECloud) DeleteTCPLoadBalancer(name, region string) error {
+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)

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 {
+func (lb *LoadBalancer) EnsureTCPLoadBalancerDeleted(name, region string) error {
-	glog.V(4).Infof("DeleteTCPLoadBalancer(%v, %v)", name, region)
+	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

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 {