diff --git a/pkg/controller/service/service_controller.go b/pkg/controller/service/service_controller.go index c642bbef4a6..a3da8b6554a 100644 --- a/pkg/controller/service/service_controller.go +++ b/pkg/controller/service/service_controller.go @@ -136,6 +136,8 @@ func New( cache.ResourceEventHandlerFuncs{ AddFunc: func(cur interface{}) { svc, ok := cur.(*v1.Service) + // Check cleanup here can provide a remedy when controller failed to handle + // changes before it exiting (e.g. crashing, restart, etc.). if ok && (wantsLoadBalancer(svc) || needsCleanup(svc)) { s.enqueueService(cur) } @@ -438,7 +440,20 @@ func (s *serviceCache) delete(serviceName string) { // needsCleanup checks if load balancer needs to be cleaned up as indicated by finalizer. func needsCleanup(service *v1.Service) bool { - return service.ObjectMeta.DeletionTimestamp != nil && servicehelper.HasLBFinalizer(service) + if !servicehelper.HasLBFinalizer(service) { + return false + } + + if service.ObjectMeta.DeletionTimestamp != nil { + return true + } + + // Service doesn't want loadBalancer but owns loadBalancer finalizer also need to be cleaned up. + if service.Spec.Type != v1.ServiceTypeLoadBalancer { + return true + } + + return false } // needsUpdate checks if load balancer needs to be updated due to change in attributes. diff --git a/pkg/controller/service/service_controller_test.go b/pkg/controller/service/service_controller_test.go index 70e588c9df5..b2e988825aa 100644 --- a/pkg/controller/service/service_controller_test.go +++ b/pkg/controller/service/service_controller_test.go @@ -833,6 +833,16 @@ func TestProcessServiceDeletion(t *testing.T) { } +// Test cases: +// index finalizer timestamp wantLB | clean-up +// 0 0 0 0 | false (No finalizer, no clean up) +// 1 0 0 1 | false (Ignored as same with case 0) +// 2 0 1 0 | false (Ignored as same with case 0) +// 3 0 1 1 | false (Ignored as same with case 0) +// 4 1 0 0 | true +// 5 1 0 1 | false +// 6 1 1 0 | true (Service is deleted, needs clean up) +// 7 1 1 1 | true (Ignored as same with case 6) func TestNeedsCleanup(t *testing.T) { testCases := []struct { desc string @@ -840,27 +850,31 @@ func TestNeedsCleanup(t *testing.T) { expectNeedsCleanup bool }{ { - desc: "service without finalizer without timestamp", + desc: "service without finalizer", svc: &v1.Service{}, expectNeedsCleanup: false, }, { - desc: "service without finalizer with timestamp", - svc: &v1.Service{ - ObjectMeta: metav1.ObjectMeta{ - DeletionTimestamp: &metav1.Time{ - Time: time.Now(), - }, - }, - }, - expectNeedsCleanup: false, - }, - { - desc: "service with finalizer without timestamp", + desc: "service with finalizer without timestamp without LB", svc: &v1.Service{ ObjectMeta: metav1.ObjectMeta{ Finalizers: []string{servicehelper.LoadBalancerCleanupFinalizer}, }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeNodePort, + }, + }, + expectNeedsCleanup: true, + }, + { + desc: "service with finalizer without timestamp with LB", + svc: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Finalizers: []string{servicehelper.LoadBalancerCleanupFinalizer}, + }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + }, }, expectNeedsCleanup: false, }, @@ -868,10 +882,10 @@ func TestNeedsCleanup(t *testing.T) { desc: "service with finalizer with timestamp", svc: &v1.Service{ ObjectMeta: metav1.ObjectMeta{ + Finalizers: []string{servicehelper.LoadBalancerCleanupFinalizer}, DeletionTimestamp: &metav1.Time{ Time: time.Now(), }, - Finalizers: []string{servicehelper.LoadBalancerCleanupFinalizer, "unrelated"}, }, }, expectNeedsCleanup: true,