Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Graceful deletion bumps object's generation #27269

Merged
merged 1 commit into from
Jul 1, 2016
Merged

Graceful deletion bumps object's generation #27269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions pkg/api/rest/delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
Expand Down Expand Up @@ -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
}
17 changes: 17 additions & 0 deletions pkg/api/rest/resttest/resttest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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) {
Expand All @@ -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)
Expand All @@ -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) {
Expand All @@ -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)
Expand All @@ -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) {
Expand All @@ -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)
Expand All @@ -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) {
Expand Down
4 changes: 4 additions & 0 deletions pkg/registry/generic/registry/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 16 additions & 8 deletions pkg/registry/generic/registry/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
}

Expand Down Expand Up @@ -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},
Expand Down Expand Up @@ -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"},
}
}
Expand Down Expand Up @@ -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)
}
}
}
Expand Down