From f363a03f0bb606d810f16262bfa2994742f74254 Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Mon, 4 Nov 2019 20:01:37 +0000 Subject: [PATCH 1/2] Refactor BlockVolumeMapper and BlockVolumeUnmapper interface - Rename MapDevice to MapPodDevice in BlockVolumeMapper - Add UnmapPodDevice in BlockVolumeUnmapper (This will be used by csi driver later) - Add CustomBlockVolumeMapper and CustomBlockVolumeUnmapper interface - Move SetUpDevice and MapPodDevice to CustomBlockVolumeMapper - Move TearDownDevice and UnmapPodDevice to CustomBlockVolumeUnmapper - Implement CustomBlockVolumeMapper only in local and csi plugin - Implement CustomBlockVolumeUnmapper only in fc, iscsi, rbd, and csi plugin - Change MapPodDevice to return path and SetUpDevice not to return path --- pkg/kubelet/kubelet_volumes_test.go | 6 +- .../reconciler/reconciler_test.go | 8 +- pkg/volume/awsebs/aws_ebs_block.go | 12 --- pkg/volume/azure_dd/azure_dd_block.go | 12 --- pkg/volume/cinder/cinder_block.go | 12 --- pkg/volume/csi/csi_block.go | 35 +++++--- pkg/volume/csi/csi_block_test.go | 45 ++++------ pkg/volume/fc/fc.go | 13 ++- pkg/volume/gcepd/gce_pd_block.go | 12 --- pkg/volume/iscsi/iscsi.go | 13 ++- pkg/volume/local/local.go | 22 ++--- pkg/volume/local/local_test.go | 24 +++-- pkg/volume/rbd/rbd.go | 13 ++- pkg/volume/testing/testing.go | 46 ++++++---- .../operationexecutor/operation_generator.go | 88 +++++++++++-------- .../volumepathhandler/volume_path_handler.go | 4 +- pkg/volume/volume.go | 48 +++++----- .../vsphere_volume/vsphere_volume_block.go | 12 --- 18 files changed, 202 insertions(+), 223 deletions(-) diff --git a/pkg/kubelet/kubelet_volumes_test.go b/pkg/kubelet/kubelet_volumes_test.go index 6527e63d195..bfc5c8b7c22 100644 --- a/pkg/kubelet/kubelet_volumes_test.go +++ b/pkg/kubelet/kubelet_volumes_test.go @@ -555,10 +555,14 @@ func (f *stubBlockVolume) SetUpDevice() (string, error) { return "", nil } -func (f stubBlockVolume) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { +func (f stubBlockVolume) MapPodDevice() error { return nil } func (f *stubBlockVolume) TearDownDevice(mapPath string, devicePath string) error { return nil } + +func (f *stubBlockVolume) UnmapPodDevice() error { + return nil +} diff --git a/pkg/kubelet/volumemanager/reconciler/reconciler_test.go b/pkg/kubelet/volumemanager/reconciler/reconciler_test.go index 54826ea7a23..255a073e11c 100644 --- a/pkg/kubelet/volumemanager/reconciler/reconciler_test.go +++ b/pkg/kubelet/volumemanager/reconciler/reconciler_test.go @@ -527,7 +527,7 @@ func Test_Run_Positive_VolumeAttachAndMap(t *testing.T) { 1 /* expectedAttachCallCount */, fakePlugin)) assert.NoError(t, volumetesting.VerifyWaitForAttachCallCount( 1 /* expectedWaitForAttachCallCount */, fakePlugin)) - assert.NoError(t, volumetesting.VerifyGetMapDeviceCallCount( + assert.NoError(t, volumetesting.VerifyGetMapPodDeviceCallCount( 1 /* expectedGetMapDeviceCallCount */, fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroTearDownDeviceCallCount(fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroDetachCallCount(fakePlugin)) @@ -631,7 +631,7 @@ 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.VerifyGetMapDeviceCallCount( + assert.NoError(t, volumetesting.VerifyGetMapPodDeviceCallCount( 1 /* expectedGetMapDeviceCallCount */, fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroTearDownDeviceCallCount(fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroDetachCallCount(fakePlugin)) @@ -731,7 +731,7 @@ func Test_Run_Positive_BlockVolumeAttachMapUnmapDetach(t *testing.T) { 1 /* expectedAttachCallCount */, fakePlugin)) assert.NoError(t, volumetesting.VerifyWaitForAttachCallCount( 1 /* expectedWaitForAttachCallCount */, fakePlugin)) - assert.NoError(t, volumetesting.VerifyGetMapDeviceCallCount( + assert.NoError(t, volumetesting.VerifyGetMapPodDeviceCallCount( 1 /* expectedGetMapDeviceCallCount */, fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroTearDownDeviceCallCount(fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroDetachCallCount(fakePlugin)) @@ -847,7 +847,7 @@ 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.VerifyGetMapDeviceCallCount( + assert.NoError(t, volumetesting.VerifyGetMapPodDeviceCallCount( 1 /* expectedGetMapDeviceCallCount */, fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroTearDownDeviceCallCount(fakePlugin)) assert.NoError(t, volumetesting.VerifyZeroDetachCallCount(fakePlugin)) diff --git a/pkg/volume/awsebs/aws_ebs_block.go b/pkg/volume/awsebs/aws_ebs_block.go index 3600a085dcb..6adfc7b9305 100644 --- a/pkg/volume/awsebs/aws_ebs_block.go +++ b/pkg/volume/awsebs/aws_ebs_block.go @@ -125,10 +125,6 @@ func (plugin *awsElasticBlockStorePlugin) newUnmapperInternal(volName string, po }}, nil } -func (c *awsElasticBlockStoreUnmapper) TearDownDevice(mapPath, devicePath string) error { - return nil -} - type awsElasticBlockStoreUnmapper struct { *awsElasticBlockStore } @@ -142,14 +138,6 @@ type awsElasticBlockStoreMapper struct { var _ volume.BlockVolumeMapper = &awsElasticBlockStoreMapper{} -func (b *awsElasticBlockStoreMapper) SetUpDevice() (string, error) { - return "", nil -} - -func (b *awsElasticBlockStoreMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return nil -} - // 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 41c00e9319f..b9aaa36316a 100644 --- a/pkg/volume/azure_dd/azure_dd_block.go +++ b/pkg/volume/azure_dd/azure_dd_block.go @@ -118,10 +118,6 @@ func (plugin *azureDataDiskPlugin) newUnmapperInternal(volName string, podUID ty return &azureDataDiskUnmapper{dataDisk: disk}, nil } -func (c *azureDataDiskUnmapper) TearDownDevice(mapPath, devicePath string) error { - return nil -} - type azureDataDiskUnmapper struct { *dataDisk } @@ -135,14 +131,6 @@ type azureDataDiskMapper struct { var _ volume.BlockVolumeMapper = &azureDataDiskMapper{} -func (b *azureDataDiskMapper) SetUpDevice() (string, error) { - return "", nil -} - -func (b *azureDataDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return nil -} - // 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/cinder/cinder_block.go b/pkg/volume/cinder/cinder_block.go index d83637459f9..de53b985d89 100644 --- a/pkg/volume/cinder/cinder_block.go +++ b/pkg/volume/cinder/cinder_block.go @@ -128,10 +128,6 @@ func (plugin *cinderPlugin) newUnmapperInternal(volName string, podUID types.UID }}, nil } -func (c *cinderPluginUnmapper) TearDownDevice(mapPath, devicePath string) error { - return nil -} - type cinderPluginUnmapper struct { *cinderVolume } @@ -145,14 +141,6 @@ type cinderVolumeMapper struct { var _ volume.BlockVolumeMapper = &cinderVolumeMapper{} -func (b *cinderVolumeMapper) SetUpDevice() (string, error) { - return "", nil -} - -func (b *cinderVolumeMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return nil -} - // GetGlobalMapPath returns global map path and error // path: plugins/kubernetes.io/{PluginName}/volumeDevices/volumeID // plugins/kubernetes.io/cinder/volumeDevices/vol-XXXXXX diff --git a/pkg/volume/csi/csi_block.go b/pkg/volume/csi/csi_block.go index 56aa2dbbf57..2570c4c9a77 100644 --- a/pkg/volume/csi/csi_block.go +++ b/pkg/volume/csi/csi_block.go @@ -48,6 +48,7 @@ type csiBlockMapper struct { } var _ volume.BlockVolumeMapper = &csiBlockMapper{} +var _ volume.CustomBlockVolumeMapper = &csiBlockMapper{} // GetGlobalMapPath returns a global map path (on the node) to a device file which will be symlinked to // Example: plugins/kubernetes.io/csi/volumeDevices/{pvname}/dev @@ -203,26 +204,26 @@ func (m *csiBlockMapper) publishVolumeForBlock( } // SetUpDevice ensures the device is attached returns path where the device is located. -func (m *csiBlockMapper) SetUpDevice() (string, error) { +func (m *csiBlockMapper) SetUpDevice() error { if !m.plugin.blockEnabled { - return "", errors.New("CSIBlockVolume feature not enabled") + return errors.New("CSIBlockVolume feature not enabled") } klog.V(4).Infof(log("blockMapper.SetUpDevice called")) // Get csiSource from spec if m.spec == nil { - return "", errors.New(log("blockMapper.SetUpDevice spec is nil")) + return errors.New(log("blockMapper.SetUpDevice spec is nil")) } csiSource, err := getCSISourceFromSpec(m.spec) if err != nil { - return "", errors.New(log("blockMapper.SetUpDevice failed to get CSI persistent source: %v", err)) + return errors.New(log("blockMapper.SetUpDevice failed to get CSI persistent source: %v", err)) } driverName := csiSource.Driver skip, err := m.plugin.skipAttach(driverName) if err != nil { - return "", errors.New(log("blockMapper.SetupDevice failed to check CSIDriver for %s: %v", driverName, err)) + return errors.New(log("blockMapper.SetupDevice failed to check CSIDriver for %s: %v", driverName, err)) } var attachment *storage.VolumeAttachment @@ -232,7 +233,7 @@ func (m *csiBlockMapper) SetUpDevice() (string, error) { attachID := getAttachmentName(csiSource.VolumeHandle, csiSource.Driver, nodeName) attachment, err = m.k8s.StorageV1().VolumeAttachments().Get(attachID, meta.GetOptions{}) if err != nil { - return "", errors.New(log("blockMapper.SetupDevice failed to get volume attachment [id=%v]: %v", attachID, err)) + return errors.New(log("blockMapper.SetupDevice failed to get volume attachment [id=%v]: %v", attachID, err)) } } @@ -247,29 +248,30 @@ func (m *csiBlockMapper) SetUpDevice() (string, error) { csiClient, err := m.csiClientGetter.Get() if err != nil { - return "", errors.New(log("blockMapper.SetUpDevice failed to get CSI client: %v", err)) + return errors.New(log("blockMapper.SetUpDevice failed to get CSI client: %v", err)) } // Call NodeStageVolume stagingPath, err := m.stageVolumeForBlock(ctx, csiClient, accessMode, csiSource, attachment) if err != nil { - return "", err + return err } // Call NodePublishVolume - publishPath, err := m.publishVolumeForBlock(ctx, csiClient, accessMode, csiSource, attachment, stagingPath) + _, err = m.publishVolumeForBlock(ctx, csiClient, accessMode, csiSource, attachment, stagingPath) if err != nil { - return "", err + return err } - return publishPath, nil -} - -func (m *csiBlockMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { return nil } +func (m *csiBlockMapper) MapPodDevice() (string, error) { + return m.getPublishPath(), nil +} + var _ volume.BlockVolumeUnmapper = &csiBlockMapper{} +var _ volume.CustomBlockVolumeUnmapper = &csiBlockMapper{} // unpublishVolumeForBlock unpublishes a block volume from publishPath func (m *csiBlockMapper) unpublishVolumeForBlock(ctx context.Context, csi csiClient, publishPath string) error { @@ -362,3 +364,8 @@ func (m *csiBlockMapper) TearDownDevice(globalMapPath, devicePath string) error return nil } + +// UnmapPodDevice unmaps the block device path. +func (m *csiBlockMapper) UnmapPodDevice() error { + return nil +} diff --git a/pkg/volume/csi/csi_block_test.go b/pkg/volume/csi/csi_block_test.go index befcd4fe28b..86d8c79b682 100644 --- a/pkg/volume/csi/csi_block_test.go +++ b/pkg/volume/csi/csi_block_test.go @@ -239,17 +239,11 @@ func TestBlockMapperSetupDevice(t *testing.T) { } t.Log("created attachement ", attachID) - devicePath, err := csiMapper.SetUpDevice() + err = csiMapper.SetUpDevice() if err != nil { t.Fatalf("mapper failed to SetupDevice: %v", err) } - // Check if SetUpDevice returns the right path - publishPath := csiMapper.getPublishPath() - if devicePath != publishPath { - t.Fatalf("mapper.SetupDevice returned unexpected path %s instead of %v", devicePath, publishPath) - } - // Check if NodeStageVolume staged to the right path stagingPath := csiMapper.getStagingPath() svols := csiMapper.csiClient.(*fakeCsiDriverClient).nodeClient.GetNodeStagedVolumes() @@ -267,12 +261,14 @@ func TestBlockMapperSetupDevice(t *testing.T) { if !ok { t.Error("csi server may not have received NodePublishVolume call") } + + publishPath := csiMapper.getPublishPath() if pvol.Path != publishPath { t.Errorf("csi server expected path %s, got %s", publishPath, pvol.Path) } } -func TestBlockMapperMapDevice(t *testing.T) { +func TestBlockMapperMapPodDevice(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIBlockVolume, true)() plug, tmpDir := newTestPlugin(t, nil) @@ -306,24 +302,18 @@ func TestBlockMapperMapDevice(t *testing.T) { } t.Log("created attachement ", attachID) - devicePath, err := csiMapper.SetUpDevice() - if err != nil { - t.Fatalf("mapper failed to SetupDevice: %v", err) - } - globalMapPath, err := csiMapper.GetGlobalMapPath(csiMapper.spec) - if err != nil { - t.Fatalf("mapper failed to GetGlobalMapPath: %v", err) - } - // Map device to global and pod device map path - volumeMapPath, volName := csiMapper.GetPodDeviceMapPath() - err = csiMapper.MapDevice(devicePath, globalMapPath, volumeMapPath, volName, csiMapper.podUID) + path, err := csiMapper.MapPodDevice() if err != nil { t.Fatalf("mapper failed to GetGlobalMapPath: %v", err) } + publishPath := csiMapper.getPublishPath() + if path != publishPath { + t.Errorf("path %s and %s doesn't match", path, publishPath) + } } -func TestBlockMapperMapDeviceNotSupportAttach(t *testing.T) { +func TestBlockMapperMapPodDeviceNotSupportAttach(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIBlockVolume, true)() defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIDriverRegistry, true)() @@ -361,21 +351,16 @@ func TestBlockMapperMapDeviceNotSupportAttach(t *testing.T) { t.Fatalf("Failed to make a new Mapper: %v", err) } csiMapper.csiClient = setupClient(t, true) - devicePath, err := csiMapper.SetUpDevice() - if err != nil { - t.Fatalf("mapper failed to SetupDevice: %v", err) - } - globalMapPath, err := csiMapper.GetGlobalMapPath(csiMapper.spec) - if err != nil { - t.Fatalf("mapper failed to GetGlobalMapPath: %v", err) - } // Map device to global and pod device map path - volumeMapPath, volName := csiMapper.GetPodDeviceMapPath() - err = csiMapper.MapDevice(devicePath, globalMapPath, volumeMapPath, volName, csiMapper.podUID) + path, err := csiMapper.MapPodDevice() if err != nil { t.Fatalf("mapper failed to GetGlobalMapPath: %v", err) } + publishPath := csiMapper.getPublishPath() + if path != publishPath { + t.Errorf("path %s and %s doesn't match", path, publishPath) + } } func TestBlockMapperTearDownDevice(t *testing.T) { diff --git a/pkg/volume/fc/fc.go b/pkg/volume/fc/fc.go index ece6e92a19c..5ad7bfc427c 100644 --- a/pkg/volume/fc/fc.go +++ b/pkg/volume/fc/fc.go @@ -414,20 +414,13 @@ type fcDiskMapper struct { var _ volume.BlockVolumeMapper = &fcDiskMapper{} -func (b *fcDiskMapper) SetUpDevice() (string, error) { - return "", nil -} - -func (b *fcDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return nil -} - type fcDiskUnmapper struct { *fcDisk deviceUtil util.DeviceUtil } var _ volume.BlockVolumeUnmapper = &fcDiskUnmapper{} +var _ volume.CustomBlockVolumeUnmapper = &fcDiskUnmapper{} func (c *fcDiskUnmapper) TearDownDevice(mapPath, devicePath string) error { err := c.manager.DetachBlockFCDisk(*c, mapPath, devicePath) @@ -442,6 +435,10 @@ func (c *fcDiskUnmapper) TearDownDevice(mapPath, devicePath string) error { return nil } +func (c *fcDiskUnmapper) UnmapPodDevice() error { + return nil +} + // GetGlobalMapPath returns global map path and error // path: plugins/kubernetes.io/{PluginName}/volumeDevices/{WWID}/{podUid} func (fc *fcDisk) GetGlobalMapPath(spec *volume.Spec) (string, error) { diff --git a/pkg/volume/gcepd/gce_pd_block.go b/pkg/volume/gcepd/gce_pd_block.go index 40344faa38f..dac6dd2da83 100644 --- a/pkg/volume/gcepd/gce_pd_block.go +++ b/pkg/volume/gcepd/gce_pd_block.go @@ -135,10 +135,6 @@ func (plugin *gcePersistentDiskPlugin) newUnmapperInternal(volName string, podUI }}, nil } -func (c *gcePersistentDiskUnmapper) TearDownDevice(mapPath, devicePath string) error { - return nil -} - type gcePersistentDiskUnmapper struct { *gcePersistentDisk } @@ -152,14 +148,6 @@ type gcePersistentDiskMapper struct { var _ volume.BlockVolumeMapper = &gcePersistentDiskMapper{} -func (b *gcePersistentDiskMapper) SetUpDevice() (string, error) { - return "", nil -} - -func (b *gcePersistentDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return nil -} - // 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 9f177deaad7..12ed5f0a7be 100644 --- a/pkg/volume/iscsi/iscsi.go +++ b/pkg/volume/iscsi/iscsi.go @@ -384,14 +384,6 @@ type iscsiDiskMapper struct { var _ volume.BlockVolumeMapper = &iscsiDiskMapper{} -func (b *iscsiDiskMapper) SetUpDevice() (string, error) { - return "", nil -} - -func (b *iscsiDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return nil -} - type iscsiDiskUnmapper struct { *iscsiDisk exec utilexec.Interface @@ -399,6 +391,7 @@ type iscsiDiskUnmapper struct { } var _ volume.BlockVolumeUnmapper = &iscsiDiskUnmapper{} +var _ volume.CustomBlockVolumeUnmapper = &iscsiDiskUnmapper{} // Even though iSCSI plugin has attacher/detacher implementation, iSCSI plugin // needs volume detach operation during TearDownDevice(). This method is only @@ -417,6 +410,10 @@ func (c *iscsiDiskUnmapper) TearDownDevice(mapPath, _ string) error { return nil } +func (c *iscsiDiskUnmapper) UnmapPodDevice() error { + return nil +} + // GetGlobalMapPath returns global map path and error // path: plugins/kubernetes.io/{PluginName}/volumeDevices/{ifaceName}/{portal-some_iqn-lun-lun_id} func (iscsi *iscsiDisk) GetGlobalMapPath(spec *volume.Spec) (string, error) { diff --git a/pkg/volume/local/local.go b/pkg/volume/local/local.go index 1f612727605..80a58a7374d 100644 --- a/pkg/volume/local/local.go +++ b/pkg/volume/local/local.go @@ -612,16 +612,18 @@ type localVolumeMapper struct { } var _ volume.BlockVolumeMapper = &localVolumeMapper{} +var _ volume.CustomBlockVolumeMapper = &localVolumeMapper{} -// SetUpDevice provides physical device path for the local PV. -func (m *localVolumeMapper) SetUpDevice() (string, error) { - globalPath := util.MakeAbsolutePath(runtime.GOOS, m.globalPath) - klog.V(4).Infof("SetupDevice returning path %s", globalPath) - return globalPath, nil +// SetUpDevice prepares the volume to the node by the plugin specific way. +func (m *localVolumeMapper) SetUpDevice() error { + return nil } -func (m *localVolumeMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return nil +// SetUpDevice provides physical device path for the local PV. +func (m *localVolumeMapper) MapPodDevice() (string, error) { + globalPath := util.MakeAbsolutePath(runtime.GOOS, m.globalPath) + klog.V(4).Infof("MapPodDevice returning path %s", globalPath) + return globalPath, nil } // localVolumeUnmapper implements the BlockVolumeUnmapper interface for local volumes. @@ -631,12 +633,6 @@ type localVolumeUnmapper struct { var _ volume.BlockVolumeUnmapper = &localVolumeUnmapper{} -// TearDownDevice will undo SetUpDevice procedure. In local PV, all of this already handled by operation_generator. -func (u *localVolumeUnmapper) TearDownDevice(mapPath, _ string) error { - klog.V(4).Infof("local: TearDownDevice completed for: %s", mapPath) - return nil -} - // GetGlobalMapPath returns global map path and error. // path: plugins/kubernetes.io/kubernetes.io/local-volume/volumeDevices/{volumeName} func (l *localVolume) GetGlobalMapPath(spec *volume.Spec) (string, error) { diff --git a/pkg/volume/local/local_test.go b/pkg/volume/local/local_test.go index 7d85118a728..3035f09f580 100644 --- a/pkg/volume/local/local_test.go +++ b/pkg/volume/local/local_test.go @@ -372,9 +372,17 @@ func TestMapUnmap(t *testing.T) { if volName != testPVName { t.Errorf("Got unexpected volNamne: %s, expected %s", volName, testPVName) } - devPath, err := mapper.SetUpDevice() - if err != nil { - t.Errorf("Failed to SetUpDevice, err: %v", err) + var devPath string + + if customMapper, ok := mapper.(volume.CustomBlockVolumeMapper); ok { + err = customMapper.SetUpDevice() + if err != nil { + t.Errorf("Failed to SetUpDevice, err: %v", err) + } + devPath, err = customMapper.MapPodDevice() + if err != nil { + t.Errorf("Failed to MapPodDevice, err: %v", err) + } } if _, err := os.Stat(devPath); err != nil { @@ -393,8 +401,14 @@ func TestMapUnmap(t *testing.T) { t.Fatalf("Got a nil Unmapper") } - if err := unmapper.TearDownDevice(globalPath, devPath); err != nil { - t.Errorf("TearDownDevice failed, err: %v", err) + if customUnmapper, ok := unmapper.(volume.CustomBlockVolumeUnmapper); ok { + if err := customUnmapper.UnmapPodDevice(); err != nil { + t.Errorf("UnmapPodDevice failed, err: %v", err) + } + + if err := customUnmapper.TearDownDevice(globalPath, devPath); err != nil { + t.Errorf("TearDownDevice failed, err: %v", err) + } } } diff --git a/pkg/volume/rbd/rbd.go b/pkg/volume/rbd/rbd.go index 9f52aaadb17..380f1711bf0 100644 --- a/pkg/volume/rbd/rbd.go +++ b/pkg/volume/rbd/rbd.go @@ -898,6 +898,7 @@ type rbdDiskMapper struct { } var _ volume.BlockVolumeUnmapper = &rbdDiskUnmapper{} +var _ volume.CustomBlockVolumeUnmapper = &rbdDiskUnmapper{} // GetGlobalMapPath returns global map path and error // path: plugins/kubernetes.io/{PluginName}/volumeDevices/{rbd pool}-image-{rbd image-name}/{podUid} @@ -912,14 +913,6 @@ func (rbd *rbd) GetPodDeviceMapPath() (string, string) { return rbd.rbdPodDeviceMapPath() } -func (rbd *rbdDiskMapper) SetUpDevice() (string, error) { - return "", nil -} - -func (rbd *rbdDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return nil -} - func (rbd *rbd) rbdGlobalMapPath(spec *volume.Spec) (string, error) { mon, err := getVolumeSourceMonitors(spec) if err != nil { @@ -1003,6 +996,10 @@ func (rbd *rbdDiskUnmapper) TearDownDevice(mapPath, _ string) error { return nil } +func (rbd *rbdDiskUnmapper) UnmapPodDevice() error { + return nil +} + func getVolumeSourceMonitors(spec *volume.Spec) ([]string, error) { if spec.Volume != nil && spec.Volume.RBD != nil { return spec.Volume.RBD.CephMonitors, nil diff --git a/pkg/volume/testing/testing.go b/pkg/volume/testing/testing.go index db65baff679..95bf7ebc1d2 100644 --- a/pkg/volume/testing/testing.go +++ b/pkg/volume/testing/testing.go @@ -804,7 +804,8 @@ type FakeVolume struct { GetDeviceMountPathCallCount int SetUpDeviceCallCount int TearDownDeviceCallCount int - MapDeviceCallCount int + MapPodDeviceCallCount int + UnmapPodDeviceCallCount int GlobalMapPathCallCount int PodDeviceMapPathCallCount int } @@ -880,11 +881,11 @@ func (fv *FakeVolume) TearDownAt(dir string) error { } // Block volume support -func (fv *FakeVolume) SetUpDevice() (string, error) { +func (fv *FakeVolume) SetUpDevice() error { fv.Lock() defer fv.Unlock() fv.SetUpDeviceCallCount++ - return "", nil + return nil } // Block volume support @@ -950,18 +951,33 @@ func (fv *FakeVolume) GetTearDownDeviceCallCount() int { } // Block volume support -func (fv *FakeVolume) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, pod types.UID) error { +func (fv *FakeVolume) UnmapPodDevice() error { fv.Lock() defer fv.Unlock() - fv.MapDeviceCallCount++ + fv.UnmapPodDeviceCallCount++ return nil } // Block volume support -func (fv *FakeVolume) GetMapDeviceCallCount() int { +func (fv *FakeVolume) GetUnmapPodDeviceCallCount() int { fv.RLock() defer fv.RUnlock() - return fv.MapDeviceCallCount + return fv.UnmapPodDeviceCallCount +} + +// Block volume support +func (fv *FakeVolume) MapPodDevice() (string, error) { + fv.Lock() + defer fv.Unlock() + fv.MapPodDeviceCallCount++ + return "", nil +} + +// Block volume support +func (fv *FakeVolume) GetMapPodDeviceCallCount() int { + fv.RLock() + defer fv.RUnlock() + return fv.MapPodDeviceCallCount } func (fv *FakeVolume) Attach(spec *Spec, nodeName types.NodeName) (string, error) { @@ -1493,22 +1509,22 @@ func VerifyGetPodDeviceMapPathCallCount( expectedPodDeviceMapPathCallCount) } -// VerifyGetMapDeviceCallCount ensures that at least one of the Mappers for this -// plugin has the expectedMapDeviceCallCount number of calls. Otherwise it +// VerifyGetMapPodDeviceCallCount ensures that at least one of the Mappers for this +// plugin has the expectedMapPodDeviceCallCount number of calls. Otherwise it // returns an error. -func VerifyGetMapDeviceCallCount( - expectedMapDeviceCallCount int, +func VerifyGetMapPodDeviceCallCount( + expectedMapPodDeviceCallCount int, fakeVolumePlugin *FakeVolumePlugin) error { for _, mapper := range fakeVolumePlugin.GetBlockVolumeMapper() { - actualCallCount := mapper.GetMapDeviceCallCount() - if actualCallCount >= expectedMapDeviceCallCount { + actualCallCount := mapper.GetMapPodDeviceCallCount() + if actualCallCount >= expectedMapPodDeviceCallCount { return nil } } return fmt.Errorf( - "No Mapper have expected MapdDeviceCallCount. Expected: <%v>.", - expectedMapDeviceCallCount) + "No Mapper have expected MapPodDeviceCallCount. Expected: <%v>.", + expectedMapPodDeviceCallCount) } // GetTestVolumePluginMgr creates, initializes, and returns a test volume plugin diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index a48a48372a4..a1123b8aba3 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -1055,20 +1055,40 @@ func (og *operationGenerator) GenerateMapVolumeFunc( klog.Infof(volumeToMount.GenerateMsgDetailed("MapVolume.WaitForAttach succeeded", fmt.Sprintf("DevicePath %q", devicePath))) } - // A plugin doesn't have attacher also needs to map device to global map path with SetUpDevice() - pluginDevicePath, mapErr := blockVolumeMapper.SetUpDevice() - if mapErr != nil { - // On failure, return error. Caller will log and retry. - return volumeToMount.GenerateError("MapVolume.SetUpDevice failed", mapErr) + // Call SetUpDevice if blockVolumeMapper implements CustomBlockVolumeMapper + if customBlockVolumeMapper, ok := blockVolumeMapper.(volume.CustomBlockVolumeMapper); ok { + mapErr := customBlockVolumeMapper.SetUpDevice() + if mapErr != nil { + // On failure, return error. Caller will log and retry. + return volumeToMount.GenerateError("MapVolume.SetUpDevice failed", mapErr) + } } - // 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 + // 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) } - if len(devicePath) == 0 { - return volumeToMount.GenerateError("MapVolume failed", goerrors.New("Device path of the volume is empty")) + + // Call MapPodDevice if blockVolumeMapper implements CustomBlockVolumeMapper + if customBlockVolumeMapper, ok := blockVolumeMapper.(volume.CustomBlockVolumeMapper); ok { + // Execute driver specific map + pluginDevicePath, mapErr := customBlockVolumeMapper.MapPodDevice() + if mapErr != nil { + // On failure, return error. Caller will log and retry. + return volumeToMount.GenerateError("MapVolume.MapPodDevice failed", mapErr) + } + + // 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 { + return volumeToMount.GenerateError("MapVolume failed", goerrors.New("Device path of the volume is empty")) + } } // When kubelet is containerized, devicePath may be a symlink at a place unavailable to @@ -1085,37 +1105,22 @@ func (og *operationGenerator) GenerateMapVolumeFunc( return volumeToMount.GenerateError("MapVolume.EvalHostSymlinks failed", err) } - // 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) - } - // Execute common map - mapErr = ioutil.MapBlockVolume(og.blkUtil, devicePath, globalMapPath, volumeMapPath, volName, volumeToMount.Pod.UID) + volumeMapPath, volName := blockVolumeMapper.GetPodDeviceMapPath() + 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.MapBlockVolume failed", mapErr) } - // 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)) + simpleMsg, detailedMsg := volumeToMount.GenerateMsg("MapVolume.MapPodDevice succeeded", fmt.Sprintf("globalMapPath %q", globalMapPath)) verbosity := klog.Level(4) og.recorder.Eventf(volumeToMount.Pod, v1.EventTypeNormal, kevents.SuccessfulMountVolume, simpleMsg) klog.V(verbosity).Infof(detailedMsg) // Device mapping for pod device map path succeeded - simpleMsg, detailedMsg = volumeToMount.GenerateMsg("MapVolume.MapDevice succeeded", fmt.Sprintf("volumeMapPath %q", volumeMapPath)) + simpleMsg, detailedMsg = volumeToMount.GenerateMsg("MapVolume.MapPodDevice succeeded", fmt.Sprintf("volumeMapPath %q", volumeMapPath)) verbosity = klog.Level(1) og.recorder.Eventf(volumeToMount.Pod, v1.EventTypeNormal, kevents.SuccessfulMountVolume, simpleMsg) klog.V(verbosity).Infof(detailedMsg) @@ -1218,6 +1223,16 @@ func (og *operationGenerator) GenerateUnmapVolumeFunc( return volumeToUnmount.GenerateError("UnmapVolume.UnmapBlockVolume failed", unmapErr) } + // Call UnmapPodDevice if blockVolumeUnmapper implements CustomBlockVolumeUnmapper + if customBlockVolumeUnmapper, ok := blockVolumeUnmapper.(volume.CustomBlockVolumeUnmapper); ok { + // Execute plugin specific unmap + unmapErr = customBlockVolumeUnmapper.UnmapPodDevice() + if unmapErr != nil { + // On failure, return error. Caller will log and retry. + return volumeToUnmount.GenerateError("UnmapVolume.UnmapPodDevice failed", unmapErr) + } + } + klog.Infof( "UnmapVolume succeeded for volume %q (OuterVolumeSpecName: %q) pod %q (UID: %q). InnerVolumeSpecName %q. PluginName %q, VolumeGidValue %q", volumeToUnmount.VolumeName, @@ -1309,11 +1324,14 @@ func (og *operationGenerator) GenerateUnmapDeviceFunc( return deviceToDetach.GenerateError("UnmapDevice failed", err) } - // Execute tear down device - unmapErr := blockVolumeUnmapper.TearDownDevice(globalMapPath, deviceToDetach.DevicePath) - if unmapErr != nil { - // On failure, return error. Caller will log and retry. - return deviceToDetach.GenerateError("UnmapDevice.TearDownDevice failed", unmapErr) + // Call TearDownDevice if blockVolumeUnmapper implements CustomBlockVolumeUnmapper + if customBlockVolumeUnmapper, ok := blockVolumeUnmapper.(volume.CustomBlockVolumeUnmapper); ok { + // Execute tear down device + unmapErr := customBlockVolumeUnmapper.TearDownDevice(globalMapPath, deviceToDetach.DevicePath) + if unmapErr != nil { + // On failure, return error. Caller will log and retry. + return deviceToDetach.GenerateError("UnmapDevice.TearDownDevice failed", unmapErr) + } } // Plugin finished TearDownDevice(). Now globalMapPath dir and plugin's stored data diff --git a/pkg/volume/util/volumepathhandler/volume_path_handler.go b/pkg/volume/util/volumepathhandler/volume_path_handler.go index ad192f437c8..e12a9137fed 100644 --- a/pkg/volume/util/volumepathhandler/volume_path_handler.go +++ b/pkg/volume/util/volumepathhandler/volume_path_handler.go @@ -252,7 +252,7 @@ func (v VolumePathHandler) IsSymlinkExist(mapPath string) (bool, error) { if fi.Mode()&os.ModeSymlink == os.ModeSymlink { return true, nil } - // If file exits but it's not symbolic link, return fale and no error + // If file exits but it's not symbolic link, return false and no error return false, nil } @@ -274,7 +274,7 @@ func (v VolumePathHandler) IsDeviceBindMountExist(mapPath string) (bool, error) if fi.Mode()&os.ModeDevice == os.ModeDevice { return true, nil } - // If file exits but it's not device, return fale and no error + // If file exits but it's not device, return false and no error return false, nil } diff --git a/pkg/volume/volume.go b/pkg/volume/volume.go index e125e9b285a..1f8c37f0908 100644 --- a/pkg/volume/volume.go +++ b/pkg/volume/volume.go @@ -152,35 +152,43 @@ type Unmounter interface { TearDownAt(dir string) error } -// BlockVolumeMapper interface provides methods to set up/map the volume. +// BlockVolumeMapper interface is a mapper interface for block volume. type BlockVolumeMapper interface { BlockVolume - // SetUpDevice prepares the volume to a self-determined directory path, - // which may or may not exist yet and returns combination of physical - // device path of a block volume and error. - // If the plugin is non-attachable, it should prepare the device - // in /dev/ (or where appropriate) and return unique device path. - // Unique device path across kubelet node reboot is required to avoid - // unexpected block volume destruction. - // If the plugin is attachable, it should not do anything here, - // just return empty string for device path. - // Instead, attachable plugin have to return unique device path - // 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. +// CustomBlockVolumeMapper interface provides custom methods to set up/map the volume. +type CustomBlockVolumeMapper interface { + BlockVolumeMapper + // SetUpDevice prepares the volume to the node by the plugin specific way. + // For most in-tree plugins, attacher.Attach() and attacher.WaitForAttach() + // will do necessary works. + // This may be called more than once, so implementations must be idempotent. + SetUpDevice() error + + // MapPodDevice maps the block device to a path and return the path. + // Unique device path across kubelet node reboot is required to avoid + // unexpected block volume destruction. + // If empty string is returned, the path retuned by attacher.Attach() and + // attacher.WaitForAttach() will be sued. + MapPodDevice() (string, error) +} + +// BlockVolumeUnmapper interface is an unmapper interface for block volume. type BlockVolumeUnmapper interface { BlockVolume - // TearDownDevice removes traces of the SetUpDevice procedure under - // a self-determined directory. +} + +// CustomBlockVolumeUnmapper interface provides custom methods to cleanup/unmap the volumes. +type CustomBlockVolumeUnmapper interface { + BlockVolumeUnmapper + // TearDownDevice removes traces of the SetUpDevice procedure. // If the plugin is non-attachable, this method detaches the volume // from a node. TearDownDevice(mapPath string, devicePath string) error + + // UnmapPodDevice removes traces of the MapPodDevice procedure. + UnmapPodDevice() error } // Provisioner is an interface that creates templates for PersistentVolumes diff --git a/pkg/volume/vsphere_volume/vsphere_volume_block.go b/pkg/volume/vsphere_volume/vsphere_volume_block.go index 5ed08ef32ad..37d52d6159b 100644 --- a/pkg/volume/vsphere_volume/vsphere_volume_block.go +++ b/pkg/volume/vsphere_volume/vsphere_volume_block.go @@ -132,24 +132,12 @@ type vsphereBlockVolumeMapper struct { *vsphereVolume } -func (v vsphereBlockVolumeMapper) SetUpDevice() (string, error) { - return "", nil -} - -func (v vsphereBlockVolumeMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error { - return nil -} - var _ volume.BlockVolumeUnmapper = &vsphereBlockVolumeUnmapper{} type vsphereBlockVolumeUnmapper struct { *vsphereVolume } -func (v *vsphereBlockVolumeUnmapper) TearDownDevice(mapPath, devicePath string) error { - return nil -} - // GetGlobalMapPath returns global map path and error // path: plugins/kubernetes.io/{PluginName}/volumeDevices/volumePath func (v *vsphereVolume) GetGlobalMapPath(spec *volume.Spec) (string, error) { From a275026ad4a30e36aae76d699c747b048cb67c5b Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Tue, 12 Nov 2019 19:44:36 +0000 Subject: [PATCH 2/2] Split CustomBlockVolumeMapper and CustomBlockVolumeUnmapper - Move SetUpDevice to BlockVolumeStager - Move MapPodDevice to BlockVolumePublisher - Move TearDownDevice to BlockVolumeUnstager - Move UnmapPodDevice to BlockVolumeUnpublisher - Implement BlockVolumePublisher only in local and csi plugin - Implement BlockVolumeUnstager only in fc, iscsi, rbd, and csi plugin - Implement BlockVolumeStager and BlockVolumeUnpublisher only in csi plugin --- pkg/volume/local/local.go | 2 +- .../util/operationexecutor/operation_generator.go | 14 +++++++++++++- pkg/volume/volume.go | 2 +- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/pkg/volume/local/local.go b/pkg/volume/local/local.go index 80a58a7374d..058d927c5ac 100644 --- a/pkg/volume/local/local.go +++ b/pkg/volume/local/local.go @@ -619,7 +619,7 @@ func (m *localVolumeMapper) SetUpDevice() error { return nil } -// SetUpDevice provides physical device path for the local PV. +// MapPodDevice provides physical device path for the local PV. func (m *localVolumeMapper) MapPodDevice() (string, error) { globalPath := util.MakeAbsolutePath(runtime.GOOS, m.globalPath) klog.V(4).Infof("MapPodDevice returning path %s", globalPath) diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index a1123b8aba3..0d91d2ba2a0 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -1065,8 +1065,9 @@ func (og *operationGenerator) GenerateMapVolumeFunc( } // Update actual state of world to reflect volume is globally mounted + markedDevicePath := devicePath markDeviceMappedErr := actualStateOfWorld.MarkDeviceAsMounted( - volumeToMount.VolumeName, devicePath, globalMapPath) + volumeToMount.VolumeName, markedDevicePath, globalMapPath) if markDeviceMappedErr != nil { // On failure, return error. Caller will log and retry. return volumeToMount.GenerateError("MapVolume.MarkDeviceAsMounted failed", markDeviceMappedErr) @@ -1105,6 +1106,17 @@ func (og *operationGenerator) GenerateMapVolumeFunc( return volumeToMount.GenerateError("MapVolume.EvalHostSymlinks failed", err) } + // Update actual state of world with the devicePath again, if devicePath has changed from markedDevicePath + // TODO: This can be improved after #82492 is merged and ASW has state. + if markedDevicePath != devicePath { + 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) + } + } + // Execute common map volumeMapPath, volName := blockVolumeMapper.GetPodDeviceMapPath() mapErr := ioutil.MapBlockVolume(og.blkUtil, devicePath, globalMapPath, volumeMapPath, volName, volumeToMount.Pod.UID) diff --git a/pkg/volume/volume.go b/pkg/volume/volume.go index 1f8c37f0908..31fa8216e3e 100644 --- a/pkg/volume/volume.go +++ b/pkg/volume/volume.go @@ -170,7 +170,7 @@ type CustomBlockVolumeMapper interface { // Unique device path across kubelet node reboot is required to avoid // unexpected block volume destruction. // If empty string is returned, the path retuned by attacher.Attach() and - // attacher.WaitForAttach() will be sued. + // attacher.WaitForAttach() will be used. MapPodDevice() (string, error) }