From a458e9bb853f63bfb3dbdb654414c6aa3295d2f8 Mon Sep 17 00:00:00 2001 From: zhouhaibing089 Date: Tue, 2 Apr 2019 13:11:30 -0700 Subject: [PATCH] namespace: remove gc finalizers based on delete options This makes the behavior being consistent with generic store, The orphan finalizer should be removed if the delete options does not specify propagarionPolicy as orphan. --- pkg/registry/core/namespace/storage/BUILD | 1 + .../core/namespace/storage/storage.go | 56 ++++- .../core/namespace/storage/storage_test.go | 213 ++++++++++++++++++ 3 files changed, 260 insertions(+), 10 deletions(-) diff --git a/pkg/registry/core/namespace/storage/BUILD b/pkg/registry/core/namespace/storage/BUILD index 257a5a75982..881e518aa6f 100644 --- a/pkg/registry/core/namespace/storage/BUILD +++ b/pkg/registry/core/namespace/storage/BUILD @@ -16,6 +16,7 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/fields:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library", "//staging/src/k8s.io/apiserver/pkg/registry/generic:go_default_library", "//staging/src/k8s.io/apiserver/pkg/registry/generic/testing:go_default_library", diff --git a/pkg/registry/core/namespace/storage/storage.go b/pkg/registry/core/namespace/storage/storage.go index b9d0558ab7f..0165044ad5d 100644 --- a/pkg/registry/core/namespace/storage/storage.go +++ b/pkg/registry/core/namespace/storage/storage.go @@ -185,17 +185,33 @@ func (r *REST) Delete(ctx context.Context, name string, options *metav1.DeleteOp existingNamespace.Status.Phase = api.NamespaceTerminating } - // Remove orphan finalizer if options.OrphanDependents = false. - if options.OrphanDependents != nil && *options.OrphanDependents == false { - // remove Orphan finalizer. - newFinalizers := []string{} - for i := range existingNamespace.ObjectMeta.Finalizers { - finalizer := existingNamespace.ObjectMeta.Finalizers[i] - if string(finalizer) != metav1.FinalizerOrphanDependents { - newFinalizers = append(newFinalizers, finalizer) - } + // the current finalizers which are on namespace + currentFinalizers := map[string]bool{} + for _, f := range existingNamespace.Finalizers { + currentFinalizers[f] = true + } + // the finalizers we should ensure on namespace + shouldHaveFinalizers := map[string]bool{ + metav1.FinalizerOrphanDependents: shouldHaveOrphanFinalizer(options, currentFinalizers[metav1.FinalizerOrphanDependents]), + metav1.FinalizerDeleteDependents: shouldHaveDeleteDependentsFinalizer(options, currentFinalizers[metav1.FinalizerDeleteDependents]), + } + // determine whether there are changes + changeNeeded := false + for finalizer, shouldHave := range shouldHaveFinalizers { + changeNeeded = currentFinalizers[finalizer] != shouldHave || changeNeeded + if shouldHave { + currentFinalizers[finalizer] = true + } else { + delete(currentFinalizers, finalizer) } - existingNamespace.ObjectMeta.Finalizers = newFinalizers + } + // make the changes if needed + if changeNeeded { + newFinalizers := []string{} + for f := range currentFinalizers { + newFinalizers = append(newFinalizers, f) + } + existingNamespace.Finalizers = newFinalizers } return existingNamespace, nil }), @@ -222,6 +238,26 @@ func (r *REST) Delete(ctx context.Context, name string, options *metav1.DeleteOp return r.store.Delete(ctx, name, options) } +func shouldHaveOrphanFinalizer(options *metav1.DeleteOptions, haveOrphanFinalizer bool) bool { + if options.OrphanDependents != nil { + return *options.OrphanDependents + } + if options.PropagationPolicy != nil { + return *options.PropagationPolicy == metav1.DeletePropagationOrphan + } + return haveOrphanFinalizer +} + +func shouldHaveDeleteDependentsFinalizer(options *metav1.DeleteOptions, haveDeleteDependentsFinalizer bool) bool { + if options.OrphanDependents != nil { + return *options.OrphanDependents == false + } + if options.PropagationPolicy != nil { + return *options.PropagationPolicy == metav1.DeletePropagationForeground + } + return haveDeleteDependentsFinalizer +} + func (e *REST) ConvertToTable(ctx context.Context, object runtime.Object, tableOptions runtime.Object) (*metav1beta1.Table, error) { return e.store.ConvertToTable(ctx, object, tableOptions) } diff --git a/pkg/registry/core/namespace/storage/storage_test.go b/pkg/registry/core/namespace/storage/storage_test.go index c1e93f2d1ca..340d7521945 100644 --- a/pkg/registry/core/namespace/storage/storage_test.go +++ b/pkg/registry/core/namespace/storage/storage_test.go @@ -22,11 +22,13 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/generic" genericregistrytest "k8s.io/apiserver/pkg/registry/generic/testing" "k8s.io/apiserver/pkg/registry/rest" etcdtesting "k8s.io/apiserver/pkg/storage/etcd/testing" + api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/registry/registrytest" ) @@ -189,6 +191,217 @@ func TestDeleteNamespaceWithCompleteFinalizers(t *testing.T) { } } +func TestDeleteWithGCFinalizers(t *testing.T) { + storage, server := newStorage(t) + defer server.Terminate(t) + defer storage.store.DestroyFunc() + + propagationBackground := metav1.DeletePropagationBackground + propagationForeground := metav1.DeletePropagationForeground + propagationOrphan := metav1.DeletePropagationOrphan + trueVar := true + + var tests = []struct { + name string + deleteOptions *metav1.DeleteOptions + + existingFinalizers []string + remainingFinalizers map[string]bool + }{ + { + name: "nil-with-orphan", + deleteOptions: nil, + existingFinalizers: []string{ + metav1.FinalizerOrphanDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerOrphanDependents: true, + }, + }, + { + name: "nil-with-delete", + deleteOptions: nil, + existingFinalizers: []string{ + metav1.FinalizerDeleteDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerDeleteDependents: true, + }, + }, + { + name: "nil-without-finalizers", + deleteOptions: nil, + existingFinalizers: []string{}, + remainingFinalizers: map[string]bool{}, + }, + { + name: "propagation-background-with-orphan", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationBackground, + }, + existingFinalizers: []string{ + metav1.FinalizerOrphanDependents, + }, + remainingFinalizers: map[string]bool{}, + }, + { + name: "propagation-background-with-delete", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationBackground, + }, + existingFinalizers: []string{ + metav1.FinalizerDeleteDependents, + }, + remainingFinalizers: map[string]bool{}, + }, + { + name: "propagation-background-without-finalizers", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationBackground, + }, + existingFinalizers: []string{}, + remainingFinalizers: map[string]bool{}, + }, + { + name: "propagation-foreground-with-orphan", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationForeground, + }, + existingFinalizers: []string{ + metav1.FinalizerOrphanDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerDeleteDependents: true, + }, + }, + { + name: "propagation-foreground-with-delete", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationForeground, + }, + existingFinalizers: []string{ + metav1.FinalizerDeleteDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerDeleteDependents: true, + }, + }, + { + name: "propagation-foreground-without-finalizers", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationForeground, + }, + existingFinalizers: []string{}, + remainingFinalizers: map[string]bool{ + metav1.FinalizerDeleteDependents: true, + }, + }, + { + name: "propagation-orphan-with-orphan", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationOrphan, + }, + existingFinalizers: []string{ + metav1.FinalizerOrphanDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerOrphanDependents: true, + }, + }, + { + name: "propagation-orphan-with-delete", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationOrphan, + }, + existingFinalizers: []string{ + metav1.FinalizerDeleteDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerOrphanDependents: true, + }, + }, + { + name: "propagation-orphan-without-finalizers", + deleteOptions: &metav1.DeleteOptions{ + PropagationPolicy: &propagationOrphan, + }, + existingFinalizers: []string{}, + remainingFinalizers: map[string]bool{ + metav1.FinalizerOrphanDependents: true, + }, + }, + { + name: "orphan-dependents-with-orphan", + deleteOptions: &metav1.DeleteOptions{ + OrphanDependents: &trueVar, + }, + existingFinalizers: []string{ + metav1.FinalizerOrphanDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerOrphanDependents: true, + }, + }, + { + name: "orphan-dependents-with-delete", + deleteOptions: &metav1.DeleteOptions{ + OrphanDependents: &trueVar, + }, + existingFinalizers: []string{ + metav1.FinalizerDeleteDependents, + }, + remainingFinalizers: map[string]bool{ + metav1.FinalizerOrphanDependents: true, + }, + }, + { + name: "orphan-dependents-without-finalizers", + deleteOptions: &metav1.DeleteOptions{ + OrphanDependents: &trueVar, + }, + existingFinalizers: []string{}, + remainingFinalizers: map[string]bool{ + metav1.FinalizerOrphanDependents: true, + }, + }, + } + + for _, test := range tests { + key := "namespaces/" + test.name + ctx := genericapirequest.NewContext() + namespace := &api.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: test.name, + Finalizers: test.existingFinalizers, + }, + Spec: api.NamespaceSpec{ + Finalizers: []api.FinalizerName{}, + }, + Status: api.NamespaceStatus{Phase: api.NamespaceActive}, + } + if err := storage.store.Storage.Create(ctx, key, namespace, nil, 0, false); err != nil { + t.Fatalf("unexpected error: %v", err) + } + var obj runtime.Object + var err error + if obj, _, err = storage.Delete(ctx, test.name, test.deleteOptions); err != nil { + t.Fatalf("unexpected error: %v", err) + } + ns, ok := obj.(*api.Namespace) + if !ok { + t.Errorf("unexpected object kind: %+v", obj) + } + if len(ns.Finalizers) != len(test.remainingFinalizers) { + t.Errorf("%s: unexpected remaining finalizers: %v", test.name, ns.Finalizers) + } + for _, f := range ns.Finalizers { + if test.remainingFinalizers[f] != true { + t.Errorf("%s: unexpected finalizer %s", test.name, f) + } + } + } +} + func TestShortNames(t *testing.T) { storage, server := newStorage(t) defer server.Terminate(t)