From 000fffd3c70d7adb272c9ca139899d329828269d Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Thu, 19 Sep 2019 10:35:58 -0400 Subject: [PATCH] Clean up redundant post-finalizer deletions --- .../deletion/namespaced_resources_deleter.go | 23 +++++-------------- .../namespaced_resources_deleter_test.go | 7 ++---- .../namespace/namespace_controller.go | 2 +- .../pkg/controller/finalizer/crd_finalizer.go | 10 -------- 4 files changed, 9 insertions(+), 33 deletions(-) diff --git a/pkg/controller/namespace/deletion/namespaced_resources_deleter.go b/pkg/controller/namespace/deletion/namespaced_resources_deleter.go index 9a50ff0cf90..d34030fe48d 100644 --- a/pkg/controller/namespace/deletion/namespaced_resources_deleter.go +++ b/pkg/controller/namespace/deletion/namespaced_resources_deleter.go @@ -45,7 +45,7 @@ type NamespacedResourcesDeleterInterface interface { func NewNamespacedResourcesDeleter(nsClient v1clientset.NamespaceInterface, metadataClient metadata.Interface, podsGetter v1clientset.PodsGetter, discoverResourcesFn func() ([]*metav1.APIResourceList, error), - finalizerToken v1.FinalizerName, deleteNamespaceWhenDone bool) NamespacedResourcesDeleterInterface { + finalizerToken v1.FinalizerName) NamespacedResourcesDeleterInterface { d := &namespacedResourcesDeleter{ nsClient: nsClient, metadataClient: metadataClient, @@ -53,9 +53,8 @@ func NewNamespacedResourcesDeleter(nsClient v1clientset.NamespaceInterface, opCache: &operationNotSupportedCache{ m: make(map[operationKey]bool), }, - discoverResourcesFn: discoverResourcesFn, - finalizerToken: finalizerToken, - deleteNamespaceWhenDone: deleteNamespaceWhenDone, + discoverResourcesFn: discoverResourcesFn, + finalizerToken: finalizerToken, } d.initOpCache() return d @@ -77,8 +76,6 @@ type namespacedResourcesDeleter struct { // The finalizer token that should be removed from the namespace // when all resources in that namespace have been deleted. finalizerToken v1.FinalizerName - // Also delete the namespace when all resources in the namespace have been deleted. - deleteNamespaceWhenDone bool } // Delete deletes all resources in the given namespace. @@ -89,7 +86,6 @@ type namespacedResourcesDeleter struct { // (updates the namespace phase if it is not yet marked terminating) // After deleting the resources: // * It removes finalizer token from the given namespace. -// * Deletes the namespace if deleteNamespaceWhenDone is true. // // Returns an error if any of those steps fail. // Returns ResourcesRemainingError if it deleted some resources but needs @@ -127,10 +123,9 @@ func (d *namespacedResourcesDeleter) Delete(nsName string) error { return nil } - // Delete the namespace if it is already finalized. - if d.deleteNamespaceWhenDone && finalized(namespace) { - // TODO(liggitt): just return in 1.16, once n-1 apiservers automatically delete when finalizers are all removed - return d.deleteNamespace(namespace) + // return if it is already finalized. + if finalized(namespace) { + return nil } // there may still be content for us to remove @@ -153,12 +148,6 @@ func (d *namespacedResourcesDeleter) Delete(nsName string) error { } return err } - - // Check if we can delete now. - if d.deleteNamespaceWhenDone && finalized(namespace) { - // TODO(liggitt): just return in 1.16, once n-1 apiservers automatically delete when finalizers are all removed - return d.deleteNamespace(namespace) - } return nil } diff --git a/pkg/controller/namespace/deletion/namespaced_resources_deleter_test.go b/pkg/controller/namespace/deletion/namespaced_resources_deleter_test.go index 2f5461cf9cb..62c0b4683ad 100644 --- a/pkg/controller/namespace/deletion/namespaced_resources_deleter_test.go +++ b/pkg/controller/namespace/deletion/namespaced_resources_deleter_test.go @@ -147,7 +147,6 @@ func testSyncNamespaceThatIsTerminating(t *testing.T, versions *metav1.APIVersio strings.Join([]string{"create", "namespaces", "finalize"}, "-"), strings.Join([]string{"list", "pods", ""}, "-"), strings.Join([]string{"update", "namespaces", "status"}, "-"), - strings.Join([]string{"delete", "namespaces", ""}, "-"), ), metadataClientActionSet: metadataClientActionSet, }, @@ -155,7 +154,6 @@ func testSyncNamespaceThatIsTerminating(t *testing.T, versions *metav1.APIVersio testNamespace: testNamespaceFinalizeComplete, kubeClientActionSet: sets.NewString( strings.Join([]string{"get", "namespaces", ""}, "-"), - strings.Join([]string{"delete", "namespaces", ""}, "-"), ), metadataClientActionSet: sets.NewString(), }, @@ -163,7 +161,6 @@ func testSyncNamespaceThatIsTerminating(t *testing.T, versions *metav1.APIVersio testNamespace: testNamespaceFinalizeComplete, kubeClientActionSet: sets.NewString( strings.Join([]string{"get", "namespaces", ""}, "-"), - strings.Join([]string{"delete", "namespaces", ""}, "-"), ), metadataClientActionSet: sets.NewString(), gvrError: fmt.Errorf("test error"), @@ -202,7 +199,7 @@ func testSyncNamespaceThatIsTerminating(t *testing.T, versions *metav1.APIVersio fn := func() ([]*metav1.APIResourceList, error) { return resources, testInput.gvrError } - d := NewNamespacedResourcesDeleter(mockClient.CoreV1().Namespaces(), metadataClient, mockClient.CoreV1(), fn, v1.FinalizerKubernetes, true) + d := NewNamespacedResourcesDeleter(mockClient.CoreV1().Namespaces(), metadataClient, mockClient.CoreV1(), fn, v1.FinalizerKubernetes) if err := d.Delete(testInput.testNamespace.Name); !matchErrors(err, testInput.expectErrorOnDelete) { t.Errorf("expected error %q when syncing namespace, got %q, %v", testInput.expectErrorOnDelete, err, testInput.expectErrorOnDelete == err) } @@ -300,7 +297,7 @@ func TestSyncNamespaceThatIsActive(t *testing.T) { return testResources(), nil } d := NewNamespacedResourcesDeleter(mockClient.CoreV1().Namespaces(), nil, mockClient.CoreV1(), - fn, v1.FinalizerKubernetes, true) + fn, v1.FinalizerKubernetes) err := d.Delete(testNamespace.Name) if err != nil { t.Errorf("Unexpected error when synching namespace %v", err) diff --git a/pkg/controller/namespace/namespace_controller.go b/pkg/controller/namespace/namespace_controller.go index ef6720b0034..4972aecaeb1 100644 --- a/pkg/controller/namespace/namespace_controller.go +++ b/pkg/controller/namespace/namespace_controller.go @@ -72,7 +72,7 @@ func NewNamespaceController( // create the controller so we can inject the enqueue function namespaceController := &NamespaceController{ queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "namespace"), - namespacedResourcesDeleter: deletion.NewNamespacedResourcesDeleter(kubeClient.CoreV1().Namespaces(), metadataClient, kubeClient.CoreV1(), discoverResourcesFn, finalizerToken, true), + namespacedResourcesDeleter: deletion.NewNamespacedResourcesDeleter(kubeClient.CoreV1().Namespaces(), metadataClient, kubeClient.CoreV1(), discoverResourcesFn, finalizerToken), } if kubeClient != nil && kubeClient.CoreV1().RESTClient().GetRateLimiter() != nil { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go index bea14d4f1ad..dcf4a4e7717 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go @@ -170,16 +170,6 @@ func (c *CRDFinalizer) sync(key string) error { // deleted or changed in the meantime, we'll get called again return nil } - if err != nil { - return err - } - - // and now issue another delete, which should clean it all up if no finalizers remain or no-op if they do - // TODO(liggitt): just return in 1.16, once n-1 apiservers automatically delete when finalizers are all removed - err = c.crdClient.CustomResourceDefinitions().Delete(crd.Name, nil) - if apierrors.IsNotFound(err) { - return nil - } return err }