Always Start pvc-protection-controller by pospispa · Pull Request #61282 · kubernetes/kubernetes

pospispa · 2018-03-16T15:31:58Z

What this PR does / why we need it:
After K8s 1.9 is upgraded to K8s 1.10 finalizer [kubernetes.io/pvc-protection] is added to PVCs
because StorageObjectInUseProtection feature is enabled by default in K8s 1.10.
However, when K8s 1.10 is downgraded to K8s 1.9 the finalizers remain in the PVCs and as pvc-protection-controller is not started by default in K8s 1.9 finalizers are not removed automatically from deleted PVCs and that's why deleted PVC are not removed but remain in Terminating phase.

That's why pvc-protection-controller is always started because the pvc-protection-controller removes finalizers from PVCs automatically when a PVC is not in active use by a pod.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #60764

Special notes for your reviewer:
This PR fixes problem with finalizers in PVCs only. It does not fix problem with finalizers in PVs.

Release note:

NONE

pospispa · 2018-03-16T15:33:55Z

@rootfs @saad-ali PTAL

saad-ali · 2018-03-16T17:01:17Z

Couple of comments:

Does this PR only enable the "remove finalizer" component or does it enable both "remove finalizer" and "add finalizer" (we want to add only if alpha flag is set)?
Does this work for both PV/PVC and Pod/PVC? I thought one of these did not exist in 1.9?

pospispa · 2018-03-16T17:14:40Z

Does this PR only enable the "remove finalizer" component or does it enable both "remove finalizer" and "add finalizer" (we want to add only if alpha flag is set)?

It allows both adding and removing finalizers. I forgot to feature flag the adding of finalizers.

Does this work for both PV/PVC and Pod/PVC? I thought one of these did not exist in 1.9?

It works only for Pod/PVC. It doesn't work for PV/PVC.

jdumars · 2018-03-16T17:29:00Z

@jpbetz PTAL - this needs to get in ASAP so we can validate test signal.

pospispa · 2018-03-16T17:34:40Z

/hold

calebamiles · 2018-03-16T17:36:37Z

@pospispa would you mind commenting on the hold for this PR for the release team. Thanks!

cc: @jdumars, @saad-ali, @kubernetes/kubernetes-release-managers

pospispa · 2018-03-16T17:47:26Z

In case the PVCProtection feature is not enabled the pvc-protection-controller is adding finalizers to PVCs, but it must be in finalizer removal only mode, therefore
/hold

k8s-ci-robot · 2018-03-16T18:10:54Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2018-03-16T18:11:00Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: pospispa

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

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

pospispa · 2018-03-16T18:13:15Z

I updated the PR so that in case the PVCProtection feature is not enabled the pvc-protection-controller does not add finalizers to PVCs, but it only removes finalizers, therefore
/hold cancel

pospispa · 2018-03-16T18:15:54Z

@saad-ali @jpbetz @calebamiles @rootfs @jdumars this PR is ready for review.

IMPORTANT: this PR contains only the Pod/PVC part, i.e. finalizers are automatically removed only from PVCs not PVs.

liggitt · 2018-03-16T21:45:41Z

when this completes review, #61299 can be reverted as part of this as well

liggitt · 2018-03-16T21:46:02Z

