diff --git a/pkg/cloudprovider/cloud.go b/pkg/cloudprovider/cloud.go index 59418aeb40d..293b8be9dab 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.ServiceAffinity) (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 c0daf8832cf..8c0234093de 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 f63569d7462..5314303853c 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) @@ -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() diff --git a/pkg/cloudprovider/openstack/openstack.go b/pkg/cloudprovider/openstack/openstack.go index 5f80d97b316..e2d94ee8c5b 100644 --- a/pkg/cloudprovider/openstack/openstack.go +++ b/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 { - 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) - vip, err := getVipByName(lb.network, name) - if err != nil { - return err + // 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 + // 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() - if err != nil { - return err + // It's ok if the pool doesn't exist, as we may still need to delete the vip + // (although I don't believe the system should ever be in that state). + 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 - err = vips.Delete(lb.network, vip.ID).ExtractErr() - if err != nil { + // We have to delete the VIP before the pool can be deleted, so we can't + // continue on if this fails. + // 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 { - pools.DisassociateMonitor(lb.network, pool.ID, monId) + if poolExists { + 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 } diff --git a/pkg/cloudprovider/servicecontroller/servicecontroller.go b/pkg/cloudprovider/servicecontroller/servicecontroller.go index 3ff6b83147d..6833b9aa7b7 100644 --- a/pkg/cloudprovider/servicecontroller/servicecontroller.go +++ b/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 {