From 2ecdc5e8d19ee7a037b86e90a4d3de56cb94b938 Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Fri, 1 Nov 2019 19:31:17 +0000 Subject: [PATCH] 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.") +}