diff --git a/pkg/controller/volume/persistentvolume/binder_test.go b/pkg/controller/volume/persistentvolume/binder_test.go index fb5f3ffadd2..3147b504cff 100644 --- a/pkg/controller/volume/persistentvolume/binder_test.go +++ b/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 { diff --git a/pkg/controller/volume/persistentvolume/framework_test.go b/pkg/controller/volume/persistentvolume/framework_test.go index 4f6d556583f..aeecce3e0aa 100644 --- a/pkg/controller/volume/persistentvolume/framework_test.go +++ b/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 diff --git a/pkg/controller/volume/persistentvolume/pv_controller.go b/pkg/controller/volume/persistentvolume/pv_controller.go index 28ee91912d3..ad5ac88bff1 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller.go +++ b/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 diff --git a/pkg/controller/volume/persistentvolume/pv_controller_test.go b/pkg/controller/volume/persistentvolume/pv_controller_test.go index 311e0e42a36..5eb60e5e1eb 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller_test.go +++ b/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) {