From 7cb5df672813326952171be6c9ab52d821b85c1a Mon Sep 17 00:00:00 2001 From: Jing Xu Date: Mon, 18 Mar 2019 17:08:48 -0700 Subject: [PATCH] Fix race condition between actual and desired state in kublet volume manager This PR fixes the issue #75345. This fix modified the checking volume in actual state when validating whether volume can be removed from desired state or not. Only if volume status is already mounted in actual state, it can be removed from desired state. For the case of mounting fails always, it can still work because the check also validate whether pod still exist in pod manager. In case of mount fails, pod should be able to removed from pod manager so that volume can also be removed from desired state. --- .../desired_state_of_world_populator.go | 10 +- .../desired_state_of_world_populator_test.go | 113 ++++++++++++++++++ 2 files changed, 119 insertions(+), 4 deletions(-) diff --git a/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go b/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go index b28cd23985..f5aa5712fb 100644 --- a/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go +++ b/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go @@ -253,13 +253,15 @@ func (dswp *desiredStateOfWorldPopulator) findAndRemoveDeletedPods() { if runningContainers { klog.V(4).Infof( - "Pod %q has been removed from pod manager. However, it still has one or more containers in the non-exited state. Therefore, it will not be removed from volume manager.", + "Pod %q still has one or more containers in the non-exited state. Therefore, it will not be removed from desired state.", format.Pod(volumeToMount.Pod)) continue } - - if !dswp.actualStateOfWorld.VolumeExists(volumeToMount.VolumeName) && podExists { - klog.V(4).Infof(volumeToMount.GenerateMsgDetailed("Actual state has not yet has this information skip removing volume from desired state", "")) + exists, _, _ := dswp.actualStateOfWorld.PodExistsInVolume(volumeToMount.PodName, volumeToMount.VolumeName) + if !exists && podExists { + klog.V(4).Infof( + volumeToMount.GenerateMsgDetailed(fmt.Sprintf("Actual state has not yet has this volume mounted information and pod (%q) still exists in pod manager, skip removing volume from desired state", + format.Pod(volumeToMount.Pod)), "")) continue } klog.V(4).Infof(volumeToMount.GenerateMsgDetailed("Removing volume from desired state", "")) diff --git a/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator_test.go b/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator_test.go index d2e90f031f..ee3939cbc3 100644 --- a/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator_test.go +++ b/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator_test.go @@ -152,6 +152,119 @@ func TestFindAndAddNewPods_FindAndRemoveDeletedPods(t *testing.T) { } +func TestFindAndRemoveDeletedPodsWithActualState(t *testing.T) { + // create dswp + mode := v1.PersistentVolumeFilesystem + pv := &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dswp-test-volume-name", + }, + Spec: v1.PersistentVolumeSpec{ + ClaimRef: &v1.ObjectReference{Namespace: "ns", Name: "file-bound"}, + VolumeMode: &mode, + }, + } + pvc := &v1.PersistentVolumeClaim{ + Spec: v1.PersistentVolumeClaimSpec{ + VolumeName: "dswp-test-volume-name", + }, + Status: v1.PersistentVolumeClaimStatus{ + Phase: v1.ClaimBound, + }, + } + dswp, fakePodManager, fakesDSW := createDswpWithVolume(t, pv, pvc) + + // create pod + containers := []v1.Container{ + { + VolumeMounts: []v1.VolumeMount{ + { + Name: "dswp-test-volume-name", + MountPath: "/mnt", + }, + }, + }, + } + pod := createPodWithVolume("dswp-test-pod", "dswp-test-volume-name", "file-bound", containers) + + fakePodManager.AddPod(pod) + + podName := util.GetUniquePodName(pod) + + generatedVolumeName := "fake-plugin/" + pod.Spec.Volumes[0].Name + + dswp.findAndAddNewPods() + + if !dswp.pods.processedPods[podName] { + t.Fatalf("Failed to record that the volumes for the specified pod: %s have been processed by the populator", podName) + } + + expectedVolumeName := v1.UniqueVolumeName(generatedVolumeName) + + volumeExists := fakesDSW.VolumeExists(expectedVolumeName) + if !volumeExists { + t.Fatalf( + "VolumeExists(%q) failed. Expected: Actual: <%v>", + expectedVolumeName, + volumeExists) + } + + if podExistsInVolume := fakesDSW.PodExistsInVolume( + podName, expectedVolumeName); !podExistsInVolume { + t.Fatalf( + "DSW PodExistsInVolume returned incorrect value. Expected: Actual: <%v>", + podExistsInVolume) + } + + verifyVolumeExistsInVolumesToMount( + t, v1.UniqueVolumeName(generatedVolumeName), false /* expectReportedInUse */, fakesDSW) + + //let the pod be terminated + podGet, exist := fakePodManager.GetPodByName(pod.Namespace, pod.Name) + if !exist { + t.Fatalf("Failed to get pod by pod name: %s and namespace: %s", pod.Name, pod.Namespace) + } + podGet.Status.Phase = v1.PodFailed + + dswp.findAndRemoveDeletedPods() + // Although Pod status is terminated, pod still exists in pod manager and actual state does not has this pod and volume information + // desired state populator will fail to delete this pod and volume first + volumeExists = fakesDSW.VolumeExists(expectedVolumeName) + if !volumeExists { + t.Fatalf( + "VolumeExists(%q) failed. Expected: Actual: <%v>", + expectedVolumeName, + volumeExists) + } + + if podExistsInVolume := fakesDSW.PodExistsInVolume( + podName, expectedVolumeName); !podExistsInVolume { + t.Fatalf( + "DSW PodExistsInVolume returned incorrect value. Expected: Actual: <%v>", + podExistsInVolume) + } + + // reconcile with actual state so that volume is added into the actual state + // desired state populator now can successfully delete the pod and volume + fakeASW := dswp.actualStateOfWorld + reconcileASW(fakeASW, fakesDSW, t) + dswp.findAndRemoveDeletedPods() + volumeExists = fakesDSW.VolumeExists(expectedVolumeName) + if volumeExists { + t.Fatalf( + "VolumeExists(%q) failed. Expected: Actual: <%v>", + expectedVolumeName, + volumeExists) + } + + if podExistsInVolume := fakesDSW.PodExistsInVolume( + podName, expectedVolumeName); podExistsInVolume { + t.Fatalf( + "DSW PodExistsInVolume returned incorrect value. Expected: Actual: <%v>", + podExistsInVolume) + } +} + func TestFindAndAddNewPods_FindAndRemoveDeletedPods_Valid_Block_VolumeDevices(t *testing.T) { // Enable BlockVolume feature gate defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, true)()