From 3afdfd7ae8c331f0e357178396037e36e60b2074 Mon Sep 17 00:00:00 2001 From: jharrod Date: Wed, 21 Aug 2024 10:02:34 -0600 Subject: [PATCH] Do not remove tracking file on multipath flush failures Return errors from multipath flushReorder multipath flush and tracking file removal --- frontend/csi/node_server.go | 10 +++--- utils/devices.go | 40 ++++++++++++++--------- utils/devices_test.go | 63 +++++++++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 19 deletions(-) diff --git a/frontend/csi/node_server.go b/frontend/csi/node_server.go index 712094f31..55388ba45 100644 --- a/frontend/csi/node_server.go +++ b/frontend/csi/node_server.go @@ -1406,16 +1406,16 @@ func (p *Plugin) nodeUnstageISCSIVolume( return status.Error(codes.Internal, errStr) } + // If there is multipath device, flush(remove) mappings + if err := utils.RemoveMultipathDeviceMapping(ctx, unmappedMpathDevice); err != nil { + return err + } + // Delete the device info we saved to the volume tracking info path so unstage can succeed. if err := p.nodeHelper.DeleteTrackingInfo(ctx, volumeId); err != nil { return status.Error(codes.Internal, err.Error()) } - // If there is multipath device, flush(remove) mappings - if unmappedMpathDevice != "" { - utils.RemoveMultipathDeviceMapping(ctx, unmappedMpathDevice) - } - return nil } diff --git a/utils/devices.go b/utils/devices.go index bf5513a76..97bc3920b 100644 --- a/utils/devices.go +++ b/utils/devices.go @@ -467,18 +467,16 @@ func multipathFlushDevice(ctx context.Context, deviceInfo *ScsiDeviceInfo) error if errors.IsISCSIDeviceFlushError(deviceErr) { Logc(ctx).WithFields( LogFields{ - "error": deviceErr, "device": devicePath, - }).Debug("Flush failed.") + }).WithError(deviceErr).Debug("Flush failed.") return deviceErr } if errors.IsTimeoutError(deviceErr) { Logc(ctx).WithFields(LogFields{ - "error": deviceErr, "device": devicePath, "lun": deviceInfo.LUN, "host": deviceInfo.Host, - }).Debug("Flush timed out.") + }).WithError(deviceErr).Debug("Flush timed out.") return deviceErr } } @@ -490,7 +488,14 @@ func multipathFlushDevice(ctx context.Context, deviceInfo *ScsiDeviceInfo) error return err } - RemoveMultipathDeviceMapping(ctx, devicePath) + if err = RemoveMultipathDeviceMapping(ctx, devicePath); err != nil { + Logc(ctx).WithFields(LogFields{ + "device": devicePath, + "lun": deviceInfo.LUN, + "host": deviceInfo.Host, + }).WithError(deviceErr).Debug("Error during multipath flush.") + return err + } return nil } @@ -1143,25 +1148,32 @@ func PrepareDeviceAtMountPathForRemoval(ctx context.Context, mountpoint string, // RemoveMultipathDeviceMapping uses "multipath -f " to flush(remove) unused map. // Unused maps can happen when Unstage is called on offline/deleted LUN. -func RemoveMultipathDeviceMapping(ctx context.Context, devicePath string) { +func RemoveMultipathDeviceMapping(ctx context.Context, devicePath string) error { Logc(ctx).WithField("devicePath", devicePath).Debug(">>>> devices.RemoveMultipathDevicemapping") defer Logc(ctx).Debug("<<<< devices.RemoveMultipathDeviceMapping") if devicePath == "" { - return + return nil } out, err := command.ExecuteWithTimeout(ctx, "multipath", 10*time.Second, false, "-f", devicePath) if err != nil { - // Nothing to do if it generates an error, but log it. - Logc(ctx).WithFields(LogFields{ - "error": err, - "output": string(out), - "devicePath": devicePath, - }).Error("Error encountered in multipath flush(remove) mapping command.") + pathAlreadyRemoved := strings.Contains(string(out), fmt.Sprintf("'%s' is not a valid argument", devicePath)) + if pathAlreadyRemoved { + Logc(ctx).WithFields(LogFields{ + "output": string(out), + "devicePath": devicePath, + }).WithError(err).Debug("Multipath device already removed.") + } else { + Logc(ctx).WithFields(LogFields{ + "output": string(out), + "devicePath": devicePath, + }).WithError(err).Error("Error encountered in multipath flush(remove) mapping command.") + return fmt.Errorf("failed to flush multipath device: %w", err) + } } - return + return nil } // removeSCSIDevice informs Linux that a device will be removed. The deviceInfo provided only needs diff --git a/utils/devices_test.go b/utils/devices_test.go index f3171d43c..94ae23d5d 100644 --- a/utils/devices_test.go +++ b/utils/devices_test.go @@ -4,10 +4,12 @@ import ( "context" "fmt" "testing" + "time" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" + mockexec "github.com/netapp/trident/mocks/mock_utils/mock_exec" "github.com/netapp/trident/mocks/mock_utils/mock_luks" ) @@ -200,3 +202,64 @@ func TestMountLUKSDevice_Negative(t *testing.T) { assert.Error(t, err) assert.False(t, luksFormatted) } + +func TestRemoveMultipathDeviceMapping(t *testing.T) { + originalCmd := command + // Reset 'command' at the end of the test + defer func() { command = originalCmd }() + + client := mockexec.NewMockCommand(gomock.NewController(t)) + command = client // Set package var to mock + + tests := []struct { + name string + devicePath string + mockReturn []byte + mockError error + expectError bool + }{ + { + name: "Happy Path", + devicePath: "/dev/mock-0", + mockReturn: []byte("mock output"), + mockError: nil, + expectError: false, + }, + { + name: "Blank Device Path", + devicePath: "", + mockReturn: nil, + mockError: nil, + expectError: false, + }, + { + name: "Device does not exist", + devicePath: "/dev/mapper/doesNotExist", + mockReturn: []byte("'/dev/mapper/doesNotExist' is not a valid argument"), + mockError: fmt.Errorf("error"), + expectError: false, + }, + { + name: "Negative case", + devicePath: "/dev/mock-0", + mockReturn: nil, + mockError: fmt.Errorf("error"), + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.devicePath != "" { + client.EXPECT().ExecuteWithTimeout(gomock.Any(), "multipath", 10*time.Second, false, "-f", tt.devicePath). + Return(tt.mockReturn, tt.mockError) + } + err := RemoveMultipathDeviceMapping(context.TODO(), tt.devicePath) + if tt.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +}