From 79ab958996f4a1d1ebcd20b695624a73d664883f Mon Sep 17 00:00:00 2001 From: Shintaro Murakami Date: Sat, 13 Jun 2020 10:40:00 +0900 Subject: [PATCH] Revert assumed PVs and PVCs in unreserve extension point --- .../volume/scheduling/scheduler_binder.go | 9 +++++ .../scheduling/scheduler_binder_fake.go | 3 ++ .../scheduling/scheduler_binder_test.go | 35 +++++++++++++++++-- .../plugins/volumebinding/volume_binding.go | 5 +-- 4 files changed, 47 insertions(+), 5 deletions(-) diff --git a/pkg/controller/volume/scheduling/scheduler_binder.go b/pkg/controller/volume/scheduling/scheduler_binder.go index ca91364e38e..710b2d5a301 100644 --- a/pkg/controller/volume/scheduling/scheduler_binder.go +++ b/pkg/controller/volume/scheduling/scheduler_binder.go @@ -118,6 +118,9 @@ type SchedulerVolumeBinder interface { // This function is called serially. AssumePodVolumes(assumedPod *v1.Pod, nodeName string) (allFullyBound bool, err error) + // RevertAssumedPodVolumes will revert assumed PV and PVC cache. + RevertAssumedPodVolumes(assumedPod *v1.Pod, nodeName string) + // BindPodVolumes will: // 1. Initiate the volume binding by making the API call to prebind the PV // to its matching PVC. @@ -387,6 +390,12 @@ func (b *volumeBinder) AssumePodVolumes(assumedPod *v1.Pod, nodeName string) (al return } +// RevertAssumedPodVolumes will revert assumed PV and PVC cache. +func (b *volumeBinder) RevertAssumedPodVolumes(assumedPod *v1.Pod, nodeName string) { + b.revertAssumedPVs(b.podBindingCache.GetBindings(assumedPod, nodeName)) + b.revertAssumedPVCs(b.podBindingCache.GetProvisionedPVCs(assumedPod, nodeName)) +} + // BindPodVolumes gets the cached bindings and PVCs to provision in podBindingCache, // makes the API update for those PVs/PVCs, and waits for the PVCs to be completely bound // by the PV controller. diff --git a/pkg/controller/volume/scheduling/scheduler_binder_fake.go b/pkg/controller/volume/scheduling/scheduler_binder_fake.go index ddf1cedf31e..a4df0089eb5 100644 --- a/pkg/controller/volume/scheduling/scheduler_binder_fake.go +++ b/pkg/controller/volume/scheduling/scheduler_binder_fake.go @@ -53,6 +53,9 @@ func (b *FakeVolumeBinder) AssumePodVolumes(assumedPod *v1.Pod, nodeName string) return b.config.AllBound, b.config.AssumeErr } +// RevertAssumedPodVolumes implements SchedulerVolumeBinder.RevertAssumedPodVolumes +func (b *FakeVolumeBinder) RevertAssumedPodVolumes(assumedPod *v1.Pod, nodeName string) {} + // BindPodVolumes implements SchedulerVolumeBinder.BindPodVolumes. func (b *FakeVolumeBinder) BindPodVolumes(assumedPod *v1.Pod) error { b.BindCalled = true diff --git a/pkg/controller/volume/scheduling/scheduler_binder_test.go b/pkg/controller/volume/scheduling/scheduler_binder_test.go index 75a84958a68..29f71301c90 100644 --- a/pkg/controller/volume/scheduling/scheduler_binder_test.go +++ b/pkg/controller/volume/scheduling/scheduler_binder_test.go @@ -465,13 +465,14 @@ func (env *testEnv) validateAssume(t *testing.T, pod *v1.Pod, bindings []*bindin } } -func (env *testEnv) validateFailedAssume(t *testing.T, pod *v1.Pod, bindings []*bindingInfo, provisionings []*v1.PersistentVolumeClaim) { +func (env *testEnv) validateCacheRestored(t *testing.T, pod *v1.Pod, bindings []*bindingInfo, provisionings []*v1.PersistentVolumeClaim) { // All PVs have been unmodified in cache pvCache := env.internalBinder.pvCache for _, b := range bindings { pv, _ := pvCache.GetPV(b.pv.Name) + apiPV, _ := pvCache.GetAPIPV(b.pv.Name) // PV could be nil if it's missing from cache - if pv != nil && pv != b.pv { + if pv != nil && pv != apiPV { t.Errorf("PV %q was modified in cache", b.pv.Name) } } @@ -1225,7 +1226,7 @@ func TestAssumePodVolumes(t *testing.T) { scenario.expectedProvisionings = scenario.provisionedPVCs } if scenario.shouldFail { - testEnv.validateFailedAssume(t, pod, scenario.expectedBindings, scenario.expectedProvisionings) + testEnv.validateCacheRestored(t, pod, scenario.bindings, scenario.provisionedPVCs) } else { testEnv.validateAssume(t, pod, scenario.expectedBindings, scenario.expectedProvisionings) } @@ -1237,6 +1238,34 @@ func TestAssumePodVolumes(t *testing.T) { } } +func TestRevertAssumedPodVolumes(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + podPVCs := []*v1.PersistentVolumeClaim{unboundPVC, provisionedPVC} + bindings := []*bindingInfo{makeBinding(unboundPVC, pvNode1a)} + pvs := []*v1.PersistentVolume{pvNode1a} + provisionedPVCs := []*v1.PersistentVolumeClaim{provisionedPVC} + expectedBindings := []*bindingInfo{makeBinding(unboundPVC, pvNode1aBound)} + expectedProvisionings := []*v1.PersistentVolumeClaim{selectedNodePVC} + + // Setup + testEnv := newTestBinder(t, ctx.Done()) + testEnv.initClaims(podPVCs, podPVCs) + pod := makePod(podPVCs) + testEnv.initPodCache(pod, "node1", bindings, provisionedPVCs) + testEnv.initVolumes(pvs, pvs) + + allbound, err := testEnv.binder.AssumePodVolumes(pod, "node1") + if allbound || err != nil { + t.Errorf("No volumes are assumed") + } + testEnv.validateAssume(t, pod, expectedBindings, expectedProvisionings) + + testEnv.binder.RevertAssumedPodVolumes(pod, "node1") + testEnv.validateCacheRestored(t, pod, bindings, provisionedPVCs) +} + func TestBindAPIUpdate(t *testing.T) { type scenarioType struct { // Inputs diff --git a/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go b/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go index 5c9ad28cec3..ffd72bed4c1 100644 --- a/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go +++ b/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go @@ -157,9 +157,10 @@ func (pl *VolumeBinding) PreBind(ctx context.Context, cs *framework.CycleState, return nil } -// Unreserve clears pod binding state. -// TODO(#90962) Revert assumed PV/PVC cache +// Unreserve clears assumed PV and PVC cache and pod binding state. +// It's idempotent, and does nothing if no cache or binding state found for the given pod. func (pl *VolumeBinding) Unreserve(ctx context.Context, cs *framework.CycleState, pod *v1.Pod, nodeName string) { + pl.Binder.RevertAssumedPodVolumes(pod, nodeName) pl.Binder.DeletePodBindings(pod) return }