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.

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

Fix pv pvc finalizer protection 19 #61370

Merged

Conversation

gnufied
Copy link
Member

@gnufied gnufied commented Mar 19, 2018

This PR includes both PRs by @pospispa #61282 and #61316

It fixes #60764 for 1.9.

It contains following changes:

  1. Enables pvc protection by default. If PVC protection feature is disabled, it removes finalizer from any PVC on add/update otherwise it defaults to adding finalizer.
  2. It enables a PV protection controller. But this controller only removes finalizer and does not add them. if PVC protection feature is disabled, it removes finalizer from any PV (whether bound/pending) on update.

cc @pospispa @msau42 @liggitt @saad-ali

/sig storage

Add support for adding/removing finalizer depending on PVC protection feature gate

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
@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 19, 2018
@k8s-github-robot 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 19, 2018
After K8s 1.9 is upgraded to K8s 1.10 finalizer [kubernetes.io/pv-protection] is added to PVs
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 PVs and as pv-protection-controller
does not exist in K8s 1.9 PV finalizers are not removed automatically from deleted PVs and that's why
deleted PV remain in the system.

That's why the finalizer removing part of the pv-protection-controller is backported from K8s 1.10 in order to remove
finalizers automatically when a PV is deleted and is not Bound to a PVC.

Related issue: kubernetes#60764
Related pv-protection-controller PR: kubernetes#58743
@gnufied gnufied force-pushed the fix-pv-pvc-finalizer-protection-19 branch from 9bf3442 to 86d4e93 Compare March 19, 2018 22:25
@msau42
Copy link
Member

msau42 commented Mar 19, 2018

Do e2e tests need to be updated too?

@msau42
Copy link
Member

msau42 commented Mar 19, 2018

I guess for 1.9 e2e tests are only run in the alpha suite, so it is fine.

@msau42
Copy link
Member

msau42 commented Mar 20, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 20, 2018
@gnufied
Copy link
Member Author

gnufied commented Mar 20, 2018

@msau42 yeah e2e tests are only run in alpha suite. I thought of adding some e2e tests that check if finalizers are removed by these controllers when feature flag is disabled, but there is no easy way to make finalizers "stick" since any API modification via test will be immediately undone by the controller.

@jpbetz jpbetz added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. labels Mar 20, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: gnufied, msau42

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:

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

@jpbetz jpbetz added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Mar 20, 2018
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 20, 2018
@gnufied
Copy link
Member Author

gnufied commented Mar 20, 2018

/retest

@jpbetz jpbetz merged commit 9c6072e into kubernetes:release-1.9 Mar 20, 2018
pospispa added a commit to pospispa/kubernetes that referenced this pull request Apr 21, 2018
StorageObjectInUseProtection feature is enabled by default in K8s 1.10+. Assume K8s cluster is used with this feature enabled, i.e. finalizers are added to all PVs and PVCs. In case the K8s cluster admin disables the StorageObjectInUseProtection feature and a user deletes a PVC that is not in active use by a pod then the PVC is not removed from the system because of the finalizer. Therefore, the user will have to remove the finalizer manually in order to have the PVC removed from the system. Note: deleted PVs won't be removed from the system also because of finalizers.

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. Also the pv-protection-controller is always started to remove finalizers from PVs automatically when a PV is not Bound to a PVC.

Related issue:
kubernetes#60764

Related PRs:
kubernetes#61370
kubernetes#61324
k8s-github-robot pushed a commit that referenced this pull request Jun 4, 2018
…ction-downgrade-issue-cherry-pick-into-K8s-1.10

Automatic merge from submit-queue.

cherry-pick into K8s 1.10: Always Start pvc-protection-controller and pv-protection-controller

**What this PR does / why we need it**:
StorageObjectInUseProtection feature is enabled by default in K8s 1.10+. Assume K8s cluster is used with this feature enabled, i.e. finalizers are added to all PVs and PVCs. In case the K8s cluster admin disables the StorageObjectInUseProtection feature and a user deletes a PVC that is not in active use by a pod then the PVC is not removed from the system because of the finalizer. Therefore, the user will have to remove the finalizer manually in order to have the PVC removed from the system. Note: deleted PVs won't be removed from the system also because of finalizers.

This problem was fixed in [K8s 1.9.6](https://github.com/kubernetes/kubernetes/releases/tag/v1.9.6) in PR #61370
This problem is also fixed in K8s 1.11+ in PR #61324
However, this problem is not fixed in K8s 1.10, that's why I've cherry-picked the PR #61324 and proposing to merge it into K8s 1.10.

**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 
N/A

Related issue: #60764

**Special notes for your reviewer**:

**Release note**:

```release-note
In case StorageObjectInUse feature is disabled and Persistent Volume (PV) or Persistent Volume Claim (PVC) contains a finalizer and the PV or PVC is deleted it is not automatically removed from the system. Now, it is automatically removed.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants