From daa181d92e85091b1e537d285139168d1ad5f10b Mon Sep 17 00:00:00 2001 From: Jonathan Dobson Date: Wed, 1 Jun 2022 14:22:57 -0600 Subject: [PATCH 1/2] kubelet: fix volume reconstruction for CSI ephemeral volumes This resolves a couple of issues for CSI volume reconstruction. 1. IsLikelyNotMountPoint is known not to work for bind mounts and was causing problems for subpaths and hostpath volumes. 2. Inline volumes were failing reconstruction due to calling GetVolumeName, which only works when there is a PV spec. --- .../volumemanager/reconciler/reconciler.go | 22 ++++++++++++------- .../operationexecutor/operation_executor.go | 6 ++--- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/pkg/kubelet/volumemanager/reconciler/reconciler.go b/pkg/kubelet/volumemanager/reconciler/reconciler.go index c9af55e62c2..348187738ec 100644 --- a/pkg/kubelet/volumemanager/reconciler/reconciler.go +++ b/pkg/kubelet/volumemanager/reconciler/reconciler.go @@ -454,14 +454,6 @@ func (rc *reconciler) reconstructVolume(volume podVolume) (*reconstructedVolume, if err != nil { return nil, err } - attachablePlugin, err := rc.volumePluginMgr.FindAttachablePluginByName(volume.pluginName) - if err != nil { - return nil, err - } - deviceMountablePlugin, err := rc.volumePluginMgr.FindDeviceMountablePluginByName(volume.pluginName) - if err != nil { - return nil, err - } // Create pod object pod := &v1.Pod{ @@ -490,6 +482,20 @@ func (rc *reconciler) reconstructVolume(volume podVolume) (*reconstructedVolume, return nil, err } + // We have to find the plugins by volume spec (NOT by plugin name) here + // in order to correctly reconstruct ephemeral volume types. + // Searching by spec checks whether the volume is actually attachable + // (i.e. has a PV) whereas searching by plugin name can only tell whether + // the plugin supports attachable volumes. + attachablePlugin, err := rc.volumePluginMgr.FindAttachablePluginBySpec(volumeSpec) + if err != nil { + return nil, err + } + deviceMountablePlugin, err := rc.volumePluginMgr.FindDeviceMountablePluginBySpec(volumeSpec) + if err != nil { + return nil, err + } + var uniqueVolumeName v1.UniqueVolumeName if attachablePlugin != nil || deviceMountablePlugin != nil { uniqueVolumeName, err = util.GetUniqueVolumeNameFromSpec(plugin, volumeSpec) diff --git a/pkg/volume/util/operationexecutor/operation_executor.go b/pkg/volume/util/operationexecutor/operation_executor.go index de301bd7014..3cbc4e61a57 100644 --- a/pkg/volume/util/operationexecutor/operation_executor.go +++ b/pkg/volume/util/operationexecutor/operation_executor.go @@ -1039,7 +1039,7 @@ func (oe *operationExecutor) ReconstructVolumeOperation( // Filesystem Volume case if volumeMode == v1.PersistentVolumeFilesystem { // Create volumeSpec from mount path - klog.V(5).Infof("Starting operationExecutor.ReconstructVolumepodName") + klog.V(5).Infof("Starting operationExecutor.ReconstructVolume for file volume on pod %q", podName) volumeSpec, err := plugin.ConstructVolumeSpec(volumeSpecName, volumePath) if err != nil { return nil, err @@ -1049,7 +1049,7 @@ func (oe *operationExecutor) ReconstructVolumeOperation( // Block Volume case // Create volumeSpec from mount path - klog.V(5).Infof("Starting operationExecutor.ReconstructVolume") + klog.V(5).Infof("Starting operationExecutor.ReconstructVolume for block volume on pod %q", podName) // volumePath contains volumeName on the path. In the case of block volume, {volumeName} is symbolic link // corresponding to raw block device. @@ -1085,7 +1085,7 @@ func (oe *operationExecutor) CheckVolumeExistenceOperation( if mounter == nil { return false, fmt.Errorf("mounter was not set for a filesystem volume") } - if isNotMount, mountCheckErr = mounter.IsLikelyNotMountPoint(mountPath); mountCheckErr != nil { + if isNotMount, mountCheckErr = mount.IsNotMountPoint(mounter, mountPath); mountCheckErr != nil { return false, fmt.Errorf("could not check whether the volume %q (spec.Name: %q) pod %q (UID: %q) is mounted with: %v", uniqueVolumeName, volumeName, From c8d3cc5c07bd289dc62ff3cbb50edf9768da57c5 Mon Sep 17 00:00:00 2001 From: Jonathan Dobson Date: Wed, 1 Jun 2022 14:23:10 -0600 Subject: [PATCH 2/2] e2e: restore volume lifecycle checks for csi-hostpath driver These tests were previously disabled to work around #79980 https://github.com/kubernetes/kubernetes/commit/f1e1f3a416b70bafadf961518c330ce3b1b5459 --- test/e2e/storage/drivers/csi.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/test/e2e/storage/drivers/csi.go b/test/e2e/storage/drivers/csi.go index 99c126e5d03..cc6c36a4d76 100644 --- a/test/e2e/storage/drivers/csi.go +++ b/test/e2e/storage/drivers/csi.go @@ -40,7 +40,6 @@ import ( "encoding/json" "errors" "fmt" - "regexp" "strconv" "strings" "sync" @@ -246,14 +245,6 @@ func (h *hostpathCSIDriver) PrepareTest(f *framework.Framework) (*storageframewo NodeName: node.Name, } - // Disable volume lifecycle checks due to issue #103651 for the one - // test that it breaks. - // TODO: enable this check once issue is resolved for csi-host-path driver - // (https://github.com/kubernetes/kubernetes/pull/104858). - if regexp.MustCompile("should unmount if pod is.*deleted while kubelet is down").MatchString(ginkgo.CurrentGinkgoTestDescription().FullTestText) { - o.DriverContainerArguments = append(o.DriverContainerArguments, "--check-volume-lifecycle=false") - } - cleanup, err := utils.CreateFromManifests(config.Framework, driverNamespace, func(item interface{}) error { if err := utils.PatchCSIDeployment(config.Framework, o, item); err != nil { return err