Merge pull request #27269 from gmarek/controllerRef

Automatic merge from submit-queue

Graceful deletion bumps object's generation

Fix. #24935

cc @caesarxuchao

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
 }

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) {

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

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)
 			}
 		}
 	}