From ed5b5bb94e7c75f22a7fc302e47dade6c0d1662d Mon Sep 17 00:00:00 2001 From: Dan Mace Date: Tue, 22 Aug 2017 15:58:44 -0400 Subject: [PATCH 1/2] Enable finalizers independent of GC enablement Decouple finalizer processing from garbage collection configuration. Finalizers should be effective even when garbage collection is disabled for a given store. Fixes https://github.com/kubernetes/kubernetes/issues/50528. --- .../pkg/registry/generic/registry/store.go | 99 ++------ .../registry/generic/registry/store_test.go | 220 +++++++++--------- 2 files changed, 131 insertions(+), 188 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go index 1de06fd9fb..9229b2a238 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go @@ -442,12 +442,8 @@ func (e *Store) WaitForInitialized(ctx genericapirequest.Context, obj runtime.Ob // shouldDeleteDuringUpdate checks if a Update is removing all the object's // finalizers. If so, it further checks if the object's -// DeletionGracePeriodSeconds is 0. If so, it returns true. If garbage collection -// is disabled it always returns false. +// DeletionGracePeriodSeconds is 0. func (e *Store) shouldDeleteDuringUpdate(ctx genericapirequest.Context, key string, obj, existing runtime.Object) bool { - if !e.EnableGarbageCollection { - return false - } newMeta, err := meta.Accessor(obj) if err != nil { utilruntime.HandleError(err) @@ -765,9 +761,16 @@ func shouldDeleteDependents(e *Store, accessor metav1.Object, options *metav1.De return false } -// deletionFinalizers returns the deletion finalizers we should set on the object and a bool indicate they did or -// did not change. +// deletionFinalizers returns the deletion finalizers we should set on the +// object and a bool indicate they did or did not change. +// +// Because the finalizers created here are only cleared by the garbage +// collector, deletionFinalizers always returns false when garbage collection is +// disabled for the Store. func deletionFinalizers(e *Store, accessor metav1.Object, options *metav1.DeleteOptions) (bool, []string) { + if !e.EnableGarbageCollection { + return false, []string{} + } shouldOrphan := shouldOrphanDependents(e, accessor, options) shouldDeleteDependentInForeground := shouldDeleteDependents(e, accessor, options) newFinalizers := []string{} @@ -816,72 +819,6 @@ func markAsDeleting(obj runtime.Object) (err error) { return nil } -// updateForGracefulDeletion and updateForGracefulDeletionAndFinalizers both -// implement deletion flows for graceful deletion. Graceful deletion is -// implemented as setting the deletion timestamp in an update. If the -// implementation of graceful deletion is changed, both of these methods -// should be changed together. - -// updateForGracefulDeletion updates the given object for graceful deletion by -// setting the deletion timestamp and grace period seconds and returns: -// -// 1. an error -// 2. a boolean indicating that the object was not found, but it should be -// ignored -// 3. a boolean indicating that the object's grace period is exhausted and it -// should be deleted immediately -// 4. a new output object with the state that was updated -// 5. a copy of the last existing state of the object -func (e *Store) updateForGracefulDeletion(ctx genericapirequest.Context, name, key string, options *metav1.DeleteOptions, preconditions storage.Preconditions, in runtime.Object) (err error, ignoreNotFound, deleteImmediately bool, out, lastExisting runtime.Object) { - lastGraceful := int64(0) - out = e.NewFunc() - err = e.Storage.GuaranteedUpdate( - ctx, - key, - out, - false, /* ignoreNotFound */ - &preconditions, - storage.SimpleUpdate(func(existing runtime.Object) (runtime.Object, error) { - graceful, pendingGraceful, err := rest.BeforeDelete(e.DeleteStrategy, ctx, existing, options) - if err != nil { - return nil, err - } - if pendingGraceful { - return nil, errAlreadyDeleting - } - if !graceful { - return nil, errDeleteNow - } - lastGraceful = *options.GracePeriodSeconds - lastExisting = existing - return existing, nil - }), - ) - switch err { - case nil: - if lastGraceful > 0 { - return nil, false, false, out, lastExisting - } - // If we are here, the registry supports grace period mechanism and - // we are intentionally delete gracelessly. In this case, we may - // enter a race with other k8s components. If other component wins - // the race, the object will not be found, and we should tolerate - // the NotFound error. See - // https://github.com/kubernetes/kubernetes/issues/19403 for - // details. - return nil, true, true, out, lastExisting - case errDeleteNow: - // we've updated the object to have a zero grace period, or it's already at 0, so - // we should fall through and truly delete the object. - return nil, false, true, out, lastExisting - case errAlreadyDeleting: - out, err = e.finalizeDelete(ctx, in, true) - return err, false, false, out, lastExisting - default: - return storeerr.InterpretUpdateError(err, e.qualifiedResourceFromContext(ctx), name), false, false, out, lastExisting - } -} - // updateForGracefulDeletionAndFinalizers updates the given object for // graceful deletion and finalization by setting the deletion timestamp and // grace period seconds (graceful deletion) and updating the list of @@ -1013,18 +950,12 @@ func (e *Store) Delete(ctx genericapirequest.Context, name string, options *meta // Handle combinations of graceful deletion and finalization by issuing // the correct updates. - if e.EnableGarbageCollection { - shouldUpdateFinalizers, _ := deletionFinalizers(e, accessor, options) - // TODO: remove the check, because we support no-op updates now. - if graceful || pendingFinalizers || shouldUpdateFinalizers { - err, ignoreNotFound, deleteImmediately, out, lastExisting = e.updateForGracefulDeletionAndFinalizers(ctx, name, key, options, preconditions, obj) - } - } else { - // TODO: remove the check on graceful, because we support no-op updates now. - if graceful { - err, ignoreNotFound, deleteImmediately, out, lastExisting = e.updateForGracefulDeletion(ctx, name, key, options, preconditions, obj) - } + shouldUpdateFinalizers, _ := deletionFinalizers(e, accessor, options) + // TODO: remove the check, because we support no-op updates now. + if graceful || pendingFinalizers || shouldUpdateFinalizers { + err, ignoreNotFound, deleteImmediately, out, lastExisting = e.updateForGracefulDeletionAndFinalizers(ctx, name, key, options, preconditions, obj) } + // !deleteImmediately covers all cases where err != nil. We keep both to be future-proof. if !deleteImmediately || err != nil { return out, false, err diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go index 800d1b257c..cbe510eed5 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go @@ -931,56 +931,62 @@ func TestGracefulStoreHandleFinalizers(t *testing.T) { testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") destroyFunc, registry := NewTestGenericStoreRegistry(t) - registry.EnableGarbageCollection = true defaultDeleteStrategy := testRESTStrategy{scheme, names.SimpleNameGenerator, true, false, true} registry.DeleteStrategy = testGracefulStrategy{defaultDeleteStrategy} defer destroyFunc() - // create pod - _, err := registry.Create(testContext, podWithFinalizer, false) - if err != nil { - t.Errorf("Unexpected error: %v", err) - } - // delete the pod with grace period=0, the pod should still exist because it has a finalizer - _, wasDeleted, err := registry.Delete(testContext, podWithFinalizer.Name, metav1.NewDeleteOptions(0)) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - if wasDeleted { - t.Errorf("unexpected, pod %s should not have been deleted immediately", podWithFinalizer.Name) - } - _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } + gcStates := []bool{true, false} + for _, gcEnabled := range gcStates { + t.Logf("garbage collection enabled: %t", gcEnabled) + registry.EnableGarbageCollection = gcEnabled - updatedPodWithFinalizer := &example.Pod{ - ObjectMeta: metav1.ObjectMeta{Name: "foo", Finalizers: []string{"foo.com/x"}, ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion}, - Spec: example.PodSpec{NodeName: "machine"}, - } - _, _, err = registry.Update(testContext, updatedPodWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(updatedPodWithFinalizer, scheme)) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } + // create pod + _, err := registry.Create(testContext, podWithFinalizer, false) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } - // the object should still exist, because it still has a finalizer - _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } + // delete the pod with grace period=0, the pod should still exist because it has a finalizer + _, wasDeleted, err := registry.Delete(testContext, podWithFinalizer.Name, metav1.NewDeleteOptions(0)) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if wasDeleted { + t.Errorf("unexpected, pod %s should not have been deleted immediately", podWithFinalizer.Name) + } + _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } - podWithNoFinalizer := &example.Pod{ - ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion}, - Spec: example.PodSpec{NodeName: "anothermachine"}, - } - _, _, err = registry.Update(testContext, podWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(podWithNoFinalizer, scheme)) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - // the pod should be removed, because its finalizer is removed - _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) - if !errors.IsNotFound(err) { - t.Fatalf("Unexpected error: %v", err) + updatedPodWithFinalizer := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Finalizers: []string{"foo.com/x"}, ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion}, + Spec: example.PodSpec{NodeName: "machine"}, + } + _, _, err = registry.Update(testContext, updatedPodWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(updatedPodWithFinalizer, scheme)) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + // the object should still exist, because it still has a finalizer + _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + podWithNoFinalizer := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion}, + Spec: example.PodSpec{NodeName: "anothermachine"}, + } + _, _, err = registry.Update(testContext, podWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(podWithNoFinalizer, scheme)) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + // the pod should be removed, because its finalizer is removed + _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) + if !errors.IsNotFound(err) { + t.Fatalf("Unexpected error: %v", err) + } } } @@ -1030,73 +1036,79 @@ func TestNonGracefulStoreHandleFinalizers(t *testing.T) { testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") destroyFunc, registry := NewTestGenericStoreRegistry(t) - registry.EnableGarbageCollection = true + defer destroyFunc() - // create pod - _, err := registry.Create(testContext, podWithFinalizer, false) - if err != nil { - t.Errorf("Unexpected error: %v", err) - } + gcStates := []bool{true, false} + for _, gcEnabled := range gcStates { + t.Logf("garbage collection enabled: %t", gcEnabled) + registry.EnableGarbageCollection = gcEnabled - // delete object with nil delete options doesn't delete the object - _, wasDeleted, err := registry.Delete(testContext, podWithFinalizer.Name, nil) - if err != nil { - t.Errorf("Unexpected error: %v", err) - } - if wasDeleted { - t.Errorf("unexpected, pod %s should not have been deleted immediately", podWithFinalizer.Name) - } + // create pod + _, err := registry.Create(testContext, podWithFinalizer, false) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } - // the object should still exist - obj, err := registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) - if err != nil { - t.Errorf("Unexpected error: %v", err) - } - podWithFinalizer, ok := obj.(*example.Pod) - if !ok { - t.Errorf("Unexpected object: %#v", obj) - } - if podWithFinalizer.ObjectMeta.DeletionTimestamp == nil { - t.Errorf("Expect the object to have DeletionTimestamp set, but got %#v", podWithFinalizer.ObjectMeta) - } - if podWithFinalizer.ObjectMeta.DeletionGracePeriodSeconds == nil || *podWithFinalizer.ObjectMeta.DeletionGracePeriodSeconds != 0 { - t.Errorf("Expect the object to have 0 DeletionGracePeriodSecond, but got %#v", podWithFinalizer.ObjectMeta) - } - if podWithFinalizer.Generation <= initialGeneration { - t.Errorf("Deletion didn't increase Generation.") - } + // delete object with nil delete options doesn't delete the object + _, wasDeleted, err := registry.Delete(testContext, podWithFinalizer.Name, nil) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if wasDeleted { + t.Errorf("unexpected, pod %s should not have been deleted immediately", podWithFinalizer.Name) + } - updatedPodWithFinalizer := &example.Pod{ - ObjectMeta: metav1.ObjectMeta{Name: "foo", Finalizers: []string{"foo.com/x"}, ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion}, - Spec: example.PodSpec{NodeName: "machine"}, - } - _, _, err = registry.Update(testContext, updatedPodWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(updatedPodWithFinalizer, scheme)) - if err != nil { - t.Errorf("Unexpected error: %v", err) - } + // the object should still exist + obj, err := registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + podWithFinalizer, ok := obj.(*example.Pod) + if !ok { + t.Errorf("Unexpected object: %#v", obj) + } + if podWithFinalizer.ObjectMeta.DeletionTimestamp == nil { + t.Errorf("Expect the object to have DeletionTimestamp set, but got %#v", podWithFinalizer.ObjectMeta) + } + if podWithFinalizer.ObjectMeta.DeletionGracePeriodSeconds == nil || *podWithFinalizer.ObjectMeta.DeletionGracePeriodSeconds != 0 { + t.Errorf("Expect the object to have 0 DeletionGracePeriodSecond, but got %#v", podWithFinalizer.ObjectMeta) + } + if podWithFinalizer.Generation <= initialGeneration { + t.Errorf("Deletion didn't increase Generation.") + } - // the object should still exist, because it still has a finalizer - obj, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) - if err != nil { - t.Errorf("Unexpected error: %v", err) - } - podWithFinalizer, ok = obj.(*example.Pod) - if !ok { - t.Errorf("Unexpected object: %#v", obj) - } + updatedPodWithFinalizer := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Finalizers: []string{"foo.com/x"}, ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion}, + Spec: example.PodSpec{NodeName: "machine"}, + } + _, _, err = registry.Update(testContext, updatedPodWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(updatedPodWithFinalizer, scheme)) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } - podWithNoFinalizer := &example.Pod{ - ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion}, - Spec: example.PodSpec{NodeName: "anothermachine"}, - } - _, _, err = registry.Update(testContext, podWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(podWithNoFinalizer, scheme)) - if err != nil { - t.Errorf("Unexpected error: %v", err) - } - // the pod should be removed, because its finalizer is removed - _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) - if !errors.IsNotFound(err) { - t.Errorf("Unexpected error: %v", err) + // the object should still exist, because it still has a finalizer + obj, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + podWithFinalizer, ok = obj.(*example.Pod) + if !ok { + t.Errorf("Unexpected object: %#v", obj) + } + + podWithNoFinalizer := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion}, + Spec: example.PodSpec{NodeName: "anothermachine"}, + } + _, _, err = registry.Update(testContext, podWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(podWithNoFinalizer, scheme)) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + // the pod should be removed, because its finalizer is removed + _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) + if !errors.IsNotFound(err) { + t.Errorf("Unexpected error: %v", err) + } } } From c845c444d52b81689e4555aec0e8175f687b6a44 Mon Sep 17 00:00:00 2001 From: Dan Mace Date: Tue, 22 Aug 2017 16:18:35 -0400 Subject: [PATCH 2/2] Clarify finalizer function --- .../pkg/registry/generic/registry/store.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go index 9229b2a238..0b1ddc8c8c 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go @@ -761,13 +761,15 @@ func shouldDeleteDependents(e *Store, accessor metav1.Object, options *metav1.De return false } -// deletionFinalizers returns the deletion finalizers we should set on the -// object and a bool indicate they did or did not change. +// deletionFinalizersForGarbageCollection analyzes the object and delete options +// to determine whether the object is in need of finalization by the garbage +// collector. If so, returns the set of deletion finalizers to apply and a bool +// indicating whether the finalizer list has changed and is in need of updating. // -// Because the finalizers created here are only cleared by the garbage -// collector, deletionFinalizers always returns false when garbage collection is -// disabled for the Store. -func deletionFinalizers(e *Store, accessor metav1.Object, options *metav1.DeleteOptions) (bool, []string) { +// The finalizers returned are intended to be handled by the garbage collector. +// If garbage collection is disabled for the store, this function returns false +// to ensure finalizers aren't set which will never be cleared. +func deletionFinalizersForGarbageCollection(e *Store, accessor metav1.Object, options *metav1.DeleteOptions) (bool, []string) { if !e.EnableGarbageCollection { return false, []string{} } @@ -858,7 +860,7 @@ func (e *Store) updateForGracefulDeletionAndFinalizers(ctx genericapirequest.Con if err != nil { return nil, err } - needsUpdate, newFinalizers := deletionFinalizers(e, existingAccessor, options) + needsUpdate, newFinalizers := deletionFinalizersForGarbageCollection(e, existingAccessor, options) if needsUpdate { existingAccessor.SetFinalizers(newFinalizers) } @@ -950,7 +952,7 @@ func (e *Store) Delete(ctx genericapirequest.Context, name string, options *meta // Handle combinations of graceful deletion and finalization by issuing // the correct updates. - shouldUpdateFinalizers, _ := deletionFinalizers(e, accessor, options) + shouldUpdateFinalizers, _ := deletionFinalizersForGarbageCollection(e, accessor, options) // TODO: remove the check, because we support no-op updates now. if graceful || pendingFinalizers || shouldUpdateFinalizers { err, ignoreNotFound, deleteImmediately, out, lastExisting = e.updateForGracefulDeletionAndFinalizers(ctx, name, key, options, preconditions, obj)