From 8c365d51c77dd8a9e99864da5117acaace7af04d Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Sat, 13 Jun 2015 12:24:26 -0400 Subject: [PATCH] servicecontroller relies on cloudprovider to delete LB if needed We previously made the cloudproviders take on the responsibility of deleting existing load balancers; this lets us simplify the servicecontroller logic and also lays the groundwork for more efficient cloudprovider LB implementations to do an in-place change on a LB. --- pkg/controller/service/servicecontroller.go | 71 ++++++++++----------- 1 file changed, 35 insertions(+), 36 deletions(-) diff --git a/pkg/controller/service/servicecontroller.go b/pkg/controller/service/servicecontroller.go index 9382109f812..6f936337f78 100644 --- a/pkg/controller/service/servicecontroller.go +++ b/pkg/controller/service/servicecontroller.go @@ -272,52 +272,51 @@ func (s *ServiceController) processDelta(delta *cache.Delta) (error, bool) { // Returns whatever error occurred along with a boolean indicator of whether it // should be retried. -func (s *ServiceController) createLoadBalancerIfNeeded(namespacedName types.NamespacedName, service, cachedService *api.Service) (error, bool) { - if cachedService != nil && !needsUpdate(cachedService, service) { +func (s *ServiceController) createLoadBalancerIfNeeded(namespacedName types.NamespacedName, service, appliedState *api.Service) (error, bool) { + if appliedState != nil && !needsUpdate(appliedState, service) { glog.Infof("LB already exists and doesn't need update for service %s", namespacedName) return nil, notRetryable } - if cachedService != nil { - // If the service already exists but needs to be updated, delete it so that - // we can recreate it cleanly. - if wantsExternalLoadBalancer(cachedService) { - glog.Infof("Deleting existing load balancer for service %s that needs an updated load balancer.", namespacedName) - s.eventRecorder.Event(service, "deleting loadbalancer", "deleting loadbalancer, will recreate with updated configuration") - if err := s.balancer.EnsureTCPLoadBalancerDeleted(s.loadBalancerName(cachedService), s.zone.Region); err != nil { - return err, retryable - } - s.eventRecorder.Event(service, "deleted loadbalancer", "deleted loadbalancer") - } - } else { - // If we don't have any cached memory of the load balancer, we have to ask - // the cloud provider for what it knows about it. - status, exists, err := s.balancer.GetTCPLoadBalancer(s.loadBalancerName(service), s.zone.Region) - if err != nil { - return fmt.Errorf("Error getting LB for service %s: %v", namespacedName, err), retryable - } - if exists && api.LoadBalancerStatusEqual(status, &service.Status.LoadBalancer) { - glog.Infof("LB already exists with status %s for previously uncached service %s", status, namespacedName) - return nil, notRetryable - } 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, status, service.Spec.DeprecatedPublicIPs) - s.eventRecorder.Event(service, "deleting loadbalancer", "deleting loadbalancer, will recreate with updated IPs") - if err := s.balancer.EnsureTCPLoadBalancerDeleted(s.loadBalancerName(service), s.zone.Region); err != nil { - return err, retryable - } - s.eventRecorder.Event(service, "deleted loadbalancer", "deleted loadbalancer") - } - } + + // Note: It is safe to just call EnsureTCPLoadBalancer. But, on some clouds that requires a delete & create, + // which may involve service interruption. Also, we would like user-friendly events. // Save the state so we can avoid a write if it doesn't change previousState := api.LoadBalancerStatusDeepCopy(&service.Status.LoadBalancer) if !wantsExternalLoadBalancer(service) { - glog.Infof("Not creating LB for service %s that doesn't want one.", namespacedName) + needDelete := true + if appliedState != nil { + if !wantsExternalLoadBalancer(appliedState) { + needDelete = false + } + } else { + // If we don't have any cached memory of the load balancer, we have to ask + // the cloud provider for what it knows about it. + // Technically EnsureTCPLoadBalancerDeleted can cope, but we want to post meaningful events + _, exists, err := s.balancer.GetTCPLoadBalancer(s.loadBalancerName(service), s.zone.Region) + if err != nil { + return fmt.Errorf("Error getting LB for service %s: %v", namespacedName, err), retryable + } + if !exists { + needDelete = false + } + } + + if needDelete { + glog.Infof("Deleting existing load balancer for service %s that no longer needs a load balancer.", namespacedName) + s.eventRecorder.Event(service, "deleting loadbalancer", "deleting loadbalancer") + if err := s.balancer.EnsureTCPLoadBalancerDeleted(s.loadBalancerName(service), s.zone.Region); err != nil { + return err, retryable + } + s.eventRecorder.Event(service, "deleted loadbalancer", "deleted loadbalancer") + } service.Status.LoadBalancer = api.LoadBalancerStatus{} } else { - glog.V(2).Infof("Creating LB for service %s", namespacedName) + glog.V(2).Infof("Ensuring LB for service %s", namespacedName) + + // TODO: We could do a dry-run here if wanted to avoid the spurious cloud-calls & events when we restart // The load balancer doesn't exist yet, so create it. s.eventRecorder.Event(service, "creating loadbalancer", "creating loadbalancer") @@ -644,7 +643,7 @@ func (s *ServiceController) updateLoadBalancerHosts(services []*cachedService, h func() { service.mu.Lock() defer service.mu.Unlock() - // If the service is nil, that means it hasn't yet been successfully dealt + // If the applied state is nil, that means it hasn't yet been successfully dealt // with by the load balancer reconciler. We can trust the load balancer // reconciler to ensure the service's load balancer is created to target // the correct nodes.