Merge pull request #80476 from matte21/fix-pvcs-leaks

PVC protection controller: fix PVC leaks

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",

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 {

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
+		// Pod event to simulate. This Pod is *not* added to
-		// initalObjects.
+		// 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 {
+		switch {
-			ctrl.podAddedDeletedUpdated(test.updatedPod, false)
+		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)
-		if test.deletedPod != nil {
+		case test.updatedPod != nil:
-			ctrl.podAddedDeletedUpdated(test.deletedPod, true)
+			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