diff --git a/pkg/api/rest/delete.go b/pkg/api/rest/delete.go index b695e830e8b..b6ddf86242b 100644 --- a/pkg/api/rest/delete.go +++ b/pkg/api/rest/delete.go @@ -44,7 +44,8 @@ type RESTGracefulDeleteStrategy interface { // should be gracefully deleted, if gracefulPending is set the object has already been gracefully deleted // (and the provided grace period is longer than the time to deletion), and an error is returned if the // condition cannot be checked or the gracePeriodSeconds is invalid. The options argument may be updated with -// default values if graceful is true. +// default values if graceful is true. Second place where we set deletionTimestamp is pkg/registry/generic/registry/store.go +// this function is responsible for setting deletionTimestamp during gracefulDeletion, other one for cascading deletions. func BeforeDelete(strategy RESTDeleteStrategy, ctx api.Context, obj runtime.Object, options *api.DeleteOptions) (graceful, gracefulPending bool, err error) { objectMeta, gvk, kerr := objectMetaAndKind(strategy, obj) if kerr != nil { @@ -56,9 +57,11 @@ func BeforeDelete(strategy RESTDeleteStrategy, ctx api.Context, obj runtime.Obje } gracefulStrategy, ok := strategy.(RESTGracefulDeleteStrategy) if !ok { + // If we're not deleting gracefully there's no point in updating Generation, as we won't update + // the obcject before deleting it. return false, false, nil } - // if the object is already being deleted + // if the object is already being deleted, no need to update generation. if objectMeta.DeletionTimestamp != nil { // if we are already being deleted, we may only shorten the deletion grace period // this means the object was gracefully deleted previously but deletionGracePeriodSeconds was not set, @@ -89,5 +92,12 @@ func BeforeDelete(strategy RESTDeleteStrategy, ctx api.Context, obj runtime.Obje now := unversioned.NewTime(unversioned.Now().Add(time.Second * time.Duration(*options.GracePeriodSeconds))) objectMeta.DeletionTimestamp = &now objectMeta.DeletionGracePeriodSeconds = options.GracePeriodSeconds + // If it's the first graceful deletion we are going to set the DeletionTimestamp to non-nil. + // Controllers of the object that's being deleted shouldn't take any nontrivial actions, hence its behavior changes. + // Thus we need to bump object's Generation (if set). This handles generation bump during graceful deletion. + // The bump for objects that don't support graceful deletion is handled in pkg/registry/generic/registry/store.go. + if objectMeta.Generation > 0 { + objectMeta.Generation++ + } return true, false, nil } diff --git a/pkg/api/rest/resttest/resttest.go b/pkg/api/rest/resttest/resttest.go index 6a7cd08a53f..ade5d7170a2 100644 --- a/pkg/api/rest/resttest/resttest.go +++ b/pkg/api/rest/resttest/resttest.go @@ -120,6 +120,7 @@ func (t *Tester) setObjectMeta(obj runtime.Object, name string) { meta.Namespace = api.NamespaceValue(t.TestContext()) } meta.GenerateName = "" + meta.Generation = 1 } func copyOrDie(obj runtime.Object) runtime.Object { @@ -742,6 +743,7 @@ func (t *Tester) testDeleteGracefulHasDefault(obj runtime.Object, createFn Creat t.Errorf("unexpected error: %v", err) } objectMeta := t.getObjectMetaOrFail(foo) + generation := objectMeta.Generation _, err := t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.Name, &api.DeleteOptions{}) if err != nil { t.Errorf("unexpected error: %v", err) @@ -758,6 +760,9 @@ func (t *Tester) testDeleteGracefulHasDefault(obj runtime.Object, createFn Creat if objectMeta.DeletionTimestamp == nil || objectMeta.DeletionGracePeriodSeconds == nil || *objectMeta.DeletionGracePeriodSeconds != expectedGrace { t.Errorf("unexpected deleted meta: %#v", objectMeta) } + if generation >= objectMeta.Generation { + t.Error("Generation wasn't bumped when deletion timestamp was set") + } } func (t *Tester) testDeleteGracefulWithValue(obj runtime.Object, createFn CreateFunc, getFn GetFunc, expectedGrace int64) { @@ -769,6 +774,7 @@ func (t *Tester) testDeleteGracefulWithValue(obj runtime.Object, createFn Create t.Errorf("unexpected error: %v", err) } objectMeta := t.getObjectMetaOrFail(foo) + generation := objectMeta.Generation _, err := t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.Name, api.NewDeleteOptions(expectedGrace+2)) if err != nil { t.Errorf("unexpected error: %v", err) @@ -785,6 +791,9 @@ func (t *Tester) testDeleteGracefulWithValue(obj runtime.Object, createFn Create if objectMeta.DeletionTimestamp == nil || objectMeta.DeletionGracePeriodSeconds == nil || *objectMeta.DeletionGracePeriodSeconds != expectedGrace+2 { t.Errorf("unexpected deleted meta: %#v", objectMeta) } + if generation >= objectMeta.Generation { + t.Error("Generation wasn't bumped when deletion timestamp was set") + } } func (t *Tester) testDeleteGracefulExtend(obj runtime.Object, createFn CreateFunc, getFn GetFunc, expectedGrace int64) { @@ -796,6 +805,7 @@ func (t *Tester) testDeleteGracefulExtend(obj runtime.Object, createFn CreateFun t.Errorf("unexpected error: %v", err) } objectMeta := t.getObjectMetaOrFail(foo) + generation := objectMeta.Generation _, err := t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.Name, api.NewDeleteOptions(expectedGrace)) if err != nil { t.Errorf("unexpected error: %v", err) @@ -817,6 +827,9 @@ func (t *Tester) testDeleteGracefulExtend(obj runtime.Object, createFn CreateFun if objectMeta.DeletionTimestamp == nil || objectMeta.DeletionGracePeriodSeconds == nil || *objectMeta.DeletionGracePeriodSeconds != expectedGrace { t.Errorf("unexpected deleted meta: %#v", objectMeta) } + if generation >= objectMeta.Generation { + t.Error("Generation wasn't bumped when deletion timestamp was set") + } } func (t *Tester) testDeleteGracefulImmediate(obj runtime.Object, createFn CreateFunc, getFn GetFunc, expectedGrace int64) { @@ -828,6 +841,7 @@ func (t *Tester) testDeleteGracefulImmediate(obj runtime.Object, createFn Create t.Errorf("unexpected error: %v", err) } objectMeta := t.getObjectMetaOrFail(foo) + generation := objectMeta.Generation _, err := t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.Name, api.NewDeleteOptions(expectedGrace)) if err != nil { t.Errorf("unexpected error: %v", err) @@ -850,6 +864,9 @@ func (t *Tester) testDeleteGracefulImmediate(obj runtime.Object, createFn Create if objectMeta.DeletionTimestamp == nil || objectMeta.DeletionGracePeriodSeconds == nil || *objectMeta.DeletionGracePeriodSeconds != 0 { t.Errorf("unexpected deleted meta: %#v", objectMeta) } + if generation >= objectMeta.Generation { + t.Error("Generation wasn't bumped when deletion timestamp was set") + } } func (t *Tester) testDeleteGracefulUsesZeroOnNil(obj runtime.Object, createFn CreateFunc, expectedGrace int64) { diff --git a/pkg/registry/generic/registry/store.go b/pkg/registry/generic/registry/store.go index 6eb42032b87..c70359a6056 100644 --- a/pkg/registry/generic/registry/store.go +++ b/pkg/registry/generic/registry/store.go @@ -479,6 +479,10 @@ func markAsDeleting(obj runtime.Object) (err error) { return kerr } now := unversioned.NewTime(time.Now()) + // This handles Generation bump for resources that don't support graceful deletion. For resources that support graceful deletion is handle in pkg/api/rest/delete.go + if objectMeta.DeletionTimestamp == nil && objectMeta.Generation > 0 { + objectMeta.Generation++ + } objectMeta.DeletionTimestamp = &now var zero int64 = 0 objectMeta.DeletionGracePeriodSeconds = &zero diff --git a/pkg/registry/generic/registry/store_test.go b/pkg/registry/generic/registry/store_test.go index 681a94343ab..f70133b1aa3 100644 --- a/pkg/registry/generic/registry/store_test.go +++ b/pkg/registry/generic/registry/store_test.go @@ -555,9 +555,10 @@ func TestStoreDelete(t *testing.T) { func TestStoreHandleFinalizers(t *testing.T) { EnableGarbageCollector = true + initialGeneration := int64(1) defer func() { EnableGarbageCollector = false }() podWithFinalizer := &api.Pod{ - ObjectMeta: api.ObjectMeta{Name: "foo", Finalizers: []string{"foo.com/x"}}, + ObjectMeta: api.ObjectMeta{Name: "foo", Finalizers: []string{"foo.com/x"}, Generation: initialGeneration}, Spec: api.PodSpec{NodeName: "machine"}, } @@ -592,6 +593,9 @@ func TestStoreHandleFinalizers(t *testing.T) { 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.") + } updatedPodWithFinalizer := &api.Pod{ ObjectMeta: api.ObjectMeta{Name: "foo", Finalizers: []string{"foo.com/x"}, ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion}, @@ -629,28 +633,29 @@ func TestStoreHandleFinalizers(t *testing.T) { func TestStoreDeleteWithOrphanDependents(t *testing.T) { EnableGarbageCollector = true + initialGeneration := int64(1) defer func() { EnableGarbageCollector = false }() podWithOrphanFinalizer := func(name string) *api.Pod { return &api.Pod{ - ObjectMeta: api.ObjectMeta{Name: name, Finalizers: []string{"foo.com/x", api.FinalizerOrphan, "bar.com/y"}}, + ObjectMeta: api.ObjectMeta{Name: name, Finalizers: []string{"foo.com/x", api.FinalizerOrphan, "bar.com/y"}, Generation: initialGeneration}, Spec: api.PodSpec{NodeName: "machine"}, } } podWithOtherFinalizers := func(name string) *api.Pod { return &api.Pod{ - ObjectMeta: api.ObjectMeta{Name: name, Finalizers: []string{"foo.com/x", "bar.com/y"}}, + ObjectMeta: api.ObjectMeta{Name: name, Finalizers: []string{"foo.com/x", "bar.com/y"}, Generation: initialGeneration}, Spec: api.PodSpec{NodeName: "machine"}, } } podWithNoFinalizer := func(name string) *api.Pod { return &api.Pod{ - ObjectMeta: api.ObjectMeta{Name: name}, + ObjectMeta: api.ObjectMeta{Name: name, Generation: initialGeneration}, Spec: api.PodSpec{NodeName: "machine"}, } } podWithOnlyOrphanFinalizer := func(name string) *api.Pod { return &api.Pod{ - ObjectMeta: api.ObjectMeta{Name: name, Finalizers: []string{api.FinalizerOrphan}}, + ObjectMeta: api.ObjectMeta{Name: name, Finalizers: []string{api.FinalizerOrphan}, Generation: initialGeneration}, Spec: api.PodSpec{NodeName: "machine"}, } } @@ -794,13 +799,16 @@ func TestStoreDeleteWithOrphanDependents(t *testing.T) { t.Fatalf("Expect the object to be a pod, but got %#v", obj) } if pod.ObjectMeta.DeletionTimestamp == nil { - t.Errorf("Expect the object to have DeletionTimestamp set, but got %#v", pod.ObjectMeta) + t.Errorf("%v: Expect the object to have DeletionTimestamp set, but got %#v", pod.Name, pod.ObjectMeta) } if pod.ObjectMeta.DeletionGracePeriodSeconds == nil || *pod.ObjectMeta.DeletionGracePeriodSeconds != 0 { - t.Errorf("Expect the object to have 0 DeletionGracePeriodSecond, but got %#v", pod.ObjectMeta) + t.Errorf("%v: Expect the object to have 0 DeletionGracePeriodSecond, but got %#v", pod.Name, pod.ObjectMeta) + } + if pod.Generation <= initialGeneration { + t.Errorf("%v: Deletion didn't increase Generation.", pod.Name) } if e, a := tc.updatedFinalizers, pod.ObjectMeta.Finalizers; !reflect.DeepEqual(e, a) { - t.Errorf("Expect object %s to have finalizers %v, got %v", pod.ObjectMeta.Name, e, a) + t.Errorf("%v: Expect object %s to have finalizers %v, got %v", pod.Name, pod.ObjectMeta.Name, e, a) } } }