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

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

Conversation

pospispa
Copy link
Contributor

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 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:

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.

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-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 21, 2018
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 21, 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 Apr 21, 2018
@pospispa
Copy link
Contributor Author

/sig storage
/kind bug
/assign @msau42
@deads2k @liggitt @jsafrane @NickrenREN I believe you may be interested.

@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. kind/bug Categorizes issue or PR as related to a bug. labels Apr 21, 2018
@pospispa
Copy link
Contributor Author

@deads2k @msau42
we've started discussion whether the PR #61324 should be cherry-picked into K8s 1.10 here, here, here and here.

As I think the PR #61324 should be cherry-picked into K8s 1.10 I've created this PR. So let's continue our discussion in this PR.

@pospispa
Copy link
Contributor Author

/retest

@msau42
Copy link
Member

msau42 commented Apr 23, 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 Apr 23, 2018
@MaciekPytel
Copy link
Contributor

@pospispa Is that identical to #61324 or was some custom change required for 1.10? If it's identical next time please use the hack/cherry_pick_pull.sh to create cherry-pick (no need to change this one).

@deads2k Can you approve? Since there was some discussion on whether this should be cherry-picked on original PR I will defer with adding cherrypick-approved until it's approved.

@pospispa
Copy link
Contributor Author

@pospispa Is that identical to #61324 or was some custom change required for 1.10?

@MaciekPytel yes, this PR is identical to #61324 I created the cherry-pick manually because I didn't know about hack/cherry_pick_pull.sh The manual cherry-pick was clean, there were no custom changes.

@MaciekPytel
Copy link
Contributor

@deads2k Can you approve (or state that you don't approve)? This needs to merge by tomorrow EOD to make it to 1.10.3.

@pospispa
Copy link
Contributor Author

/retest

1 similar comment
@pospispa
Copy link
Contributor Author

/retest

@saad-ali
Copy link
Member

From sig-storage:

/approve

@saad-ali saad-ali added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label May 29, 2018
@saad-ali saad-ali requested a review from MaciekPytel May 29, 2018 20:02
@saad-ali
Copy link
Member

Assigning to 1.10 branch manager @MaciekPytel

@MaciekPytel MaciekPytel added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. labels May 30, 2018
@MaciekPytel
Copy link
Contributor

@deads2k Can you approve?

@pospispa
Copy link
Contributor Author

/retest

@mikedanese
Copy link
Member

/approve

@BenTheElder
Copy link
Member

/lgtm
[bumping the bot]

@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@BenTheElder @MaciekPytel @msau42 @pospispa

Pull Request Labels
  • sig/storage: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/bug: Fixes a bug discovered during the current release.
Help

@BenTheElder
Copy link
Member

BenTheElder commented Jun 2, 2018

update: it seems that @mikedanese is only an approver for this on master / 1.11 but the approvers mechanism checks against the target branch, so potentially we should cherry pick back approver changes... :|

@pospispa
Copy link
Contributor Author

pospispa commented Jun 3, 2018

/retest

@liggitt
Copy link
Member

liggitt commented Jun 4, 2018

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, liggitt, mikedanese, msau42, pospispa, saad-ali

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@pospispa
Copy link
Contributor Author

pospispa commented Jun 4, 2018

/retest

@k8s-github-robot
Copy link

Automatic merge from submit-queue.

@k8s-github-robot k8s-github-robot merged commit bbcb3aa into kubernetes:release-1.10 Jun 4, 2018
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. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants