From 87e292c959508244462e73651f506fa8828e2424 Mon Sep 17 00:00:00 2001 From: Zihong Zheng Date: Tue, 12 Nov 2019 14:59:46 -0800 Subject: [PATCH 1/2] Promote service load balancer finalizer feature gate to GA --- pkg/features/kube_features.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index a3f887d6a91..12b9a10ab7c 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -432,7 +432,9 @@ const ( CSIMigrationOpenStack featuregate.Feature = "CSIMigrationOpenStack" // owner: @MrHohn - // beta: v1.16 + // alpha: v1.15 + // beta: v1.16 + // GA: v1.17 // // Enables Finalizer Protection for Service LoadBalancers. ServiceLoadBalancerFinalizer featuregate.Feature = "ServiceLoadBalancerFinalizer" @@ -568,7 +570,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS KubeletPodResources: {Default: true, PreRelease: featuregate.Beta}, WindowsGMSA: {Default: true, PreRelease: featuregate.Beta}, WindowsRunAsUserName: {Default: true, PreRelease: featuregate.Beta}, - ServiceLoadBalancerFinalizer: {Default: true, PreRelease: featuregate.Beta}, + ServiceLoadBalancerFinalizer: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, LocalStorageCapacityIsolationFSQuotaMonitoring: {Default: false, PreRelease: featuregate.Alpha}, NonPreemptingPriority: {Default: false, PreRelease: featuregate.Alpha}, VolumePVCDataSource: {Default: true, PreRelease: featuregate.Beta}, From 0833e8840e8548efefe7dc9e5ae5e3b702b043b1 Mon Sep 17 00:00:00 2001 From: Zihong Zheng Date: Tue, 12 Nov 2019 15:01:50 -0800 Subject: [PATCH 2/2] Amend service controller code/test regarding finalizer GA --- pkg/controller/service/controller.go | 48 +++++++---------------- pkg/controller/service/controller_test.go | 32 +++++++-------- 2 files changed, 30 insertions(+), 50 deletions(-) diff --git a/pkg/controller/service/controller.go b/pkg/controller/service/controller.go index 04f98131b2c..f3c3f2d9b51 100644 --- a/pkg/controller/service/controller.go +++ b/pkg/controller/service/controller.go @@ -79,11 +79,6 @@ const ( // originated from: https://github.com/kubernetes/kubernetes/blob/28e800245e/pkg/features/kube_features.go#L178 serviceNodeExclusionFeature = "ServiceNodeExclusion" - // serviceLoadBalancerFinalizerFeature is the feature gate name that - // enables Finalizer Protection for Service LoadBalancers. - // originated from: https://github.com/kubernetes/kubernetes/blob/28e800245e/pkg/features/kube_features.go#L433 - serviceLoadBalancerFinalizerFeature = "ServiceLoadBalancerFinalizer" - // legacyNodeRoleBehaviro is the feature gate name that enables legacy // behavior to vary cluster functionality on the node-role.kubernetes.io // labels. @@ -103,12 +98,13 @@ type serviceCache struct { // Controller keeps cloud provider service resources // (like load balancers) in sync with the registry. type Controller struct { - cloud cloudprovider.Interface - knownHosts []*v1.Node - servicesToUpdate []*v1.Service - kubeClient clientset.Interface - clusterName string - balancer cloudprovider.LoadBalancer + cloud cloudprovider.Interface + knownHosts []*v1.Node + servicesToUpdate []*v1.Service + kubeClient clientset.Interface + clusterName string + balancer cloudprovider.LoadBalancer + // TODO(#85155): Stop relying on this and remove the cache completely. cache *serviceCache serviceLister corelisters.ServiceLister serviceListerSynced cache.InformerSynced @@ -170,15 +166,8 @@ func New( s.enqueueService(cur) } }, - DeleteFunc: func(old interface{}) { - if utilfeature.DefaultFeatureGate.Enabled(serviceLoadBalancerFinalizerFeature) { - // No need to handle deletion event if finalizer feature gate is - // enabled. Because the deletion would be handled by the update - // path when the deletion timestamp is added. - return - } - s.enqueueService(old) - }, + // No need to handle deletion event because the deletion would be handled by + // the update path when the deletion timestamp is added. }, serviceSyncPeriod, ) @@ -335,10 +324,8 @@ func (s *Controller) syncLoadBalancerIfNeeded(service *v1.Service, key string) ( return op, fmt.Errorf("failed to delete load balancer: %v", err) } } - // Always try to remove finalizer when load balancer is deleted. - // It will be a no-op if finalizer does not exist. - // Note this also clears up finalizer if the cluster is downgraded - // from a version that attaches finalizer to a version that doesn't. + // Always remove finalizer when load balancer is deleted, this ensures Services + // can be deleted after all corresponding load balancer resources are deleted. if err := s.removeFinalizer(service); err != nil { return op, fmt.Errorf("failed to remove load balancer cleanup finalizer: %v", err) } @@ -348,15 +335,10 @@ func (s *Controller) syncLoadBalancerIfNeeded(service *v1.Service, key string) ( op = ensureLoadBalancer klog.V(2).Infof("Ensuring load balancer for service %s", key) s.eventRecorder.Event(service, v1.EventTypeNormal, "EnsuringLoadBalancer", "Ensuring load balancer") - if utilfeature.DefaultFeatureGate.Enabled(serviceLoadBalancerFinalizerFeature) { - // Always try to add finalizer prior to load balancer creation. - // It will be a no-op if finalizer already exists. - // Note this also retrospectively puts on finalizer if the cluster - // is upgraded from a version that doesn't attach finalizer to a - // version that does. - if err := s.addFinalizer(service); err != nil { - return op, fmt.Errorf("failed to add load balancer cleanup finalizer: %v", err) - } + // Always add a finalizer prior to creating load balancers, this ensures Services + // can't be deleted until all corresponding load balancer resources are also deleted. + if err := s.addFinalizer(service); err != nil { + return op, fmt.Errorf("failed to add load balancer cleanup finalizer: %v", err) } newStatus, err = s.ensureLoadBalancer(service) if err != nil { diff --git a/pkg/controller/service/controller_test.go b/pkg/controller/service/controller_test.go index 0e142b20277..967d323e743 100644 --- a/pkg/controller/service/controller_test.go +++ b/pkg/controller/service/controller_test.go @@ -93,7 +93,6 @@ func newController() (*Controller, *fakecloud.Cloud, *fake.Clientset) { func TestSyncLoadBalancerIfNeeded(t *testing.T) { testCases := []struct { desc string - enableFeatureGate bool service *v1.Service lbExists bool expectOp loadBalancerOperation @@ -155,9 +154,10 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) { Type: v1.ServiceTypeLoadBalancer, }, }, - expectOp: ensureLoadBalancer, - expectCreateAttempt: true, - expectPatchStatus: true, + expectOp: ensureLoadBalancer, + expectCreateAttempt: true, + expectPatchStatus: true, + expectPatchFinalizer: true, }, { desc: "tcp service that wants LB", @@ -175,9 +175,10 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) { Type: v1.ServiceTypeLoadBalancer, }, }, - expectOp: ensureLoadBalancer, - expectCreateAttempt: true, - expectPatchStatus: true, + expectOp: ensureLoadBalancer, + expectCreateAttempt: true, + expectPatchStatus: true, + expectPatchFinalizer: true, }, { desc: "sctp service that wants LB", @@ -195,9 +196,10 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) { Type: v1.ServiceTypeLoadBalancer, }, }, - expectOp: ensureLoadBalancer, - expectCreateAttempt: true, - expectPatchStatus: true, + expectOp: ensureLoadBalancer, + expectCreateAttempt: true, + expectPatchStatus: true, + expectPatchFinalizer: true, }, // Finalizer test cases below. { @@ -259,8 +261,7 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) { expectPatchFinalizer: true, }, { - desc: "service without finalizer that wants LB", - enableFeatureGate: true, + desc: "service without finalizer that wants LB", service: &v1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: "basic-service1", @@ -281,8 +282,7 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) { expectPatchFinalizer: true, }, { - desc: "service with finalizer that wants LB", - enableFeatureGate: true, + desc: "service with finalizer that wants LB", service: &v1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: "basic-service1", @@ -307,8 +307,6 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, serviceLoadBalancerFinalizerFeature, tc.enableFeatureGate)() - controller, cloud, client := newController() cloud.Exists = tc.lbExists key := fmt.Sprintf("%s/%s", tc.service.Namespace, tc.service.Name) @@ -392,7 +390,7 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) { } } if numPatches != expectNumPatches { - t.Errorf("Expected %d patches, got %d instead. Actions: %v", numPatches, expectNumPatches, actions) + t.Errorf("Got %d patches, expect %d instead. Actions: %v", numPatches, expectNumPatches, actions) } }) }