-
Notifications
You must be signed in to change notification settings - Fork 39.2k
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
Always Start pvc-protection-controller and pv-protection-controller #61324
Always Start pvc-protection-controller and pv-protection-controller #61324
Conversation
d4bd21b
to
e3229aa
Compare
@liggitt @jsafrane @NickrenREN PTAL |
/test pull-kubernetes-e2e-gce |
/milestone v1.10 based on comment in #60764 |
Until announced, 1.10 PRs should be opened against master. 1.10 branch is still fast-forwarding. |
I'm not sure this is necessary if the feature gate default is the same as it will be in 1.11 |
e3229aa
to
1c9fe2c
Compare
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
Rebased to master.
@liggitt OK, I agree that when the feature gate is the same this is not necessary. However, when the |
/retest |
@deads2k I believe I've addressed all your comments. Would you approve the PR? Should I squash the commits? |
@deads2k Let me, please, re-phrase which issue is this PR trying to fix: This PR is adding into K8s 1.11+ automatic removal of finalizers nevertheless the StorageObjectInUseProtection feature is enabled or disabled. So do we need automatic removal of finalizers also in K8s 1.10? Note: the automatic removal of finalizers was added into K8s 1.9.6 in PR #61370 |
} | ||
|
||
// NewPVCProtectionController returns a new *{VCProtectionController. | ||
func NewPVCProtectionController(pvcInformer coreinformers.PersistentVolumeClaimInformer, podInformer coreinformers.PodInformer, cl clientset.Interface) *Controller { | ||
e := &Controller{ | ||
client: cl, | ||
queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "pvcprotection"), | ||
storageObjectInUseProtectionEnabled: utilfeature.DefaultFeatureGate.Enabled(features.StorageObjectInUseProtection), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't take the feature gate dependency here, pass it in. Feature gates at the very edges, config everywhere else. Globals are evil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k please, does it now look good from you point of view?
ctx.InformerFactory.Core().V1().PersistentVolumeClaims(), | ||
ctx.InformerFactory.Core().V1().Pods(), | ||
ctx.ClientBuilder.ClientOrDie("pvc-protection-controller"), | ||
utilfeature.DefaultFeatureGate.Enabled(features.StorageObjectInUseProtection), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is ok for now. Eventually we'll want to push it back into the context/config, but this a fair point to start.
/approve |
/approve please squash before merge. |
7e0d723
to
81c2747
Compare
@deads2k thank you for the approval. I've squashed the commits. |
/retest |
After K8s 1.10 is upgraded to K8s 1.11 finalizer [kubernetes.io/pvc-protection] is added to PVCs because StorageObjectInUseProtection feature will be GA in K8s 1.11. However, when K8s 1.11 is downgraded to K8s 1.10 and the StorageObjectInUseProtection feature is disabled the finalizers remain in the PVCs and as pvc-protection-controller is not started in K8s 1.10 finalizers are not removed automatically from deleted PVCs and that's why deleted PVC are not removed from the system but remain in Terminating phase. The same applies to pv-protection-controller and [kubernetes.io/pvc-protection] finalizer in PVs. 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
81c2747
to
d3ddf7e
Compare
/retest |
/lgtm Yes, I think we'll have to cherry pick this into 1.10, otherwise we'll have a gap in behavior between 1.9 and 1.11. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, msau42, pospispa 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 |
/retest Review the full test history for this PR. Silence the bot with an |
/test all [submit-queue is verifying that this PR is safe to merge] |
@pospispa: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
Automatic merge from submit-queue (batch tested with PRs 61324, 62880, 62765). If you want to cherry-pick this change to another branch, please follow the instructions here. |
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
…ction-feature-to-GA-2nd-attempt Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Bring StorageObjectInUseProtection feature to GA **What this PR does / why we need it**: It brings `StorageObjectInUseProtection` feature to GA. **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 **Special notes for your reviewer**: Related features: kubernetes/enhancements#498 and kubernetes/enhancements#499 Related PR: #61324 **Release note**: ```release-note StorageObjectInUseProtection feature is GA. ```
…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. ```
What this PR does / why we need it:
After K8s 1.10 is upgraded to K8s 1.11 finalizer
[kubernetes.io/pvc-protection]
is added to PVCsbecause
StorageObjectInUseProtection
feature will be GA in K8s 1.11.However, when K8s 1.11 is downgraded to K8s 1.10 and the
StorageObjectInUseProtection
feature is disabled the finalizers remain in the PVCs and aspvc-protection-controller
is not started in K8s 1.10 finalizers are not removed automatically from deleted PVCs and that's why deleted PVC are not removed from the system but remain inTerminating
phase.The same applies to
pv-protection-controller
and[kubernetes.io/pvc-protection]
finalizer in PVs.That's why
pvc-protection-controller
is always started because thepvc-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 notBound
to a PVC.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
This issue #60764 is for downgrade from K8s 1.10 to K8s 1.9.
This PR fixes the same problem but for downgrade from K8s 1.11 to K8s 1.10.
Special notes for your reviewer:
Release note: