From 0c81a2f6b01aafd11e7658bf7469b2ec9d42dc91 Mon Sep 17 00:00:00 2001 From: andyzhangx Date: Tue, 3 Mar 2020 11:55:48 +0000 Subject: [PATCH 1/7] fix: ignore dir check in csi node stage/publish --- pkg/volume/csi/csi_attacher.go | 27 +++++---------------------- pkg/volume/csi/csi_mounter.go | 22 ---------------------- 2 files changed, 5 insertions(+), 44 deletions(-) diff --git a/pkg/volume/csi/csi_attacher.go b/pkg/volume/csi/csi_attacher.go index fc5c3a28bb8..c9e3b4b780e 100644 --- a/pkg/volume/csi/csi_attacher.go +++ b/pkg/volume/csi/csi_attacher.go @@ -228,23 +228,6 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo return errors.New(log("attacher.MountDevice failed, deviceMountPath is empty")) } - corruptedDir := false - mounted, err := isDirMounted(c.plugin, deviceMountPath) - if err != nil { - klog.Error(log("attacher.MountDevice failed while checking mount status for dir [%s]", deviceMountPath)) - if isCorruptedDir(deviceMountPath) { - corruptedDir = true // leave to CSI driver to handle corrupted mount - klog.Warning(log("attacher.MountDevice detected corrupted mount for dir [%s]", deviceMountPath)) - } else { - return err - } - } - - if mounted && !corruptedDir { - klog.V(4).Info(log("attacher.MountDevice skipping mount, dir already mounted [%s]", deviceMountPath)) - return nil - } - // Setup if spec == nil { return errors.New(log("attacher.MountDevice failed, spec is nil")) @@ -293,11 +276,11 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo // Store volume metadata for UnmountDevice. Keep it around even if the // driver does not support NodeStage, UnmountDevice still needs it. - if err = os.MkdirAll(deviceMountPath, 0750); err != nil && !corruptedDir { - return errors.New(log("attacher.MountDevice failed to create dir %#v: %v", deviceMountPath, err)) - } - klog.V(4).Info(log("created target path successfully [%s]", deviceMountPath)) dataDir := filepath.Dir(deviceMountPath) + if err = os.MkdirAll(dataDir, 0750); err != nil { + return errors.New(log("attacher.MountDevice failed to create dir %#v: %v", dataDir, err)) + } + klog.V(4).Info(log("created target parent path successfully [%s]", dataDir)) data := map[string]string{ volDataKey.volHandle: csiSource.VolumeHandle, volDataKey.driverName: csiSource.Driver, @@ -315,7 +298,7 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo if err != nil && volumetypes.IsOperationFinishedError(err) { // clean up metadata klog.Errorf(log("attacher.MountDevice failed: %v", err)) - if err := removeMountDir(c.plugin, deviceMountPath); err != nil { + if err := removeMountDir(c.plugin, dataDir); err != nil { klog.Error(log("attacher.MountDevice failed to remove mount dir after error [%s]: %v", deviceMountPath, err)) } } diff --git a/pkg/volume/csi/csi_mounter.go b/pkg/volume/csi/csi_mounter.go index b807326cf72..7626234ac83 100644 --- a/pkg/volume/csi/csi_mounter.go +++ b/pkg/volume/csi/csi_mounter.go @@ -106,22 +106,6 @@ func (c *csiMountMgr) SetUp(mounterArgs volume.MounterArgs) error { func (c *csiMountMgr) SetUpAt(dir string, mounterArgs volume.MounterArgs) error { klog.V(4).Infof(log("Mounter.SetUpAt(%s)", dir)) - corruptedDir := false - mounted, err := isDirMounted(c.plugin, dir) - if err != nil { - if isCorruptedDir(dir) { - corruptedDir = true // leave to CSI driver to handle corrupted mount - klog.Warning(log("mounter.SetUpAt detected corrupted mount for dir [%s]", dir)) - } else { - return errors.New(log("mounter.SetUpAt failed while checking mount status for dir [%s]: %v", dir, err)) - } - } - - if mounted && !corruptedDir { - klog.V(4).Info(log("mounter.SetUpAt skipping mount, dir already mounted [%s]", dir)) - return nil - } - csi, err := c.csiClientGetter.Get() if err != nil { return volumetypes.NewTransientOperationFailure(log("mounter.SetUpAt failed to get CSI client: %v", err)) @@ -217,12 +201,6 @@ func (c *csiMountMgr) SetUpAt(dir string, mounterArgs volume.MounterArgs) error return fmt.Errorf("volume source not found in volume.Spec") } - // create target_dir before call to NodePublish - if err := os.MkdirAll(dir, 0750); err != nil && !corruptedDir { - return errors.New(log("mounter.SetUpAt failed to create dir %#v: %v", dir, err)) - } - klog.V(4).Info(log("created target path successfully [%s]", dir)) - nodePublishSecrets = map[string]string{} if secretRef != nil { nodePublishSecrets, err = getCredentialsFromSecret(c.k8s, secretRef) From b3a27c44bf25104787eea4026b7a07ea9dbb7de9 Mon Sep 17 00:00:00 2001 From: andyzhangx Date: Wed, 4 Mar 2020 11:33:06 +0000 Subject: [PATCH 2/7] fix comments --- pkg/volume/csi/csi_attacher.go | 15 ++++++++++----- pkg/volume/csi/csi_mounter.go | 11 +++++++++++ 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/pkg/volume/csi/csi_attacher.go b/pkg/volume/csi/csi_attacher.go index c9e3b4b780e..9733b6de11b 100644 --- a/pkg/volume/csi/csi_attacher.go +++ b/pkg/volume/csi/csi_attacher.go @@ -276,11 +276,16 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo // Store volume metadata for UnmountDevice. Keep it around even if the // driver does not support NodeStage, UnmountDevice still needs it. - dataDir := filepath.Dir(deviceMountPath) - if err = os.MkdirAll(dataDir, 0750); err != nil { - return errors.New(log("attacher.MountDevice failed to create dir %#v: %v", dataDir, err)) + if err = os.MkdirAll(deviceMountPath, 0750); err != nil { + if isCorruptedDir(deviceMountPath) { + // leave to CSI driver to handle corrupted mount + klog.Warning(log("attacher.MountDevice detected corrupted mount for dir [%s]", deviceMountPath)) + } else { + return errors.New(log("attacher.MountDevice failed to create dir %#v: %v", deviceMountPath, err)) + } } - klog.V(4).Info(log("created target parent path successfully [%s]", dataDir)) + klog.V(4).Info(log("created target path successfully [%s]", deviceMountPath)) + dataDir := filepath.Dir(deviceMountPath) data := map[string]string{ volDataKey.volHandle: csiSource.VolumeHandle, volDataKey.driverName: csiSource.Driver, @@ -298,7 +303,7 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo if err != nil && volumetypes.IsOperationFinishedError(err) { // clean up metadata klog.Errorf(log("attacher.MountDevice failed: %v", err)) - if err := removeMountDir(c.plugin, dataDir); err != nil { + if err := removeMountDir(c.plugin, deviceMountPath); err != nil { klog.Error(log("attacher.MountDevice failed to remove mount dir after error [%s]: %v", deviceMountPath, err)) } } diff --git a/pkg/volume/csi/csi_mounter.go b/pkg/volume/csi/csi_mounter.go index 7626234ac83..fbcb16e75af 100644 --- a/pkg/volume/csi/csi_mounter.go +++ b/pkg/volume/csi/csi_mounter.go @@ -201,6 +201,17 @@ func (c *csiMountMgr) SetUpAt(dir string, mounterArgs volume.MounterArgs) error return fmt.Errorf("volume source not found in volume.Spec") } + // create target_dir before call to NodePublish + if err := os.MkdirAll(dir, 0750); err != nil { + if isCorruptedDir(dir) { + // leave to CSI driver to handle corrupted mount + klog.Warning(log("mounter.SetUpAt detected corrupted mount for dir [%s]", dir)) + } else { + return errors.New(log("mounter.SetUpAt failed to create dir %#v: %v", dir, err)) + } + } + klog.V(4).Info(log("created target path successfully [%s]", dir)) + nodePublishSecrets = map[string]string{} if secretRef != nil { nodePublishSecrets, err = getCredentialsFromSecret(c.k8s, secretRef) From 8d5c65b8cd6b02d18e665056a750e314d63b6143 Mon Sep 17 00:00:00 2001 From: andyzhangx Date: Tue, 21 Apr 2020 07:15:02 +0000 Subject: [PATCH 3/7] fix: comments(only create parent dir) --- pkg/volume/csi/csi_attacher.go | 14 +++++--------- pkg/volume/csi/csi_mounter.go | 12 ++++-------- 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/pkg/volume/csi/csi_attacher.go b/pkg/volume/csi/csi_attacher.go index 9733b6de11b..fe17e4b7ed6 100644 --- a/pkg/volume/csi/csi_attacher.go +++ b/pkg/volume/csi/csi_attacher.go @@ -276,16 +276,12 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo // Store volume metadata for UnmountDevice. Keep it around even if the // driver does not support NodeStage, UnmountDevice still needs it. - if err = os.MkdirAll(deviceMountPath, 0750); err != nil { - if isCorruptedDir(deviceMountPath) { - // leave to CSI driver to handle corrupted mount - klog.Warning(log("attacher.MountDevice detected corrupted mount for dir [%s]", deviceMountPath)) - } else { - return errors.New(log("attacher.MountDevice failed to create dir %#v: %v", deviceMountPath, err)) - } + parentDir := filepath.Dir(deviceMountPath) + if err = os.MkdirAll(parentDir, 0750); err != nil { + return errors.New(log("attacher.MountDevice failed to create dir %#v: %v", parentDir, err)) } - klog.V(4).Info(log("created target path successfully [%s]", deviceMountPath)) - dataDir := filepath.Dir(deviceMountPath) + klog.V(4).Info(log("created target path successfully [%s]", parentDir)) + dataDir := parentDir data := map[string]string{ volDataKey.volHandle: csiSource.VolumeHandle, volDataKey.driverName: csiSource.Driver, diff --git a/pkg/volume/csi/csi_mounter.go b/pkg/volume/csi/csi_mounter.go index fbcb16e75af..77560bf1a18 100644 --- a/pkg/volume/csi/csi_mounter.go +++ b/pkg/volume/csi/csi_mounter.go @@ -202,15 +202,11 @@ func (c *csiMountMgr) SetUpAt(dir string, mounterArgs volume.MounterArgs) error } // create target_dir before call to NodePublish - if err := os.MkdirAll(dir, 0750); err != nil { - if isCorruptedDir(dir) { - // leave to CSI driver to handle corrupted mount - klog.Warning(log("mounter.SetUpAt detected corrupted mount for dir [%s]", dir)) - } else { - return errors.New(log("mounter.SetUpAt failed to create dir %#v: %v", dir, err)) - } + parentDir := filepath.Dir(dir) + if err := os.MkdirAll(parentDir, 0750); err != nil { + return errors.New(log("mounter.SetUpAt failed to create dir %#v: %v", parentDir, err)) } - klog.V(4).Info(log("created target path successfully [%s]", dir)) + klog.V(4).Info(log("created target path successfully [%s]", parentDir)) nodePublishSecrets = map[string]string{} if secretRef != nil { From 55ffd9d5fca3b67c3a511a989dd816350a626b87 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Mon, 9 Nov 2020 19:08:20 +0100 Subject: [PATCH 4/7] Add unit test for staging path creation --- pkg/volume/csi/csi_attacher_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pkg/volume/csi/csi_attacher_test.go b/pkg/volume/csi/csi_attacher_test.go index 7cb15a929eb..3adf621c507 100644 --- a/pkg/volume/csi/csi_attacher_test.go +++ b/pkg/volume/csi/csi_attacher_test.go @@ -1236,6 +1236,18 @@ func TestAttacherMountDevice(t *testing.T) { t.Errorf("expected mount options: %v, got: %v", tc.spec.PersistentVolume.Spec.MountOptions, vol.MountFlags) } } + + // Verify the deviceMountPath was created by the plugin + if tc.stageUnstageSet { + s, err := os.Stat(tc.deviceMountPath) + if err != nil { + t.Errorf("expected staging directory %s to be created and be a directory, got error: %s", tc.deviceMountPath, err) + } else { + if !s.IsDir() { + t.Errorf("expected staging directory %s to be directory, got something else", tc.deviceMountPath) + } + } + } }) } } From 9a765f8c2d97a5180737a4a39e217b49180b67d6 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Fri, 6 Nov 2020 15:47:25 +0100 Subject: [PATCH 5/7] Restore staging path creation CSI says about NodeStage (=MountDevice): // The CO SHALL ensure [...] // that the path is directory and that the process serving the // request has `read` and `write` permission to that directory. The // CO SHALL be responsible for creating the directory if it does not // exist. --- pkg/volume/csi/csi_attacher.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/pkg/volume/csi/csi_attacher.go b/pkg/volume/csi/csi_attacher.go index fe17e4b7ed6..acfff89388f 100644 --- a/pkg/volume/csi/csi_attacher.go +++ b/pkg/volume/csi/csi_attacher.go @@ -276,12 +276,11 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo // Store volume metadata for UnmountDevice. Keep it around even if the // driver does not support NodeStage, UnmountDevice still needs it. - parentDir := filepath.Dir(deviceMountPath) - if err = os.MkdirAll(parentDir, 0750); err != nil { - return errors.New(log("attacher.MountDevice failed to create dir %#v: %v", parentDir, err)) + if err = os.MkdirAll(deviceMountPath, 0750); err != nil { + return errors.New(log("attacher.MountDevice failed to create dir %#v: %v", deviceMountPath, err)) } - klog.V(4).Info(log("created target path successfully [%s]", parentDir)) - dataDir := parentDir + klog.V(4).Info(log("created target path successfully [%s]", deviceMountPath)) + dataDir := filepath.Dir(deviceMountPath) data := map[string]string{ volDataKey.volHandle: csiSource.VolumeHandle, volDataKey.driverName: csiSource.Driver, From b3b53cbf63cb3bbb6561e64831b7563df17d8f94 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Fri, 6 Nov 2020 16:16:06 +0100 Subject: [PATCH 6/7] Mark MountDevice as uncertain after failed resize When NodeExpand fails between MountDevice and SetUp, make sure the next NodeExpand attempt is called before SetUp. --- .../util/operationexecutor/operation_generator.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index 5a98946c6f3..142cd5acfde 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -607,6 +607,17 @@ func (og *operationGenerator) GenerateMountVolumeFunc( if resizeError != nil { klog.Errorf("MountVolume.NodeExpandVolume failed with %v", resizeError) + + // Resize failed. To make sure NodeExpand is re-tried again on the next attempt + // *before* SetUp(), mark the mounted device as uncertain. + markDeviceUncertainErr := actualStateOfWorld.MarkDeviceAsUncertain( + volumeToMount.VolumeName, devicePath, deviceMountPath) + if markDeviceUncertainErr != nil { + // just log, return the resizeError error instead + klog.Infof(volumeToMount.GenerateMsgDetailed( + "MountVolume.MountDevice failed to mark volume as uncertain", + markDeviceUncertainErr.Error())) + } return volumeToMount.GenerateError("MountVolume.MountDevice failed while expanding volume", resizeError) } } From 6060c57ba3a944bd133968fd669980c97af846c6 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Fri, 6 Nov 2020 17:33:07 +0100 Subject: [PATCH 7/7] Call MountDevice only once Cann MountDevice only when the volume was not device-mounted before. --- pkg/volume/util/operationexecutor/operation_generator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index 142cd5acfde..50ee980b5e6 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -568,7 +568,7 @@ func (og *operationGenerator) GenerateMountVolumeFunc( DevicePath: devicePath, } - if volumeDeviceMounter != nil { + if volumeDeviceMounter != nil && actualStateOfWorld.GetDeviceMountState(volumeToMount.VolumeName) != DeviceGloballyMounted { deviceMountPath, err := volumeDeviceMounter.GetDeviceMountPath(volumeToMount.VolumeSpec) if err != nil {