Merge pull request #95909 from pohly/pv-controller-delete-pv-fix

PV controller: don't delete PVs when PVC is not known yet

pkg/controller/volume/persistentvolume/binder_test.go

@@ -528,6 +528,40 @@ func TestSync(t *testing.T) {
 			newClaimArray("claim4-8", "uid4-8", "10Gi", "volume4-8-x", v1.ClaimBound, nil),
 			noevents, noerrors, testSyncVolume,
 		},
+		{
+			// syncVolume with volume bound to bound claim.
+			// Check that the volume is not deleted.
+			"4-9 - volume bound to bound claim, with PersistentVolumeReclaimDelete",
+			newVolumeArray("volume4-9", "10Gi", "uid4-9", "claim4-9", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classEmpty),
+			newVolumeArray("volume4-9", "10Gi", "uid4-9", "claim4-9", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
+			newClaimArray("claim4-9", "uid4-9", "10Gi", "volume4-9", v1.ClaimBound, nil),
+			newClaimArray("claim4-9", "uid4-9", "10Gi", "volume4-9", v1.ClaimBound, nil),
+			noevents, noerrors, testSyncVolume,
+		},
+		{
+			// syncVolume with volume bound to missing claim.
+			// Check that a volume deletion is attempted. It fails because there is no deleter.
+			"4-10 - volume bound to missing claim",
+			newVolumeArray("volume4-10", "10Gi", "uid4-10", "claim4-10", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classEmpty),
+			func() []*v1.PersistentVolume {
+				volumes := newVolumeArray("volume4-10", "10Gi", "uid4-10", "claim4-10", v1.VolumeFailed, v1.PersistentVolumeReclaimDelete, classEmpty)
+				volumes[0].Status.Message = `Error getting deleter volume plugin for volume "volume4-10": no volume plugin matched`
+				return volumes
+			}(),
+			noclaims,
+			noclaims,
+			noevents, noerrors, testSyncVolume,
+		},
+		{
+			// syncVolume with volume bound to claim which exists in etcd but not in the local cache.
+			// Check that nothing changes, in contrast to case 4-10 above.
+			"4-11 - volume bound to unknown claim",
+			newVolumeArray("volume4-11", "10Gi", "uid4-11", "claim4-11", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classEmpty),
+			newVolumeArray("volume4-11", "10Gi", "uid4-11", "claim4-11", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
+			newClaimArray("claim4-11", "uid4-11", "10Gi", "volume4-11", v1.ClaimBound, nil, annSkipLocalStore),
+			newClaimArray("claim4-11", "uid4-11", "10Gi", "volume4-11", v1.ClaimBound, nil, annSkipLocalStore),
+			noevents, noerrors, testSyncVolume,
+		},
 
 		// PVC with class
 		{

pkg/controller/volume/persistentvolume/framework_test.go

@@ -47,6 +47,10 @@ import (
 	"k8s.io/kubernetes/pkg/volume/util/recyclerclient"
 )
 
+func init() {
+	klog.InitFlags(nil)
+}
+
 // This is a unit test framework for persistent volume controller.
 // It fills the controller with test claims/volumes and can simulate these
 // scenarios:
@@ -91,6 +95,11 @@ type controllerTest struct {
 	test testCall
 }
 
+// annSkipLocalStore can be used to mark initial PVs or PVCs that are meant to be added only
+// to the fake apiserver (i.e. available via Get) but not to the local store (i.e. the controller
+// won't have them in its cache).
+const annSkipLocalStore = "pv-testing-skip-local-store"
+
 type testCall func(ctrl *PersistentVolumeController, reactor *pvtesting.VolumeReactor, test controllerTest) error
 
 const testNamespace = "default"
@@ -628,9 +637,7 @@ func evaluateTestResults(ctrl *PersistentVolumeController, reactor *pvtesting.Vo
 //    controllerTest.testCall *once*.
 // 3. Compare resulting volumes and claims with expected volumes and claims.
 func runSyncTests(t *testing.T, tests []controllerTest, storageClasses []*storage.StorageClass, pods []*v1.Pod) {
-	for _, test := range tests {
-		klog.V(4).Infof("starting test %q", test.name)
-
+	doit := func(t *testing.T, test controllerTest) {
 		// Initialize the controller
 		client := &fake.Clientset{}
 		ctrl, err := newTestController(client, nil, true)
@@ -639,9 +646,15 @@ func runSyncTests(t *testing.T, tests []controllerTest, storageClasses []*storag
 		}
 		reactor := newVolumeReactor(client, ctrl, nil, nil, test.errors)
 		for _, claim := range test.initialClaims {
+			if metav1.HasAnnotation(claim.ObjectMeta, annSkipLocalStore) {
+				continue
+			}
 			ctrl.claims.Add(claim)
 		}
 		for _, volume := range test.initialVolumes {
+			if metav1.HasAnnotation(volume.ObjectMeta, annSkipLocalStore) {
+				continue
+			}
 			ctrl.volumes.store.Add(volume)
 		}
 		reactor.AddClaims(test.initialClaims)
@@ -675,6 +688,13 @@ func runSyncTests(t *testing.T, tests []controllerTest, storageClasses []*storag
 
 		evaluateTestResults(ctrl, reactor.VolumeReactor, test, t)
 	}
+
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			doit(t, test)
+		})
+	}
 }
 
 // Test multiple calls to syncClaim/syncVolume and periodic sync of all

pkg/controller/volume/persistentvolume/pv_controller.go

@@ -581,9 +581,10 @@ func (ctrl *PersistentVolumeController) syncVolume(volume *v1.PersistentVolume)
 		if err != nil {
 			return err
 		}
-		if !found && metav1.HasAnnotation(volume.ObjectMeta, pvutil.AnnBoundByController) {
-			// If PV is bound by external PV binder (e.g. kube-scheduler), it's
-			// possible on heavy load that corresponding PVC is not synced to
+		if !found {
+			// If the PV was created by an external PV provisioner or
+			// bound by external PV binder (e.g. kube-scheduler), it's
+			// possible under heavy load that the corresponding PVC is not synced to
 			// controller local cache yet. So we need to double-check PVC in
 			//   1) informer cache
 			//   2) apiserver if not found in informer cache

pkg/controller/volume/persistentvolume/pv_controller_test.go

@@ -78,6 +78,17 @@ func TestControllerSync(t *testing.T) {
 				return nil
 			},
 		},
+		{
+			"5-2-2 - complete bind when PV and PVC both exist",
+			newVolumeArray("volume5-2", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimRetain, classEmpty),
+			newVolumeArray("volume5-2", "1Gi", "uid5-2", "claim5-2", v1.VolumeBound, v1.PersistentVolumeReclaimRetain, classEmpty, pvutil.AnnBoundByController),
+			newClaimArray("claim5-2", "uid5-2", "1Gi", "", v1.ClaimPending, nil),
+			newClaimArray("claim5-2", "uid5-2", "1Gi", "volume5-2", v1.ClaimBound, nil, pvutil.AnnBoundByController, pvutil.AnnBindCompleted),
+			noevents, noerrors,
+			func(ctrl *PersistentVolumeController, reactor *pvtesting.VolumeReactor, test controllerTest) error {
+				return nil
+			},
+		},
 		{
 			// deleteClaim with a bound claim makes bound volume released.
 			"5-3 - delete claim",
@@ -273,9 +284,7 @@ func TestControllerSync(t *testing.T) {
 		},
 	}
 
-	for _, test := range tests {
-		klog.V(4).Infof("starting test %q", test.name)
-
+	doit := func(test controllerTest) {
 		// Initialize the controller
 		client := &fake.Clientset{}
 
@@ -352,6 +361,13 @@ func TestControllerSync(t *testing.T) {
 
 		evaluateTestResults(ctrl, reactor.VolumeReactor, test, t)
 	}
+
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			doit(test)
+		})
+	}
 }
 
 func storeVersion(t *testing.T, prefix string, c cache.Store, version string, expectedReturn bool) {