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

@@ -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
 }
 

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 <devicePath>" 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

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)
+			}
+		})
+	}
+}