From 6f8bdb7cc1e28585edb5d47212ffb4d0ab3bf546 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Wed, 2 Dec 2020 11:03:11 +0100 Subject: [PATCH] Fix FibreChannel volume plugin corrupting filesystem on detach FibreChannel volume plugin misses one important step when removing a device: "multipath -f". It flushes all multipath buffers to its individual paths. Without it, a filesystem on the device may get corrupted. --- pkg/volume/fc/attacher.go | 7 +++++-- pkg/volume/fc/fc.go | 12 ++++++++---- pkg/volume/fc/fc_test.go | 2 +- pkg/volume/fc/fc_util.go | 16 ++++++++++++++++ 4 files changed, 30 insertions(+), 7 deletions(-) diff --git a/pkg/volume/fc/attacher.go b/pkg/volume/fc/attacher.go index 83e09a8d7b6..c94c95d47a6 100644 --- a/pkg/volume/fc/attacher.go +++ b/pkg/volume/fc/attacher.go @@ -132,6 +132,7 @@ func (attacher *fcAttacher) MountDevice(spec *volume.Spec, devicePath string, de type fcDetacher struct { mounter mount.Interface manager diskManager + host volume.VolumeHost } var _ volume.Detacher = &fcDetacher{} @@ -142,6 +143,7 @@ func (plugin *fcPlugin) NewDetacher() (volume.Detacher, error) { return &fcDetacher{ mounter: plugin.host.GetMounter(plugin.GetPluginName()), manager: &fcUtil{}, + host: plugin.host, }, nil } @@ -170,7 +172,7 @@ func (detacher *fcDetacher) UnmountDevice(deviceMountPath string) error { if devName == "" { return nil } - unMounter := volumeSpecToUnmounter(detacher.mounter) + unMounter := volumeSpecToUnmounter(detacher.mounter, detacher.host) err = detacher.manager.DetachDisk(*unMounter, devName) if err != nil { return fmt.Errorf("fc: failed to detach disk: %s\nError: %v", devName, err) @@ -230,12 +232,13 @@ func volumeSpecToMounter(spec *volume.Spec, host volume.VolumeHost) (*fcDiskMoun }, nil } -func volumeSpecToUnmounter(mounter mount.Interface) *fcDiskUnmounter { +func volumeSpecToUnmounter(mounter mount.Interface, host volume.VolumeHost) *fcDiskUnmounter { return &fcDiskUnmounter{ fcDisk: &fcDisk{ io: &osIOHandler{}, }, mounter: mounter, deviceUtil: volumeutil.NewDeviceHandler(volumeutil.NewIOHandler()), + exec: host.GetExec(fcPluginName), } } diff --git a/pkg/volume/fc/fc.go b/pkg/volume/fc/fc.go index 23cb77f1274..a70dab19021 100644 --- a/pkg/volume/fc/fc.go +++ b/pkg/volume/fc/fc.go @@ -189,10 +189,10 @@ func (plugin *fcPlugin) newBlockVolumeMapperInternal(spec *volume.Spec, podUID t func (plugin *fcPlugin) NewUnmounter(volName string, podUID types.UID) (volume.Unmounter, error) { // Inject real implementations here, test through the internal function. - return plugin.newUnmounterInternal(volName, podUID, &fcUtil{}, plugin.host.GetMounter(plugin.GetPluginName())) + return plugin.newUnmounterInternal(volName, podUID, &fcUtil{}, plugin.host.GetMounter(plugin.GetPluginName()), plugin.host.GetExec(plugin.GetPluginName())) } -func (plugin *fcPlugin) newUnmounterInternal(volName string, podUID types.UID, manager diskManager, mounter mount.Interface) (volume.Unmounter, error) { +func (plugin *fcPlugin) newUnmounterInternal(volName string, podUID types.UID, manager diskManager, mounter mount.Interface, exec utilexec.Interface) (volume.Unmounter, error) { return &fcDiskUnmounter{ fcDisk: &fcDisk{ podUID: podUID, @@ -203,14 +203,15 @@ func (plugin *fcPlugin) newUnmounterInternal(volName string, podUID types.UID, m }, mounter: mounter, deviceUtil: util.NewDeviceHandler(util.NewIOHandler()), + exec: exec, }, nil } func (plugin *fcPlugin) NewBlockVolumeUnmapper(volName string, podUID types.UID) (volume.BlockVolumeUnmapper, error) { - return plugin.newUnmapperInternal(volName, podUID, &fcUtil{}) + return plugin.newUnmapperInternal(volName, podUID, &fcUtil{}, plugin.host.GetExec(plugin.GetPluginName())) } -func (plugin *fcPlugin) newUnmapperInternal(volName string, podUID types.UID, manager diskManager) (volume.BlockVolumeUnmapper, error) { +func (plugin *fcPlugin) newUnmapperInternal(volName string, podUID types.UID, manager diskManager, exec utilexec.Interface) (volume.BlockVolumeUnmapper, error) { return &fcDiskUnmapper{ fcDisk: &fcDisk{ podUID: podUID, @@ -219,6 +220,7 @@ func (plugin *fcPlugin) newUnmapperInternal(volName string, podUID types.UID, ma plugin: plugin, io: &osIOHandler{}, }, + exec: exec, deviceUtil: util.NewDeviceHandler(util.NewIOHandler()), }, nil } @@ -373,6 +375,7 @@ type fcDiskUnmounter struct { *fcDisk mounter mount.Interface deviceUtil util.DeviceUtil + exec utilexec.Interface } var _ volume.Unmounter = &fcDiskUnmounter{} @@ -400,6 +403,7 @@ var _ volume.BlockVolumeMapper = &fcDiskMapper{} type fcDiskUnmapper struct { *fcDisk deviceUtil util.DeviceUtil + exec utilexec.Interface } var _ volume.BlockVolumeUnmapper = &fcDiskUnmapper{} diff --git a/pkg/volume/fc/fc_test.go b/pkg/volume/fc/fc_test.go index c6f929bef63..1467f065a8d 100644 --- a/pkg/volume/fc/fc_test.go +++ b/pkg/volume/fc/fc_test.go @@ -190,7 +190,7 @@ func doTestPlugin(t *testing.T, spec *volume.Spec) { fakeManager2 := newFakeDiskManager() defer fakeManager2.Cleanup() - unmounter, err := plug.(*fcPlugin).newUnmounterInternal("vol1", types.UID("poduid"), fakeManager2, fakeMounter) + unmounter, err := plug.(*fcPlugin).newUnmounterInternal("vol1", types.UID("poduid"), fakeManager2, fakeMounter, fakeExec) if err != nil { t.Errorf("Failed to make a new Unmounter: %v", err) } diff --git a/pkg/volume/fc/fc_util.go b/pkg/volume/fc/fc_util.go index 90fc994576e..f50d39aeef3 100644 --- a/pkg/volume/fc/fc_util.go +++ b/pkg/volume/fc/fc_util.go @@ -27,6 +27,7 @@ import ( "k8s.io/klog/v2" "k8s.io/mount-utils" + utilexec "k8s.io/utils/exec" "k8s.io/kubernetes/pkg/volume" volumeutil "k8s.io/kubernetes/pkg/volume/util" @@ -257,6 +258,9 @@ func (util *fcUtil) DetachDisk(c fcDiskUnmounter, devicePath string) error { // Find slave if strings.HasPrefix(dstPath, "/dev/dm-") { devices = c.deviceUtil.FindSlaveDevicesOnMultipath(dstPath) + if err := util.deleteMultipathDevice(c.exec, dstPath); err != nil { + return err + } } else { // Add single devicepath to devices devices = append(devices, dstPath) @@ -354,6 +358,9 @@ func (util *fcUtil) DetachBlockFCDisk(c fcDiskUnmapper, mapPath, devicePath stri if len(dm) != 0 { // Find all devices which are managed by multipath devices = c.deviceUtil.FindSlaveDevicesOnMultipath(dm) + if err := util.deleteMultipathDevice(c.exec, dm); err != nil { + return err + } } else { // Add single device path to devices devices = append(devices, dstPath) @@ -373,6 +380,15 @@ func (util *fcUtil) DetachBlockFCDisk(c fcDiskUnmapper, mapPath, devicePath stri return nil } +func (util *fcUtil) deleteMultipathDevice(exec utilexec.Interface, dmDevice string) error { + out, err := exec.Command("multipath", "-f", dmDevice).CombinedOutput() + if err != nil { + return fmt.Errorf("failed to flush multipath device %s: %s\n%s", dmDevice, err, string(out)) + } + klog.V(4).Infof("Flushed multipath device: %s", dmDevice) + return nil +} + func checkPathExists(path string) (bool, error) { if pathExists, pathErr := mount.PathExists(path); pathErr != nil { return pathExists, fmt.Errorf("Error checking if path exists: %v", pathErr)