From e323306ce010c9940c6afc6eedbb12862c8ba07a Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Tue, 21 Dec 2021 15:36:34 +0100 Subject: [PATCH 1/3] Add new watchers to PV controller tests Add fake Pod and Node watchers to the tests. It only reduces test noise: Failed to watch *v1.Pod: unhandled watch: testing.WatchActionImpl{ActionImpl:testing.ActionImpl{Namespace:"", Verb:"watch", Resource:schema.GroupVersionResource{Group:"", Version:"v1", Resource:"pods"}, Subresource:""}, WatchRestrictions:testing.WatchRestrictions{Labels:labels.internalSelector(nil), Fields:fields.andTerm{}, ResourceVersion:""}} --- pkg/controller/volume/persistentvolume/pv_controller_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/controller/volume/persistentvolume/pv_controller_test.go b/pkg/controller/volume/persistentvolume/pv_controller_test.go index 1c694d9642b..0a03cc4dc5b 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller_test.go +++ b/pkg/controller/volume/persistentvolume/pv_controller_test.go @@ -324,6 +324,8 @@ func TestControllerSync(t *testing.T) { fakeClaimWatch := watch.NewFake() client.PrependWatchReactor("persistentvolumeclaims", core.DefaultWatchReactor(fakeClaimWatch, nil)) client.PrependWatchReactor("storageclasses", core.DefaultWatchReactor(watch.NewFake(), nil)) + client.PrependWatchReactor("nodes", core.DefaultWatchReactor(watch.NewFake(), nil)) + client.PrependWatchReactor("pods", core.DefaultWatchReactor(watch.NewFake(), nil)) informers := informers.NewSharedInformerFactory(client, controller.NoResyncPeriodFunc()) ctrl, err := newTestController(client, informers, true) From 045ca75c0370030a43302ed510e28cedce548dc5 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Tue, 21 Dec 2021 15:36:44 +0100 Subject: [PATCH 2/3] Deflake PV metrics unit test Test 5-7 tries to delete a PVC at the very same time when it detects that the PV controller started processing the PVC. The controller then sometimes can't update the PVC and generate an event for it that the test expects. From PV controller logs (not shown in CI): > I1221 14:36:34.548160 104481 pv_controller.go:815] updating PersistentVolumeClaim[default/claim5-7] status: set phase Lost failed: cannot update claim claim5-7: claim not found Typical error in CI: > FAIL: TestControllerSync (83.22s) > framework_test.go:202: Event "Warning ClaimLost" not emitted Therefore wait for the PVC to be fully processed before deleting the PVC to avoid races. --- .../volume/persistentvolume/pv_controller_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pkg/controller/volume/persistentvolume/pv_controller_test.go b/pkg/controller/volume/persistentvolume/pv_controller_test.go index 0a03cc4dc5b..c12de0d9bb6 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller_test.go +++ b/pkg/controller/volume/persistentvolume/pv_controller_test.go @@ -225,6 +225,18 @@ func TestControllerSync(t *testing.T) { if err != nil { return err } + + // Wait for the PVC to get fully processed. This avoids races between PV controller and DeleteClaimEvent + // below. + err = wait.Poll(10*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { + obj := ctrl.claims.List()[0] + claim := obj.(*v1.PersistentVolumeClaim) + return claim.Status.Phase == v1.ClaimLost, nil + }) + if err != nil { + return err + } + // trying to remove the claim as well obj := ctrl.claims.List()[0] claim := obj.(*v1.PersistentVolumeClaim) From 0f9832d095f4a870f57eaac3c5201dae18d62820 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Tue, 21 Dec 2021 15:36:48 +0100 Subject: [PATCH 3/3] Fix PV name in unit test Test 5-5 should use PV with "5-5"i in the name. It makes log analysys much easier. --- pkg/controller/volume/persistentvolume/pv_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/volume/persistentvolume/pv_controller_test.go b/pkg/controller/volume/persistentvolume/pv_controller_test.go index c12de0d9bb6..8f69f3c3770 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller_test.go +++ b/pkg/controller/volume/persistentvolume/pv_controller_test.go @@ -140,7 +140,7 @@ func TestControllerSync(t *testing.T) { // delete the corresponding volume from apiserver, and report latency metric "5-5 - delete claim and delete volume report metric", volumesWithAnnotation(pvutil.AnnDynamicallyProvisioned, "gcr.io/vendor-csi", - newVolumeArray("volume5-6", "10Gi", "uid5-6", "claim5-6", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classExternal, pvutil.AnnBoundByController)), + newVolumeArray("volume5-5", "10Gi", "uid5-5", "claim5-5", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classExternal, pvutil.AnnBoundByController)), novolumes, claimWithAnnotation(pvutil.AnnStorageProvisioner, "gcr.io/vendor-csi", newClaimArray("claim5-5", "uid5-5", "1Gi", "volume5-5", v1.ClaimBound, &classExternal, pvutil.AnnBoundByController, pvutil.AnnBindCompleted)),