From 3cf80e76c8bc4b8e0e5f7c683e2aa694945975ca Mon Sep 17 00:00:00 2001 From: Alex Robinson Date: Sun, 17 May 2015 22:36:48 -0700 Subject: [PATCH] Properly handle nil cached services in the service controller's node reconciler. Add a test that catches the former bug. --- .../servicecontroller/servicecontroller.go | 12 +++++++++++- .../servicecontroller/servicecontroller_test.go | 10 ++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/pkg/cloudprovider/servicecontroller/servicecontroller.go b/pkg/cloudprovider/servicecontroller/servicecontroller.go index 1251bf19b52..2d2e8181608 100644 --- a/pkg/cloudprovider/servicecontroller/servicecontroller.go +++ b/pkg/cloudprovider/servicecontroller/servicecontroller.go @@ -203,7 +203,10 @@ func (s *ServiceController) processDelta(delta *cache.Delta) (error, bool) { if err != nil { return err, retry } - // Always update the cache upon success + // Always update the cache upon success. + // NOTE: Since we update the cached service if and only if we successully + // processed it, a cached service being nil implies that it hasn't yet + // been successfully processed. cachedService.service = service s.cache.set(namespacedName.String(), cachedService) case cache.Deleted: @@ -554,6 +557,13 @@ 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 + // 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. + if service.service == nil { + return + } if err := s.lockedUpdateLoadBalancerHosts(service.service, hosts); err != nil { glog.Errorf("External error while updating TCP load balancer: %v.", err) servicesToRetry = append(servicesToRetry, service) diff --git a/pkg/cloudprovider/servicecontroller/servicecontroller_test.go b/pkg/cloudprovider/servicecontroller/servicecontroller_test.go index ed3c0176699..b54f98f2b29 100644 --- a/pkg/cloudprovider/servicecontroller/servicecontroller_test.go +++ b/pkg/cloudprovider/servicecontroller/servicecontroller_test.go @@ -184,6 +184,16 @@ func TestUpdateNodesInExternalLoadBalancer(t *testing.T) { {Name: "a999", Region: region, Hosts: []string{"node0", "node1", "node73"}}, }, }, + { + // One service has an external load balancer and one is nil: one call. + services: []*api.Service{ + newService("s0", "234", true), + nil, + }, + expectedUpdateCalls: []fake_cloud.FakeUpdateBalancerCall{ + {Name: "a234", Region: region, Hosts: []string{"node0", "node1", "node73"}}, + }, + }, } for _, item := range table { cloud := &fake_cloud.FakeCloud{}