From 3569287993ed5b04e85473a3d29c2a6cb441498f Mon Sep 17 00:00:00 2001 From: Vladimir Vivien Date: Mon, 21 May 2018 08:44:17 -0400 Subject: [PATCH] Refactor of GenerateMapDeviceFunc to delegate Map call to volume plugin. --- pkg/kubelet/kubelet_volumes_test.go | 5 ++ .../reconciler/reconciler_test.go | 32 +++--------- pkg/volume/aws_ebs/aws_ebs_block.go | 5 ++ pkg/volume/azure_dd/azure_dd_block.go | 5 ++ pkg/volume/fc/fc.go | 4 ++ pkg/volume/gce_pd/gce_pd_block.go | 5 ++ pkg/volume/iscsi/iscsi.go | 4 ++ pkg/volume/local/local.go | 4 ++ pkg/volume/rbd/rbd.go | 4 ++ pkg/volume/testing/testing.go | 34 +++++++++++++ pkg/volume/util/BUILD | 1 + .../operationexecutor/operation_generator.go | 50 +++++++++---------- pkg/volume/util/util.go | 28 +++++++++++ pkg/volume/volume.go | 3 ++ 14 files changed, 133 insertions(+), 51 deletions(-) diff --git a/pkg/kubelet/kubelet_volumes_test.go b/pkg/kubelet/kubelet_volumes_test.go index efdcfcc3bf..a82aa06163 100644 --- a/pkg/kubelet/kubelet_volumes_test.go +++ b/pkg/kubelet/kubelet_volumes_test.go @@ -463,6 +463,11 @@ func (f *stubBlockVolume) GetPodDeviceMapPath() (string, string) { func (f *stubBlockVolume) SetUpDevice() (string, error) { return "", nil } + +func (f stubBlockVolume) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { + return nil +} + func (f *stubBlockVolume) TearDownDevice(mapPath string, devicePath string) error { return nil } diff --git a/pkg/kubelet/volumemanager/reconciler/reconciler_test.go b/pkg/kubelet/volumemanager/reconciler/reconciler_test.go index 31a4875ecd..38eaba2f03 100644 --- a/pkg/kubelet/volumemanager/reconciler/reconciler_test.go +++ b/pkg/kubelet/volumemanager/reconciler/reconciler_test.go @@ -509,12 +509,8 @@ func Test_Run_Positive_VolumeAttachAndMap(t *testing.T) { 1 /* expectedAttachCallCount */, fakePlugin)) assert.NoError(t, volumetesting.VerifyWaitForAttachCallCount( 1 /* expectedWaitForAttachCallCount */, fakePlugin)) - assert.NoError(t, volumetesting.VerifyGetGlobalMapPathCallCount( - 1 /* expectedGetGlobalMapPathCallCount */, fakePlugin)) - assert.NoError(t, volumetesting.VerifyGetPodDeviceMapPathCallCount( - 1 /* expectedPodDeviceMapPathCallCount */, fakePlugin)) - assert.NoError(t, volumetesting.VerifySetUpDeviceCallCount( - 1 /* expectedSetUpDeviceCallCount */, fakePlugin)) + assert.NoError(t, volumetesting.VerifyGetMapDeviceCallCount( + 1 /* expectedGetMapDeviceCallCount */, fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroTearDownDeviceCallCount(fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroDetachCallCount(fakePlugin)) @@ -601,12 +597,8 @@ func Test_Run_Positive_BlockVolumeMapControllerAttachEnabled(t *testing.T) { assert.NoError(t, volumetesting.VerifyZeroAttachCalls(fakePlugin)) assert.NoError(t, volumetesting.VerifyWaitForAttachCallCount( 1 /* expectedWaitForAttachCallCount */, fakePlugin)) - assert.NoError(t, volumetesting.VerifyGetGlobalMapPathCallCount( - 1 /* expectedGetGlobalMapPathCallCount */, fakePlugin)) - assert.NoError(t, volumetesting.VerifyGetPodDeviceMapPathCallCount( - 1 /* expectedPodDeviceMapPathCallCount */, fakePlugin)) - assert.NoError(t, volumetesting.VerifySetUpDeviceCallCount( - 1 /* expectedSetUpCallCount */, fakePlugin)) + assert.NoError(t, volumetesting.VerifyGetMapDeviceCallCount( + 1 /* expectedGetMapDeviceCallCount */, fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroTearDownDeviceCallCount(fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroDetachCallCount(fakePlugin)) @@ -692,12 +684,8 @@ func Test_Run_Positive_BlockVolumeAttachMapUnmapDetach(t *testing.T) { 1 /* expectedAttachCallCount */, fakePlugin)) assert.NoError(t, volumetesting.VerifyWaitForAttachCallCount( 1 /* expectedWaitForAttachCallCount */, fakePlugin)) - assert.NoError(t, volumetesting.VerifyGetGlobalMapPathCallCount( - 1 /* expectedGetGlobalMapPathCallCount */, fakePlugin)) - assert.NoError(t, volumetesting.VerifyGetPodDeviceMapPathCallCount( - 1 /* expectedPodDeviceMapPathCallCount */, fakePlugin)) - assert.NoError(t, volumetesting.VerifySetUpDeviceCallCount( - 1 /* expectedSetUpCallCount */, fakePlugin)) + assert.NoError(t, volumetesting.VerifyGetMapDeviceCallCount( + 1 /* expectedGetMapDeviceCallCount */, fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroTearDownDeviceCallCount(fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroDetachCallCount(fakePlugin)) @@ -796,12 +784,8 @@ func Test_Run_Positive_VolumeUnmapControllerAttachEnabled(t *testing.T) { assert.NoError(t, volumetesting.VerifyZeroAttachCalls(fakePlugin)) assert.NoError(t, volumetesting.VerifyWaitForAttachCallCount( 1 /* expectedWaitForAttachCallCount */, fakePlugin)) - assert.NoError(t, volumetesting.VerifyGetGlobalMapPathCallCount( - 1 /* expectedGetGlobalMapPathCallCount */, fakePlugin)) - assert.NoError(t, volumetesting.VerifyGetPodDeviceMapPathCallCount( - 1 /* expectedPodDeviceMapPathCallCount */, fakePlugin)) - assert.NoError(t, volumetesting.VerifySetUpDeviceCallCount( - 1 /* expectedSetUpCallCount */, fakePlugin)) + assert.NoError(t, volumetesting.VerifyGetMapDeviceCallCount( + 1 /* expectedGetMapDeviceCallCount */, fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroTearDownDeviceCallCount(fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroDetachCallCount(fakePlugin)) diff --git a/pkg/volume/aws_ebs/aws_ebs_block.go b/pkg/volume/aws_ebs/aws_ebs_block.go index 37e748707c..5eaefb23f6 100644 --- a/pkg/volume/aws_ebs/aws_ebs_block.go +++ b/pkg/volume/aws_ebs/aws_ebs_block.go @@ -29,6 +29,7 @@ import ( "k8s.io/kubernetes/pkg/util/mount" kstrings "k8s.io/kubernetes/pkg/util/strings" "k8s.io/kubernetes/pkg/volume" + "k8s.io/kubernetes/pkg/volume/util" "k8s.io/kubernetes/pkg/volume/util/volumepathhandler" ) @@ -155,6 +156,10 @@ func (b *awsElasticBlockStoreMapper) SetUpDevice() (string, error) { return "", nil } +func (b *awsElasticBlockStoreMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { + return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) +} + // GetGlobalMapPath returns global map path and error // path: plugins/kubernetes.io/{PluginName}/volumeDevices/volumeID // plugins/kubernetes.io/aws-ebs/volumeDevices/vol-XXXXXX diff --git a/pkg/volume/azure_dd/azure_dd_block.go b/pkg/volume/azure_dd/azure_dd_block.go index 9ded780ef4..af1d195047 100644 --- a/pkg/volume/azure_dd/azure_dd_block.go +++ b/pkg/volume/azure_dd/azure_dd_block.go @@ -27,6 +27,7 @@ import ( "k8s.io/kubernetes/pkg/util/mount" kstrings "k8s.io/kubernetes/pkg/util/strings" "k8s.io/kubernetes/pkg/volume" + "k8s.io/kubernetes/pkg/volume/util" "k8s.io/kubernetes/pkg/volume/util/volumepathhandler" ) @@ -137,6 +138,10 @@ func (b *azureDataDiskMapper) SetUpDevice() (string, error) { return "", nil } +func (b *azureDataDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { + return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) +} + // GetGlobalMapPath returns global map path and error // path: plugins/kubernetes.io/{PluginName}/volumeDevices/volumeID // plugins/kubernetes.io/azure-disk/volumeDevices/vol-XXXXXX diff --git a/pkg/volume/fc/fc.go b/pkg/volume/fc/fc.go index fb7a570155..4160fe055c 100644 --- a/pkg/volume/fc/fc.go +++ b/pkg/volume/fc/fc.go @@ -443,6 +443,10 @@ func (b *fcDiskMapper) SetUpDevice() (string, error) { return "", nil } +func (b *fcDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { + return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) +} + type fcDiskUnmapper struct { *fcDisk deviceUtil util.DeviceUtil diff --git a/pkg/volume/gce_pd/gce_pd_block.go b/pkg/volume/gce_pd/gce_pd_block.go index f4398d13b5..24dcfce731 100644 --- a/pkg/volume/gce_pd/gce_pd_block.go +++ b/pkg/volume/gce_pd/gce_pd_block.go @@ -28,6 +28,7 @@ import ( "k8s.io/kubernetes/pkg/util/mount" kstrings "k8s.io/kubernetes/pkg/util/strings" "k8s.io/kubernetes/pkg/volume" + "k8s.io/kubernetes/pkg/volume/util" "k8s.io/kubernetes/pkg/volume/util/volumepathhandler" ) @@ -151,6 +152,10 @@ func (b *gcePersistentDiskMapper) SetUpDevice() (string, error) { return "", nil } +func (b *gcePersistentDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { + return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) +} + // GetGlobalMapPath returns global map path and error // path: plugins/kubernetes.io/{PluginName}/volumeDevices/pdName func (pd *gcePersistentDisk) GetGlobalMapPath(spec *volume.Spec) (string, error) { diff --git a/pkg/volume/iscsi/iscsi.go b/pkg/volume/iscsi/iscsi.go index 22f3fe81c6..687f2cc6a3 100644 --- a/pkg/volume/iscsi/iscsi.go +++ b/pkg/volume/iscsi/iscsi.go @@ -367,6 +367,10 @@ func (b *iscsiDiskMapper) SetUpDevice() (string, error) { return "", nil } +func (b *iscsiDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { + return ioutil.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) +} + type iscsiDiskUnmapper struct { *iscsiDisk exec mount.Exec diff --git a/pkg/volume/local/local.go b/pkg/volume/local/local.go index 618adea56b..854c6d9682 100644 --- a/pkg/volume/local/local.go +++ b/pkg/volume/local/local.go @@ -390,6 +390,10 @@ func (m *localVolumeMapper) SetUpDevice() (string, error) { return globalPath, nil } +func (m *localVolumeMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { + return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) +} + // localVolumeUnmapper implements the BlockVolumeUnmapper interface for local volumes. type localVolumeUnmapper struct { *localVolume diff --git a/pkg/volume/rbd/rbd.go b/pkg/volume/rbd/rbd.go index 283403f273..92e3d559f8 100644 --- a/pkg/volume/rbd/rbd.go +++ b/pkg/volume/rbd/rbd.go @@ -878,6 +878,10 @@ func (rbd *rbdDiskMapper) SetUpDevice() (string, error) { return "", nil } +func (rbd *rbdDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { + return volutil.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID) +} + func (rbd *rbd) rbdGlobalMapPath(spec *volume.Spec) (string, error) { mon, err := getVolumeSourceMonitors(spec) if err != nil { diff --git a/pkg/volume/testing/testing.go b/pkg/volume/testing/testing.go index fb805812d6..8ba41ed009 100644 --- a/pkg/volume/testing/testing.go +++ b/pkg/volume/testing/testing.go @@ -512,6 +512,7 @@ type FakeVolume struct { GetDeviceMountPathCallCount int SetUpDeviceCallCount int TearDownDeviceCallCount int + MapDeviceCallCount int GlobalMapPathCallCount int PodDeviceMapPathCallCount int } @@ -642,6 +643,21 @@ func (fv *FakeVolume) GetTearDownDeviceCallCount() int { return fv.TearDownDeviceCallCount } +// Block volume support +func (fv *FakeVolume) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, pod types.UID) error { + fv.Lock() + defer fv.Unlock() + fv.MapDeviceCallCount++ + return nil +} + +// Block volume support +func (fv *FakeVolume) GetMapDeviceCallCount() int { + fv.RLock() + defer fv.RUnlock() + return fv.MapDeviceCallCount +} + func (fv *FakeVolume) Attach(spec *Spec, nodeName types.NodeName) (string, error) { fv.Lock() defer fv.Unlock() @@ -1121,6 +1137,24 @@ func VerifyGetPodDeviceMapPathCallCount( expectedPodDeviceMapPathCallCount) } +// VerifyGetMapDeviceCallCount ensures that at least one of the Mappers for this +// plugin has the expectedMapDeviceCallCount number of calls. Otherwise it +// returns an error. +func VerifyGetMapDeviceCallCount( + expectedMapDeviceCallCount int, + fakeVolumePlugin *FakeVolumePlugin) error { + for _, mapper := range fakeVolumePlugin.GetBlockVolumeMapper() { + actualCallCount := mapper.GetMapDeviceCallCount() + if actualCallCount >= expectedMapDeviceCallCount { + return nil + } + } + + return fmt.Errorf( + "No Mapper have expected MapdDeviceCallCount. Expected: <%v>.", + expectedMapDeviceCallCount) +} + // GetTestVolumePluginMgr creates, initializes, and returns a test volume plugin // manager and fake volume plugin using a fake volume host. func GetTestVolumePluginMgr( diff --git a/pkg/volume/util/BUILD b/pkg/volume/util/BUILD index 1a74224fb8..08d5c3ef03 100644 --- a/pkg/volume/util/BUILD +++ b/pkg/volume/util/BUILD @@ -60,6 +60,7 @@ go_library( "//pkg/util/mount:go_default_library", "//pkg/volume:go_default_library", "//pkg/volume/util/types:go_default_library", + "//pkg/volume/util/volumepathhandler:go_default_library", "//vendor/github.com/golang/glog:go_default_library", "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index 582d74b685..51f321ae9d 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -856,37 +856,19 @@ func (og *operationGenerator) GenerateMapVolumeFunc( // On failure, return error. Caller will log and retry. return volumeToMount.GenerateError("MapVolume.SetUp failed", mapErr) } - // Update devicePath for none attachable plugin case + + // if pluginDevicePath is provided, assume attacher may not provide device + // or attachment flow uses SetupDevice to get device path + if len(pluginDevicePath) != 0 { + devicePath = pluginDevicePath + } if len(devicePath) == 0 { - if len(pluginDevicePath) != 0 { - devicePath = pluginDevicePath - } else { - return volumeToMount.GenerateError("MapVolume failed", fmt.Errorf("Device path of the volume is empty")) - } - } - // Update actual state of world to reflect volume is globally mounted - markDeviceMappedErr := actualStateOfWorld.MarkDeviceAsMounted( - volumeToMount.VolumeName, devicePath, globalMapPath) - if markDeviceMappedErr != nil { - // On failure, return error. Caller will log and retry. - return volumeToMount.GenerateError("MapVolume.MarkDeviceAsMounted failed", markDeviceMappedErr) + return volumeToMount.GenerateError("MapVolume failed", fmt.Errorf("Device path of the volume is empty")) } - mapErr = og.blkUtil.MapDevice(devicePath, globalMapPath, string(volumeToMount.Pod.UID)) - if mapErr != nil { - // On failure, return error. Caller will log and retry. - return volumeToMount.GenerateError("MapVolume.MapDevice failed", mapErr) - } - - // Device mapping for global map path succeeded - simpleMsg, detailedMsg := volumeToMount.GenerateMsg("MapVolume.MapDevice succeeded", fmt.Sprintf("globalMapPath %q", globalMapPath)) - verbosity := glog.Level(4) - og.recorder.Eventf(volumeToMount.Pod, v1.EventTypeNormal, kevents.SuccessfulMountVolume, simpleMsg) - glog.V(verbosity).Infof(detailedMsg) - - // Map device to pod device map path under the given pod directory using symbolic link + // Map device to global and pod device map path volumeMapPath, volName := blockVolumeMapper.GetPodDeviceMapPath() - mapErr = og.blkUtil.MapDevice(devicePath, volumeMapPath, volName) + 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) @@ -901,6 +883,20 @@ func (og *operationGenerator) GenerateMapVolumeFunc( return volumeToMount.GenerateError("MapVolume.AttachFileDevice failed", err) } + // Update actual state of world to reflect volume is globally mounted + markDeviceMappedErr := actualStateOfWorld.MarkDeviceAsMounted( + volumeToMount.VolumeName, devicePath, globalMapPath) + if markDeviceMappedErr != nil { + // On failure, return error. Caller will log and retry. + return volumeToMount.GenerateError("MapVolume.MarkDeviceAsMounted failed", markDeviceMappedErr) + } + + // Device mapping for global map path succeeded + simpleMsg, detailedMsg := volumeToMount.GenerateMsg("MapVolume.MapDevice succeeded", fmt.Sprintf("globalMapPath %q", globalMapPath)) + verbosity := glog.Level(4) + og.recorder.Eventf(volumeToMount.Pod, v1.EventTypeNormal, kevents.SuccessfulMountVolume, simpleMsg) + glog.V(verbosity).Infof(detailedMsg) + // Device mapping for pod device map path succeeded simpleMsg, detailedMsg = volumeToMount.GenerateMsg("MapVolume.MapDevice succeeded", fmt.Sprintf("volumeMapPath %q", volumeMapPath)) verbosity = glog.Level(1) diff --git a/pkg/volume/util/util.go b/pkg/volume/util/util.go index 18956ef89b..1e7b709340 100644 --- a/pkg/volume/util/util.go +++ b/pkg/volume/util/util.go @@ -50,6 +50,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" utypes "k8s.io/apimachinery/pkg/types" "k8s.io/kubernetes/pkg/volume/util/types" + "k8s.io/kubernetes/pkg/volume/util/volumepathhandler" ) const ( @@ -741,3 +742,30 @@ func MakeAbsolutePath(goos, path string) string { // Otherwise, add 'c:\' return "c:\\" + path } + +// MapBlockVolume is a utility function to provide a common way of mounting +// 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( + devicePath, + globalMapPath, + podVolumeMapPath, + volumeMapName string, + podUID utypes.UID, +) error { + blkUtil := volumepathhandler.NewBlockVolumePathHandler() + + // map devicePath to global node path + mapErr := blkUtil.MapDevice(devicePath, globalMapPath, string(podUID)) + if mapErr != nil { + return mapErr + } + + // map devicePath to pod volume path + mapErr = blkUtil.MapDevice(devicePath, podVolumeMapPath, volumeMapName) + if mapErr != nil { + return mapErr + } + + return nil +} diff --git a/pkg/volume/volume.go b/pkg/volume/volume.go index 161d84faf4..9d24875716 100644 --- a/pkg/volume/volume.go +++ b/pkg/volume/volume.go @@ -162,6 +162,9 @@ type BlockVolumeMapper interface { // at attacher.Attach() and attacher.WaitForAttach(). // This may be called more than once, so implementations must be idempotent. SetUpDevice() (string, error) + + // Map maps the block device path for the specified spec and pod. + MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error } // BlockVolumeUnmapper interface provides methods to cleanup/unmap the volumes.