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