Merge pull request #91705 from mrkm4ntr/revert-assumed-in-unreserve

Revert assumed PVs and PVCs in unreserve extension point

pkg/controller/volume/scheduling/scheduler_binder.go

@@ -124,6 +124,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.
@@ -381,6 +384,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.

pkg/controller/volume/scheduling/scheduler_binder_fake.go

@@ -58,6 +58,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

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)
 		}
 	}
@@ -1237,7 +1238,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)
 		}
@@ -1249,6 +1250,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

pkg/scheduler/framework/plugins/volumebinding/volume_binding.go

@@ -205,9 +205,10 @@ func (pl *VolumeBinding) PreBind(ctx context.Context, cs *framework.CycleState,
 	return nil
 }
 
-// Unreserve clears pod binding state.
+// Unreserve clears assumed PV and PVC cache and pod binding state.
-// TODO(#90962) Revert assumed PV/PVC cache
+// 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
 }