From b4fed83a4aa2e935e5e762bb7e6ea055e18f5e47 Mon Sep 17 00:00:00 2001 From: matte21 Date: Mon, 22 Jul 2019 21:42:52 +0200 Subject: [PATCH] PVC protection controller: get rid of PVC leaks Make the PVC protection controller robust to cases where a Pod X is deleted, then a Pod Y with the same namespaced name is created and the two events are delivered via a single update notification. Both pods should be processed, because X might be blocking deletion of a PVC which is not referenced by Y. Prior to this commit only the newer pod is processed, which means that it is possible to leak PVCs. Also, add unit tests to reflect the change. --- pkg/controller/volume/pvcprotection/BUILD | 1 + .../pvc_protection_controller.go | 37 ++++++++-- .../pvc_protection_controller_test.go | 74 ++++++++++++++++--- 3 files changed, 93 insertions(+), 19 deletions(-) diff --git a/pkg/controller/volume/pvcprotection/BUILD b/pkg/controller/volume/pvcprotection/BUILD index fcf1058f3d9..9d6c4b77b63 100644 --- a/pkg/controller/volume/pvcprotection/BUILD +++ b/pkg/controller/volume/pvcprotection/BUILD @@ -38,6 +38,7 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/client-go/informers:go_default_library", "//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library", "//staging/src/k8s.io/client-go/testing:go_default_library", diff --git a/pkg/controller/volume/pvcprotection/pvc_protection_controller.go b/pkg/controller/volume/pvcprotection/pvc_protection_controller.go index a79d538611c..979679b3403 100644 --- a/pkg/controller/volume/pvcprotection/pvc_protection_controller.go +++ b/pkg/controller/volume/pvcprotection/pvc_protection_controller.go @@ -20,7 +20,7 @@ import ( "fmt" "time" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/labels" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -79,13 +79,13 @@ func NewPVCProtectionController(pvcInformer coreinformers.PersistentVolumeClaimI e.podListerSynced = podInformer.Informer().HasSynced podInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - e.podAddedDeletedUpdated(obj, false) + e.podAddedDeletedUpdated(nil, obj, false) }, DeleteFunc: func(obj interface{}) { - e.podAddedDeletedUpdated(obj, true) + e.podAddedDeletedUpdated(nil, obj, true) }, UpdateFunc: func(old, new interface{}) { - e.podAddedDeletedUpdated(new, false) + e.podAddedDeletedUpdated(old, new, false) }, }) @@ -257,27 +257,48 @@ func (c *Controller) pvcAddedUpdated(obj interface{}) { } // podAddedDeletedUpdated reacts to Pod events -func (c *Controller) podAddedDeletedUpdated(obj interface{}, deleted bool) { +func (c *Controller) podAddedDeletedUpdated(old, new interface{}, deleted bool) { + if pod := c.parsePod(new); pod != nil { + c.enqueuePVCs(pod, deleted) + + // An update notification might mask the deletion of a pod X and the + // following creation of a pod Y with the same namespaced name as X. If + // that's the case X needs to be processed as well to handle the case + // where it is blocking deletion of a PVC not referenced by Y, otherwise + // such PVC will never be deleted. + if oldPod := c.parsePod(old); oldPod != nil && oldPod.UID != pod.UID { + c.enqueuePVCs(oldPod, true) + } + } +} + +func (*Controller) parsePod(obj interface{}) *v1.Pod { + if obj == nil { + return nil + } 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 + return nil } pod, ok = tombstone.Obj.(*v1.Pod) if !ok { utilruntime.HandleError(fmt.Errorf("Tombstone contained object that is not a Pod %#v", obj)) - return + return nil } } + return pod +} +func (c *Controller) enqueuePVCs(pod *v1.Pod, deleted bool) { // Filter out pods that can't help us to remove a finalizer on PVC if !deleted && !volumeutil.IsPodTerminated(pod, pod.Status) && pod.Spec.NodeName != "" { return } - klog.V(4).Infof("Got event on pod %s/%s", pod.Namespace, pod.Name) + klog.V(4).Infof("Enqueuing PVCs for Pod %s/%s (UID=%s)", pod.Namespace, pod.Name, pod.UID) // Enqueue all PVCs that the pod uses for _, volume := range pod.Spec.Volumes { diff --git a/pkg/controller/volume/pvcprotection/pvc_protection_controller_test.go b/pkg/controller/volume/pvcprotection/pvc_protection_controller_test.go index ff164278c65..f65b55f75ec 100644 --- a/pkg/controller/volume/pvcprotection/pvc_protection_controller_test.go +++ b/pkg/controller/volume/pvcprotection/pvc_protection_controller_test.go @@ -24,12 +24,13 @@ import ( "github.com/davecgh/go-spew/spew" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" clienttesting "k8s.io/client-go/testing" @@ -49,6 +50,7 @@ const ( defaultPVCName = "pvc1" defaultPodName = "pod1" defaultNodeName = "node1" + defaultUID = "uid1" ) func pod() *v1.Pod { @@ -56,6 +58,7 @@ func pod() *v1.Pod { ObjectMeta: metav1.ObjectMeta{ Name: defaultPodName, Namespace: defaultNS, + UID: defaultUID, }, Spec: v1.PodSpec{ NodeName: defaultNodeName, @@ -100,6 +103,11 @@ func withStatus(phase v1.PodPhase, pod *v1.Pod) *v1.Pod { return pod } +func withUID(uid types.UID, pod *v1.Pod) *v1.Pod { + pod.ObjectMeta.UID = uid + return pod +} + func pvc() *v1.PersistentVolumeClaim { return &v1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ @@ -152,13 +160,13 @@ func TestPVCProtectionController(t *testing.T) { // Optional client reactors. reactors []reaction // PVC event to simulate. This PVC will be automatically added to - // initalObjects. + // initialObjects. updatedPVC *v1.PersistentVolumeClaim // Pod event to simulate. This Pod will be automatically added to - // initalObjects. + // initialObjects. updatedPod *v1.Pod - // Pod event to similate. This Pod is *not* added to - // initalObjects. + // Pod event to simulate. This Pod is *not* added to + // initialObjects. deletedPod *v1.Pod // List of expected kubeclient actions that should happen during the // test. @@ -177,7 +185,7 @@ func TestPVCProtectionController(t *testing.T) { storageObjectInUseProtectionEnabled: true, }, { - name: "StorageObjectInUseProtection Disabled, PVC without finalizer -> finalizer is added", + name: "StorageObjectInUseProtection Disabled, PVC without finalizer -> finalizer is not added", updatedPVC: pvc(), expectedActions: []clienttesting.Action{}, storageObjectInUseProtectionEnabled: false, @@ -315,6 +323,48 @@ func TestPVCProtectionController(t *testing.T) { }, storageObjectInUseProtectionEnabled: true, }, + { + name: "pod delete and create with same namespaced name seen as an update, old pod used deleted PVC -> finalizer is removed", + initialObjects: []runtime.Object{ + deleted(withProtectionFinalizer(pvc())), + }, + deletedPod: withPVC(defaultPVCName, pod()), + updatedPod: withUID("uid2", pod()), + expectedActions: []clienttesting.Action{ + clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())), + }, + storageObjectInUseProtectionEnabled: true, + }, + { + name: "pod delete and create with same namespaced name seen as an update, old pod used non-deleted PVC -> finalizer is not removed", + initialObjects: []runtime.Object{ + withProtectionFinalizer(pvc()), + }, + deletedPod: withPVC(defaultPVCName, pod()), + updatedPod: withUID("uid2", pod()), + expectedActions: []clienttesting.Action{}, + storageObjectInUseProtectionEnabled: true, + }, + { + name: "pod delete and create with same namespaced name seen as an update, both pods reference deleted PVC -> finalizer is not removed", + initialObjects: []runtime.Object{ + deleted(withProtectionFinalizer(pvc())), + }, + deletedPod: withPVC(defaultPVCName, pod()), + updatedPod: withUID("uid2", withPVC(defaultPVCName, pod())), + expectedActions: []clienttesting.Action{}, + storageObjectInUseProtectionEnabled: true, + }, + { + name: "pod update from unscheduled to scheduled, deleted PVC is referenced -> finalizer is not removed", + initialObjects: []runtime.Object{ + deleted(withProtectionFinalizer(pvc())), + }, + deletedPod: unscheduled(withPVC(defaultPVCName, pod())), + updatedPod: withPVC(defaultPVCName, pod()), + expectedActions: []clienttesting.Action{}, + storageObjectInUseProtectionEnabled: true, + }, } for _, test := range tests { @@ -358,11 +408,13 @@ func TestPVCProtectionController(t *testing.T) { if test.updatedPVC != nil { ctrl.pvcAddedUpdated(test.updatedPVC) } - if test.updatedPod != nil { - ctrl.podAddedDeletedUpdated(test.updatedPod, false) - } - if test.deletedPod != nil { - ctrl.podAddedDeletedUpdated(test.deletedPod, true) + switch { + case test.deletedPod != nil && test.updatedPod != nil && test.deletedPod.Namespace == test.updatedPod.Namespace && test.deletedPod.Name == test.updatedPod.Name: + ctrl.podAddedDeletedUpdated(test.deletedPod, test.updatedPod, false) + case test.updatedPod != nil: + ctrl.podAddedDeletedUpdated(nil, test.updatedPod, false) + case test.deletedPod != nil: + ctrl.podAddedDeletedUpdated(nil, test.deletedPod, true) } // Process the controller queue until we get expected results