From 2ecdc5e8d19ee7a037b86e90a4d3de56cb94b938 Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Fri, 1 Nov 2019 19:31:17 +0000 Subject: [PATCH 01/11] Change globalMapPath to bind mount from symlink This change is needed to make descriptor lock per pod, in the next commit. If losetup is called for symlink, path in the output for losetup is resolved, as a result, we can't distinguish which path the lock is taken. --- pkg/volume/testing/testing.go | 11 +- .../operationexecutor/operation_generator.go | 8 +- pkg/volume/util/util.go | 12 +- pkg/volume/util/volumepathhandler/BUILD | 1 + .../volumepathhandler/volume_path_handler.go | 187 +++++++++++------- .../volume_path_handler_linux.go | 103 +++++++++- .../volume_path_handler_unsupported.go | 8 + 7 files changed, 246 insertions(+), 84 deletions(-) diff --git a/pkg/volume/testing/testing.go b/pkg/volume/testing/testing.go index b34d977da0c..cc57be3184d 100644 --- a/pkg/volume/testing/testing.go +++ b/pkg/volume/testing/testing.go @@ -1138,12 +1138,12 @@ type FakeVolumePathHandler struct { sync.RWMutex } -func (fv *FakeVolumePathHandler) MapDevice(devicePath string, mapDir string, linkName string) error { +func (fv *FakeVolumePathHandler) MapDevice(devicePath string, mapDir string, linkName string, bindMount bool) error { // nil is success, else error return nil } -func (fv *FakeVolumePathHandler) UnmapDevice(mapDir string, linkName string) error { +func (fv *FakeVolumePathHandler) UnmapDevice(mapDir string, linkName string, bindMount bool) error { // nil is success, else error return nil } @@ -1158,7 +1158,12 @@ func (fv *FakeVolumePathHandler) IsSymlinkExist(mapPath string) (bool, error) { return true, nil } -func (fv *FakeVolumePathHandler) GetDeviceSymlinkRefs(devPath string, mapPath string) ([]string, error) { +func (fv *FakeVolumePathHandler) IsBindMountExist(mapPath string) (bool, error) { + // nil is success, else error + return true, nil +} + +func (fv *FakeVolumePathHandler) GetDeviceBindMountRefs(devPath string, mapPath string) ([]string, error) { // nil is success, else error return []string{}, nil } diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index 54f81649bbe..9d69b683161 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -1210,7 +1210,7 @@ func (og *operationGenerator) GenerateUnmapVolumeFunc( // Try to unmap volumeName symlink under pod device map path dir // pods/{podUid}/volumeDevices/{escapeQualifiedPluginName}/{volumeName} podDeviceUnmapPath, volName := blockVolumeUnmapper.GetPodDeviceMapPath() - unmapDeviceErr := og.blkUtil.UnmapDevice(podDeviceUnmapPath, volName) + unmapDeviceErr := og.blkUtil.UnmapDevice(podDeviceUnmapPath, volName, false /* bindMount */) if unmapDeviceErr != nil { // On failure, return error. Caller will log and retry. return volumeToUnmount.GenerateError("UnmapVolume.UnmapDevice on pod device map path failed", unmapDeviceErr) @@ -1218,7 +1218,7 @@ func (og *operationGenerator) GenerateUnmapVolumeFunc( // Try to unmap podUID symlink under global map path dir // plugins/kubernetes.io/{PluginName}/volumeDevices/{volumePluginDependentPath}/{podUID} globalUnmapPath := volumeToUnmount.DeviceMountPath - unmapDeviceErr = og.blkUtil.UnmapDevice(globalUnmapPath, string(volumeToUnmount.PodUID)) + unmapDeviceErr = og.blkUtil.UnmapDevice(globalUnmapPath, string(volumeToUnmount.PodUID), true /* bindMount */) if unmapDeviceErr != nil { // On failure, return error. Caller will log and retry. return volumeToUnmount.GenerateError("UnmapVolume.UnmapDevice on global map path failed", unmapDeviceErr) @@ -1306,9 +1306,9 @@ func (og *operationGenerator) GenerateUnmapDeviceFunc( // Search under globalMapPath dir if all symbolic links from pods have been removed already. // If symbolic links are there, pods may still refer the volume. globalMapPath := deviceToDetach.DeviceMountPath - refs, err := og.blkUtil.GetDeviceSymlinkRefs(deviceToDetach.DevicePath, globalMapPath) + refs, err := og.blkUtil.GetDeviceBindMountRefs(deviceToDetach.DevicePath, globalMapPath) if err != nil { - return deviceToDetach.GenerateError("UnmapDevice.GetDeviceSymlinkRefs check failed", err) + return deviceToDetach.GenerateError("UnmapDevice.GetDeviceBindMountRefs check failed", err) } if len(refs) > 0 { err = fmt.Errorf("The device %q is still referenced from other Pods %v", globalMapPath, refs) diff --git a/pkg/volume/util/util.go b/pkg/volume/util/util.go index 42fea454baf..f097edaa58f 100644 --- a/pkg/volume/util/util.go +++ b/pkg/volume/util/util.go @@ -515,16 +515,18 @@ func MapBlockVolume( ) error { blkUtil := volumepathhandler.NewBlockVolumePathHandler() - // map devicePath to global node path - mapErr := blkUtil.MapDevice(devicePath, globalMapPath, string(podUID)) + // map devicePath to global node path as bind mount + mapErr := blkUtil.MapDevice(devicePath, globalMapPath, string(podUID), true /* bindMount */) if mapErr != nil { - return mapErr + return fmt.Errorf("blkUtil.MapDevice failed. devicePath: %s, globalMapPath:%s, podUID: %s, bindMount: %v: %v", + devicePath, globalMapPath, string(podUID), true, mapErr) } // map devicePath to pod volume path - mapErr = blkUtil.MapDevice(devicePath, podVolumeMapPath, volumeMapName) + mapErr = blkUtil.MapDevice(devicePath, podVolumeMapPath, volumeMapName, false /* bindMount */) if mapErr != nil { - return mapErr + return fmt.Errorf("blkUtil.MapDevice failed. devicePath: %s, podVolumeMapPath:%s, volumeMapName: %s, bindMount: %v: %v", + devicePath, podVolumeMapPath, volumeMapName, false, mapErr) } return nil diff --git a/pkg/volume/util/volumepathhandler/BUILD b/pkg/volume/util/volumepathhandler/BUILD index e344849a517..d1d562a8108 100644 --- a/pkg/volume/util/volumepathhandler/BUILD +++ b/pkg/volume/util/volumepathhandler/BUILD @@ -10,6 +10,7 @@ go_library( importpath = "k8s.io/kubernetes/pkg/volume/util/volumepathhandler", visibility = ["//visibility:public"], deps = [ + "//pkg/util/mount:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//vendor/k8s.io/klog:go_default_library", ], diff --git a/pkg/volume/util/volumepathhandler/volume_path_handler.go b/pkg/volume/util/volumepathhandler/volume_path_handler.go index a05db3e25b5..d245e558925 100644 --- a/pkg/volume/util/volumepathhandler/volume_path_handler.go +++ b/pkg/volume/util/volumepathhandler/volume_path_handler.go @@ -23,12 +23,14 @@ import ( "path/filepath" "k8s.io/klog" + "k8s.io/kubernetes/pkg/util/mount" "k8s.io/apimachinery/pkg/types" ) const ( losetupPath = "losetup" + statPath = "stat" ErrDeviceNotFound = "device not found" ErrDeviceNotSupported = "device not supported" ) @@ -36,15 +38,17 @@ const ( // BlockVolumePathHandler defines a set of operations for handling block volume-related operations type BlockVolumePathHandler interface { // MapDevice creates a symbolic link to block device under specified map path - MapDevice(devicePath string, mapPath string, linkName string) error + MapDevice(devicePath string, mapPath string, linkName string, bindMount bool) error // UnmapDevice removes a symbolic link to block device under specified map path - UnmapDevice(mapPath string, linkName string) error + UnmapDevice(mapPath string, linkName string, bindMount bool) error // RemovePath removes a file or directory on specified map path RemoveMapPath(mapPath string) error // IsSymlinkExist retruns true if specified symbolic link exists IsSymlinkExist(mapPath string) (bool, error) - // GetDeviceSymlinkRefs searches symbolic links under global map path - GetDeviceSymlinkRefs(devPath string, mapPath string) ([]string, error) + // IsBindMountExist retruns true if specified bind mount exists + IsBindMountExist(mapPath string) (bool, error) + // GetDeviceBindMountRefs searches bind mounts under global map path + GetDeviceBindMountRefs(devPath string, mapPath string) ([]string, error) // FindGlobalMapPathUUIDFromPod finds {pod uuid} symbolic link under globalMapPath // corresponding to map path symlink, and then return global map path with pod uuid. FindGlobalMapPathUUIDFromPod(pluginDir, mapPath string, podUID types.UID) (string, error) @@ -68,7 +72,7 @@ type VolumePathHandler struct { } // MapDevice creates a symbolic link to block device under specified map path -func (v VolumePathHandler) MapDevice(devicePath string, mapPath string, linkName string) error { +func (v VolumePathHandler) MapDevice(devicePath string, mapPath string, linkName string, bindMount bool) error { // Example of global map path: // globalMapPath/linkName: plugins/kubernetes.io/{PluginName}/{DefaultKubeletVolumeDevicesDirName}/{volumePluginDependentPath}/{podUid} // linkName: {podUid} @@ -98,25 +102,105 @@ func (v VolumePathHandler) MapDevice(devicePath string, mapPath string, linkName if err = os.MkdirAll(mapPath, 0750); err != nil { return fmt.Errorf("Failed to mkdir %s, error %v", mapPath, err) } + + if bindMount { + return mapBindMountDevice(v, devicePath, mapPath, linkName) + } + return mapSymlinkDevice(v, devicePath, mapPath, linkName) +} + +func mapBindMountDevice(v VolumePathHandler, devicePath string, mapPath string, linkName string) error { + // Check bind mount exists + linkPath := filepath.Join(mapPath, string(linkName)) + + file, err := os.Stat(linkPath) + if err != nil { + if !os.IsNotExist(err) { + return fmt.Errorf("Failed to stat file %s: %v", linkPath, err) + } + + klog.Warningf("Warning: Path to bind mount %v has not yet been created", linkPath) + // Create file + newFile, err := os.OpenFile(linkPath, os.O_CREATE|os.O_RDWR, 0750) + if err != nil { + return err + } + if err := newFile.Close(); err != nil { + return err + } + } else { + // Check if device file + // TODO: Need to check if this device file is actually the expected bind mount + if file.Mode()&os.ModeDevice == os.ModeDevice { + klog.Warningf("Warning: Map skipped because bind mount already exist on the path: %v", linkPath) + return nil + } + + klog.Warningf("Warning: file %s is already exist but not mounted, skip creating file", linkPath) + } + + // Bind mount file + mounter := &mount.SafeFormatAndMount{Interface: mount.New(""), Exec: mount.NewOSExec()} + if err := mounter.Mount(devicePath, linkPath, "" /* fsType */, []string{"bind"}); err != nil { + klog.Errorf("Failed to bind mount devicePath: %s to linkPath %s: %v", devicePath, linkPath, err) + return err + } + + return nil +} + +func mapSymlinkDevice(v VolumePathHandler, devicePath string, mapPath string, linkName string) error { // Remove old symbolic link(or file) then create new one. // This should be done because current symbolic link is // stale across node reboot. linkPath := filepath.Join(mapPath, string(linkName)) - if err = os.Remove(linkPath); err != nil && !os.IsNotExist(err) { + if err := os.Remove(linkPath); err != nil && !os.IsNotExist(err) { return err } - err = os.Symlink(devicePath, linkPath) - return err + return os.Symlink(devicePath, linkPath) } // UnmapDevice removes a symbolic link associated to block device under specified map path -func (v VolumePathHandler) UnmapDevice(mapPath string, linkName string) error { +func (v VolumePathHandler) UnmapDevice(mapPath string, linkName string, bindMount bool) error { if len(mapPath) == 0 { return fmt.Errorf("Failed to unmap device from map path. mapPath is empty") } klog.V(5).Infof("UnmapDevice: mapPath %s", mapPath) klog.V(5).Infof("UnmapDevice: linkName %s", linkName) + if bindMount { + return unmapBindMountDevice(v, mapPath, linkName) + } + return unmapSymlinkDevice(v, mapPath, linkName) +} + +func unmapBindMountDevice(v VolumePathHandler, mapPath string, linkName string) error { + // Check bind mount exists + linkPath := filepath.Join(mapPath, string(linkName)) + if isMountExist, checkErr := v.IsBindMountExist(linkPath); checkErr != nil { + return checkErr + } else if !isMountExist { + klog.Warningf("Warning: Unmap skipped because bind mount does not exist on the path: %v", linkPath) + // TODO: consider deleting empty file if it exists + return nil + } + + // Unmount file + mounter := &mount.SafeFormatAndMount{Interface: mount.New(""), Exec: mount.NewOSExec()} + if err := mounter.Unmount(linkPath); err != nil { + klog.Errorf("Failed to unmount linkPath %s: %v", linkPath, err) + return err + } + + // Remove file + if err := os.Remove(linkPath); err != nil && !os.IsNotExist(err) { + return err + } + + return nil +} + +func unmapSymlinkDevice(v VolumePathHandler, mapPath string, linkName string) error { // Check symbolic link exists linkPath := filepath.Join(mapPath, string(linkName)) if islinkExist, checkErr := v.IsSymlinkExist(linkPath); checkErr != nil { @@ -125,8 +209,7 @@ func (v VolumePathHandler) UnmapDevice(mapPath string, linkName string) error { klog.Warningf("Warning: Unmap skipped because symlink does not exist on the path: %v", linkPath) return nil } - err := os.Remove(linkPath) - return err + return os.Remove(linkPath) } // RemoveMapPath removes a file or directory on specified map path @@ -163,70 +246,42 @@ func (v VolumePathHandler) IsSymlinkExist(mapPath string) (bool, error) { return false, err } -// GetDeviceSymlinkRefs searches symbolic links under global map path -func (v VolumePathHandler) GetDeviceSymlinkRefs(devPath string, mapPath string) ([]string, error) { +// IsBindMountExist returns true if specified file exists and the type is device. +// If file doesn't exist, or file exists but not device, return false with no error. +// On other cases, return false with error from Lstat(). +func (v VolumePathHandler) IsBindMountExist(mapPath string) (bool, error) { + fi, err := os.Lstat(mapPath) + if err == nil { + // If file exits and it's device, return true and no error + if fi.Mode()&os.ModeDevice == os.ModeDevice { + return true, nil + } + // If file exits but it's not device, return fale and no error + return false, nil + } + // If file doesn't exist, return false and no error + if os.IsNotExist(err) { + return false, nil + } + // Return error from Lstat() + return false, err +} + +// GetDeviceBindMountRefs searches bind mounts under global map path +func (v VolumePathHandler) GetDeviceBindMountRefs(devPath string, mapPath string) ([]string, error) { var refs []string files, err := ioutil.ReadDir(mapPath) if err != nil { return nil, fmt.Errorf("Directory cannot read %v", err) } for _, file := range files { - if file.Mode()&os.ModeSymlink != os.ModeSymlink { + if file.Mode()&os.ModeDevice != os.ModeDevice { continue } filename := file.Name() - fp, err := os.Readlink(filepath.Join(mapPath, filename)) - if err != nil { - return nil, fmt.Errorf("Symbolic link cannot be retrieved %v", err) - } - klog.V(5).Infof("GetDeviceSymlinkRefs: filepath: %v, devPath: %v", fp, devPath) - if fp == devPath { - refs = append(refs, filepath.Join(mapPath, filename)) - } + // TODO: Might need to check if the file is actually linked to devPath + refs = append(refs, filepath.Join(mapPath, filename)) } - klog.V(5).Infof("GetDeviceSymlinkRefs: refs %v", refs) + klog.V(5).Infof("GetDeviceBindMountRefs: refs %v", refs) return refs, nil } - -// FindGlobalMapPathUUIDFromPod finds {pod uuid} symbolic link under globalMapPath -// corresponding to map path symlink, and then return global map path with pod uuid. -// ex. mapPath symlink: pods/{podUid}}/{DefaultKubeletVolumeDevicesDirName}/{escapeQualifiedPluginName}/{volumeName} -> /dev/sdX -// globalMapPath/{pod uuid}: plugins/kubernetes.io/{PluginName}/{DefaultKubeletVolumeDevicesDirName}/{volumePluginDependentPath}/{pod uuid} -> /dev/sdX -func (v VolumePathHandler) FindGlobalMapPathUUIDFromPod(pluginDir, mapPath string, podUID types.UID) (string, error) { - var globalMapPathUUID string - // Find symbolic link named pod uuid under plugin dir - err := filepath.Walk(pluginDir, func(path string, fi os.FileInfo, err error) error { - if err != nil { - return err - } - if (fi.Mode()&os.ModeSymlink == os.ModeSymlink) && (fi.Name() == string(podUID)) { - klog.V(5).Infof("FindGlobalMapPathFromPod: path %s, mapPath %s", path, mapPath) - if res, err := compareSymlinks(path, mapPath); err == nil && res { - globalMapPathUUID = path - } - } - return nil - }) - if err != nil { - return "", err - } - klog.V(5).Infof("FindGlobalMapPathFromPod: globalMapPathUUID %s", globalMapPathUUID) - // Return path contains global map path + {pod uuid} - return globalMapPathUUID, nil -} - -func compareSymlinks(global, pod string) (bool, error) { - devGlobal, err := os.Readlink(global) - if err != nil { - return false, err - } - devPod, err := os.Readlink(pod) - if err != nil { - return false, err - } - klog.V(5).Infof("CompareSymlinks: devGloBal %s, devPod %s", devGlobal, devPod) - if devGlobal == devPod { - return true, nil - } - return false, nil -} diff --git a/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go b/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go index 7170edc7de0..db9ecfad301 100644 --- a/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go +++ b/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go @@ -19,12 +19,15 @@ limitations under the License. package volumepathhandler import ( + "bufio" "errors" "fmt" "os" "os/exec" + "path/filepath" "strings" + "k8s.io/apimachinery/pkg/types" "k8s.io/klog" ) @@ -64,7 +67,7 @@ func (v VolumePathHandler) GetLoopDevice(path string) (string, error) { klog.V(2).Infof("Failed device discover command for path %s: %v %s", path, err, out) return "", err } - return parseLosetupOutputForDevice(out) + return parseLosetupOutputForDevice(out, path) } func makeLoopDevice(path string) (string, error) { @@ -75,7 +78,14 @@ func makeLoopDevice(path string) (string, error) { klog.V(2).Infof("Failed device create command for path: %s %v %s ", path, err, out) return "", err } - return parseLosetupOutputForDevice(out) + + // losetup -f --show {path} returns device in the format: + // /dev/loop1 + if len(out) == 0 { + return "", errors.New(ErrDeviceNotFound) + } + + return strings.TrimSpace(string(out)), nil } // RemoveLoopDevice removes specified loopback device @@ -93,16 +103,97 @@ func (v VolumePathHandler) RemoveLoopDevice(device string) error { return nil } -func parseLosetupOutputForDevice(output []byte) (string, error) { +func parseLosetupOutputForDevice(output []byte, path string) (string, error) { if len(output) == 0 { return "", errors.New(ErrDeviceNotFound) } - // losetup returns device in the format: - // /dev/loop1: [0073]:148662 (/dev/sda) - device := strings.TrimSpace(strings.SplitN(string(output), ":", 2)[0]) + // losetup -j {path} returns device in the format: + // /dev/loop1: [0073]:148662 (/dev/{globalMapPath}/{pod1-UUID}) + // /dev/loop2: [0073]:148662 (/dev/{globalMapPath}/{pod2-UUID}) + s := string(output) + // Find line that exact matches the path + var matched string + scanner := bufio.NewScanner(strings.NewReader(s)) + for scanner.Scan() { + if strings.HasSuffix(scanner.Text(), "("+path+")") { + matched = scanner.Text() + break + } + } + if len(matched) == 0 { + return "", errors.New(ErrDeviceNotFound) + } + s = matched + + device := strings.TrimSpace(strings.SplitN(s, ":", 2)[0]) if len(device) == 0 { return "", errors.New(ErrDeviceNotFound) } return device, nil } + +// FindGlobalMapPathUUIDFromPod finds {pod uuid} bind mount under globalMapPath +// corresponding to map path symlink, and then return global map path with pod uuid. +// ex. mapPath symlink: pods/{podUid}}/{DefaultKubeletVolumeDevicesDirName}/{escapeQualifiedPluginName}/{volumeName} -> /dev/sdX +// globalMapPath/{pod uuid} bind mount: plugins/kubernetes.io/{PluginName}/{DefaultKubeletVolumeDevicesDirName}/{volumePluginDependentPath}/{pod uuid} -> /dev/sdX +func (v VolumePathHandler) FindGlobalMapPathUUIDFromPod(pluginDir, mapPath string, podUID types.UID) (string, error) { + var globalMapPathUUID string + // Find symbolic link named pod uuid under plugin dir + err := filepath.Walk(pluginDir, func(path string, fi os.FileInfo, err error) error { + if err != nil { + return err + } + if (fi.Mode()&os.ModeDevice == os.ModeDevice) && (fi.Name() == string(podUID)) { + klog.V(5).Infof("FindGlobalMapPathFromPod: path %s, mapPath %s", path, mapPath) + if res, err := compareBindMountAndSymlinks(path, mapPath); err == nil && res { + globalMapPathUUID = path + } + } + return nil + }) + if err != nil { + return "", err + } + klog.V(5).Infof("FindGlobalMapPathFromPod: globalMapPathUUID %s", globalMapPathUUID) + // Return path contains global map path + {pod uuid} + return globalMapPathUUID, nil +} + +func compareBindMountAndSymlinks(global, pod string) (bool, error) { + devNumGlobal, err := getDeviceMajorMinor(global) + if err != nil { + return false, err + } + devPod, err := os.Readlink(pod) + if err != nil { + return false, err + } + devNumPod, err := getDeviceMajorMinor(devPod) + if err != nil { + return false, err + } + klog.V(5).Infof("CompareBindMountAndSymlinks: devNumGlobal %s, devNumPod %s", devNumGlobal, devNumPod) + if devNumGlobal == devNumPod { + return true, nil + } + return false, nil +} + +func getDeviceMajorMinor(path string) (string, error) { + args := []string{"-c", "%t:%T", path} + cmd := exec.Command(statPath, args...) + out, err := cmd.CombinedOutput() + if err != nil { + klog.V(2).Infof("Failed to stat path: %s %v %s ", path, err, out) + return "", err + } + + // stat -c "%t:%T" {path} outputs following format(major:minor in hex): + // fc:10 + if len(out) == 0 { + return "", errors.New(ErrDeviceNotFound) + } + + return strings.TrimSpace(string(out)), nil +} diff --git a/pkg/volume/util/volumepathhandler/volume_path_handler_unsupported.go b/pkg/volume/util/volumepathhandler/volume_path_handler_unsupported.go index 266398b1da8..03d4c1dba2b 100644 --- a/pkg/volume/util/volumepathhandler/volume_path_handler_unsupported.go +++ b/pkg/volume/util/volumepathhandler/volume_path_handler_unsupported.go @@ -20,6 +20,8 @@ package volumepathhandler import ( "fmt" + + "k8s.io/apimachinery/pkg/types" ) // AttachFileDevice takes a path to a regular file and makes it available as an @@ -37,3 +39,9 @@ func (v VolumePathHandler) GetLoopDevice(path string) (string, error) { func (v VolumePathHandler) RemoveLoopDevice(device string) error { return fmt.Errorf("RemoveLoopDevice not supported for this build.") } + +// FindGlobalMapPathUUIDFromPod finds {pod uuid} bind mount under globalMapPath +// corresponding to map path symlink, and then return global map path with pod uuid. +func (v VolumePathHandler) FindGlobalMapPathUUIDFromPod(pluginDir, mapPath string, podUID types.UID) (string, error) { + return "", fmt.Errorf("FindGlobalMapPathUUIDFromPod not supported for this build.") +} From 68be3947b8f0b1dd87240c672fe73e86533d39a0 Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Fri, 1 Nov 2019 19:31:56 +0000 Subject: [PATCH 02/11] Make descriptor lock per pod and release it per pod This change is needed to avoid unmapVolumeFunc for one pod blocked, when the lock for the same volume is taken for another pod. --- pkg/volume/testing/testing.go | 10 +++--- .../operationexecutor/operation_generator.go | 32 ++++++------------- .../volumepathhandler/volume_path_handler.go | 5 +-- .../volume_path_handler_linux.go | 25 +++++++++++++-- .../volume_path_handler_unsupported.go | 11 ++++--- 5 files changed, 46 insertions(+), 37 deletions(-) diff --git a/pkg/volume/testing/testing.go b/pkg/volume/testing/testing.go index cc57be3184d..f93aaf62bd8 100644 --- a/pkg/volume/testing/testing.go +++ b/pkg/volume/testing/testing.go @@ -1178,16 +1178,16 @@ func (fv *FakeVolumePathHandler) AttachFileDevice(path string) (string, error) { return "", nil } +func (fv *FakeVolumePathHandler) DetachFileDevice(path string) error { + // nil is success, else error + return nil +} + func (fv *FakeVolumePathHandler) GetLoopDevice(path string) (string, error) { // nil is success, else error return "/dev/loop1", nil } -func (fv *FakeVolumePathHandler) RemoveLoopDevice(device string) error { - // nil is success, else error - return nil -} - // FindEmptyDirectoryUsageOnTmpfs finds the expected usage of an empty directory existing on // a tmpfs filesystem on this system. func FindEmptyDirectoryUsageOnTmpfs() (*resource.Quantity, error) { diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index 9d69b683161..4b665ee2a67 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -1096,7 +1096,7 @@ func (og *operationGenerator) GenerateMapVolumeFunc( // that the block device is silently removed and attached another device with same name. // Container runtime can't handler this problem. To avoid unexpected condition fd lock // for the block device is required. - _, err = og.blkUtil.AttachFileDevice(devicePath) + _, err = og.blkUtil.AttachFileDevice(filepath.Join(globalMapPath, string(volumeToMount.Pod.UID))) if err != nil { return volumeToMount.GenerateError("MapVolume.AttachFileDevice failed", err) } @@ -1207,6 +1207,14 @@ func (og *operationGenerator) GenerateUnmapVolumeFunc( } unmapVolumeFunc := func() (error, error) { + globalUnmapPath := volumeToUnmount.DeviceMountPath + + // Release file descriptor lock. + err := og.blkUtil.DetachFileDevice(filepath.Join(globalUnmapPath, string(volumeToUnmount.PodUID))) + if err != nil { + return volumeToUnmount.GenerateError("UnmapVolume.UnmapDevice Detaching descriptor lock failed", err) + } + // Try to unmap volumeName symlink under pod device map path dir // pods/{podUid}/volumeDevices/{escapeQualifiedPluginName}/{volumeName} podDeviceUnmapPath, volName := blockVolumeUnmapper.GetPodDeviceMapPath() @@ -1217,7 +1225,6 @@ func (og *operationGenerator) GenerateUnmapVolumeFunc( } // Try to unmap podUID symlink under global map path dir // plugins/kubernetes.io/{PluginName}/volumeDevices/{volumePluginDependentPath}/{podUID} - globalUnmapPath := volumeToUnmount.DeviceMountPath unmapDeviceErr = og.blkUtil.UnmapDevice(globalUnmapPath, string(volumeToUnmount.PodUID), true /* bindMount */) if unmapDeviceErr != nil { // On failure, return error. Caller will log and retry. @@ -1315,27 +1322,6 @@ func (og *operationGenerator) GenerateUnmapDeviceFunc( return deviceToDetach.GenerateError("UnmapDevice failed", err) } - // The block volume is not referenced from Pods. Release file descriptor lock. - // This should be done before calling TearDownDevice, because some plugins that do local detach - // in TearDownDevice will fail in detaching device due to the refcnt on the loopback device. - klog.V(4).Infof("UnmapDevice: deviceToDetach.DevicePath: %v", deviceToDetach.DevicePath) - loopPath, err := og.blkUtil.GetLoopDevice(deviceToDetach.DevicePath) - if err != nil { - if err.Error() == volumepathhandler.ErrDeviceNotFound { - klog.Warningf(deviceToDetach.GenerateMsgDetailed("UnmapDevice: Couldn't find loopback device which takes file descriptor lock", fmt.Sprintf("device path: %q", deviceToDetach.DevicePath))) - } else { - errInfo := "UnmapDevice.GetLoopDevice failed to get loopback device, " + fmt.Sprintf("device path: %q", deviceToDetach.DevicePath) - return deviceToDetach.GenerateError(errInfo, err) - } - } else { - if len(loopPath) != 0 { - err = og.blkUtil.RemoveLoopDevice(loopPath) - if err != nil { - return deviceToDetach.GenerateError("UnmapDevice.RemoveLoopDevice failed", err) - } - } - } - // Execute tear down device unmapErr := blockVolumeUnmapper.TearDownDevice(globalMapPath, deviceToDetach.DevicePath) if unmapErr != nil { diff --git a/pkg/volume/util/volumepathhandler/volume_path_handler.go b/pkg/volume/util/volumepathhandler/volume_path_handler.go index d245e558925..cf9c23beb68 100644 --- a/pkg/volume/util/volumepathhandler/volume_path_handler.go +++ b/pkg/volume/util/volumepathhandler/volume_path_handler.go @@ -55,10 +55,11 @@ type BlockVolumePathHandler interface { // AttachFileDevice takes a path to a regular file and makes it available as an // attached block device. AttachFileDevice(path string) (string, error) + // DetachFileDevice takes a path to the attached block device and + // detach it from block device. + DetachFileDevice(path string) error // GetLoopDevice returns the full path to the loop device associated with the given path. GetLoopDevice(path string) (string, error) - // RemoveLoopDevice removes specified loopback device - RemoveLoopDevice(device string) error } // NewBlockVolumePathHandler returns a new instance of BlockVolumeHandler. diff --git a/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go b/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go index db9ecfad301..272f074d11e 100644 --- a/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go +++ b/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go @@ -50,6 +50,27 @@ func (v VolumePathHandler) AttachFileDevice(path string) (string, error) { return blockDevicePath, nil } +// DetachFileDevice takes a path to the attached block device and +// detach it from block device. +func (v VolumePathHandler) DetachFileDevice(path string) error { + loopPath, err := v.GetLoopDevice(path) + if err != nil { + if err.Error() == ErrDeviceNotFound { + klog.Warningf("DetachFileDevice: Couldn't find loopback device which takes file descriptor lock. device path: %q", path) + } else { + return err + } + } else { + if len(loopPath) != 0 { + err = removeLoopDevice(loopPath) + if err != nil { + return err + } + } + } + return nil +} + // GetLoopDevice returns the full path to the loop device associated with the given path. func (v VolumePathHandler) GetLoopDevice(path string) (string, error) { _, err := os.Stat(path) @@ -88,8 +109,8 @@ func makeLoopDevice(path string) (string, error) { return strings.TrimSpace(string(out)), nil } -// RemoveLoopDevice removes specified loopback device -func (v VolumePathHandler) RemoveLoopDevice(device string) error { +// removeLoopDevice removes specified loopback device +func removeLoopDevice(device string) error { args := []string{"-d", device} cmd := exec.Command(losetupPath, args...) out, err := cmd.CombinedOutput() diff --git a/pkg/volume/util/volumepathhandler/volume_path_handler_unsupported.go b/pkg/volume/util/volumepathhandler/volume_path_handler_unsupported.go index 03d4c1dba2b..f4411c6cdb7 100644 --- a/pkg/volume/util/volumepathhandler/volume_path_handler_unsupported.go +++ b/pkg/volume/util/volumepathhandler/volume_path_handler_unsupported.go @@ -30,16 +30,17 @@ func (v VolumePathHandler) AttachFileDevice(path string) (string, error) { return "", fmt.Errorf("AttachFileDevice not supported for this build.") } +// DetachFileDevice takes a path to the attached block device and +// detach it from block device. +func (v VolumePathHandler) DetachFileDevice(path string) error { + return fmt.Errorf("DetachFileDevice not supported for this build.") +} + // GetLoopDevice returns the full path to the loop device associated with the given path. func (v VolumePathHandler) GetLoopDevice(path string) (string, error) { return "", fmt.Errorf("GetLoopDevice not supported for this build.") } -// RemoveLoopDevice removes specified loopback device -func (v VolumePathHandler) RemoveLoopDevice(device string) error { - return fmt.Errorf("RemoveLoopDevice not supported for this build.") -} - // FindGlobalMapPathUUIDFromPod finds {pod uuid} bind mount under globalMapPath // corresponding to map path symlink, and then return global map path with pod uuid. func (v VolumePathHandler) FindGlobalMapPathUUIDFromPod(pluginDir, mapPath string, podUID types.UID) (string, error) { From 8a159d725345264e5a01182d5511eefe1d02f90f Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Fri, 1 Nov 2019 19:32:23 +0000 Subject: [PATCH 03/11] Move MapBlockVolume call to operation_generator and add UnmapBlockVolume --- pkg/volume/awsebs/aws_ebs_block.go | 3 +- pkg/volume/azure_dd/azure_dd_block.go | 3 +- pkg/volume/cinder/cinder_block.go | 3 +- pkg/volume/csi/csi_block.go | 3 +- pkg/volume/csi/csi_block_test.go | 64 ------------------- pkg/volume/fc/fc.go | 2 +- pkg/volume/gcepd/gce_pd_block.go | 3 +- pkg/volume/iscsi/iscsi.go | 2 +- pkg/volume/local/local.go | 2 +- pkg/volume/rbd/rbd.go | 2 +- .../operationexecutor/operation_generator.go | 41 ++++-------- pkg/volume/util/util.go | 48 +++++++++++++- .../vsphere_volume/vsphere_volume_block.go | 3 +- 13 files changed, 69 insertions(+), 110 deletions(-) diff --git a/pkg/volume/awsebs/aws_ebs_block.go b/pkg/volume/awsebs/aws_ebs_block.go index 71bc11c98b1..3600a085dcb 100644 --- a/pkg/volume/awsebs/aws_ebs_block.go +++ b/pkg/volume/awsebs/aws_ebs_block.go @@ -29,7 +29,6 @@ import ( "k8s.io/klog" "k8s.io/kubernetes/pkg/util/mount" "k8s.io/kubernetes/pkg/volume" - "k8s.io/kubernetes/pkg/volume/util" "k8s.io/kubernetes/pkg/volume/util/volumepathhandler" "k8s.io/legacy-cloud-providers/aws" utilstrings "k8s.io/utils/strings" @@ -148,7 +147,7 @@ func (b *awsElasticBlockStoreMapper) SetUpDevice() (string, error) { } func (b *awsElasticBlockStoreMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) + return nil } // GetGlobalMapPath returns global map path and error diff --git a/pkg/volume/azure_dd/azure_dd_block.go b/pkg/volume/azure_dd/azure_dd_block.go index 39bdb28edb9..41c00e9319f 100644 --- a/pkg/volume/azure_dd/azure_dd_block.go +++ b/pkg/volume/azure_dd/azure_dd_block.go @@ -28,7 +28,6 @@ import ( "k8s.io/klog" "k8s.io/kubernetes/pkg/util/mount" "k8s.io/kubernetes/pkg/volume" - "k8s.io/kubernetes/pkg/volume/util" "k8s.io/kubernetes/pkg/volume/util/volumepathhandler" utilstrings "k8s.io/utils/strings" ) @@ -141,7 +140,7 @@ func (b *azureDataDiskMapper) SetUpDevice() (string, error) { } func (b *azureDataDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) + return nil } // GetGlobalMapPath returns global map path and error diff --git a/pkg/volume/cinder/cinder_block.go b/pkg/volume/cinder/cinder_block.go index 483170ef284..d83637459f9 100644 --- a/pkg/volume/cinder/cinder_block.go +++ b/pkg/volume/cinder/cinder_block.go @@ -28,7 +28,6 @@ import ( "k8s.io/klog" "k8s.io/kubernetes/pkg/util/mount" "k8s.io/kubernetes/pkg/volume" - "k8s.io/kubernetes/pkg/volume/util" "k8s.io/kubernetes/pkg/volume/util/volumepathhandler" utilstrings "k8s.io/utils/strings" ) @@ -151,7 +150,7 @@ func (b *cinderVolumeMapper) SetUpDevice() (string, error) { } func (b *cinderVolumeMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) + return nil } // GetGlobalMapPath returns global map path and error diff --git a/pkg/volume/csi/csi_block.go b/pkg/volume/csi/csi_block.go index 5e6dc145caa..56aa2dbbf57 100644 --- a/pkg/volume/csi/csi_block.go +++ b/pkg/volume/csi/csi_block.go @@ -31,7 +31,6 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" "k8s.io/kubernetes/pkg/volume" - ioutil "k8s.io/kubernetes/pkg/volume/util" utilstrings "k8s.io/utils/strings" ) @@ -267,7 +266,7 @@ func (m *csiBlockMapper) SetUpDevice() (string, error) { } func (m *csiBlockMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return ioutil.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) + return nil } var _ volume.BlockVolumeUnmapper = &csiBlockMapper{} diff --git a/pkg/volume/csi/csi_block_test.go b/pkg/volume/csi/csi_block_test.go index b41bd0301ba..befcd4fe28b 100644 --- a/pkg/volume/csi/csi_block_test.go +++ b/pkg/volume/csi/csi_block_test.go @@ -315,44 +315,12 @@ func TestBlockMapperMapDevice(t *testing.T) { t.Fatalf("mapper failed to GetGlobalMapPath: %v", err) } - // Actual SetupDevice should create a symlink to or a bind mout of device in devicePath. - // Create dummy file there before calling MapDevice to test it properly. - fd, err := os.Create(devicePath) - if err != nil { - t.Fatalf("mapper failed to create dummy file in devicePath: %v", err) - } - if err := fd.Close(); err != nil { - t.Fatalf("mapper failed to close dummy file in devicePath: %v", err) - } - // Map device to global and pod device map path volumeMapPath, volName := csiMapper.GetPodDeviceMapPath() err = csiMapper.MapDevice(devicePath, globalMapPath, volumeMapPath, volName, csiMapper.podUID) if err != nil { t.Fatalf("mapper failed to GetGlobalMapPath: %v", err) } - - // Check if symlink {globalMapPath}/{podUID} exists - globalMapFilePath := filepath.Join(globalMapPath, string(csiMapper.podUID)) - if _, err := os.Stat(globalMapFilePath); err != nil { - if os.IsNotExist(err) { - t.Errorf("mapper.MapDevice failed, symlink in globalMapPath not created: %v", err) - t.Errorf("mapper.MapDevice devicePath:%v, globalMapPath: %v, globalMapFilePath: %v", - devicePath, globalMapPath, globalMapFilePath) - } else { - t.Errorf("mapper.MapDevice failed: %v", err) - } - } - - // Check if symlink {volumeMapPath}/{volName} exists - volumeMapFilePath := filepath.Join(volumeMapPath, volName) - if _, err := os.Stat(volumeMapFilePath); err != nil { - if os.IsNotExist(err) { - t.Errorf("mapper.MapDevice failed, symlink in volumeMapPath not created: %v", err) - } else { - t.Errorf("mapper.MapDevice failed: %v", err) - } - } } func TestBlockMapperMapDeviceNotSupportAttach(t *testing.T) { @@ -402,44 +370,12 @@ func TestBlockMapperMapDeviceNotSupportAttach(t *testing.T) { t.Fatalf("mapper failed to GetGlobalMapPath: %v", err) } - // Actual SetupDevice should create a symlink to or a bind mout of device in devicePath. - // Create dummy file there before calling MapDevice to test it properly. - fd, err := os.Create(devicePath) - if err != nil { - t.Fatalf("mapper failed to create dummy file in devicePath: %v", err) - } - if err := fd.Close(); err != nil { - t.Fatalf("mapper failed to close dummy file in devicePath: %v", err) - } - // Map device to global and pod device map path volumeMapPath, volName := csiMapper.GetPodDeviceMapPath() err = csiMapper.MapDevice(devicePath, globalMapPath, volumeMapPath, volName, csiMapper.podUID) if err != nil { t.Fatalf("mapper failed to GetGlobalMapPath: %v", err) } - - // Check if symlink {globalMapPath}/{podUID} exists - globalMapFilePath := filepath.Join(globalMapPath, string(csiMapper.podUID)) - if _, err := os.Stat(globalMapFilePath); err != nil { - if os.IsNotExist(err) { - t.Errorf("mapper.MapDevice failed, symlink in globalMapPath not created: %v", err) - t.Errorf("mapper.MapDevice devicePath:%v, globalMapPath: %v, globalMapFilePath: %v", - devicePath, globalMapPath, globalMapFilePath) - } else { - t.Errorf("mapper.MapDevice failed: %v", err) - } - } - - // Check if symlink {volumeMapPath}/{volName} exists - volumeMapFilePath := filepath.Join(volumeMapPath, volName) - if _, err := os.Stat(volumeMapFilePath); err != nil { - if os.IsNotExist(err) { - t.Errorf("mapper.MapDevice failed, symlink in volumeMapPath not created: %v", err) - } else { - t.Errorf("mapper.MapDevice failed: %v", err) - } - } } func TestBlockMapperTearDownDevice(t *testing.T) { diff --git a/pkg/volume/fc/fc.go b/pkg/volume/fc/fc.go index 4892f4b4a4d..ece6e92a19c 100644 --- a/pkg/volume/fc/fc.go +++ b/pkg/volume/fc/fc.go @@ -419,7 +419,7 @@ func (b *fcDiskMapper) SetUpDevice() (string, error) { } func (b *fcDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) + return nil } type fcDiskUnmapper struct { diff --git a/pkg/volume/gcepd/gce_pd_block.go b/pkg/volume/gcepd/gce_pd_block.go index 59f4e1c821b..40344faa38f 100644 --- a/pkg/volume/gcepd/gce_pd_block.go +++ b/pkg/volume/gcepd/gce_pd_block.go @@ -29,7 +29,6 @@ import ( "k8s.io/klog" "k8s.io/kubernetes/pkg/util/mount" "k8s.io/kubernetes/pkg/volume" - "k8s.io/kubernetes/pkg/volume/util" "k8s.io/kubernetes/pkg/volume/util/volumepathhandler" utilstrings "k8s.io/utils/strings" ) @@ -158,7 +157,7 @@ func (b *gcePersistentDiskMapper) SetUpDevice() (string, error) { } func (b *gcePersistentDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) + return nil } // GetGlobalMapPath returns global map path and error diff --git a/pkg/volume/iscsi/iscsi.go b/pkg/volume/iscsi/iscsi.go index 7b5f7039f6d..9f177deaad7 100644 --- a/pkg/volume/iscsi/iscsi.go +++ b/pkg/volume/iscsi/iscsi.go @@ -389,7 +389,7 @@ func (b *iscsiDiskMapper) SetUpDevice() (string, error) { } func (b *iscsiDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return ioutil.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) + return nil } type iscsiDiskUnmapper struct { diff --git a/pkg/volume/local/local.go b/pkg/volume/local/local.go index ae9083317b2..1f612727605 100644 --- a/pkg/volume/local/local.go +++ b/pkg/volume/local/local.go @@ -621,7 +621,7 @@ func (m *localVolumeMapper) SetUpDevice() (string, error) { } func (m *localVolumeMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) + return nil } // localVolumeUnmapper implements the BlockVolumeUnmapper interface for local volumes. diff --git a/pkg/volume/rbd/rbd.go b/pkg/volume/rbd/rbd.go index 91821bbbf4a..9f52aaadb17 100644 --- a/pkg/volume/rbd/rbd.go +++ b/pkg/volume/rbd/rbd.go @@ -917,7 +917,7 @@ func (rbd *rbdDiskMapper) SetUpDevice() (string, error) { } func (rbd *rbdDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return volutil.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) + return nil } func (rbd *rbd) rbdGlobalMapPath(spec *volume.Spec) (string, error) { diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index 4b665ee2a67..39b8a176f5d 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -39,6 +39,7 @@ import ( "k8s.io/kubernetes/pkg/volume" "k8s.io/kubernetes/pkg/volume/csi" "k8s.io/kubernetes/pkg/volume/util" + ioutil "k8s.io/kubernetes/pkg/volume/util" "k8s.io/kubernetes/pkg/volume/util/hostutil" volumetypes "k8s.io/kubernetes/pkg/volume/util/types" "k8s.io/kubernetes/pkg/volume/util/volumepathhandler" @@ -1084,21 +1085,19 @@ func (og *operationGenerator) GenerateMapVolumeFunc( return volumeToMount.GenerateError("MapVolume.EvalHostSymlinks failed", err) } - // Map device to global and pod device map path + // Execute driver specific map volumeMapPath, volName := blockVolumeMapper.GetPodDeviceMapPath() mapErr = blockVolumeMapper.MapDevice(devicePath, globalMapPath, volumeMapPath, volName, volumeToMount.Pod.UID) if mapErr != nil { // On failure, return error. Caller will log and retry. - return volumeToMount.GenerateError("MapVolume.MapDevice failed", mapErr) + return volumeToMount.GenerateError("MapVolume.MapPodDevice failed", mapErr) } - // Take filedescriptor lock to keep a block device opened. Otherwise, there is a case - // that the block device is silently removed and attached another device with same name. - // Container runtime can't handler this problem. To avoid unexpected condition fd lock - // for the block device is required. - _, err = og.blkUtil.AttachFileDevice(filepath.Join(globalMapPath, string(volumeToMount.Pod.UID))) - if err != nil { - return volumeToMount.GenerateError("MapVolume.AttachFileDevice failed", err) + // Execute common map + mapErr = ioutil.MapBlockVolume(og.blkUtil, devicePath, globalMapPath, volumeMapPath, volName, volumeToMount.Pod.UID) + if mapErr != nil { + // On failure, return error. Caller will log and retry. + return volumeToMount.GenerateError("MapVolume.MapDevice failed", mapErr) } // Update actual state of world to reflect volume is globally mounted @@ -1207,28 +1206,16 @@ func (og *operationGenerator) GenerateUnmapVolumeFunc( } unmapVolumeFunc := func() (error, error) { - globalUnmapPath := volumeToUnmount.DeviceMountPath - - // Release file descriptor lock. - err := og.blkUtil.DetachFileDevice(filepath.Join(globalUnmapPath, string(volumeToUnmount.PodUID))) - if err != nil { - return volumeToUnmount.GenerateError("UnmapVolume.UnmapDevice Detaching descriptor lock failed", err) - } - - // Try to unmap volumeName symlink under pod device map path dir // pods/{podUid}/volumeDevices/{escapeQualifiedPluginName}/{volumeName} podDeviceUnmapPath, volName := blockVolumeUnmapper.GetPodDeviceMapPath() - unmapDeviceErr := og.blkUtil.UnmapDevice(podDeviceUnmapPath, volName, false /* bindMount */) - if unmapDeviceErr != nil { - // On failure, return error. Caller will log and retry. - return volumeToUnmount.GenerateError("UnmapVolume.UnmapDevice on pod device map path failed", unmapDeviceErr) - } - // Try to unmap podUID symlink under global map path dir // plugins/kubernetes.io/{PluginName}/volumeDevices/{volumePluginDependentPath}/{podUID} - unmapDeviceErr = og.blkUtil.UnmapDevice(globalUnmapPath, string(volumeToUnmount.PodUID), true /* bindMount */) - if unmapDeviceErr != nil { + globalUnmapPath := volumeToUnmount.DeviceMountPath + + // Execute common unmap + unmapErr := ioutil.UnmapBlockVolume(og.blkUtil, globalUnmapPath, podDeviceUnmapPath, volName, volumeToUnmount.PodUID) + if unmapErr != nil { // On failure, return error. Caller will log and retry. - return volumeToUnmount.GenerateError("UnmapVolume.UnmapDevice on global map path failed", unmapDeviceErr) + return volumeToUnmount.GenerateError("UnmapVolume.UnmapBlockVolume failed", unmapErr) } klog.Infof( diff --git a/pkg/volume/util/util.go b/pkg/volume/util/util.go index f097edaa58f..581895715fd 100644 --- a/pkg/volume/util/util.go +++ b/pkg/volume/util/util.go @@ -503,18 +503,17 @@ func MakeAbsolutePath(goos, path string) string { return "c:\\" + path } -// MapBlockVolume is a utility function to provide a common way of mounting +// MapBlockVolume is a utility function to provide a common way of mapping // block device path for a specified volume and pod. This function should be // called by volume plugins that implements volume.BlockVolumeMapper.Map() method. func MapBlockVolume( + blkUtil volumepathhandler.BlockVolumePathHandler, devicePath, globalMapPath, podVolumeMapPath, volumeMapName string, podUID utypes.UID, ) error { - blkUtil := volumepathhandler.NewBlockVolumePathHandler() - // map devicePath to global node path as bind mount mapErr := blkUtil.MapDevice(devicePath, globalMapPath, string(podUID), true /* bindMount */) if mapErr != nil { @@ -529,6 +528,49 @@ func MapBlockVolume( devicePath, podVolumeMapPath, volumeMapName, false, mapErr) } + // Take file descriptor lock to keep a block device opened. Otherwise, there is a case + // that the block device is silently removed and attached another device with the same name. + // Container runtime can't handle this problem. To avoid unexpected condition fd lock + // for the block device is required. + _, mapErr = blkUtil.AttachFileDevice(filepath.Join(globalMapPath, string(podUID))) + if mapErr != nil { + return fmt.Errorf("blkUtil.AttachFileDevice failed. globalMapPath:%s, podUID: %s: %v", + globalMapPath, string(podUID), mapErr) + } + + return nil +} + +// UnmapBlockVolume is a utility function to provide a common way of unmapping +// block device path for a specified volume and pod. This function should be +// called by volume plugins that implements volume.BlockVolumeMapper.Map() method. +func UnmapBlockVolume( + blkUtil volumepathhandler.BlockVolumePathHandler, + globalUnmapPath, + podDeviceUnmapPath, + volumeMapName string, + podUID utypes.UID, +) error { + // Release file descriptor lock. + err := blkUtil.DetachFileDevice(filepath.Join(globalUnmapPath, string(podUID))) + if err != nil { + return fmt.Errorf("blkUtil.DetachFileDevice failed. globalUnmapPath:%s, podUID: %s: %v", + globalUnmapPath, string(podUID), err) + } + + // unmap devicePath from pod volume path + unmapDeviceErr := blkUtil.UnmapDevice(podDeviceUnmapPath, volumeMapName, false /* bindMount */) + if unmapDeviceErr != nil { + return fmt.Errorf("blkUtil.DetachFileDevice failed. podDeviceUnmapPath:%s, volumeMapName: %s, bindMount: %v: %v", + podDeviceUnmapPath, volumeMapName, false, unmapDeviceErr) + } + + // unmap devicePath from global node path + unmapDeviceErr = blkUtil.UnmapDevice(globalUnmapPath, string(podUID), true /* bindMount */) + if unmapDeviceErr != nil { + return fmt.Errorf("blkUtil.DetachFileDevice failed. globalUnmapPath:%s, podUID: %s, bindMount: %v: %v", + globalUnmapPath, string(podUID), true, unmapDeviceErr) + } return nil } diff --git a/pkg/volume/vsphere_volume/vsphere_volume_block.go b/pkg/volume/vsphere_volume/vsphere_volume_block.go index 8f1ffd3b3cf..5ed08ef32ad 100644 --- a/pkg/volume/vsphere_volume/vsphere_volume_block.go +++ b/pkg/volume/vsphere_volume/vsphere_volume_block.go @@ -29,7 +29,6 @@ import ( "k8s.io/klog" "k8s.io/kubernetes/pkg/util/mount" "k8s.io/kubernetes/pkg/volume" - "k8s.io/kubernetes/pkg/volume/util" "k8s.io/kubernetes/pkg/volume/util/volumepathhandler" utilstrings "k8s.io/utils/strings" ) @@ -138,7 +137,7 @@ func (v vsphereBlockVolumeMapper) SetUpDevice() (string, error) { } func (v vsphereBlockVolumeMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) + return nil } var _ volume.BlockVolumeUnmapper = &vsphereBlockVolumeUnmapper{} From 5a351e3014c06dc75b629f5583e5868d340b68cd Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Mon, 4 Nov 2019 21:07:48 +0000 Subject: [PATCH 04/11] Check and return error first in IsSymlinkExist and IsBindMountExist --- .../volumepathhandler/volume_path_handler.go | 45 ++++++++++--------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/pkg/volume/util/volumepathhandler/volume_path_handler.go b/pkg/volume/util/volumepathhandler/volume_path_handler.go index cf9c23beb68..7a9e05a5bab 100644 --- a/pkg/volume/util/volumepathhandler/volume_path_handler.go +++ b/pkg/volume/util/volumepathhandler/volume_path_handler.go @@ -231,20 +231,20 @@ func (v VolumePathHandler) RemoveMapPath(mapPath string) error { // On other cases, return false with error from Lstat(). func (v VolumePathHandler) IsSymlinkExist(mapPath string) (bool, error) { fi, err := os.Lstat(mapPath) - if err == nil { - // If file exits and it's symbolic link, return true and no error - if fi.Mode()&os.ModeSymlink == os.ModeSymlink { - return true, nil + if err != nil { + // If file doesn't exist, return false and no error + if os.IsNotExist(err) { + return false, nil } - // If file exits but it's not symbolic link, return fale and no error - return false, nil + // Return error from Lstat() + return false, err } - // If file doesn't exist, return false and no error - if os.IsNotExist(err) { - return false, nil + // If file exits and it's symbolic link, return true and no error + if fi.Mode()&os.ModeSymlink == os.ModeSymlink { + return true, nil } - // Return error from Lstat() - return false, err + // If file exits but it's not symbolic link, return fale and no error + return false, nil } // IsBindMountExist returns true if specified file exists and the type is device. @@ -252,20 +252,21 @@ func (v VolumePathHandler) IsSymlinkExist(mapPath string) (bool, error) { // On other cases, return false with error from Lstat(). func (v VolumePathHandler) IsBindMountExist(mapPath string) (bool, error) { fi, err := os.Lstat(mapPath) - if err == nil { - // If file exits and it's device, return true and no error - if fi.Mode()&os.ModeDevice == os.ModeDevice { - return true, nil + if err != nil { + // If file doesn't exist, return false and no error + if os.IsNotExist(err) { + return false, nil } - // If file exits but it's not device, return fale and no error - return false, nil + + // Return error from Lstat() + return false, err } - // If file doesn't exist, return false and no error - if os.IsNotExist(err) { - return false, nil + // If file exits and it's device, return true and no error + if fi.Mode()&os.ModeDevice == os.ModeDevice { + return true, nil } - // Return error from Lstat() - return false, err + // If file exits but it's not device, return fale and no error + return false, nil } // GetDeviceBindMountRefs searches bind mounts under global map path From 7abb704e7b629fa9cec5a55f622b195ccb692933 Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Mon, 4 Nov 2019 22:10:36 +0000 Subject: [PATCH 05/11] Improve comments for volume path hanlder and volume.go --- .../volume_path_handler_linux.go | 28 +++++++++++++++++-- pkg/volume/volume.go | 4 +-- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go b/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go index 272f074d11e..155c492ff41 100644 --- a/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go +++ b/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go @@ -130,10 +130,14 @@ func parseLosetupOutputForDevice(output []byte, path string) (string, error) { } // losetup -j {path} returns device in the format: - // /dev/loop1: [0073]:148662 (/dev/{globalMapPath}/{pod1-UUID}) - // /dev/loop2: [0073]:148662 (/dev/{globalMapPath}/{pod2-UUID}) + // /dev/loop1: [0073]:148662 ({path}) + // /dev/loop2: [0073]:148662 (/dev/sdX) + // + // losetup -j shows all the loop device for the same device that has the same + // major/minor number, by resolving symlink and matching major/minor number. + // Therefore, there will be other path than {path} in output, as shown in above output. s := string(output) - // Find line that exact matches the path + // Find the line that exact matches to the path, or "({path})" var matched string scanner := bufio.NewScanner(strings.NewReader(s)) for scanner.Scan() { @@ -147,6 +151,8 @@ func parseLosetupOutputForDevice(output []byte, path string) (string, error) { } s = matched + // Get device name, or the 0th field of the output separated with ":". + // We don't need 1st field or later to be splitted, so passing 2 to SplitN. device := strings.TrimSpace(strings.SplitN(s, ":", 2)[0]) if len(device) == 0 { return "", errors.New(ErrDeviceNotFound) @@ -181,26 +187,42 @@ func (v VolumePathHandler) FindGlobalMapPathUUIDFromPod(pluginDir, mapPath strin return globalMapPathUUID, nil } +// compareBindMountAndSymlinks returns if global path (bind mount) and +// pod path (symlink) are pointing to the same device. +// If there is an error in checking it returns error. func compareBindMountAndSymlinks(global, pod string) (bool, error) { + // To check if bind mount and symlink are pointing to the same device, + // we need to check if they are pointing to the devices that have same major/minor number. + + // Get the major/minor number for global path devNumGlobal, err := getDeviceMajorMinor(global) if err != nil { return false, err } + + // Get the symlinked device from the pod path devPod, err := os.Readlink(pod) if err != nil { return false, err } + // Get the major/minor number for the symlinked device from the pod path devNumPod, err := getDeviceMajorMinor(devPod) if err != nil { return false, err } klog.V(5).Infof("CompareBindMountAndSymlinks: devNumGlobal %s, devNumPod %s", devNumGlobal, devNumPod) + + // Check if the major/minor number are the same if devNumGlobal == devNumPod { return true, nil } return false, nil } +// getDeviceMajorMinor returns major/minor number for the path with belwo format: +// major:minor (in hex) +// ex) +// fc:10 func getDeviceMajorMinor(path string) (string, error) { args := []string{"-c", "%t:%T", path} cmd := exec.Command(statPath, args...) diff --git a/pkg/volume/volume.go b/pkg/volume/volume.go index cb78d94515a..e125e9b285a 100644 --- a/pkg/volume/volume.go +++ b/pkg/volume/volume.go @@ -41,12 +41,12 @@ type Volume interface { // and pod device map path. type BlockVolume interface { // GetGlobalMapPath returns a global map path which contains - // symbolic links associated to a block device. + // bind mount associated to a block device. // ex. plugins/kubernetes.io/{PluginName}/{DefaultKubeletVolumeDevicesDirName}/{volumePluginDependentPath}/{pod uuid} GetGlobalMapPath(spec *Spec) (string, error) // GetPodDeviceMapPath returns a pod device map path // and name of a symbolic link associated to a block device. - // ex. pods/{podUid}}/{DefaultKubeletVolumeDevicesDirName}/{escapeQualifiedPluginName}/{volumeName} + // ex. pods/{podUid}/{DefaultKubeletVolumeDevicesDirName}/{escapeQualifiedPluginName}/, {volumeName} GetPodDeviceMapPath() (string, string) } From bee6514d798235f7545dab6c1041ff6946352dec Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Mon, 4 Nov 2019 23:00:25 +0000 Subject: [PATCH 06/11] Remove klog for output error instead return err with context --- .../volumepathhandler/volume_path_handler.go | 39 +++++++++---------- .../volume_path_handler_linux.go | 27 ++++++------- 2 files changed, 32 insertions(+), 34 deletions(-) diff --git a/pkg/volume/util/volumepathhandler/volume_path_handler.go b/pkg/volume/util/volumepathhandler/volume_path_handler.go index 7a9e05a5bab..8403872993c 100644 --- a/pkg/volume/util/volumepathhandler/volume_path_handler.go +++ b/pkg/volume/util/volumepathhandler/volume_path_handler.go @@ -82,13 +82,13 @@ func (v VolumePathHandler) MapDevice(devicePath string, mapPath string, linkName // podDeviceMapPath/linkName: pods/{podUid}/{DefaultKubeletVolumeDevicesDirName}/{escapeQualifiedPluginName}/{volumeName} // linkName: {volumeName} if len(devicePath) == 0 { - return fmt.Errorf("Failed to map device to map path. devicePath is empty") + return fmt.Errorf("failed to map device to map path. devicePath is empty") } if len(mapPath) == 0 { - return fmt.Errorf("Failed to map device to map path. mapPath is empty") + return fmt.Errorf("failed to map device to map path. mapPath is empty") } if !filepath.IsAbs(mapPath) { - return fmt.Errorf("The map path should be absolute: map path: %s", mapPath) + return fmt.Errorf("the map path should be absolute: map path: %s", mapPath) } klog.V(5).Infof("MapDevice: devicePath %s", devicePath) klog.V(5).Infof("MapDevice: mapPath %s", mapPath) @@ -97,11 +97,10 @@ func (v VolumePathHandler) MapDevice(devicePath string, mapPath string, linkName // Check and create mapPath _, err := os.Stat(mapPath) if err != nil && !os.IsNotExist(err) { - klog.Errorf("cannot validate map path: %s", mapPath) - return err + return fmt.Errorf("cannot validate map path: %s: %v", mapPath, err) } if err = os.MkdirAll(mapPath, 0750); err != nil { - return fmt.Errorf("Failed to mkdir %s, error %v", mapPath, err) + return fmt.Errorf("failed to mkdir %s: %v", mapPath, err) } if bindMount { @@ -117,17 +116,17 @@ func mapBindMountDevice(v VolumePathHandler, devicePath string, mapPath string, file, err := os.Stat(linkPath) if err != nil { if !os.IsNotExist(err) { - return fmt.Errorf("Failed to stat file %s: %v", linkPath, err) + return fmt.Errorf("failed to stat file %s: %v", linkPath, err) } klog.Warningf("Warning: Path to bind mount %v has not yet been created", linkPath) // Create file newFile, err := os.OpenFile(linkPath, os.O_CREATE|os.O_RDWR, 0750) if err != nil { - return err + return fmt.Errorf("failed to open file %s: %v", linkPath, err) } if err := newFile.Close(); err != nil { - return err + return fmt.Errorf("failed to close file %s: %v", linkPath, err) } } else { // Check if device file @@ -143,8 +142,7 @@ func mapBindMountDevice(v VolumePathHandler, devicePath string, mapPath string, // Bind mount file mounter := &mount.SafeFormatAndMount{Interface: mount.New(""), Exec: mount.NewOSExec()} if err := mounter.Mount(devicePath, linkPath, "" /* fsType */, []string{"bind"}); err != nil { - klog.Errorf("Failed to bind mount devicePath: %s to linkPath %s: %v", devicePath, linkPath, err) - return err + return fmt.Errorf("failed to bind mount devicePath: %s to linkPath %s: %v", devicePath, linkPath, err) } return nil @@ -156,7 +154,7 @@ func mapSymlinkDevice(v VolumePathHandler, devicePath string, mapPath string, li // stale across node reboot. linkPath := filepath.Join(mapPath, string(linkName)) if err := os.Remove(linkPath); err != nil && !os.IsNotExist(err) { - return err + return fmt.Errorf("failed to remove file %s: %v", linkPath, err) } return os.Symlink(devicePath, linkPath) } @@ -164,7 +162,7 @@ func mapSymlinkDevice(v VolumePathHandler, devicePath string, mapPath string, li // UnmapDevice removes a symbolic link associated to block device under specified map path func (v VolumePathHandler) UnmapDevice(mapPath string, linkName string, bindMount bool) error { if len(mapPath) == 0 { - return fmt.Errorf("Failed to unmap device from map path. mapPath is empty") + return fmt.Errorf("failed to unmap device from map path. mapPath is empty") } klog.V(5).Infof("UnmapDevice: mapPath %s", mapPath) klog.V(5).Infof("UnmapDevice: linkName %s", linkName) @@ -189,13 +187,12 @@ func unmapBindMountDevice(v VolumePathHandler, mapPath string, linkName string) // Unmount file mounter := &mount.SafeFormatAndMount{Interface: mount.New(""), Exec: mount.NewOSExec()} if err := mounter.Unmount(linkPath); err != nil { - klog.Errorf("Failed to unmount linkPath %s: %v", linkPath, err) - return err + return fmt.Errorf("failed to unmount linkPath %s: %v", linkPath, err) } // Remove file if err := os.Remove(linkPath); err != nil && !os.IsNotExist(err) { - return err + return fmt.Errorf("failed to remove file %s: %v", linkPath, err) } return nil @@ -216,12 +213,12 @@ func unmapSymlinkDevice(v VolumePathHandler, mapPath string, linkName string) er // RemoveMapPath removes a file or directory on specified map path func (v VolumePathHandler) RemoveMapPath(mapPath string) error { if len(mapPath) == 0 { - return fmt.Errorf("Failed to remove map path. mapPath is empty") + return fmt.Errorf("failed to remove map path. mapPath is empty") } klog.V(5).Infof("RemoveMapPath: mapPath %s", mapPath) err := os.RemoveAll(mapPath) if err != nil && !os.IsNotExist(err) { - return err + return fmt.Errorf("failed to remove directory %s: %v", mapPath, err) } return nil } @@ -237,7 +234,7 @@ func (v VolumePathHandler) IsSymlinkExist(mapPath string) (bool, error) { return false, nil } // Return error from Lstat() - return false, err + return false, fmt.Errorf("failed to Lstat file %s: %v", mapPath, err) } // If file exits and it's symbolic link, return true and no error if fi.Mode()&os.ModeSymlink == os.ModeSymlink { @@ -259,7 +256,7 @@ func (v VolumePathHandler) IsBindMountExist(mapPath string) (bool, error) { } // Return error from Lstat() - return false, err + return false, fmt.Errorf("failed to Lstat file %s: %v", mapPath, err) } // If file exits and it's device, return true and no error if fi.Mode()&os.ModeDevice == os.ModeDevice { @@ -274,7 +271,7 @@ func (v VolumePathHandler) GetDeviceBindMountRefs(devPath string, mapPath string var refs []string files, err := ioutil.ReadDir(mapPath) if err != nil { - return nil, fmt.Errorf("Directory cannot read %v", err) + return nil, fmt.Errorf("directory cannot read %v", err) } for _, file := range files { if file.Mode()&os.ModeDevice != os.ModeDevice { diff --git a/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go b/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go index 155c492ff41..4d2b18e8225 100644 --- a/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go +++ b/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go @@ -36,7 +36,7 @@ import ( func (v VolumePathHandler) AttachFileDevice(path string) (string, error) { blockDevicePath, err := v.GetLoopDevice(path) if err != nil && err.Error() != ErrDeviceNotFound { - return "", err + return "", fmt.Errorf("GetLoopDevice failed for path %s: %v", path, err) } // If no existing loop device for the path, create one @@ -44,7 +44,7 @@ func (v VolumePathHandler) AttachFileDevice(path string) (string, error) { klog.V(4).Infof("Creating device for path: %s", path) blockDevicePath, err = makeLoopDevice(path) if err != nil { - return "", err + return "", fmt.Errorf("makeLoopDevice failed for path %s: %v", path, err) } } return blockDevicePath, nil @@ -56,15 +56,15 @@ func (v VolumePathHandler) DetachFileDevice(path string) error { loopPath, err := v.GetLoopDevice(path) if err != nil { if err.Error() == ErrDeviceNotFound { - klog.Warningf("DetachFileDevice: Couldn't find loopback device which takes file descriptor lock. device path: %q", path) + klog.Warningf("couldn't find loopback device which takes file descriptor lock. Skip detaching device. device path: %q", path) } else { - return err + return fmt.Errorf("GetLoopDevice failed for path %s: %v", path, err) } } else { if len(loopPath) != 0 { err = removeLoopDevice(loopPath) if err != nil { - return err + return fmt.Errorf("removeLoopDevice failed for path %s: %v", path, err) } } } @@ -86,7 +86,7 @@ func (v VolumePathHandler) GetLoopDevice(path string) (string, error) { out, err := cmd.CombinedOutput() if err != nil { klog.V(2).Infof("Failed device discover command for path %s: %v %s", path, err, out) - return "", err + return "", fmt.Errorf("losetup -j %s failed: %v", path, err) } return parseLosetupOutputForDevice(out, path) } @@ -97,7 +97,7 @@ func makeLoopDevice(path string) (string, error) { out, err := cmd.CombinedOutput() if err != nil { klog.V(2).Infof("Failed device create command for path: %s %v %s ", path, err, out) - return "", err + return "", fmt.Errorf("losetup -f --show %s failed: %v", path, err) } // losetup -f --show {path} returns device in the format: @@ -119,7 +119,7 @@ func removeLoopDevice(device string) error { return nil } klog.V(2).Infof("Failed to remove loopback device: %s: %v %s", device, err, out) - return err + return fmt.Errorf("losetup -d %s failed: %v", device, err) } return nil } @@ -162,6 +162,7 @@ func parseLosetupOutputForDevice(output []byte, path string) (string, error) { // FindGlobalMapPathUUIDFromPod finds {pod uuid} bind mount under globalMapPath // corresponding to map path symlink, and then return global map path with pod uuid. +// (See pkg/volume/volume.go for details on a global map path and a pod device map path.) // ex. mapPath symlink: pods/{podUid}}/{DefaultKubeletVolumeDevicesDirName}/{escapeQualifiedPluginName}/{volumeName} -> /dev/sdX // globalMapPath/{pod uuid} bind mount: plugins/kubernetes.io/{PluginName}/{DefaultKubeletVolumeDevicesDirName}/{volumePluginDependentPath}/{pod uuid} -> /dev/sdX func (v VolumePathHandler) FindGlobalMapPathUUIDFromPod(pluginDir, mapPath string, podUID types.UID) (string, error) { @@ -180,7 +181,7 @@ func (v VolumePathHandler) FindGlobalMapPathUUIDFromPod(pluginDir, mapPath strin return nil }) if err != nil { - return "", err + return "", fmt.Errorf("FindGlobalMapPathUUIDFromPod failed: %v", err) } klog.V(5).Infof("FindGlobalMapPathFromPod: globalMapPathUUID %s", globalMapPathUUID) // Return path contains global map path + {pod uuid} @@ -197,18 +198,18 @@ func compareBindMountAndSymlinks(global, pod string) (bool, error) { // Get the major/minor number for global path devNumGlobal, err := getDeviceMajorMinor(global) if err != nil { - return false, err + return false, fmt.Errorf("getDeviceMajorMinor failed for path %s: %v", global, err) } // Get the symlinked device from the pod path devPod, err := os.Readlink(pod) if err != nil { - return false, err + return false, fmt.Errorf("failed to readlink path %s: %v", pod, err) } // Get the major/minor number for the symlinked device from the pod path devNumPod, err := getDeviceMajorMinor(devPod) if err != nil { - return false, err + return false, fmt.Errorf("getDeviceMajorMinor failed for path %s: %v", devPod, err) } klog.V(5).Infof("CompareBindMountAndSymlinks: devNumGlobal %s, devNumPod %s", devNumGlobal, devNumPod) @@ -229,7 +230,7 @@ func getDeviceMajorMinor(path string) (string, error) { out, err := cmd.CombinedOutput() if err != nil { klog.V(2).Infof("Failed to stat path: %s %v %s ", path, err, out) - return "", err + return "", fmt.Errorf("stat -c %%t:%%T %s failed: %v", path, err) } // stat -c "%t:%T" {path} outputs following format(major:minor in hex): From dd945424e15db9b15f8d5f1e6da43d8fc4ff95e0 Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Tue, 12 Nov 2019 15:12:51 +0000 Subject: [PATCH 07/11] Rename IsBindMountExist to IsDeviceBindMountExist --- pkg/volume/testing/testing.go | 2 +- .../util/volumepathhandler/volume_path_handler.go | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/pkg/volume/testing/testing.go b/pkg/volume/testing/testing.go index f93aaf62bd8..db65baff679 100644 --- a/pkg/volume/testing/testing.go +++ b/pkg/volume/testing/testing.go @@ -1158,7 +1158,7 @@ func (fv *FakeVolumePathHandler) IsSymlinkExist(mapPath string) (bool, error) { return true, nil } -func (fv *FakeVolumePathHandler) IsBindMountExist(mapPath string) (bool, error) { +func (fv *FakeVolumePathHandler) IsDeviceBindMountExist(mapPath string) (bool, error) { // nil is success, else error return true, nil } diff --git a/pkg/volume/util/volumepathhandler/volume_path_handler.go b/pkg/volume/util/volumepathhandler/volume_path_handler.go index 8403872993c..8dbe1a43acd 100644 --- a/pkg/volume/util/volumepathhandler/volume_path_handler.go +++ b/pkg/volume/util/volumepathhandler/volume_path_handler.go @@ -45,8 +45,8 @@ type BlockVolumePathHandler interface { RemoveMapPath(mapPath string) error // IsSymlinkExist retruns true if specified symbolic link exists IsSymlinkExist(mapPath string) (bool, error) - // IsBindMountExist retruns true if specified bind mount exists - IsBindMountExist(mapPath string) (bool, error) + // IsDeviceBindMountExist retruns true if specified bind mount exists + IsDeviceBindMountExist(mapPath string) (bool, error) // GetDeviceBindMountRefs searches bind mounts under global map path GetDeviceBindMountRefs(devPath string, mapPath string) ([]string, error) // FindGlobalMapPathUUIDFromPod finds {pod uuid} symbolic link under globalMapPath @@ -119,7 +119,6 @@ func mapBindMountDevice(v VolumePathHandler, devicePath string, mapPath string, return fmt.Errorf("failed to stat file %s: %v", linkPath, err) } - klog.Warningf("Warning: Path to bind mount %v has not yet been created", linkPath) // Create file newFile, err := os.OpenFile(linkPath, os.O_CREATE|os.O_RDWR, 0750) if err != nil { @@ -176,7 +175,7 @@ func (v VolumePathHandler) UnmapDevice(mapPath string, linkName string, bindMoun func unmapBindMountDevice(v VolumePathHandler, mapPath string, linkName string) error { // Check bind mount exists linkPath := filepath.Join(mapPath, string(linkName)) - if isMountExist, checkErr := v.IsBindMountExist(linkPath); checkErr != nil { + if isMountExist, checkErr := v.IsDeviceBindMountExist(linkPath); checkErr != nil { return checkErr } else if !isMountExist { klog.Warningf("Warning: Unmap skipped because bind mount does not exist on the path: %v", linkPath) @@ -244,10 +243,10 @@ func (v VolumePathHandler) IsSymlinkExist(mapPath string) (bool, error) { return false, nil } -// IsBindMountExist returns true if specified file exists and the type is device. +// IsDeviceBindMountExist returns true if specified file exists and the type is device. // If file doesn't exist, or file exists but not device, return false with no error. // On other cases, return false with error from Lstat(). -func (v VolumePathHandler) IsBindMountExist(mapPath string) (bool, error) { +func (v VolumePathHandler) IsDeviceBindMountExist(mapPath string) (bool, error) { fi, err := os.Lstat(mapPath) if err != nil { // If file doesn't exist, return false and no error From a2cbc028f4a2b24c06c15558dfabb89d9f6e42f0 Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Tue, 12 Nov 2019 15:20:47 +0000 Subject: [PATCH 08/11] Remove remaining empty file in unmapBindMountDevice --- .../util/volumepathhandler/volume_path_handler.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/pkg/volume/util/volumepathhandler/volume_path_handler.go b/pkg/volume/util/volumepathhandler/volume_path_handler.go index 8dbe1a43acd..8d00deaf27a 100644 --- a/pkg/volume/util/volumepathhandler/volume_path_handler.go +++ b/pkg/volume/util/volumepathhandler/volume_path_handler.go @@ -179,7 +179,19 @@ func unmapBindMountDevice(v VolumePathHandler, mapPath string, linkName string) return checkErr } else if !isMountExist { klog.Warningf("Warning: Unmap skipped because bind mount does not exist on the path: %v", linkPath) - // TODO: consider deleting empty file if it exists + + // Check if linkPath still exists + if _, err := os.Stat(linkPath); err != nil { + if !os.IsNotExist(err) { + return fmt.Errorf("failed to check if path %s exists: %v", linkPath, err) + } + // linkPath has already been removed + return nil + } + // Remove file + if err := os.Remove(linkPath); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to remove file %s: %v", linkPath, err) + } return nil } From aee875a85582495a5398779070028de1f8def2c9 Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Tue, 12 Nov 2019 15:32:03 +0000 Subject: [PATCH 09/11] Fix error messages in operation_generator.go --- pkg/volume/util/operationexecutor/operation_generator.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index 39b8a176f5d..a48a48372a4 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -1039,7 +1039,7 @@ func (og *operationGenerator) GenerateMapVolumeFunc( blockVolumeMapper.GetGlobalMapPath(volumeToMount.VolumeSpec) if err != nil { // On failure, return error. Caller will log and retry. - return volumeToMount.GenerateError("MapVolume.GetDeviceMountPath failed", err) + return volumeToMount.GenerateError("MapVolume.GetGlobalMapPath failed", err) } if volumeAttacher != nil { // Wait for attachable volumes to finish attaching @@ -1059,7 +1059,7 @@ func (og *operationGenerator) GenerateMapVolumeFunc( pluginDevicePath, mapErr := blockVolumeMapper.SetUpDevice() if mapErr != nil { // On failure, return error. Caller will log and retry. - return volumeToMount.GenerateError("MapVolume.SetUp failed", mapErr) + return volumeToMount.GenerateError("MapVolume.SetUpDevice failed", mapErr) } // if pluginDevicePath is provided, assume attacher may not provide device @@ -1090,14 +1090,14 @@ func (og *operationGenerator) GenerateMapVolumeFunc( mapErr = blockVolumeMapper.MapDevice(devicePath, globalMapPath, volumeMapPath, volName, volumeToMount.Pod.UID) if mapErr != nil { // On failure, return error. Caller will log and retry. - return volumeToMount.GenerateError("MapVolume.MapPodDevice failed", mapErr) + return volumeToMount.GenerateError("MapVolume.MapDevice failed", mapErr) } // Execute common map mapErr = ioutil.MapBlockVolume(og.blkUtil, devicePath, globalMapPath, volumeMapPath, volName, volumeToMount.Pod.UID) if mapErr != nil { // On failure, return error. Caller will log and retry. - return volumeToMount.GenerateError("MapVolume.MapDevice failed", mapErr) + return volumeToMount.GenerateError("MapVolume.MapBlockVolume failed", mapErr) } // Update actual state of world to reflect volume is globally mounted From 8a09460c2f7ba8f6acd8a6fb7603ed3ac4805eb6 Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Tue, 12 Nov 2019 15:50:16 +0000 Subject: [PATCH 10/11] Change getDeviceMajorMinor to use unix.Stat --- pkg/volume/util/volumepathhandler/BUILD | 10 +++++++- .../volume_path_handler_linux.go | 24 +++++++++---------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/pkg/volume/util/volumepathhandler/BUILD b/pkg/volume/util/volumepathhandler/BUILD index d1d562a8108..df8126ee228 100644 --- a/pkg/volume/util/volumepathhandler/BUILD +++ b/pkg/volume/util/volumepathhandler/BUILD @@ -13,7 +13,15 @@ go_library( "//pkg/util/mount:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//vendor/k8s.io/klog:go_default_library", - ], + ] + select({ + "@io_bazel_rules_go//go/platform:android": [ + "//vendor/golang.org/x/sys/unix:go_default_library", + ], + "@io_bazel_rules_go//go/platform:linux": [ + "//vendor/golang.org/x/sys/unix:go_default_library", + ], + "//conditions:default": [], + }), ) filegroup( diff --git a/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go b/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go index 4d2b18e8225..66cc7c73f08 100644 --- a/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go +++ b/pkg/volume/util/volumepathhandler/volume_path_handler_linux.go @@ -27,6 +27,8 @@ import ( "path/filepath" "strings" + "golang.org/x/sys/unix" + "k8s.io/apimachinery/pkg/types" "k8s.io/klog" ) @@ -220,24 +222,20 @@ func compareBindMountAndSymlinks(global, pod string) (bool, error) { return false, nil } -// getDeviceMajorMinor returns major/minor number for the path with belwo format: +// getDeviceMajorMinor returns major/minor number for the path with below format: // major:minor (in hex) // ex) // fc:10 func getDeviceMajorMinor(path string) (string, error) { - args := []string{"-c", "%t:%T", path} - cmd := exec.Command(statPath, args...) - out, err := cmd.CombinedOutput() - if err != nil { - klog.V(2).Infof("Failed to stat path: %s %v %s ", path, err, out) - return "", fmt.Errorf("stat -c %%t:%%T %s failed: %v", path, err) + var stat unix.Stat_t + + if err := unix.Stat(path, &stat); err != nil { + return "", fmt.Errorf("failed to stat path %s: %v", path, err) } - // stat -c "%t:%T" {path} outputs following format(major:minor in hex): - // fc:10 - if len(out) == 0 { - return "", errors.New(ErrDeviceNotFound) - } + devNumber := uint64(stat.Rdev) + major := unix.Major(devNumber) + minor := unix.Minor(devNumber) - return strings.TrimSpace(string(out)), nil + return fmt.Sprintf("%x:%x", major, minor), nil } From 560d9c56eb2082038f932d415c32985e0cce334a Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Thu, 14 Nov 2019 15:57:53 +0000 Subject: [PATCH 11/11] Change mount.NewOSExec to utilexec.New --- pkg/volume/util/volumepathhandler/BUILD | 1 + pkg/volume/util/volumepathhandler/volume_path_handler.go | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/volume/util/volumepathhandler/BUILD b/pkg/volume/util/volumepathhandler/BUILD index df8126ee228..23b791e6c30 100644 --- a/pkg/volume/util/volumepathhandler/BUILD +++ b/pkg/volume/util/volumepathhandler/BUILD @@ -13,6 +13,7 @@ go_library( "//pkg/util/mount:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//vendor/k8s.io/klog:go_default_library", + "//vendor/k8s.io/utils/exec:go_default_library", ] + select({ "@io_bazel_rules_go//go/platform:android": [ "//vendor/golang.org/x/sys/unix:go_default_library", diff --git a/pkg/volume/util/volumepathhandler/volume_path_handler.go b/pkg/volume/util/volumepathhandler/volume_path_handler.go index 8d00deaf27a..ad192f437c8 100644 --- a/pkg/volume/util/volumepathhandler/volume_path_handler.go +++ b/pkg/volume/util/volumepathhandler/volume_path_handler.go @@ -24,6 +24,7 @@ import ( "k8s.io/klog" "k8s.io/kubernetes/pkg/util/mount" + utilexec "k8s.io/utils/exec" "k8s.io/apimachinery/pkg/types" ) @@ -139,7 +140,7 @@ func mapBindMountDevice(v VolumePathHandler, devicePath string, mapPath string, } // Bind mount file - mounter := &mount.SafeFormatAndMount{Interface: mount.New(""), Exec: mount.NewOSExec()} + mounter := &mount.SafeFormatAndMount{Interface: mount.New(""), Exec: utilexec.New()} if err := mounter.Mount(devicePath, linkPath, "" /* fsType */, []string{"bind"}); err != nil { return fmt.Errorf("failed to bind mount devicePath: %s to linkPath %s: %v", devicePath, linkPath, err) } @@ -196,7 +197,7 @@ func unmapBindMountDevice(v VolumePathHandler, mapPath string, linkName string) } // Unmount file - mounter := &mount.SafeFormatAndMount{Interface: mount.New(""), Exec: mount.NewOSExec()} + mounter := &mount.SafeFormatAndMount{Interface: mount.New(""), Exec: utilexec.New()} if err := mounter.Unmount(linkPath); err != nil { return fmt.Errorf("failed to unmount linkPath %s: %v", linkPath, err) }