Re: [kubernetes/kubernetes] Postpone Deletion of a Persistent Volume Claim in case It Is Used by a Pod (#55824)
Kubernetes Submit Queue
[MILESTONENOTIFIER] Milestone Pull Request Needs Approval
@gmarek @pospispa @thockin @kubernetes/sig-storage-misc
Action required: This pull request must have the status/approved-for-milestone label applied by a SIG maintainer. If the label is not applied within 6 days, the pull request will be moved out of the v1.9 milestone.
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/feature: New functionality.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
Jan Šafránek
Approved 1.9 milestone.
Jan Šafránek
Added unit tests. Ready for review
To be added in subsequent / parallel PRs:
- e2e test
- scheduler predicate to skip pod with PVCs with deletionTimestamp (these should not be started)
/unassign @gmarek
(since scheduler changes were removed)
Kubernetes Submit Queue
[MILESTONENOTIFIER] Milestone Pull Request Current
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/feature: New functionality.
—
Jan Šafránek
/assign @liggitt
Can you please check the policy change (=last commit)?
/assign @deads2k
Can you please check the admission plugin (=2nd commit) and controller startup (4th one)?
Full review would be nice. The code is quite simple, but there's lot of boilerplate to build a new admission plugin and a new controller :-\
Kubernetes Submit Queue
[MILESTONENOTIFIER] Milestone Pull Request Current
@deads2k @liggitt @pospispa @thockin
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/feature: New functionality.
—
Jan Šafránek
I wonder what to do with MILESTONENOTIFIER. Is there a label missing?
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + DeleteFunc: e.pvcAddedDeletedUpdated,
+ UpdateFunc: func(old, new interface{}) {
+ e.pvcAddedDeletedUpdated(new)
+ },
+ })
+ e.pvcLister = pvcInformer.Lister()
+ e.pvcListerSynced = pvcInformer.Informer().HasSynced
+
+ podInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+ AddFunc: e.podAddedDeletedUpdated,
+ DeleteFunc: e.podAddedDeletedUpdated,
+ UpdateFunc: func(old, new interface{}) {
+ e.podAddedDeletedUpdated(new)
+ },
+ })
+ e.podLister = podInformer.Lister()
set lister prior to adding event handlers
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + queue workqueue.RateLimitingInterface
+}
+
+// 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"),
+ }
+ if cl != nil && cl.CoreV1().RESTClient().GetRateLimiter() != nil {
+ metrics.RegisterMetricAndTrackRateLimiterUsage("persistentvolumeclaim_protection_controller", cl.CoreV1().RESTClient().GetRateLimiter())
+ }
+
+ pvcInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+ AddFunc: e.pvcAddedDeletedUpdated,
+ DeleteFunc: e.pvcAddedDeletedUpdated,
don't need to process pvc deletion, right?
k8s-ci-robot
@pospispa: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-verify | 36b00cd | link | /test pull-kubernetes-verify |
| pull-kubernetes-e2e-gke-gci | 95db8d7 | link | /test pull-kubernetes-e2e-gke-gci |
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.
k8s-ci-robot
Jan Šafránek
/retest
#55537
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> +}
+
+func (c *Controller) runWorker() {
+ for c.processNextWorkItem() {
+ }
+}
+
+// processNextWorkItem deals with one pvcKey off the queue. It returns false when it's time to quit.
+func (c *Controller) processNextWorkItem() bool {
+ pvcKey, quit := c.queue.Get()
+ if quit {
+ return false
+ }
+ defer c.queue.Done(pvcKey)
+
+ pvcNamespace, pvcName, err := cache.SplitMetaNamespaceKey(pvcKey.(string))
return early on error, if you get a parse error on the key, there's no point requeuing
k8s-ci-robot
@pospispa: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-kops-aws | 95db8d7 | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-e2e-gke-gci | 95db8d7 | link | /test pull-kubernetes-e2e-gke-gci |
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.
—
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + return
+ }
+ glog.V(4).Infof("Got event on PVC %s", key)
+
+ if (!volumeutil.IsPVCBeingDeleted(pvc) && !volumeutil.IsProtectionFinalizerPresent(pvc)) || (volumeutil.IsPVCBeingDeleted(pvc) && volumeutil.IsProtectionFinalizerPresent(pvc)) {
+ c.queue.Add(key)
+ }
+}
+
+// podAddedDeletedUpdated reacts to Pod events
+func (c *Controller) podAddedDeletedUpdated(obj interface{}) {
+ pod, ok := obj.(*v1.Pod)
+ if !ok {
+ tombstone, ok := obj.(cache.DeletedFinalStateUnknown)
+ if !ok {
+ utilruntime.HandleError(fmt.Errorf("Couldn't get object from tombstone %#v", obj))
return in this case
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + if (!volumeutil.IsPVCBeingDeleted(pvc) && !volumeutil.IsProtectionFinalizerPresent(pvc)) || (volumeutil.IsPVCBeingDeleted(pvc) && volumeutil.IsProtectionFinalizerPresent(pvc)) {
+ c.queue.Add(key)
+ }
+}
+
+// podAddedDeletedUpdated reacts to Pod events
+func (c *Controller) podAddedDeletedUpdated(obj interface{}) {
+ pod, ok := obj.(*v1.Pod)
+ if !ok {
+ tombstone, ok := obj.(cache.DeletedFinalStateUnknown)
+ if !ok {
+ utilruntime.HandleError(fmt.Errorf("Couldn't get object from tombstone %#v", obj))
+ }
+ pod, ok = tombstone.Obj.(*v1.Pod)
+ if !ok {
+ utilruntime.HandleError(fmt.Errorf("Tombstone contained object that is not a Pod %#v", obj))
return here
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + c.queue.Add(key)
+ }
+}
+
+// podAddedDeletedUpdated reacts to Pod events
+func (c *Controller) podAddedDeletedUpdated(obj interface{}) {
+ pod, ok := obj.(*v1.Pod)
+ if !ok {
+ tombstone, ok := obj.(cache.DeletedFinalStateUnknown)
+ if !ok {
+ utilruntime.HandleError(fmt.Errorf("Couldn't get object from tombstone %#v", obj))
+ }
+ pod, ok = tombstone.Obj.(*v1.Pod)
+ if !ok {
+ utilruntime.HandleError(fmt.Errorf("Tombstone contained object that is not a Pod %#v", obj))
+ }
return in this case
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + }
+ if cl != nil && cl.CoreV1().RESTClient().GetRateLimiter() != nil {
+ metrics.RegisterMetricAndTrackRateLimiterUsage("persistentvolumeclaim_protection_controller", cl.CoreV1().RESTClient().GetRateLimiter())
+ }
+
+ pvcInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+ AddFunc: e.pvcAddedDeletedUpdated,
+ DeleteFunc: e.pvcAddedDeletedUpdated,
+ UpdateFunc: func(old, new interface{}) {
+ e.pvcAddedDeletedUpdated(new)
+ },
+ })
+ e.pvcLister = pvcInformer.Lister()
+ e.pvcListerSynced = pvcInformer.Informer().HasSynced
+
+ podInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
pods get updated a lot... these should filter to only requeue the pvc in the states/transitions listed in https://github.com/kubernetes/community/blob/master/contributors/design-proposals/storage/postpone-pvc-deletion-if-used-in-a-pod.md#pvc-finalizing-controller
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + if volumeutil.IsPVCBeingDeleted(pvc) && volumeutil.IsProtectionFinalizerPresent(pvc) {
+ // PVC should be deleted. Check if it's used and remove finalizer if
+ // it's not.
+ isUsed, err := c.isBeingUsed(pvc)
+ if err != nil {
+ return err
+ }
+ if !isUsed {
+ return c.removeFinalizer(pvc)
+ }
+ }
+
+ if !volumeutil.IsPVCBeingDeleted(pvc) && !volumeutil.IsProtectionFinalizerPresent(pvc) {
+ // PVC is not being deleted -> it should have the finalizer.
+ // The finalizer should be added by admission plugin, this is just
+ // a safeguard.
(and to protect previously existing PVCs)
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + _, err := c.client.CoreV1().PersistentVolumeClaims(claimClone.Namespace).Update(claimClone)
+ if err != nil {
+ glog.V(3).Infof("Error removing protection finalizer from PVC %s/%s: %v", pvc.Namespace, pvc.Name, err)
+ return err
+ }
+ glog.V(3).Infof("Removed protection finalizer from PVC %s/%s", pvc.Namespace, pvc.Name)
+ return nil
+}
+
+func (c *Controller) isBeingUsed(pvc *v1.PersistentVolumeClaim) (bool, error) {
+ pods, err := c.podLister.Pods(pvc.Namespace).List(labels.Everything())
+ if err != nil {
+ return false, err
+ }
+ for _, pod := range pods {
+ if pod.Status.Phase != v1.PodSucceeded && pod.Status.Phase != v1.PodFailed && pod.Status.Phase != v1.PodUnknown {
https://github.com/kubernetes/community/blob/master/contributors/design-proposals/storage/postpone-pvc-deletion-if-used-in-a-pod.md#pvc-finalizing-controller references referring to a node (pod.Spec.NodeName).. I don't see that check here
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + _, err := c.client.CoreV1().PersistentVolumeClaims(claimClone.Namespace).Update(claimClone)
+ if err != nil {
+ glog.V(3).Infof("Error removing protection finalizer from PVC %s/%s: %v", pvc.Namespace, pvc.Name, err)
+ return err
+ }
+ glog.V(3).Infof("Removed protection finalizer from PVC %s/%s", pvc.Namespace, pvc.Name)
+ return nil
+}
+
+func (c *Controller) isBeingUsed(pvc *v1.PersistentVolumeClaim) (bool, error) {
+ pods, err := c.podLister.Pods(pvc.Namespace).List(labels.Everything())
+ if err != nil {
+ return false, err
+ }
+ for _, pod := range pods {
+ if pod.Status.Phase != v1.PodSucceeded && pod.Status.Phase != v1.PodFailed && pod.Status.Phase != v1.PodUnknown {
also, tweak this to continue early, rather than nesting
if pod.Spec.NodeName == "" {
continue
}
if pod.Status.Phase != v1.PodSucceeded && pod.Status.Phase != v1.PodFailed {
continue
}
...
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + _, err := c.client.CoreV1().PersistentVolumeClaims(claimClone.Namespace).Update(claimClone)
+ if err != nil {
+ glog.V(3).Infof("Error removing protection finalizer from PVC %s/%s: %v", pvc.Namespace, pvc.Name, err)
+ return err
+ }
+ glog.V(3).Infof("Removed protection finalizer from PVC %s/%s", pvc.Namespace, pvc.Name)
+ return nil
+}
+
+func (c *Controller) isBeingUsed(pvc *v1.PersistentVolumeClaim) (bool, error) {
+ pods, err := c.podLister.Pods(pvc.Namespace).List(labels.Everything())
+ if err != nil {
+ return false, err
+ }
+ for _, pod := range pods {
+ if pod.Status.Phase != v1.PodSucceeded && pod.Status.Phase != v1.PodFailed && pod.Status.Phase != v1.PodUnknown {
need ack from @kubernetes/sig-storage-pr-reviews @kubernetes/sig-node-pr-reviews on which pod phase implies the kubelet is done unmounting the volume - I'm not sure Unknown is safe
Kubernetes Submit Queue
[MILESTONENOTIFIER] Milestone Pull Request Current
Pull Request Labelssig/nodesig/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/feature: New functionality.
—
Jan Šafránek
PR with scheduler changes: #55957
Jan Šafránek
@jsafrane commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + DeleteFunc: e.pvcAddedDeletedUpdated,
+ UpdateFunc: func(old, new interface{}) {
+ e.pvcAddedDeletedUpdated(new)
+ },
+ })
+ e.pvcLister = pvcInformer.Lister()
+ e.pvcListerSynced = pvcInformer.Informer().HasSynced
+
+ podInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+ AddFunc: e.podAddedDeletedUpdated,
+ DeleteFunc: e.podAddedDeletedUpdated,
+ UpdateFunc: func(old, new interface{}) {
+ e.podAddedDeletedUpdated(new)
+ },
+ })
+ e.podLister = podInformer.Lister()
fixed
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + queue workqueue.RateLimitingInterface
+}
+
+// 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"),
+ }
+ if cl != nil && cl.CoreV1().RESTClient().GetRateLimiter() != nil {
+ metrics.RegisterMetricAndTrackRateLimiterUsage("persistentvolumeclaim_protection_controller", cl.CoreV1().RESTClient().GetRateLimiter())
+ }
+
+ pvcInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+ AddFunc: e.pvcAddedDeletedUpdated,
+ DeleteFunc: e.pvcAddedDeletedUpdated,
removed
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> +}
+
+func (c *Controller) runWorker() {
+ for c.processNextWorkItem() {
+ }
+}
+
+// processNextWorkItem deals with one pvcKey off the queue. It returns false when it's time to quit.
+func (c *Controller) processNextWorkItem() bool {
+ pvcKey, quit := c.queue.Get()
+ if quit {
+ return false
+ }
+ defer c.queue.Done(pvcKey)
+
+ pvcNamespace, pvcName, err := cache.SplitMetaNamespaceKey(pvcKey.(string))
fixed
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + if volumeutil.IsPVCBeingDeleted(pvc) && volumeutil.IsProtectionFinalizerPresent(pvc) {
+ // PVC should be deleted. Check if it's used and remove finalizer if
+ // it's not.
+ isUsed, err := c.isBeingUsed(pvc)
+ if err != nil {
+ return err
+ }
+ if !isUsed {
+ return c.removeFinalizer(pvc)
+ }
+ }
+
+ if !volumeutil.IsPVCBeingDeleted(pvc) && !volumeutil.IsProtectionFinalizerPresent(pvc) {
+ // PVC is not being deleted -> it should have the finalizer.
+ // The finalizer should be added by admission plugin, this is just
+ // a safeguard.
Note added
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + _, err := c.client.CoreV1().PersistentVolumeClaims(claimClone.Namespace).Update(claimClone)
+ if err != nil {
+ glog.V(3).Infof("Error removing protection finalizer from PVC %s/%s: %v", pvc.Namespace, pvc.Name, err)
+ return err
+ }
+ glog.V(3).Infof("Removed protection finalizer from PVC %s/%s", pvc.Namespace, pvc.Name)
+ return nil
+}
+
+func (c *Controller) isBeingUsed(pvc *v1.PersistentVolumeClaim) (bool, error) {
+ pods, err := c.podLister.Pods(pvc.Namespace).List(labels.Everything())
+ if err != nil {
+ return false, err
+ }
+ for _, pod := range pods {
+ if pod.Status.Phase != v1.PodSucceeded && pod.Status.Phase != v1.PodFailed && pod.Status.Phase != v1.PodUnknown {
referring to a node (pod.Spec.NodeName)
Good catch! Added check for NodeName (+unit test). Is it enough to check the informer though? Scheduler might have scheduled the pod already. The proposal says "live pod list", i.e. API call. @liggitt, what do you think?
tweak this to continue early
tweaked
on which pod phase implies the kubelet is done unmounting the volume
Good question... I think this is the check we use to trigger unmounting. There is IMO no check that would say that unmounting is done though.
IMO it's enough to be sure that no container is running. The PV is getting deleted soon, we can delete it few moments before it's unmounted/detached.
@gnufied, @jingxu97, any opinion?
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + return
+ }
+ glog.V(4).Infof("Got event on PVC %s", key)
+
+ if (!volumeutil.IsPVCBeingDeleted(pvc) && !volumeutil.IsProtectionFinalizerPresent(pvc)) || (volumeutil.IsPVCBeingDeleted(pvc) && volumeutil.IsProtectionFinalizerPresent(pvc)) {
+ c.queue.Add(key)
+ }
+}
+
+// podAddedDeletedUpdated reacts to Pod events
+func (c *Controller) podAddedDeletedUpdated(obj interface{}) {
+ pod, ok := obj.(*v1.Pod)
+ if !ok {
+ tombstone, ok := obj.(cache.DeletedFinalStateUnknown)
+ if !ok {
+ utilruntime.HandleError(fmt.Errorf("Couldn't get object from tombstone %#v", obj))
fixed
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + if (!volumeutil.IsPVCBeingDeleted(pvc) && !volumeutil.IsProtectionFinalizerPresent(pvc)) || (volumeutil.IsPVCBeingDeleted(pvc) && volumeutil.IsProtectionFinalizerPresent(pvc)) {
+ c.queue.Add(key)
+ }
+}
+
+// podAddedDeletedUpdated reacts to Pod events
+func (c *Controller) podAddedDeletedUpdated(obj interface{}) {
+ pod, ok := obj.(*v1.Pod)
+ if !ok {
+ tombstone, ok := obj.(cache.DeletedFinalStateUnknown)
+ if !ok {
+ utilruntime.HandleError(fmt.Errorf("Couldn't get object from tombstone %#v", obj))
+ }
+ pod, ok = tombstone.Obj.(*v1.Pod)
+ if !ok {
+ utilruntime.HandleError(fmt.Errorf("Tombstone contained object that is not a Pod %#v", obj))
fixed
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + }
+ if cl != nil && cl.CoreV1().RESTClient().GetRateLimiter() != nil {
+ metrics.RegisterMetricAndTrackRateLimiterUsage("persistentvolumeclaim_protection_controller", cl.CoreV1().RESTClient().GetRateLimiter())
+ }
+
+ pvcInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+ AddFunc: e.pvcAddedDeletedUpdated,
+ DeleteFunc: e.pvcAddedDeletedUpdated,
+ UpdateFunc: func(old, new interface{}) {
+ e.pvcAddedDeletedUpdated(new)
+ },
+ })
+ e.pvcLister = pvcInformer.Lister()
+ e.pvcListerSynced = pvcInformer.Informer().HasSynced
+
+ podInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
I guess there is no way how to get a shared informer with a filter like scheduler does manually
I'll filter pods in podAddedDeletedUpdated.
Jan Šafránek
Thanks @liggitt for thorough review. I addressed all comments, PTAL.
The only one item remains open - should the controller do API call to check that scheduler did not schedule the pod when checking pod.Spec.NodeName == ""? I'm still using informer in the code and it may have old data.
k8s-ci-robot
@pospispa: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce-device-plugin-gpu | 01e3a35 | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
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.
—
k8s-ci-robot
@pospispa: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gke-gci | 95db8d7 | link | /test pull-kubernetes-e2e-gke-gci |
| pull-kubernetes-e2e-gce-device-plugin-gpu | 01e3a35 | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-e2e-gce | 01e3a35 | link | /test pull-kubernetes-e2e-gce |
Saad Ali
Please add E2E tests in this or follow up PR. Otherwise LGTM
/lgtm
/approve
Kubernetes Submit Queue
[MILESTONENOTIFIER] Milestone Pull Request Current
@deads2k @liggitt @pospispa @saad-ali @thockin
sig/nodesig/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/feature: New functionality.
—
Kubernetes Submit Queue
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: pospispa, saad-ali
We suggest the following additional approver: thockin
Assign the PR to them by writing /assign @thockin in a comment when ready.
Associated issue: 45143
The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:- cluster/OWNERS
- cmd/kube-apiserver/OWNERS
- cmd/kube-controller-manager/OWNERS
- hack/OWNERS
- pkg/OWNERS
- plugin/OWNERS
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment
k8s-ci-robot
@pospispa: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gke-gci | 95db8d7 | link | /test pull-kubernetes-e2e-gke-gci |
| pull-kubernetes-e2e-gce-device-plugin-gpu | 01e3a35 | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-e2e-gce | 01e3a35 | link | /test pull-kubernetes-e2e-gce |
| pull-kubernetes-node-e2e | 266d5a0 | link | /test pull-kubernetes-node-e2e |
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.
—
k8s-ci-robot
@pospispa: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gke-gci | 95db8d7 | link | /test pull-kubernetes-e2e-gke-gci |
| pull-kubernetes-node-e2e | 266d5a0 | link | /test pull-kubernetes-node-e2e |
| pull-kubernetes-bazel-test | 266d5a0 | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-e2e-kops-aws | 266d5a0 | link | /test pull-kubernetes-e2e-kops-aws |
k8s-ci-robot
@pospispa: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-node-e2e | 266d5a0 | link | /test pull-kubernetes-node-e2e |
| pull-kubernetes-bazel-test | 266d5a0 | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-e2e-kops-aws | 266d5a0 | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-e2e-gke-gci | 266d5a0 | link | /test pull-kubernetes-e2e-gke-gci |
Jan Šafránek
Fixed an unit test.
k8s-ci-robot
@pospispa: The following test failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gke-gci | 606a828 | link | /test pull-kubernetes-e2e-gke-gci |
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.
—
Michelle Au
@msau42 commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + _, err := c.client.CoreV1().PersistentVolumeClaims(claimClone.Namespace).Update(claimClone)
+ if err != nil {
+ glog.V(3).Infof("Error removing protection finalizer from PVC %s/%s: %v", pvc.Namespace, pvc.Name, err)
+ return err
+ }
+ glog.V(3).Infof("Removed protection finalizer from PVC %s/%s", pvc.Namespace, pvc.Name)
+ return nil
+}
+
+func (c *Controller) isBeingUsed(pvc *v1.PersistentVolumeClaim) (bool, error) {
+ pods, err := c.podLister.Pods(pvc.Namespace).List(labels.Everything())
+ if err != nil {
+ return false, err
+ }
+ for _, pod := range pods {
+ if pod.Status.Phase != v1.PodSucceeded && pod.Status.Phase != v1.PodFailed && pod.Status.Phase != v1.PodUnknown {
When the pod is in failed phase, then it is done unmounting.
I guess there is no concern about potentially corrupting the data if we delete it while it's being unmounted, since we're going to end up deleting it in the end anyway...
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + _, err := c.client.CoreV1().PersistentVolumeClaims(claimClone.Namespace).Update(claimClone)
+ if err != nil {
+ glog.V(3).Infof("Error removing protection finalizer from PVC %s/%s: %v", pvc.Namespace, pvc.Name, err)
+ return err
+ }
+ glog.V(3).Infof("Removed protection finalizer from PVC %s/%s", pvc.Namespace, pvc.Name)
+ return nil
+}
+
+func (c *Controller) isBeingUsed(pvc *v1.PersistentVolumeClaim) (bool, error) {
+ pods, err := c.podLister.Pods(pvc.Namespace).List(labels.Everything())
+ if err != nil {
+ return false, err
+ }
+ for _, pod := range pods {
+ if pod.Status.Phase != v1.PodSucceeded && pod.Status.Phase != v1.PodFailed && pod.Status.Phase != v1.PodUnknown {
I would wait for positive confirmation the pod is terminated (failed/succeeded phase, sounds like?)
We don't know the retain policy on the PV, I wouldn't assume data corruption is acceptable
Michelle Au
@msau42 commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + _, err := c.client.CoreV1().PersistentVolumeClaims(claimClone.Namespace).Update(claimClone)
+ if err != nil {
+ glog.V(3).Infof("Error removing protection finalizer from PVC %s/%s: %v", pvc.Namespace, pvc.Name, err)
+ return err
+ }
+ glog.V(3).Infof("Removed protection finalizer from PVC %s/%s", pvc.Namespace, pvc.Name)
+ return nil
+}
+
+func (c *Controller) isBeingUsed(pvc *v1.PersistentVolumeClaim) (bool, error) {
+ pods, err := c.podLister.Pods(pvc.Namespace).List(labels.Everything())
+ if err != nil {
+ return false, err
+ }
+ for _, pod := range pods {
+ if pod.Status.Phase != v1.PodSucceeded && pod.Status.Phase != v1.PodFailed && pod.Status.Phase != v1.PodUnknown {
It looks like the PR got updated to use an existing helper method for checking pod termination, which is failed or succeeded, so we should be fine now.
Only if the PV retain policy is Delete, then we would've run the risk of deleting the volume while it was unmounting.
Jordan Liggitt
@liggitt commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + _, err := c.client.CoreV1().PersistentVolumeClaims(claimClone.Namespace).Update(claimClone)
+ if err != nil {
+ glog.V(3).Infof("Error removing protection finalizer from PVC %s/%s: %v", pvc.Namespace, pvc.Name, err)
+ return err
+ }
+ glog.V(3).Infof("Removed protection finalizer from PVC %s/%s", pvc.Namespace, pvc.Name)
+ return nil
+}
+
+func (c *Controller) isBeingUsed(pvc *v1.PersistentVolumeClaim) (bool, error) {
+ pods, err := c.podLister.Pods(pvc.Namespace).List(labels.Everything())
+ if err != nil {
+ return false, err
+ }
+ for _, pod := range pods {
+ if pod.Status.Phase != v1.PodSucceeded && pod.Status.Phase != v1.PodFailed && pod.Status.Phase != v1.PodUnknown {
Only if the PV retain policy is Delete, then we would've run the risk of deleting the volume while it was unmounting.
I thought there were also corruption cases possible if a PV got reused by another PVC, but either way, sounds like we're waiting for the right thing now
Kubernetes Submit Queue
@pospispa PR needs rebase
Jan Šafránek
@jsafrane commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + _, err := c.client.CoreV1().PersistentVolumeClaims(claimClone.Namespace).Update(claimClone)
+ if err != nil {
+ glog.V(3).Infof("Error removing protection finalizer from PVC %s/%s: %v", pvc.Namespace, pvc.Name, err)
+ return err
+ }
+ glog.V(3).Infof("Removed protection finalizer from PVC %s/%s", pvc.Namespace, pvc.Name)
+ return nil
+}
+
+func (c *Controller) isBeingUsed(pvc *v1.PersistentVolumeClaim) (bool, error) {
+ pods, err := c.podLister.Pods(pvc.Namespace).List(labels.Everything())
+ if err != nil {
+ return false, err
+ }
+ for _, pod := range pods {
+ if pod.Status.Phase != v1.PodSucceeded && pod.Status.Phase != v1.PodFailed && pod.Status.Phase != v1.PodUnknown {
I am afraid there is no state change of anything when a PVC gets unmounted and detached.
We have three retention policies:
-
delete: it does not matter if the PV gets deleted before it's unmounted & detached. Note that cloud providers don't allow that anyway (and PV controller re-tries deletion until it succeeds).
-
recycle: supported only on NFS. The last pod that used the PV is stopped before PVC is deleted (endured by this controller). So it's fine to start recycling. NFS does not mind being mounted several times.
-
retain: the PV will keep its data. It does not matter if PVC was deleted too early, the PV will be unmounted & detached in the usual way. Only admin can bind retained PV to another PVC.
If we want to keep PVs & PVCs from being deleted while they're attached or mounted, then kubelet and A/D controller need to add their finalizers. Currently they can unmount & detach volumes even if the corresponding PVC & PV was deleted. There are lot of corner cases around that and we could delete lot of tricky code if the PV & PVC were available until they're unmounted & detached, but we have to support inline volumes in pods and they can be deleted at any time... I wonder, would be a finalizer on a pod allowed? It seems quite dangerous to me.
Clayton Coleman
@smarterclayton commented on this pull request.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + return
+ }
+ glog.V(4).Infof("Got event on PVC %s", key)
+
+ if (!volumeutil.IsPVCBeingDeleted(pvc) && !volumeutil.IsProtectionFinalizerPresent(pvc)) || (volumeutil.IsPVCBeingDeleted(pvc) && volumeutil.IsProtectionFinalizerPresent(pvc)) {
+ c.queue.Add(key)
+ }
+}
+
+// podAddedDeletedUpdated reacts to Pod events
+func (c *Controller) podAddedDeletedUpdated(obj interface{}) {
+ pod, ok := obj.(*v1.Pod)
+ if !ok {
+ tombstone, ok := obj.(cache.DeletedFinalStateUnknown)
+ if !ok {
+ utilruntime.HandleError(fmt.Errorf("Couldn't get object from tombstone %#v", obj))
not related to this PR but this is the primary gotcha of every informer driven controller. We need to do better because oversights on this always return in failures in production.
Clayton Coleman
@smarterclayton commented on this pull request.
In plugin/pkg/admission/persistentvolumeclaim/pvcprotection/admission.go:
> +
+func (c *pvcProtectionPlugin) SetInternalKubeInformerFactory(f informers.SharedInformerFactory) {
+ informer := f.Core().InternalVersion().PersistentVolumeClaims()
+ c.lister = informer.Lister()
+ c.SetReadyFunc(informer.Informer().HasSynced)
+}
+
+// ValidateInitialization ensures lister is set.
+func (c *pvcProtectionPlugin) ValidateInitialization() error {
+ if c.lister == nil {
+ return fmt.Errorf("missing lister")
+ }
+ return nil
+}
+
+// Admit sets finalizer on all PVCs. The finalizer is removed by
Add two comments
- the full feature requires all kubelets to be running at least the version this was introduced + have the feature gate enabled (and what the consequences are if the kubelets aren't enabled)
- the change to user behavior - PVC deletion used to be instant, now it can be delayed indefinitely, which may break user scripting and cluster workflow automation.
I assume all of the dynamic provisioner plugins will be updated to be tolerate a delete call not actually deleting the underlying resource? This was the source of many errors on other resources that we changed like this.
Kubernetes Submit Queue
@pospispa PR needs rebase
Jan Šafránek
@jsafrane commented on this pull request.
In plugin/pkg/admission/persistentvolumeclaim/pvcprotection/admission.go:
> +
+func (c *pvcProtectionPlugin) SetInternalKubeInformerFactory(f informers.SharedInformerFactory) {
+ informer := f.Core().InternalVersion().PersistentVolumeClaims()
+ c.lister = informer.Lister()
+ c.SetReadyFunc(informer.Informer().HasSynced)
+}
+
+// ValidateInitialization ensures lister is set.
+func (c *pvcProtectionPlugin) ValidateInitialization() error {
+ if c.lister == nil {
+ return fmt.Errorf("missing lister")
+ }
+ return nil
+}
+
+// Admit sets finalizer on all PVCs. The finalizer is removed by
I assume all of the dynamic provisioner plugins will be updated to be tolerate a delete call not actually deleting the underlying resource?
As it is already implemented now, storage assets are removed only after corresponding PVCs are deleted. If we postpone deletion of a PVC in this PR, we automatically postpone deletion of the storage asset, no change needed.
Jan Šafránek
@jsafrane commented on this pull request.
In plugin/pkg/admission/persistentvolumeclaim/pvcprotection/admission.go:
> +
+func (c *pvcProtectionPlugin) SetInternalKubeInformerFactory(f informers.SharedInformerFactory) {
+ informer := f.Core().InternalVersion().PersistentVolumeClaims()
+ c.lister = informer.Lister()
+ c.SetReadyFunc(informer.Informer().HasSynced)
+}
+
+// ValidateInitialization ensures lister is set.
+func (c *pvcProtectionPlugin) ValidateInitialization() error {
+ if c.lister == nil {
+ return fmt.Errorf("missing lister")
+ }
+ return nil
+}
+
+// Admit sets finalizer on all PVCs. The finalizer is removed by
Add two comments
Do you mean comment in the admission plugin? That feels out of place. Adding to kubelet check.
- what the consequences are if the kubelets aren't enabled
The same as now, kubelet may run a pod whose PVC was marked as deleted or deleted. There is a tiny window of opportunity when scheduler schedules a pod, user deleted a PVC and the controller removed finalizer on PVC, so it should not happen that often.
Added a comment into kubelet
- the change to user behavior
Yes, it is.
Jan Šafránek
rebased + updated comment in kubelet
Jan Šafránek
@jsafrane pushed 1 commit.
- 12243d3 Add policy for the new controller
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
k8s-ci-robot
@pospispa: The following test failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-node-e2e | 12243d3 | link | /test pull-kubernetes-node-e2e |
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.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
k8s-ci-robot
k8s-ci-robot
@pospispa: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-node-e2e | 12243d3 | link | /test pull-kubernetes-node-e2e |
| pull-kubernetes-e2e-gke-gci | 12243d3 | link | /test pull-kubernetes-e2e-gke-gci |
| pull-kubernetes-e2e-kops-aws | 12243d3 | link | /test pull-kubernetes-e2e-kops-aws |
Jan Šafránek
/retest
Jan Šafránek
@smarterclayton, may I ask you for approval? Only few people can approve new features.
Kubernetes Submit Queue
[MILESTONENOTIFIER] Milestone Pull Request Needs Attention
@deads2k @liggitt @pospispa @saad-ali @thockin @kubernetes/sig-node-misc @kubernetes/sig-storage-misc
Action required: During code slush, pull requests in the milestone should be in progress.
If this pull request is not being actively worked on, please remove it from the milestone.
If it is being worked on, please add the status/in-progress label so it can be tracked with other in-flight pull requests.
Note: If this pull request is not resolved or labeled as priority/critical-urgent by Wed, Nov 22 it will be moved out of the v1.9 milestone.
sig/nodesig/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/feature: New functionality.
Kubernetes Submit Queue
Clayton Coleman
Two questions:
- @saad-ali i assume this still has sig-storage approval given risk?
- is this disabled by default / behind a flag gate safely and we've verified that? what's our fallback plan if this goes pear shaped?
Saad Ali
Taking a look
Saad Ali
@saad-ali i assume this still has sig-storage approval given risk?
Yes, since all the new functionality (admission controller and new PVProtection controller) is hidden behind feature gate.
I would like to see the alpha gate around all of the new admission controller code (instead of just Admit) but that's relatively minor.
Saad Ali
/lgtm
/approve
Kubernetes Submit Queue
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: pospispa, saad-ali
We suggest the following additional approver: thockin
Assign the PR to them by writing /assign @thockin in a comment when ready.
Associated issue: 45143
The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:- cluster/OWNERS
- cmd/kube-apiserver/OWNERS
- cmd/kube-controller-manager/OWNERS
- pkg/OWNERS
- plugin/OWNERS
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment
Kubernetes Submit Queue
@pospispa PR needs rebase
Jan Šafránek
rebased
Pavel Pospisil
@jsafrane thank you very much for finishing this PR while I was away on PTO.
/lgtm
k8s-ci-robot
@pospispa: you cannot LGTM your own PR.
In response to this:
@jsafrane thank you very much for finishing this PR while I was away on PTO.
/lgtm
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.
—
Kubernetes Submit Queue
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: pospispa, saad-ali
We suggest the following additional approver: thockin
Assign the PR to them by writing /assign @thockin in a comment when ready.
Associated issue: 45143
The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:- cluster/OWNERS
- cmd/kube-apiserver/OWNERS
- cmd/kube-controller-manager/OWNERS
- pkg/OWNERS
- plugin/OWNERS
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment
—
Michelle Au
/lgtm
Kubernetes Submit Queue
[MILESTONENOTIFIER] Milestone Pull Request Current
@deads2k @liggitt @msau42 @pospispa @saad-ali @thockin
Note: If this pull request is not resolved or labeled as priority/critical-urgent by Wed, Nov 22 it will be moved out of the v1.9 milestone.
sig/nodesig/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/feature: New functionality.
—
Kubernetes Submit Queue
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: msau42, pospispa, saad-ali
We suggest the following additional approver: thockin
Assign the PR to them by writing /assign @thockin in a comment when ready.
Associated issue: 45143
The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:- cluster/OWNERS
- cmd/kube-apiserver/OWNERS
- cmd/kube-controller-manager/OWNERS
- pkg/OWNERS
- plugin/OWNERS
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment
—
Tim Hockin
@thockin commented on this pull request.
I dislike that the admission and the controller are so far apart. We should find a way to let them live in a common parent, and yet get linked/registered into the different pieces that need them.
In pkg/volume/util/finalizer.go:
> + +package util + +import ( + "k8s.io/api/core/v1" +) + +const ( + // Name of finalizer on PVCs that have a running pod. + PVCProtectionFinalizer = "kubernetes.io/pvc-protection" +) + +// IsPVCBeingDeleted returns: +// true: in case PVC is being deleted, i.e. ObjectMeta.DeletionTimestamp is set +// false: in case PVC is not being deleted, i.e. ObjectMeta.DeletionTimestamp is nil +func IsPVCBeingDeleted(pvc *v1.PersistentVolumeClaim) bool {
This seems like a silly util function, to me
In pkg/volume/util/finalizer.go:
> + for _, finalizer := range pvc.Finalizers {
+ if finalizer != PVCProtectionFinalizer {
+ newFinalizers = append(newFinalizers, finalizer)
+ }
+ }
+ if len(newFinalizers) == 0 {
+ // Sanitize for unit tests so we don't need to distinguish empty array
+ // and nil.
+ newFinalizers = nil
+ }
+ pvc.Finalizers = newFinalizers
+}
+
+// AddProtectionFinalizer adds PVCProtectionFinalizer to pvc. It expects that
+// pvc is writable (i.e. is not informer's cached copy.)
+func AddProtectionFinalizer(pvc *v1.PersistentVolumeClaim) {
This util function seems to be called from one place - no need for a util function
In pkg/volume/util/finalizer.go:
> +
+// IsProtectionFinalizerPresent returns true in case PVCProtectionFinalizer is
+// present among the pvc.Finalizers
+func IsProtectionFinalizerPresent(pvc *v1.PersistentVolumeClaim) bool {
+ for _, finalizer := range pvc.Finalizers {
+ if finalizer == PVCProtectionFinalizer {
+ return true
+ }
+ }
+ return false
+}
+
+// RemoveProtectionFinalizer returns pvc without PVCProtectionFinalizer in case
+// it's present in pvc.Finalizers. It expects that pvc is writable (i.e. is not
+// informer's cached copy.)
+func RemoveProtectionFinalizer(pvc *v1.PersistentVolumeClaim) {
This util function seems to be called from one place - no need for a util function, or put it near where it is called. Or generify it to pkg/util/slice as "RemoveString()" or somethign
In pkg/volume/util/finalizer.go:
> + +const ( + // Name of finalizer on PVCs that have a running pod. + PVCProtectionFinalizer = "kubernetes.io/pvc-protection" +) + +// IsPVCBeingDeleted returns: +// true: in case PVC is being deleted, i.e. ObjectMeta.DeletionTimestamp is set +// false: in case PVC is not being deleted, i.e. ObjectMeta.DeletionTimestamp is nil +func IsPVCBeingDeleted(pvc *v1.PersistentVolumeClaim) bool { + return pvc.ObjectMeta.DeletionTimestamp != nil +} + +// IsProtectionFinalizerPresent returns true in case PVCProtectionFinalizer is +// present among the pvc.Finalizers +func IsProtectionFinalizerPresent(pvc *v1.PersistentVolumeClaim) bool {
We already have pkg/util/slice ContainsString(), should use that instead.
In pkg/controller/volume/pvcprotection/pvc_protection_controller.go:
> + return
+ }
+ glog.V(4).Infof("Got event on PVC %s", key)
+
+ if (!volumeutil.IsPVCBeingDeleted(pvc) && !volumeutil.IsProtectionFinalizerPresent(pvc)) || (volumeutil.IsPVCBeingDeleted(pvc) && volumeutil.IsProtectionFinalizerPresent(pvc)) {
+ c.queue.Add(key)
+ }
+}
+
+// podAddedDeletedUpdated reacts to Pod events
+func (c *Controller) podAddedDeletedUpdated(obj interface{}) {
+ pod, ok := obj.(*v1.Pod)
+ if !ok {
+ tombstone, ok := obj.(cache.DeletedFinalStateUnknown)
+ if !ok {
+ utilruntime.HandleError(fmt.Errorf("Couldn't get object from tombstone %#v", obj))
For the less informed - what is the failure mode that leads to this?
Tim Hockin
@thockin commented on this pull request.
This feels like just one piece of the solution - don't we also want to inhibit PV deletion if a PVC references it (harder because not same namespace)?
I am still worried that this opens the door to arbitrary such relationships, which we mostly have not wanted (and could be very detrimental to overall performance of the API).
But I am OK to push this to alpha
Tim Hockin
/lgtm
/approve
Saad Ali
This feels like just one piece of the solution - don't we also want to inhibit PV deletion if a PVC references it (harder because not same namespace)?
That's part of the plan. Just weren't able to get to it this quarter.
Kubernetes Submit Queue
Kubernetes Submit Queue
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: msau42, pospispa, saad-ali, thockin
Associated issue: 45143
The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:cluster/OWNERS[thockin]cmd/kube-apiserver/OWNERS[thockin]cmd/kube-controller-manager/OWNERS[thockin]pkg/OWNERS[thockin]plugin/OWNERS[thockin]
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment
—
Michelle Au
@msau42 commented on this pull request.
In plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go:
> @@ -315,6 +315,14 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) {
eventsRule(),
},
})
+ addControllerRole(&controllerRoles, &controllerRoleBindings, rbac.ClusterRole{
Does this need to be behind a feature gate?
Kubernetes Submit Queue
@pospispa PR needs rebase
Kubernetes Submit Queue
[MILESTONENOTIFIER] Milestone Pull Request Current
@deads2k @liggitt @msau42 @pospispa @saad-ali @thockin
Note: This pull request is marked as priority/critical-urgent, and must be updated every 1 day during code freeze.
Example update:
ACK. In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required
sig/nodesig/storage: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.kind/feature: New functionality.
—
Jordan Liggitt
@liggitt commented on this pull request.
In plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go:
> @@ -315,6 +315,14 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) {
eventsRule(),
},
})
+ addControllerRole(&controllerRoles, &controllerRoleBindings, rbac.ClusterRole{
that would make sense
Pavel Pospisil
@pospispa commented on this pull request.
In plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go:
> @@ -315,6 +315,14 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) {
eventsRule(),
},
})
+ addControllerRole(&controllerRoles, &controllerRoleBindings, rbac.ClusterRole{
Added the feature gate.
Pavel Pospisil
rebased
Jan Šafránek
/lgtm
(rebased + feature gate added to policy since last Tim's review)
Kubernetes Submit Queue
[MILESTONENOTIFIER] Milestone Pull Request Current
@deads2k @jsafrane @liggitt @msau42 @pospispa @saad-ali @thockin
Note: This pull request is marked as priority/critical-urgent, and must be updated every 1 day during code freeze.
Example update:
ACK. In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required
sig/nodesig/storage: Pull Request will be escalated to these SIGs if needed.priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.kind/feature: New functionality.
—
Kubernetes Submit Queue
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: jsafrane, msau42, pospispa, saad-ali, thockin
Associated issue: 45143
The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:cluster/OWNERS[thockin]cmd/kube-apiserver/OWNERS[thockin]cmd/kube-controller-manager/OWNERS[thockin]pkg/OWNERS[thockin]plugin/OWNERS[thockin]
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment
—
Jan Šafránek
ACK. In progress
ETA: 23/11/2017
Risks: missing complete feature in 1.9
[just waited for rebase]
Kubernetes Submit Queue
/test all [submit-queue is verifying that this PR is safe to merge]
k8s-ci-robot
@pospispa: The following test failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-unit | b1b1d31 | link | /test pull-kubernetes-unit |
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.
Jan Šafránek
/retest
#56262
Kubernetes Submit Queue
Automatic merge from submit-queue (batch tested with PRs 55824, 53179). If you want to cherry-pick this change to another branch, please follow the instructions here.
Kubernetes Submit Queue
Merged #55824.
k8s-ci-robot
@pospispa: The following test failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-unit | b1b1d31 | link | /test pull-kubernetes-unit |
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.
—
Pavel Pospisil
@pospispa commented on this pull request.
In pkg/volume/util/finalizer.go:
> + +package util + +import ( + "k8s.io/api/core/v1" +) + +const ( + // Name of finalizer on PVCs that have a running pod. + PVCProtectionFinalizer = "kubernetes.io/pvc-protection" +) + +// IsPVCBeingDeleted returns: +// true: in case PVC is being deleted, i.e. ObjectMeta.DeletionTimestamp is set +// false: in case PVC is not being deleted, i.e. ObjectMeta.DeletionTimestamp is nil +func IsPVCBeingDeleted(pvc *v1.PersistentVolumeClaim) bool {
OK, the func is removed in #56298
Pavel Pospisil
> + +const ( + // Name of finalizer on PVCs that have a running pod. + PVCProtectionFinalizer = "kubernetes.io/pvc-protection" +) + +// IsPVCBeingDeleted returns: +// true: in case PVC is being deleted, i.e. ObjectMeta.DeletionTimestamp is set +// false: in case PVC is not being deleted, i.e. ObjectMeta.DeletionTimestamp is nil +func IsPVCBeingDeleted(pvc *v1.PersistentVolumeClaim) bool { + return pvc.ObjectMeta.DeletionTimestamp != nil +} + +// IsProtectionFinalizerPresent returns true in case PVCProtectionFinalizer is +// present among the pvc.Finalizers +func IsProtectionFinalizerPresent(pvc *v1.PersistentVolumeClaim) bool {
Reworked in #56298
Pavel Pospisil
> +
+// IsProtectionFinalizerPresent returns true in case PVCProtectionFinalizer is
+// present among the pvc.Finalizers
+func IsProtectionFinalizerPresent(pvc *v1.PersistentVolumeClaim) bool {
+ for _, finalizer := range pvc.Finalizers {
+ if finalizer == PVCProtectionFinalizer {
+ return true
+ }
+ }
+ return false
+}
+
+// RemoveProtectionFinalizer returns pvc without PVCProtectionFinalizer in case
+// it's present in pvc.Finalizers. It expects that pvc is writable (i.e. is not
+// informer's cached copy.)
+func RemoveProtectionFinalizer(pvc *v1.PersistentVolumeClaim) {
Created func RemoveString() in #56298
Pavel Pospisil
> + for _, finalizer := range pvc.Finalizers {
+ if finalizer != PVCProtectionFinalizer {
+ newFinalizers = append(newFinalizers, finalizer)
+ }
+ }
+ if len(newFinalizers) == 0 {
+ // Sanitize for unit tests so we don't need to distinguish empty array
+ // and nil.
+ newFinalizers = nil
+ }
+ pvc.Finalizers = newFinalizers
+}
+
+// AddProtectionFinalizer adds PVCProtectionFinalizer to pvc. It expects that
+// pvc is writable (i.e. is not informer's cached copy.)
+func AddProtectionFinalizer(pvc *v1.PersistentVolumeClaim) {
Removed the func in #56298