the finalizers on PV objects must be removed as well (e.g. partial backport of #58743)

liggitt · 2018-03-16T23:50:46Z

pkg/controller/volume/pvcprotection/pvc_protection_controller_test.go

setting global feature gates in unit tests is racy... consider passing a feature gate to the controller and checking the local one rather than mutating global state here (and leaving it mutated after the unit test runs).

See https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/auth/authorizer/node/node_authorizer.go#L53 as an example

After K8s 1.9 is upgraded to K8s 1.10 finalizer [kubernetes.io/pvc-protection] is added to PVCs because StorageObjectInUseProtection feature is enabled by default in K8s 1.10. However, when K8s 1.10 is downgraded to K8s 1.9 the finalizers remain in the PVCs and as pvc-protection-controller is not started by default in K8s 1.9 finalizers are not removed automatically from deleted PVCs and that's why deleted PVC are not removed but remain in Terminating phase. That's why pvc-protection-controller is always started because the pvc-protection-controller removes finalizers from PVCs automatically when a PVC is not in active use by a pod. Related issue: kubernetes#60764

pospispa · 2018-03-17T12:08:17Z

the finalizers on PV objects must be removed as well (e.g. partial backport of #58743)

@liggitt I believe that if I split finalizer removal from PVCs and PVs into 2 separate PRs that it'll simplify the reviews. That's why I plan to create a new separate PR for the PV finalizer removal.

liggitt · 2018-03-17T12:22:52Z

That's fine, just wanted to make sure it was clear that this PR wasn't the extent of the changes needed in 1.9

pospispa · 2018-03-17T14:39:39Z

@liggitt I believe I've addressed all your comments. PTAL. Would you LGTM?

pospispa · 2018-03-17T14:55:40Z

the finalizers on PV objects must be removed as well (e.g. partial backport of #58743)

@liggitt I've created PR #61316 that removes finalizers from PVs.

jpbetz · 2018-03-18T22:44:34Z

/hold
Hold all unmerged cherrypicks for cut of patch release on Monday.

gnufied · 2018-03-19T18:01:43Z

pkg/controller/volume/pvcprotection/pvc_protection_controller.go

 }

 func (c *Controller) addFinalizer(pvc *v1.PersistentVolumeClaim) error {
+	// Skip adding Finalizer in case the PVC Protection feature is not enabled


I think I am missing something but which bits will by default remove the finalizer if feature gate is disabled? I do not see that in the code.

I think what we need to add is - when feature gate is disabled then we need to return false by default from isBeingUsed function so as controller can run in "remove finalizer" only mode. As it exists - this code will not remove the finalizer from PVCs which are in use by pods, even if feature gate is disabled.

gnufied · 2018-03-19T21:18:33Z

@liggitt @pospispa @saad-ali I am not sure that controller change introduced in #61282 and #61316 is enough. We are still left with the problem that - if user upgrades to 1.10 and then downgrades to 1.9 (with aforementioned controller fixes), he can't delete the PVCs until he "touches" the PVCs in some manner because controller will only remove finalizer if it notices add/update event on PVC. We may have to introduce some sort of additional loop that removes finalizers from existing PVC objects, even if they aren't updated.

liggitt · 2018-03-19T21:24:04Z

We are still left with the problem that - if user upgrades to 1.10 and then downgrades to 1.9 (with aforementioned controller fixes), he can't delete the PVCs until he "touches" the PVCs in some manner because controller will only remove finalizer if it notices add/update event on PVC.

No. On startup, every informer gets an Add event for every existing object.

liggitt · 2018-03-19T21:26:23Z

Also, the delete that sets the deletionTimestamp (but leaves the record in place until all finalizers are removed) would trigger an Update watch event

k8s-github-robot · 2018-03-20T16:34:34Z

This PR is not for the master branch but does not have the cherrypick-approved label. Adding the do-not-merge/cherry-pick-not-approved label.

pospispa · 2018-03-20T20:31:20Z

Replaced by PR #61370 so I'm closing this PR.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 16, 2018

k8s-ci-robot requested review from deads2k and sttts March 16, 2018 15:32

k8s-github-robot added the do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. label Mar 16, 2018

pospispa mentioned this pull request Mar 16, 2018

[job failed] 1.9-master upgrade|downgrade jobs #60764

Closed

pospispa force-pushed the 60764-K8s-1.9-PVC-protection-downgrade-issue branch from 03c3671 to d61a1e1 Compare March 16, 2018 17:00

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 16, 2018

pospispa force-pushed the 60764-K8s-1.9-PVC-protection-downgrade-issue branch from d61a1e1 to 4b0ae05 Compare March 16, 2018 18:10

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 16, 2018

pospispa force-pushed the 60764-K8s-1.9-PVC-protection-downgrade-issue branch from 4b0ae05 to f40fe59 Compare March 16, 2018 18:17

pospispa force-pushed the 60764-K8s-1.9-PVC-protection-downgrade-issue branch from f40fe59 to 8cfc579 Compare March 16, 2018 19:19

liggitt mentioned this pull request Mar 16, 2018

Remove PV/PVC finalizers on downgrade from >= 1.10 to < 1.10 #61299

Closed

liggitt reviewed Mar 16, 2018

View reviewed changes

pospispa force-pushed the 60764-K8s-1.9-PVC-protection-downgrade-issue branch from 8cfc579 to 213c8b4 Compare March 17, 2018 12:04

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 17, 2018

pospispa mentioned this pull request Mar 17, 2018

Backport pv-protection-controller Finalizer Removal Part #61316

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 18, 2018

gnufied reviewed Mar 19, 2018

View reviewed changes

gnufied mentioned this pull request Mar 19, 2018

Fix pv pvc finalizer protection 19 #61370

Merged

jpbetz removed the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Mar 20, 2018

k8s-github-robot added the do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. label Mar 20, 2018

pospispa closed this Mar 20, 2018

Conversation

pospispa commented Mar 16, 2018

Uh oh!

pospispa commented Mar 16, 2018

Uh oh!

saad-ali commented Mar 16, 2018

Uh oh!

pospispa commented Mar 16, 2018

Uh oh!

jdumars commented Mar 16, 2018

Uh oh!

pospispa commented Mar 16, 2018

Uh oh!

calebamiles commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pospispa commented Mar 16, 2018

Uh oh!

k8s-ci-robot commented Mar 16, 2018

Uh oh!

k8s-ci-robot commented Mar 16, 2018

Uh oh!

pospispa commented Mar 16, 2018

Uh oh!

pospispa commented Mar 16, 2018

Uh oh!

liggitt commented Mar 16, 2018

Uh oh!

liggitt commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liggitt Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

pospispa commented Mar 17, 2018

Uh oh!

liggitt commented Mar 17, 2018

Uh oh!

pospispa commented Mar 17, 2018

Uh oh!

pospispa commented Mar 17, 2018

Uh oh!

jpbetz commented Mar 18, 2018

Uh oh!

gnufied Mar 19, 2018

Choose a reason for hiding this comment

Uh oh!

gnufied Mar 19, 2018

Choose a reason for hiding this comment

Uh oh!

gnufied commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liggitt commented Mar 19, 2018

Uh oh!

liggitt commented Mar 19, 2018

Uh oh!

k8s-github-robot commented Mar 20, 2018

Uh oh!

pospispa commented Mar 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

calebamiles commented Mar 16, 2018 •

edited

Loading

liggitt commented Mar 16, 2018 •

edited

Loading

gnufied commented Mar 19, 2018 •

edited

Loading