From 8a159d725345264e5a01182d5511eefe1d02f90f Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Fri, 1 Nov 2019 19:32:23 +0000 Subject: [PATCH] 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{}