Only initiate block device rescans if every path is available during iSCSI volume expansion

This commit changes CSI NodeExpandVolume such that block device rescans are initiated if and only if all block device paths are available.

Author: jwebster7

utils/devices/devices_linux.go

@@ -1,4 +1,4 @@
-// Copyright 2024 NetApp, Inc. All Rights Reserved.
+// Copyright 2025 NetApp, Inc. All Rights Reserved.
 
 // NOTE: This file should only contain functions for handling devices for linux flavor
 

utils/iscsi/iscsi.go

@@ -594,6 +594,49 @@ func (client *Client) AddSession(
 	}
 }
 
+// filterDevicesBySize builds a map of disk devices to their size, filtered by a minimum size requirement.
+// If any errors occur when checking the size of a device, it captures the error and moves onto the next device.
+func (client *Client) filterDevicesBySize(
+	ctx context.Context, deviceInfo *models.ScsiDeviceInfo, minSize int64,
+) (map[string]int64, error) {
+	var errs error
+	deviceSizeMap := make(map[string]int64, 0)
+	for _, diskDevice := range deviceInfo.Devices {
+		size, err := client.devices.GetDiskSize(ctx, devices.DevPrefix+diskDevice)
+		if err != nil {
+			errs = errors.Join(errs, err)
+			// Only consider devices whose size can be gathered.
+			continue
+		}
+
+		if size < minSize {
+			// Only consider devices that are undersized.
+			deviceSizeMap[diskDevice] = size
+		}
+	}
+
+	if errs != nil {
+		return nil, errs
+	}
+	return deviceSizeMap, nil
+}
+
+// rescanDevices accepts a map of disk devices to sizes and initiates a rescan for each device.
+// If any rescan fails it captures the error and moves onto the next rescanning the next device.
+func (client *Client) rescanDevices(ctx context.Context, deviceSizeMap map[string]int64) error {
+	var errs error
+	for diskDevice := range deviceSizeMap {
+		if err := client.rescanDisk(ctx, diskDevice); err != nil {
+			errs = errors.Join(errs, fmt.Errorf("failed to rescan disk %s: %s", diskDevice, err))
+		}
+	}
+
+	if errs != nil {
+		return errs
+	}
+	return nil
+}
+
 func (client *Client) RescanDevices(ctx context.Context, targetIQN string, lunID int32, minSize int64) error {
 	GenerateRequestContextForLayer(ctx, LogLayerUtils)
 
@@ -610,36 +653,39 @@ func (client *Client) RescanDevices(ctx context.Context, targetIQN string, lunID
 		return fmt.Errorf("error getting iSCSI device information: %s", err)
 	}
 
-	allLargeEnough := true
-	for _, diskDevice := range deviceInfo.Devices {
-		size, err := client.devices.GetDiskSize(ctx, devices.DevPrefix+diskDevice)
-		if err != nil {
-			return err
+	// Get all disk devices that require a rescan.
+	devicesBySize, err := client.filterDevicesBySize(ctx, deviceInfo, minSize)
+	if err != nil {
+		Logc(ctx).WithError(err).Error("Failed to read disk size for devices.")
+		return err
+	}
+
+	if len(devicesBySize) != 0 {
+		fields = LogFields{
+			"lunID":   lunID,
+			"devices": devicesBySize,
+			"minSize": minSize,
 		}
-		if size < minSize {
-			allLargeEnough = false
-		} else {
-			continue
+
+		Logc(ctx).WithFields(fields).Debug("Found devices that require a rescan.")
+		if err := client.rescanDevices(ctx, devicesBySize); err != nil {
+			Logc(ctx).WithError(err).Error("Failed to initiate rescanning for devices.")
+			return err
 		}
 
-		err = client.rescanDisk(ctx, diskDevice)
+		// Sleep for a second to give the SCSI subsystem time to rescan the devices.
+		time.Sleep(time.Second)
+
+		// Reread the devices to check if any are undersized.
+		devicesBySize, err = client.filterDevicesBySize(ctx, deviceInfo, minSize)
 		if err != nil {
-			Logc(ctx).WithField("diskDevice", diskDevice).Error("Failed to rescan disk.")
-			return fmt.Errorf("failed to rescan disk %s: %s", diskDevice, err)
+			Logc(ctx).WithError(err).Error("Failed to read disk size for devices after rescan.")
+			return err
 		}
-	}
 
-	if !allLargeEnough {
-		time.Sleep(time.Second)
-		for _, diskDevice := range deviceInfo.Devices {
-			size, err := client.devices.GetDiskSize(ctx, devices.DevPrefix+diskDevice)
-			if err != nil {
-				return err
-			}
-			if size < minSize {
-				Logc(ctx).Error("Disk size not large enough after resize.")
-				return fmt.Errorf("disk size not large enough after resize: %d, %d", size, minSize)
-			}
+		if len(devicesBySize) != 0 {
+			Logc(ctx).WithFields(fields).Error("Some devices are still undersized after rescan.")
+			return fmt.Errorf("devices are still undersized after rescan")
 		}
 	}
 
@@ -653,15 +699,16 @@ func (client *Client) RescanDevices(ctx context.Context, targetIQN string, lunID
 		fields = LogFields{"size": size, "minSize": minSize}
 		if size < minSize {
 			Logc(ctx).WithFields(fields).Debug("Reloading the multipath device.")
-			err := client.reloadMultipathDevice(ctx, multipathDevice)
-			if err != nil {
+			if err := client.reloadMultipathDevice(ctx, multipathDevice); err != nil {
 				return err
 			}
 			time.Sleep(time.Second)
-			size, err = client.devices.GetDiskSize(ctx, devices.DevPrefix+multipathDevice)
+
+			size, err := client.devices.GetDiskSize(ctx, devices.DevPrefix+multipathDevice)
 			if err != nil {
 				return err
 			}
+
 			if size < minSize {
 				Logc(ctx).Error("Multipath device not large enough after resize.")
 				return fmt.Errorf("multipath device not large enough after resize: %d < %d", size, minSize)

utils/iscsi/iscsi_test.go

@@ -2283,6 +2283,95 @@ func TestClient_AddSession(t *testing.T) {
 	}
 }
 
+func TestClient_filterDevicesBySize(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	mockDevices := mock_devices.NewMockDevices(mockCtrl)
+
+	client := NewDetailed(
+		"",
+		nil,
+		nil,
+		nil,
+		mockDevices,
+		nil,
+		nil,
+		nil,
+		afero.Afero{},
+		nil,
+	)
+
+	ctx := context.TODO()
+	deviceInfo := &models.ScsiDeviceInfo{
+		Devices: []string{"sda", "sdb"},
+	}
+	minSize := int64(10)
+
+	// Negative case.
+	mockDevices.EXPECT().GetDiskSize(ctx, "/dev/sda").Return(int64(1), nil)
+	mockDevices.EXPECT().GetDiskSize(ctx, "/dev/sdb").Return(int64(0), errors.New("failed to open disk"))
+	deviceSizeMap, err := client.filterDevicesBySize(ctx, deviceInfo, minSize)
+	assert.Error(t, err)
+	assert.Nil(t, deviceSizeMap)
+	assert.NotEqual(t, len(deviceInfo.Devices), len(deviceSizeMap))
+
+	// Positive case #1: Only one device needs a resize.
+	mockDevices.EXPECT().GetDiskSize(ctx, "/dev/sda").Return(minSize, nil)
+	mockDevices.EXPECT().GetDiskSize(ctx, "/dev/sdb").Return(int64(1), nil)
+	deviceSizeMap, err = client.filterDevicesBySize(ctx, deviceInfo, minSize)
+	assert.NoError(t, err)
+	assert.NotNil(t, deviceSizeMap)
+	assert.NotEqual(t, len(deviceInfo.Devices), len(deviceSizeMap))
+
+	// Positive case #2: All devices need to resize.
+	mockDevices.EXPECT().GetDiskSize(ctx, "/dev/sda").Return(int64(1), nil)
+	mockDevices.EXPECT().GetDiskSize(ctx, "/dev/sdb").Return(int64(1), nil)
+	deviceSizeMap, err = client.filterDevicesBySize(ctx, deviceInfo, minSize)
+	assert.NoError(t, err)
+	assert.NotNil(t, deviceSizeMap)
+	assert.Equal(t, len(deviceInfo.Devices), len(deviceSizeMap))
+}
+
+func TestClient_rescanDevices(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	mockDevices := mock_devices.NewMockDevices(mockCtrl)
+
+	fs := afero.NewMemMapFs()
+	_, err := fs.Create("/sys/block/sda/device/rescan")
+	assert.NoError(t, err)
+
+	client := NewDetailed(
+		"",
+		nil,
+		nil,
+		nil,
+		mockDevices,
+		nil,
+		nil,
+		nil,
+		afero.Afero{Fs: fs},
+		nil,
+	)
+
+	ctx := context.TODO()
+	deviceSizeMap := map[string]int64{
+		"sda": 1,
+		"sdb": 1,
+	}
+
+	// Should fail because a device path does not exist.
+	mockDevices.EXPECT().ListAllDevices(ctx).AnyTimes()
+	err = client.rescanDevices(ctx, deviceSizeMap)
+	assert.Error(t, err)
+
+	// Add the missing device path.
+	_, err = fs.Create("/sys/block/sdb/device/rescan")
+	assert.NoError(t, err)
+
+	// Should succeed now that the device path exists.
+	err = client.rescanDevices(ctx, deviceSizeMap)
+	assert.NoError(t, err)
+}
+
 func TestClient_RescanDevices(t *testing.T) {
 	type parameters struct {
 		targetIQN string
@@ -2447,8 +2536,12 @@ func TestClient_RescanDevices(t *testing.T) {
 			getDeviceClient: func(controller *gomock.Controller) devices.Devices {
 				mockDevices := mock_devices.NewMockDevices(controller)
 				mockDevices.EXPECT().FindMultipathDeviceForDevice(context.TODO(), "sda").Return("dm-0").Times(1)
+
+				// This will be called twice because we read from each disk twice during an expansion.
+				mockDevices.EXPECT().GetDiskSize(context.TODO(), "/dev/sda").Return(int64(0), nil).Times(1)
+				mockDevices.EXPECT().ListAllDevices(context.TODO()).Times(2)
 				mockDevices.EXPECT().GetDiskSize(context.TODO(), "/dev/sda").Return(int64(1), nil).Times(1)
-				mockDevices.EXPECT().GetDiskSize(context.TODO(), "/dev/dm-0").Return(int64(1), nil).Times(1)
+				mockDevices.EXPECT().GetDiskSize(context.TODO(), "/dev/dm-0").Return(int64(1), nil)
 				return mockDevices
 			},
 			getCommandClient: func(controller *gomock.Controller) tridentexec.Command {
@@ -2684,6 +2777,7 @@ func TestClient_RescanDevices(t *testing.T) {
 			assertError: assert.NoError,
 		},
 		"happy path": {
+			minSize:   10,
 			targetIQN: targetIQN,
 			getReconcileUtils: func(controller *gomock.Controller) IscsiReconcileUtils {
 				mockReconcileUtils := mock_iscsi.NewMockIscsiReconcileUtils(controller)
@@ -2696,8 +2790,12 @@ func TestClient_RescanDevices(t *testing.T) {
 			getDeviceClient: func(controller *gomock.Controller) devices.Devices {
 				mockDevices := mock_devices.NewMockDevices(controller)
 				mockDevices.EXPECT().FindMultipathDeviceForDevice(context.TODO(), "sda").Return("dm-0").Times(1)
-				mockDevices.EXPECT().GetDiskSize(context.TODO(), "/dev/sda").Return(int64(0), nil)
-				mockDevices.EXPECT().GetDiskSize(context.TODO(), "/dev/dm-0").Return(int64(0), nil)
+
+				// This will be called twice because we read from each disk twice during an expansion.
+				mockDevices.EXPECT().GetDiskSize(context.TODO(), "/dev/sda").Return(int64(0), nil).Times(1)
+				mockDevices.EXPECT().ListAllDevices(context.TODO()).Times(2)
+				mockDevices.EXPECT().GetDiskSize(context.TODO(), "/dev/sda").Return(int64(10), nil).Times(1)
+				mockDevices.EXPECT().GetDiskSize(context.TODO(), "/dev/dm-0").Return(int64(10), nil)
 				return mockDevices
 			},
 			getCommandClient: func(controller *gomock.Controller) tridentexec.Command {
@@ -2706,6 +2804,8 @@ func TestClient_RescanDevices(t *testing.T) {
 			},
 			getFileSystemUtils: func() afero.Fs {
 				fs := afero.NewMemMapFs()
+				_, err := fs.Create("/sys/block/sda/device/rescan")
+				assert.NoError(t, err)
 				return fs
 			},
 			assertError: assert.NoError,
@@ -2728,110 +2828,6 @@ func TestClient_RescanDevices(t *testing.T) {
 	}
 }
 
-func TestClient_rescanDisk(t *testing.T) {
-	type parameters struct {
-		getFileSystemUtils func() afero.Fs
-		assertError        assert.ErrorAssertionFunc
-		getDevices         func(controller *gomock.Controller) devices.Devices
-	}
-	const deviceName = "sda"
-	tests := map[string]parameters{
-		"happy path": {
-			getFileSystemUtils: func() afero.Fs {
-				fs := afero.NewMemMapFs()
-				_, err := fs.Create(fmt.Sprintf("/sys/block/%s/device/rescan", deviceName))
-				assert.NoError(t, err)
-				return fs
-			},
-			getDevices: func(controller *gomock.Controller) devices.Devices {
-				mockDevices := mock_devices.NewMockDevices(controller)
-				mockDevices.EXPECT().ListAllDevices(context.TODO()).Times(2)
-				return mockDevices
-			},
-			assertError: assert.NoError,
-		},
-		"error opening rescan file": {
-			getFileSystemUtils: func() afero.Fs {
-				fs := afero.NewMemMapFs()
-				return fs
-			},
-			getDevices: func(controller *gomock.Controller) devices.Devices {
-				mockDevices := mock_devices.NewMockDevices(controller)
-				mockDevices.EXPECT().ListAllDevices(context.TODO())
-				return mockDevices
-			},
-			assertError: assert.Error,
-		},
-		"error writing to file": {
-			getFileSystemUtils: func() afero.Fs {
-				f := &aferoFileWrapper{
-					WriteStringError: errors.New("some error"),
-					File:             mem.NewFileHandle(&mem.FileData{}),
-				}
-
-				memMapFs := afero.NewMemMapFs()
-				_, err := memMapFs.Create(fmt.Sprintf("/sys/block/%s/device/rescan", deviceName))
-				assert.NoError(t, err)
-
-				fs := &aferoWrapper{
-					openFileResponse: f,
-					openResponse:     f,
-					Fs:               memMapFs,
-				}
-
-				return fs
-			},
-			getDevices: func(controller *gomock.Controller) devices.Devices {
-				mockDevices := mock_devices.NewMockDevices(controller)
-				mockDevices.EXPECT().ListAllDevices(context.TODO())
-				return mockDevices
-			},
-			assertError: assert.Error,
-		},
-		"failed writing to file": {
-			getFileSystemUtils: func() afero.Fs {
-				f := &aferoFileWrapper{
-					WriteStringCount: 0,
-					File:             mem.NewFileHandle(&mem.FileData{}),
-				}
-
-				memMapFs := afero.NewMemMapFs()
-				_, err := memMapFs.Create(fmt.Sprintf("/sys/block/%s/device/rescan", deviceName))
-				assert.NoError(t, err)
-
-				fs := &aferoWrapper{
-					openFileResponse: f,
-					openResponse:     f,
-					Fs:               memMapFs,
-				}
-
-				return fs
-			},
-			getDevices: func(controller *gomock.Controller) devices.Devices {
-				mockDevices := mock_devices.NewMockDevices(controller)
-				mockDevices.EXPECT().ListAllDevices(context.TODO())
-				return mockDevices
-			},
-			assertError: assert.Error,
-		},
-	}
-
-	for name, params := range tests {
-		t.Run(name, func(t *testing.T) {
-			var deviceClient devices.Devices
-			if params.getDevices != nil {
-				deviceClient = params.getDevices(gomock.NewController(t))
-			}
-			client := NewDetailed("", nil, nil, nil, deviceClient, nil, nil, nil,
-				afero.Afero{Fs: params.getFileSystemUtils()}, nil)
-			err := client.rescanDisk(context.TODO(), deviceName)
-			if params.assertError != nil {
-				params.assertError(t, err)
-			}
-		})
-	}
-}
-
 func TestClient_reloadMultipathDevice(t *testing.T) {
 	type parameters struct {
 		multipathDeviceName string