Enable finalizers independent of GC enablement #51148

ironcladlou · 2017-08-22T20:02:42Z

Decouple finalizer processing from garbage collection configuration.
Finalizers should be effective even when garbage collection is disabled
for a given store.

Fixes #50528.

Finalizers are now honored on custom resources, and on other resources even when garbage collection is disabled via the apiserver flag `--enable-garbage-collector=false`

/cc @kubernetes/sig-api-machinery-bugs
/cc @caesarxuchao @liggitt @sttts @pmorie

Decouple finalizer processing from garbage collection configuration. Finalizers should be effective even when garbage collection is disabled for a given store. Fixes kubernetes#50528.

ironcladlou · 2017-08-22T20:04:49Z

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

 func (e *Store) shouldDeleteDuringUpdate(ctx genericapirequest.Context, key string, obj, existing runtime.Object) bool {
-	if !e.EnableGarbageCollection {
-		return false
-	}


Please closely scrutinize this particular change to ensure there aren't some unintended consequences.

liggitt · 2017-08-22T20:08:29Z

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

+//
+// Because the finalizers created here are only cleared by the garbage
+// collector, deletionFinalizers always returns false when garbage collection is
+// disabled for the Store.
 func deletionFinalizers(e *Store, accessor metav1.Object, options *metav1.DeleteOptions) (bool, []string) {


rename to something like deletionFinalizersForGarbageCollection?

Renamed, and also attempted to make the godocs more clear.

ironcladlou · 2017-08-23T13:00:09Z

/retest

deads2k · 2017-08-23T15:11:58Z

lgtm

caesarxuchao

lgtm.

caesarxuchao · 2017-08-24T23:20:19Z

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

-	} else {
-		// TODO: remove the check on graceful, because we support no-op updates now.
-		if graceful {
-			err, ignoreNotFound, deleteImmediately, out, lastExisting = e.updateForGracefulDeletion(ctx, name, key, options, preconditions, obj)


We used to keep both functions to be able to turn off all finalizers. The finalizers feature has been stablized long enough to remove the redundant code.

caesarxuchao · 2017-08-24T23:23:19Z

staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go

-	if err != nil {
-		t.Fatalf("Unexpected error: %v", err)
-	}
+	gcStates := []bool{true, false}


It looks like you didn't change the tests, you just added the case where gcStates=false, right?

Yeah, ignoring white space (add ?w=1) made that clear. The diff was terrifying otherwise

liggitt · 2017-08-25T00:32:14Z

/lgtm

k8s-github-robot · 2017-08-25T00:33:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, liggitt

Associated issue: 50528

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

Needs approval from an approver in each of these OWNERS Files:

~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

liggitt · 2017-08-25T00:35:15Z

Please add a release note that finalizers on custom resources are now respected.

Since this also affects external servers building on the generic store in the 1.7 timeframe (like service catalog) that had to disable garbage collection because GC didn't support discovered resources, I think we should pick this change to 1.7.x as well

k8s-github-robot · 2017-08-25T04:51:00Z

Automatic merge from submit-queue (batch tested with PRs 51148, 50816, 49741, 50858, 51223)

wojtek-t · 2017-08-25T06:05:41Z

@liggitt @caesarxuchao @ironcladlou - can you please add a user-friendly release note if you want this cherrypicked?

liggitt · 2017-08-25T06:10:25Z

done

caesarxuchao · 2017-08-25T06:13:35Z

@liggitt, could you also add "finalizers are now respected even if the garbage collector is disabled via the apiserver flag --enable-garbage-collector"?

liggitt · 2017-08-25T06:23:06Z

done

wojtek-t · 2017-08-25T07:55:35Z

Automated cherrypick is resulting in some conficts. Can one of you please create the cherrypick?

wojtek-t · 2017-08-28T08:35:03Z

@liggitt @caesarxuchao @ironcladlou ^^

Decouple finalizer processing from garbage collection configuration. Finalizers should be effective even when garbage collection is disabled for a given store. Fixes kubernetes#50528. Backport of kubernetes#51148.

ironcladlou · 2017-08-28T18:52:46Z

1.7 backport PR is #51469

@caesarxuchao

Automatic merge from submit-queue Enable finalizers independent of GC enablement Decouple finalizer processing from garbage collection configuration. Finalizers should be effective even when garbage collection is disabled for a given store. Fixes #50528. Backport of #51148. ```release-note Finalizers are now honored on custom resources, and on other resources even when garbage collection is disabled via the apiserver flag `--enable-garbage-collector=false` ``` /cc @kubernetes/sig-api-machinery-bugs /cc @caesarxuchao @liggitt @sttts @pmorie