Merge pull request #7852 from a-robinson/tp

Prevent stranding of partial load balancers resources

pkg/cloudprovider/cloud.go

@@ -66,8 +66,13 @@ type TCPLoadBalancer interface {
 	CreateTCPLoadBalancer(name, region string, externalIP net.IP, ports []int, hosts []string, affinityType api.ServiceAffinity) (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)
@@ -411,6 +411,7 @@ func (gce *GCECloud) DeleteTCPLoadBalancer(name, region string) error {
 		err = gce.waitForRegionOp(op, region)
 		if err != nil {
 			glog.Warningf("Failed waiting for Forwarding Rule %s to be deleted: got error %s.", name, err.Error())
+			return err
 		}
 	}
 	op, err = gce.service.TargetPools.Delete(gce.projectID, region, name).Do()

pkg/cloudprovider/openstack/openstack.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"io"
 	"net"
+	"net/http"
 	"regexp"
 	"time"
 
@@ -630,31 +631,49 @@ 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)
 
-	vip, err := getVipByName(lb.network, name)
+	// TODO(#8352): Because we look up the pool using the VIP object, if the VIP
-	if err != nil {
+	// is already gone we can't attempt to delete the pool. We should instead
-		return err
+	// continue even if the VIP doesn't exist and attempt to delete the pool by
+	// name.
+	vip, vipErr := getVipByName(lb.network, name)
+	if vipErr == ErrNotFound {
+		return nil
+	} else if vipErr != nil {
+		return vipErr
 	}
 
-	pool, err := pools.Get(lb.network, vip.PoolID).Extract()
+	// It's ok if the pool doesn't exist, as we may still need to delete the vip
-	if err != nil {
+	// (although I don't believe the system should ever be in that state).
-		return err
+	pool, poolErr := pools.Get(lb.network, vip.PoolID).Extract()
+	if poolErr != nil {
+		detailedErr, ok := poolErr.(*gophercloud.UnexpectedResponseCodeError)
+		if !ok || detailedErr.Actual != http.StatusNotFound {
+			return poolErr
+		}
 	}
+	poolExists := (poolErr == nil)
 
-	// Have to delete VIP before pool can be deleted
+	// We have to delete the VIP before the pool can be deleted, so we can't
-	err = vips.Delete(lb.network, vip.ID).ExtractErr()
+	// continue on if this fails.
-	if err != nil {
+	// TODO(#8352): Only do this if the VIP exists once we can delete pools by
+	// name rather than by ID.
+	err := vips.Delete(lb.network, vip.ID).ExtractErr()
+	if err != nil && err != ErrNotFound {
 		return err
 	}
 
 	// Ignore errors for everything following here
 
-	for _, monId := range pool.MonitorIDs {
+	if poolExists {
-		pools.DisassociateMonitor(lb.network, pool.ID, monId)
+		for _, monId := range pool.MonitorIDs {
+			// TODO(#8352): Delete the monitor, don't just disassociate it.
+			pools.DisassociateMonitor(lb.network, pool.ID, monId)
+		}
+		pools.Delete(lb.network, pool.ID)
 	}
-	pools.Delete(lb.network, pool.ID)
 
 	return nil
 }

pkg/cloudprovider/servicecontroller/servicecontroller.go

@@ -210,7 +210,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
 		}
@@ -233,7 +233,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
 			}
 		}
@@ -254,7 +254,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
 			}
 		}
@@ -347,23 +347,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 {