Merge pull request #30661 from saad-ali/safeDeviceUnmount

Automatic merge from submit-queue

Prevent device unmount from deleting dir on failed unmount

This PR cleans up the device unmount code for attachable volumes. Specifically it:
* Prevents deletion of directory via `os.Remove` unless unmount succeeds.
* Moves common shared device unmount logic to a common util file.

pkg/volume/aws_ebs/attacher.go

@@ -28,6 +28,7 @@ import (
 	"k8s.io/kubernetes/pkg/util/exec"
 	"k8s.io/kubernetes/pkg/util/mount"
 	"k8s.io/kubernetes/pkg/volume"
+	volumeutil "k8s.io/kubernetes/pkg/volume/util"
 )
 
 type awsElasticBlockStoreAttacher struct {
@@ -218,7 +219,7 @@ func (detacher *awsElasticBlockStoreDetacher) WaitForDetach(devicePath string, t
 		select {
 		case <-ticker.C:
 			glog.V(5).Infof("Checking device %q is detached.", devicePath)
-			if pathExists, err := pathExists(devicePath); err != nil {
+			if pathExists, err := volumeutil.PathExists(devicePath); err != nil {
 				return fmt.Errorf("Error checking if device path exists: %v", err)
 			} else if !pathExists {
 				return nil
@@ -230,10 +231,5 @@ func (detacher *awsElasticBlockStoreDetacher) WaitForDetach(devicePath string, t
 }
 
 func (detacher *awsElasticBlockStoreDetacher) UnmountDevice(deviceMountPath string) error {
-	volume := path.Base(deviceMountPath)
+	return volumeutil.UnmountPath(deviceMountPath, detacher.mounter)
-	if err := unmountPDAndRemoveGlobalPath(deviceMountPath, detacher.mounter); err != nil {
-		glog.Errorf("Error unmounting %q: %v", volume, err)
-	}
-
-	return nil
 }

pkg/volume/aws_ebs/aws_util.go

@@ -18,14 +18,13 @@ package aws_ebs
 
 import (
 	"fmt"
-	"os"
 	"time"
 
 	"github.com/golang/glog"
 	"k8s.io/kubernetes/pkg/cloudprovider"
 	"k8s.io/kubernetes/pkg/cloudprovider/providers/aws"
-	"k8s.io/kubernetes/pkg/util/mount"
 	"k8s.io/kubernetes/pkg/volume"
+	volumeutil "k8s.io/kubernetes/pkg/volume/util"
 )
 
 const (
@@ -104,7 +103,7 @@ func (util *AWSDiskUtil) CreateVolume(c *awsElasticBlockStoreProvisioner) (strin
 // Returns the first path that exists, or empty string if none exist.
 func verifyDevicePath(devicePaths []string) (string, error) {
 	for _, path := range devicePaths {
-		if pathExists, err := pathExists(path); err != nil {
+		if pathExists, err := volumeutil.PathExists(path); err != nil {
 			return "", fmt.Errorf("Error checking if path exists: %v", err)
 		} else if pathExists {
 			return path, nil
@@ -114,24 +113,11 @@ func verifyDevicePath(devicePaths []string) (string, error) {
 	return "", nil
 }
 
-// Unmount the global mount path, which should be the only one, and delete it.
-func unmountPDAndRemoveGlobalPath(globalMountPath string, mounter mount.Interface) error {
-	if pathExists, pathErr := pathExists(globalMountPath); pathErr != nil {
-		return fmt.Errorf("Error checking if path exists: %v", pathErr)
-	} else if !pathExists {
-		glog.V(5).Infof("Warning: Unmount skipped because path does not exist: %v", globalMountPath)
-		return nil
-	}
-	err := mounter.Unmount(globalMountPath)
-	os.Remove(globalMountPath)
-	return err
-}
-
 // Returns the first path that exists, or empty string if none exist.
 func verifyAllPathsRemoved(devicePaths []string) (bool, error) {
 	allPathsRemoved := true
 	for _, path := range devicePaths {
-		if exists, err := pathExists(path); err != nil {
+		if exists, err := volumeutil.PathExists(path); err != nil {
 			return false, fmt.Errorf("Error checking if path exists: %v", err)
 		} else {
 			allPathsRemoved = allPathsRemoved && !exists
@@ -159,18 +145,6 @@ func getDiskByIdPaths(partition string, devicePath string) []string {
 	return devicePaths
 }
 
-// Checks if the specified path exists
-func pathExists(path string) (bool, error) {
-	_, err := os.Stat(path)
-	if err == nil {
-		return true, nil
-	} else if os.IsNotExist(err) {
-		return false, nil
-	} else {
-		return false, err
-	}
-}
-
 // Return cloud provider
 func getCloudProvider(cloudProvider cloudprovider.Interface) (*aws.Cloud, error) {
 	awsCloudProvider, ok := cloudProvider.(*aws.Cloud)

pkg/volume/cinder/attacher.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/kubernetes/pkg/util/exec"
 	"k8s.io/kubernetes/pkg/util/mount"
 	"k8s.io/kubernetes/pkg/volume"
+	volumeutil "k8s.io/kubernetes/pkg/volume/util"
 )
 
 type cinderDiskAttacher struct {
@@ -130,7 +131,7 @@ func (attacher *cinderDiskAttacher) WaitForAttach(spec *volume.Spec, devicePath
 		case <-ticker.C:
 			glog.V(5).Infof("Checking Cinder disk %q is attached.", volumeID)
 			probeAttachedVolume()
-			exists, err := pathExists(devicePath)
+			exists, err := volumeutil.PathExists(devicePath)
 			if exists && err == nil {
 				glog.Infof("Successfully found attached Cinder disk %q.", volumeID)
 				return devicePath, nil
@@ -250,7 +251,7 @@ func (detacher *cinderDiskDetacher) WaitForDetach(devicePath string, timeout tim
 		select {
 		case <-ticker.C:
 			glog.V(5).Infof("Checking device %q is detached.", devicePath)
-			if pathExists, err := pathExists(devicePath); err != nil {
+			if pathExists, err := volumeutil.PathExists(devicePath); err != nil {
 				return fmt.Errorf("Error checking if device path exists: %v", err)
 			} else if !pathExists {
 				return nil
@@ -262,35 +263,5 @@ func (detacher *cinderDiskDetacher) WaitForDetach(devicePath string, timeout tim
 }
 
 func (detacher *cinderDiskDetacher) UnmountDevice(deviceMountPath string) error {
-	volume := path.Base(deviceMountPath)
+	return volumeutil.UnmountPath(deviceMountPath, detacher.mounter)
-	if err := unmountPDAndRemoveGlobalPath(deviceMountPath, detacher.mounter); err != nil {
-		glog.Errorf("Error unmounting %q: %v", volume, err)
-	}
-
-	return nil
-}
-
-// Checks if the specified path exists
-func pathExists(path string) (bool, error) {
-	_, err := os.Stat(path)
-	if err == nil {
-		return true, nil
-	} else if os.IsNotExist(err) {
-		return false, nil
-	} else {
-		return false, err
-	}
-}
-
-// Unmount the global mount path, which should be the only one, and delete it.
-func unmountPDAndRemoveGlobalPath(globalMountPath string, mounter mount.Interface) error {
-	if pathExists, pathErr := pathExists(globalMountPath); pathErr != nil {
-		return fmt.Errorf("Error checking if path exists: %v", pathErr)
-	} else if !pathExists {
-		glog.V(5).Infof("Warning: Unmount skipped because path does not exist: %v", globalMountPath)
-		return nil
-	}
-	err := mounter.Unmount(globalMountPath)
-	os.Remove(globalMountPath)
-	return err
 }

pkg/volume/gce_pd/attacher.go

@@ -30,6 +30,7 @@ import (
 	"k8s.io/kubernetes/pkg/util/mount"
 	"k8s.io/kubernetes/pkg/util/sets"
 	"k8s.io/kubernetes/pkg/volume"
+	volumeutil "k8s.io/kubernetes/pkg/volume/util"
 )
 
 type gcePersistentDiskAttacher struct {
@@ -244,7 +245,7 @@ func (detacher *gcePersistentDiskDetacher) WaitForDetach(devicePath string, time
 		select {
 		case <-ticker.C:
 			glog.V(5).Infof("Checking device %q is detached.", devicePath)
-			if pathExists, err := pathExists(devicePath); err != nil {
+			if pathExists, err := volumeutil.PathExists(devicePath); err != nil {
 				return fmt.Errorf("Error checking if device path exists: %v", err)
 			} else if !pathExists {
 				return nil
@@ -256,5 +257,5 @@ func (detacher *gcePersistentDiskDetacher) WaitForDetach(devicePath string, time
 }
 
 func (detacher *gcePersistentDiskDetacher) UnmountDevice(deviceMountPath string) error {
-	return unmountPDAndRemoveGlobalPath(deviceMountPath, detacher.host.GetMounter())
+	return volumeutil.UnmountPath(deviceMountPath, detacher.host.GetMounter())
 }

pkg/volume/gce_pd/gce_util.go

@@ -18,7 +18,6 @@ package gce_pd
 
 import (
 	"fmt"
-	"os"
 	"path"
 	"path/filepath"
 	"strings"
@@ -28,9 +27,9 @@ import (
 	"k8s.io/kubernetes/pkg/cloudprovider"
 	gcecloud "k8s.io/kubernetes/pkg/cloudprovider/providers/gce"
 	"k8s.io/kubernetes/pkg/util/exec"
-	"k8s.io/kubernetes/pkg/util/mount"
 	"k8s.io/kubernetes/pkg/util/sets"
 	"k8s.io/kubernetes/pkg/volume"
+	volumeutil "k8s.io/kubernetes/pkg/volume/util"
 )
 
 const (
@@ -114,7 +113,7 @@ func verifyDevicePath(devicePaths []string, sdBeforeSet sets.String) (string, er
 	}
 
 	for _, path := range devicePaths {
-		if pathExists, err := pathExists(path); err != nil {
+		if pathExists, err := volumeutil.PathExists(path); err != nil {
 			return "", fmt.Errorf("Error checking if path exists: %v", err)
 		} else if pathExists {
 			return path, nil
@@ -124,20 +123,6 @@ func verifyDevicePath(devicePaths []string, sdBeforeSet sets.String) (string, er
 	return "", nil
 }
 
-// Unmount the global PD mount, which should be the only one, and delete it.
-// Does nothing if globalMountPath does not exist.
-func unmountPDAndRemoveGlobalPath(globalMountPath string, mounter mount.Interface) error {
-	if pathExists, pathErr := pathExists(globalMountPath); pathErr != nil {
-		return fmt.Errorf("Error checking if path exists: %v", pathErr)
-	} else if !pathExists {
-		glog.V(5).Infof("Warning: Unmount skipped because path does not exist: %v", globalMountPath)
-		return nil
-	}
-	err := mounter.Unmount(globalMountPath)
-	os.Remove(globalMountPath)
-	return err
-}
-
 // Returns the first path that exists, or empty string if none exist.
 func verifyAllPathsRemoved(devicePaths []string) (bool, error) {
 	allPathsRemoved := true
@@ -146,7 +131,7 @@ func verifyAllPathsRemoved(devicePaths []string) (bool, error) {
 			// udevadm errors should not block disk detachment, log and continue
 			glog.Errorf("%v", err)
 		}
-		if exists, err := pathExists(path); err != nil {
+		if exists, err := volumeutil.PathExists(path); err != nil {
 			return false, fmt.Errorf("Error checking if path exists: %v", err)
 		} else {
 			allPathsRemoved = allPathsRemoved && !exists
@@ -172,18 +157,6 @@ func getDiskByIdPaths(pdName string, partition string) []string {
 	return devicePaths
 }
 
-// Checks if the specified path exists
-func pathExists(path string) (bool, error) {
-	_, err := os.Stat(path)
-	if err == nil {
-		return true, nil
-	} else if os.IsNotExist(err) {
-		return false, nil
-	} else {
-		return false, err
-	}
-}
-
 // Return cloud provider
 func getCloudProvider(cloudProvider cloudprovider.Interface) (*gcecloud.GCECloud, error) {
 	var err error

pkg/volume/util/util.go

@@ -17,10 +17,12 @@ limitations under the License.
 package util
 
 import (
+	"fmt"
 	"os"
 	"path"
 
 	"github.com/golang/glog"
+	"k8s.io/kubernetes/pkg/util/mount"
 )
 
 const readyFileName = "ready"
@@ -60,3 +62,35 @@ func SetReady(dir string) {
 	}
 	file.Close()
 }
+
+// UnmountPath is a common unmount routine that unmounts the given path and
+// deletes the remaining directory if successful.
+func UnmountPath(mountPath string, mounter mount.Interface) error {
+	if pathExists, pathErr := PathExists(mountPath); 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", mountPath)
+		return nil
+	}
+
+	err := mounter.Unmount(mountPath)
+	if err == nil {
+		// Only delete directory on successful unmount
+		glog.V(5).Infof("Unmounted %q. Deleting path.", mountPath)
+		return os.Remove(mountPath)
+	}
+
+	return err
+}
+
+// PathExists returns true if the specified path exists.
+func PathExists(path string) (bool, error) {
+	_, err := os.Stat(path)
+	if err == nil {
+		return true, nil
+	} else if os.IsNotExist(err) {
+		return false, nil
+	} else {
+		return false, err
+	}
+}