namespace: remove gc finalizers based on delete options #76051

zhouhaibing089 · 2019-04-02T20:16:37Z

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.

/kind bug
/sig api-machinery

Fixes #76042

NONE

k8s-ci-robot · 2019-04-02T20:16:45Z

Hi @zhouhaibing089. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

caesarxuchao · 2019-04-04T20:47:46Z

/assign

caesarxuchao · 2019-04-05T16:43:00Z

/ok-to-test

caesarxuchao · 2019-04-05T19:45:10Z

pkg/registry/core/namespace/storage/storage_test.go

+		}
+		if obj, _, err := storage.Delete(ctx, test.name, test.deleteOptions); err != nil {
+			t.Errorf("unexpected error: %v", err)
+		} else {


Use t.Fatal in the first case and remove the "else" here to reduce indention.

Sounds good.

caesarxuchao · 2019-04-05T19:45:15Z

pkg/registry/core/namespace/storage/storage.go

+					newFinalizers = append(newFinalizers, metav1.FinalizerDeleteDependents)
+				}
+
+				existingNamespace.ObjectMeta.Finalizers = newFinalizers


Check if finalizers have changed to avoid unnecessary update.

kubernetes/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go

Lines 740 to 744 in 20cece6

oldFinalizerSet := sets.NewString(accessor.GetFinalizers()...)

newFinalizersSet := sets.NewString(newFinalizers...)

if oldFinalizerSet.Equal(newFinalizersSet) {

return false, accessor.GetFinalizers()

}

Also this is not the logic of the generic store, which will preserve the existing finalizers in the object is the deleteOptions doesn't explicitly set the mode of the deletion. See

kubernetes/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go

Lines 619 to 623 in 20cece6

// shouldOrphanDependents returns true if the finalizer for orphaning should be set

// updated for FinalizerOrphanDependents. In the order of highest to lowest

// priority, there are three factors affect whether to add/remove the

// FinalizerOrphanDependents: options, existing finalizers of the object,

// and e.DeleteStrategy.DefaultGarbageCollectionPolicy.

and

kubernetes/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go

Lines 669 to 673 in 20cece6

// shouldDeleteDependents returns true if the finalizer for foreground deletion should be set

// updated for FinalizerDeleteDependents. In the order of highest to lowest

// priority, there are three factors affect whether to add/remove the

// FinalizerDeleteDependents: options, existing finalizers of the object, and

// e.DeleteStrategy.DefaultGarbageCollectionPolicy.

Also, could you double check how the namespace controller will react if the namespace deletion is blocked by others?

The old code doesn't honor the foregroundDeletion, which could cause the namespace deletion to be blocked.

Ok, thanks for catching this. Now the behavior is changed to preserve finalizers if the delete options does not say explicitly.

could you double check how the namespace controller will react if the namespace deletion is blocked by others?

Checking now.

@caesarxuchao: Namespace controller does not care about finalizers in object meta too much, it only cares about the finalizer kubernetes in the spec.

If kubernetes finalizer is removed, and it gets an event, it just sends a delete call again.

caesarxuchao · 2019-04-05T19:46:03Z

pkg/registry/core/namespace/storage/storage_test.go

+				Finalizers: []string{
+					metav1.FinalizerOrphanDependents,
+					metav1.FinalizerDeleteDependents,
+				},


Also test a namespace that originally doesn't have finalizers?

Yes, new tests added.

zhouhaibing089 · 2019-04-05T21:56:05Z

/test pull-kubernetes-bazel-test

pkg/registry/core/namespace/storage/storage.go

pkg/registry/core/namespace/storage/storage_test.go

zhouhaibing089 · 2019-04-12T15:56:57Z

/retest

caesarxuchao · 2019-04-12T20:49:23Z

/lgtm
/approve

zhouhaibing089 · 2019-04-16T01:48:56Z

/assign @lavalamp

lavalamp · 2019-04-26T22:09:06Z

pkg/registry/core/namespace/storage/storage.go

-						if string(finalizer) != metav1.FinalizerOrphanDependents {
-							newFinalizers = append(newFinalizers, finalizer)
-						}
+				// remove orphan and foregroundDeletion first, add them back if needed


Why is the logic structured this way? I am finding it very hard to follow. I would expect:

// 1. collect data currentFinalizers := map[string]bool{} for range existingNamespace.ObjectMeta.Finalizers { ... } // 2. compute which finalizers it should have shouldHaveFinalizers := map[string]bool{ metav1.FinalizerOrphanDependents: shouldHaveOrphanFinalizer(options, ...), metav1.FinalizerDeleteDependents: shouldHaveForgroundFinalizer(options, ...), } // 3. do we need to make changes? for k, v := range shouldHaveFinalizers { if v2, ok := currentFinalizers[k]; !ok || v != v2 { changeNeeded = true; break } } // 4. Make changes if changeNeeded { ... }

Putting the "shouldHaveXXXFinalizer" logic in a separate function will make it easier to unit test.

Ok, let me organize the code in the suggested way.

Thanks, sorry for being a pain!

