github/kubernetes
1
0
Fork 0
mirror of https://github.com/k3s-io/kubernetes.git synced 2025-07-22 19:31:44 +00:00
Code Issues Projects Releases Wiki Activity

PVC protection controller: get rid of PVC leaks

Browse Source 
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.
This commit is contained in:
matte21 2019-07-22 21:42:52 +02:00
parent 7e6b70fbb5
commit b4fed83a4a
3 changed files with 93 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
pkg/controller/volume/pvcprotection/BUILD
View File

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

37
pkg/controller/volume/pvcprotection/pvc_protection_controller.go
View File

@ -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 {

74
pkg/controller/volume/pvcprotection/pvc_protection_controller_test.go
View File

@ -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