From 629718ef0b138dc2e340ae7f07a661bbccb6a2fd Mon Sep 17 00:00:00 2001 From: mtanino Date: Mon, 18 Sep 2017 19:29:32 -0400 Subject: [PATCH] Refactoring and improvements for iSCSI and FC This PR makes following changes. - Simplify volume tearDown path for iSCSI and FC using util.UnmountPath(). - Log lastErr during iscsi connection If iscsid fails to connect second portal, currently the error is ignored silently. The lastErr should be logged to find the root cause of problem. - Remove iscsi plugin directory after iscsi connection is successfully closed. --- pkg/volume/fc/disk_manager.go | 30 ----------------------------- pkg/volume/fc/fc.go | 8 +------- pkg/volume/iscsi/attacher.go | 5 +++++ pkg/volume/iscsi/disk_manager.go | 33 -------------------------------- pkg/volume/iscsi/iscsi.go | 8 +------- pkg/volume/iscsi/iscsi_util.go | 3 +++ 6 files changed, 10 insertions(+), 77 deletions(-) diff --git a/pkg/volume/fc/disk_manager.go b/pkg/volume/fc/disk_manager.go index 33a0c422cff..8562d219b1a 100644 --- a/pkg/volume/fc/disk_manager.go +++ b/pkg/volume/fc/disk_manager.go @@ -89,33 +89,3 @@ func diskSetUp(manager diskManager, b fcDiskMounter, volPath string, mounter mou return nil } - -// utility to tear down a disk based filesystem -func diskTearDown(manager diskManager, c fcDiskUnmounter, volPath string, mounter mount.Interface) error { - noMnt, err := mounter.IsLikelyNotMountPoint(volPath) - if err != nil { - glog.Errorf("cannot validate mountpoint %s", volPath) - return err - } - if noMnt { - return os.Remove(volPath) - } - - if err := mounter.Unmount(volPath); err != nil { - glog.Errorf("failed to unmount %s", volPath) - return err - } - - noMnt, mntErr := mounter.IsLikelyNotMountPoint(volPath) - if mntErr != nil { - glog.Errorf("isMountpoint check failed: %v", mntErr) - return err - } - if noMnt { - if err := os.Remove(volPath); err != nil { - return err - } - } - return nil - -} diff --git a/pkg/volume/fc/fc.go b/pkg/volume/fc/fc.go index 096a24df387..01d1c7c33b3 100644 --- a/pkg/volume/fc/fc.go +++ b/pkg/volume/fc/fc.go @@ -240,13 +240,7 @@ func (c *fcDiskUnmounter) TearDown() error { } func (c *fcDiskUnmounter) TearDownAt(dir string) error { - if pathExists, pathErr := util.PathExists(dir); pathErr != nil { - return fmt.Errorf("Error checking if path exists: %v", pathErr) - } else if !pathExists { - glog.Warningf("Warning: Unmount skipped because path does not exist: %v", dir) - return nil - } - return diskTearDown(c.manager, *c, dir, c.mounter) + return util.UnmountPath(dir, c.mounter) } func getVolumeSource(spec *volume.Spec) (*v1.FCVolumeSource, bool, error) { diff --git a/pkg/volume/iscsi/attacher.go b/pkg/volume/iscsi/attacher.go index 4ff499cc408..4cea774eaa4 100644 --- a/pkg/volume/iscsi/attacher.go +++ b/pkg/volume/iscsi/attacher.go @@ -147,6 +147,11 @@ func (detacher *iscsiDetacher) UnmountDevice(deviceMountPath string) error { if err != nil { return fmt.Errorf("iscsi: failed to detach disk: %s\nError: %v", deviceMountPath, err) } + glog.V(4).Infof("iscsi: %q is unmounted, deleting the directory", deviceMountPath) + err = os.RemoveAll(deviceMountPath) + if err != nil { + return fmt.Errorf("iscsi: failed to delete the directory: %s\nError: %v", deviceMountPath, err) + } glog.V(4).Infof("iscsi: successfully detached disk: %s", deviceMountPath) return nil } diff --git a/pkg/volume/iscsi/disk_manager.go b/pkg/volume/iscsi/disk_manager.go index 4741aa09f98..ea00c7ebcbe 100644 --- a/pkg/volume/iscsi/disk_manager.go +++ b/pkg/volume/iscsi/disk_manager.go @@ -94,36 +94,3 @@ func diskSetUp(manager diskManager, b iscsiDiskMounter, volPath string, mounter return nil } - -// utility to tear down a disk based filesystem -func diskTearDown(manager diskManager, c iscsiDiskUnmounter, volPath string, mounter mount.Interface) error { - notMnt, err := mounter.IsLikelyNotMountPoint(volPath) - if err != nil { - glog.Errorf("cannot validate mountpoint %s", volPath) - return err - } - if notMnt { - return os.Remove(volPath) - } - _, err = mount.GetMountRefs(mounter, volPath) - if err != nil { - glog.Errorf("failed to get reference count %s", volPath) - return err - } - if err := mounter.Unmount(volPath); err != nil { - glog.Errorf("failed to unmount %s", volPath) - return err - } - notMnt, mntErr := mounter.IsLikelyNotMountPoint(volPath) - if mntErr != nil { - glog.Errorf("IsLikelyNotMountPoint check failed: %v", mntErr) - return err - } - if notMnt { - if err := os.Remove(volPath); err != nil { - return err - } - } - return nil - -} diff --git a/pkg/volume/iscsi/iscsi.go b/pkg/volume/iscsi/iscsi.go index 6c702566928..055fbf8a9c1 100644 --- a/pkg/volume/iscsi/iscsi.go +++ b/pkg/volume/iscsi/iscsi.go @@ -267,13 +267,7 @@ func (c *iscsiDiskUnmounter) TearDown() error { } func (c *iscsiDiskUnmounter) TearDownAt(dir string) error { - if pathExists, pathErr := ioutil.PathExists(dir); pathErr != nil { - return fmt.Errorf("Error checking if path exists: %v", pathErr) - } else if !pathExists { - glog.Warningf("Warning: Unmount skipped because path does not exist: %v", dir) - return nil - } - return diskTearDown(c.manager, *c, dir, c.mounter) + return ioutil.UnmountPath(dir, c.mounter) } func portalMounter(portal string) string { diff --git a/pkg/volume/iscsi/iscsi_util.go b/pkg/volume/iscsi/iscsi_util.go index 471e2bb035f..46f9d8fe313 100644 --- a/pkg/volume/iscsi/iscsi_util.go +++ b/pkg/volume/iscsi/iscsi_util.go @@ -293,6 +293,9 @@ func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) (string, error) { glog.Errorf("iscsi: failed to get any path for iscsi disk, last err seen:\n%v", lastErr) return "", fmt.Errorf("failed to get any path for iscsi disk, last err seen:\n%v", lastErr) } + if lastErr != nil { + glog.Errorf("iscsi: last error occurred during iscsi init:\n%v", lastErr) + } //Make sure we use a valid devicepath to find mpio device. devicePath = devicePaths[0]