From 68be3947b8f0b1dd87240c672fe73e86533d39a0 Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Fri, 1 Nov 2019 19:31:56 +0000 Subject: [PATCH] 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) {