From 5686664a1d6507cfcab11a20a4aada13077b6c9b Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 27 Oct 2020 11:16:58 +0100 Subject: [PATCH 1/4] PV controller: don't delete PVs when PVC is not known yet Normally, the PV controller knows about the PVC that triggers the creation of a PV before it sees the PV, because the PV controller must set the volume.beta.kubernetes.io/storage-provisioner annotation that tells an external provisioner to create the PV. When restarting, the PV controller first syncs its caches, so that case is also covered. However, the creator of a PVC might decided to set that annotation itself to speed up volume creation. While unusual, it's not forbidden and thus part of the external Kubernetes API. Whether it makes sense depends on the intentions of the user. When that is done and there is heavy load, an external provisioner might see the PVC and create a PV before the PV controller sees the PVC. If the PV controller then encounters the PV before the PVC, it incorrectly concludes that the PV needs to be deleted instead of being bound. The same issue occurred earlier for external binding and the existing code for looking up a PVC in the cache or in the apiserver solves the issue also for volume provisioning, it just needs to be enabled also for PVs without the pv.kubernetes.io/bound-by-controller annotation. --- pkg/controller/volume/persistentvolume/pv_controller.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/controller/volume/persistentvolume/pv_controller.go b/pkg/controller/volume/persistentvolume/pv_controller.go index 61b1ed1f3ef..e3d7dfbe2a8 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller.go +++ b/pkg/controller/volume/persistentvolume/pv_controller.go @@ -577,9 +577,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 From 06f934ea1f1f31566672f3999dbec5c0685bdd0e Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 28 Oct 2020 10:37:27 +0100 Subject: [PATCH 2/4] pv controller test: enable klog output This makes it possible to run tests with -v=5 and thus actually get some output. --- pkg/controller/volume/persistentvolume/framework_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/controller/volume/persistentvolume/framework_test.go b/pkg/controller/volume/persistentvolume/framework_test.go index 4f6d556583f..f032a0278a8 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: From 22f81e9e0b1e54d049ffc63b8c0b1db09e6b0c70 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 28 Oct 2020 10:39:59 +0100 Subject: [PATCH 3/4] pv controller test: use sub tests This makes it possible to run individual tests. --- .../volume/persistentvolume/framework_test.go | 11 ++++++++--- .../volume/persistentvolume/pv_controller_test.go | 11 ++++++++--- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/pkg/controller/volume/persistentvolume/framework_test.go b/pkg/controller/volume/persistentvolume/framework_test.go index f032a0278a8..cebdcff40f1 100644 --- a/pkg/controller/volume/persistentvolume/framework_test.go +++ b/pkg/controller/volume/persistentvolume/framework_test.go @@ -632,9 +632,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) @@ -679,6 +677,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_test.go b/pkg/controller/volume/persistentvolume/pv_controller_test.go index ed29de9c084..896148f0c2b 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller_test.go +++ b/pkg/controller/volume/persistentvolume/pv_controller_test.go @@ -273,9 +273,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{} @@ -353,6 +351,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) { From 24f57647870c770cbb4431bd7b1e4e8be0b1fb67 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 28 Oct 2020 10:40:42 +0100 Subject: [PATCH 4/4] pv controller test: more test cases The main goal was to cover retrieval of a PVC from the apiserver when it isn't known yet. This is achieved by adding PVCs and (for the sake of completeness) PVs to the reactor, but not the controller, when a special annotation is set. The approach with a special annotation was chosen because it doesn't affect other tests. The other test cases were added while checking the existing tests because (at least at first glance) the situations seemed to be not covered. --- .../volume/persistentvolume/binder_test.go | 34 +++++++++++++++++++ .../volume/persistentvolume/framework_test.go | 11 ++++++ .../persistentvolume/pv_controller_test.go | 11 ++++++ 3 files changed, 56 insertions(+) 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 cebdcff40f1..aeecce3e0aa 100644 --- a/pkg/controller/volume/persistentvolume/framework_test.go +++ b/pkg/controller/volume/persistentvolume/framework_test.go @@ -95,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" @@ -641,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) diff --git a/pkg/controller/volume/persistentvolume/pv_controller_test.go b/pkg/controller/volume/persistentvolume/pv_controller_test.go index 896148f0c2b..5c39e17a9dc 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",