Updated. Thanks! (That's a great suggestion!)

Thanks, much easier to follow now. (Next time we change this I'll recommend not mutating the "current" map, but this is fine for now. :) )

lavalamp · 2019-04-26T22:17:03Z

Why is the code different between namespace and other resources?

Thanks for the extensive tests.

zhouhaibing089 · 2019-04-26T22:35:20Z

Why is the code different between namespace and other resources?

@lavalamp: It should not be different. And that's exactly the thing this change would like to fix.

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.

zhouhaibing089 · 2019-04-30T18:54:14Z

Kindly ping @caesarxuchao and @lavalamp

lavalamp · 2019-04-30T20:03:19Z

/lgtm
/approve

k8s-ci-robot · 2019-04-30T20:03:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, lavalamp, zhouhaibing089

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/registry/OWNERS~~ [lavalamp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

caesarxuchao · 2019-05-14T20:42:32Z

Note that the behavior of the namespace regarding finalizers is still slightly different from other built-in resources. When an UPDATE request removes the last metadata.finalizers, even if the spec.finalizers is empty, the namespace is not deleted by the apiserver. Clients need to send yet another DELETE request to delete the namespace.

CRD has the similar behavior.

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Apr 2, 2019

k8s-ci-robot requested review from justinsb and wojtek-t April 2, 2019 20:17

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 2, 2019

k8s-ci-robot assigned caesarxuchao Apr 4, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 5, 2019

caesarxuchao mentioned this pull request Apr 5, 2019

Adding more e2e tests for federated namespace cascading deletion and fixing bugs #36283

Merged

caesarxuchao reviewed Apr 5, 2019

View reviewed changes

zhouhaibing089 force-pushed the rm-orphan-by-default branch from 03278a2 to 51f7344 Compare April 5, 2019 21:24

caesarxuchao reviewed Apr 11, 2019

View reviewed changes

zhouhaibing089 force-pushed the rm-orphan-by-default branch 2 times, most recently from 3ee2e45 to d9af2c2 Compare April 12, 2019 01:04

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2019

k8s-ci-robot assigned lavalamp Apr 16, 2019

lavalamp reviewed Apr 26, 2019

View reviewed changes

namespace: remove gc finalizers based on delete options

a458e9b

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.

zhouhaibing089 force-pushed the rm-orphan-by-default branch from d9af2c2 to a458e9b Compare April 26, 2019 23:13

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 26, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 30, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2019

k8s-ci-robot merged commit b4cc2a5 into kubernetes:master Apr 30, 2019

zhouhaibing089 mentioned this pull request May 4, 2019

A Story about DeleteOptions zhouhaibing089/blog#33

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

namespace: remove gc finalizers based on delete options #76051

namespace: remove gc finalizers based on delete options #76051

zhouhaibing089 commented Apr 2, 2019 •

edited

k8s-ci-robot commented Apr 2, 2019

caesarxuchao commented Apr 4, 2019

caesarxuchao commented Apr 5, 2019

caesarxuchao Apr 5, 2019

zhouhaibing089 Apr 5, 2019

caesarxuchao Apr 5, 2019

caesarxuchao Apr 5, 2019

caesarxuchao Apr 5, 2019

zhouhaibing089 Apr 5, 2019

zhouhaibing089 Apr 5, 2019 •

edited

caesarxuchao Apr 5, 2019

zhouhaibing089 Apr 5, 2019

zhouhaibing089 commented Apr 5, 2019

zhouhaibing089 commented Apr 12, 2019

caesarxuchao commented Apr 12, 2019

zhouhaibing089 commented Apr 16, 2019

lavalamp Apr 26, 2019

zhouhaibing089 Apr 26, 2019

lavalamp Apr 26, 2019

zhouhaibing089 Apr 26, 2019

lavalamp Apr 30, 2019

lavalamp commented Apr 26, 2019

zhouhaibing089 commented Apr 26, 2019

zhouhaibing089 commented Apr 30, 2019

lavalamp commented Apr 30, 2019

k8s-ci-robot commented Apr 30, 2019

caesarxuchao commented May 14, 2019

	oldFinalizerSet := sets.NewString(accessor.GetFinalizers()...)
	newFinalizersSet := sets.NewString(newFinalizers...)
	if oldFinalizerSet.Equal(newFinalizersSet) {
	return false, accessor.GetFinalizers()
	}

	// shouldOrphanDependents returns true if the finalizer for orphaning should be set
	// updated for FinalizerOrphanDependents. In the order of highest to lowest
	// priority, there are three factors affect whether to add/remove the
	// FinalizerOrphanDependents: options, existing finalizers of the object,
	// and e.DeleteStrategy.DefaultGarbageCollectionPolicy.

	// shouldDeleteDependents returns true if the finalizer for foreground deletion should be set
	// updated for FinalizerDeleteDependents. In the order of highest to lowest
	// priority, there are three factors affect whether to add/remove the
	// FinalizerDeleteDependents: options, existing finalizers of the object, and
	// e.DeleteStrategy.DefaultGarbageCollectionPolicy.

namespace: remove gc finalizers based on delete options #76051

namespace: remove gc finalizers based on delete options #76051

Conversation

zhouhaibing089 commented Apr 2, 2019 • edited

k8s-ci-robot commented Apr 2, 2019

caesarxuchao commented Apr 4, 2019

caesarxuchao commented Apr 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouhaibing089 Apr 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouhaibing089 commented Apr 5, 2019

zhouhaibing089 commented Apr 12, 2019

caesarxuchao commented Apr 12, 2019

zhouhaibing089 commented Apr 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Apr 26, 2019

zhouhaibing089 commented Apr 26, 2019

zhouhaibing089 commented Apr 30, 2019

lavalamp commented Apr 30, 2019

k8s-ci-robot commented Apr 30, 2019

caesarxuchao commented May 14, 2019

zhouhaibing089 commented Apr 2, 2019 •

edited

zhouhaibing089 Apr 5, 2019 •

edited