Revert publish info field from rawDevicePath to devicePath and attempt to recover the missing devicePath

This change reverts publish info field from rawDevicePath to devicePath. It also makes a better effort to recover the devicePath field for Filesystem-based volumes (non-raw block) that may have been lost during pre-23.01 to 23.01/23.04 upgrades.

Author: rohit-arora-dev

CHANGELOG.md

@@ -7,6 +7,12 @@
 **Fixes:**
 
 - Fixed ONTAP ZAPI request to ensure LUN serial number is queried when getting LUN attributes.
+- Revert publish info field from `rawDevicePath` to `devicePath`; added logic to populate and recover (in some cases)
+  `devicePath` field.
+
+**Enhancements:**
+- Added additional logic to verify iSCSI multipath device serial number and size.
+- Additional verification for iSCSI volumes to ensure correct multipath device is unstaged.
 
 ## v23.04.0
 

frontend/csi/node_helpers/kubernetes/plugin.go

@@ -17,6 +17,7 @@ import (
 	"github.com/netapp/trident/frontend/csi"
 	nodehelpers "github.com/netapp/trident/frontend/csi/node_helpers"
 	. "github.com/netapp/trident/logging"
+	"github.com/netapp/trident/utils"
 )
 
 var osFs = afero.NewOsFs()
@@ -109,13 +110,18 @@ func (h *helper) reconcileVolumePublishInfo(ctx context.Context) error {
 		h.publishedPaths = publishedPaths
 	}
 
+	pvToDeviceMappings, err := utils.PVMountpointMappings(ctx)
+	if err != nil {
+		Logc(ctx).Errorf("Unable to get devices for mounted PVs.")
+	}
+
 	for _, file := range files {
 		// On Windows, the staging path is a directory whose name is the volume ID (e.g. pvc-<uuid>). We don't want to
 		// attempt to reconcile directories.
 		if file.IsDir() {
 			continue
 		}
-		err := h.reconcileVolumePublishInfoFile(ctx, file.Name())
+		err := h.reconcileVolumePublishInfoFile(ctx, file.Name(), pvToDeviceMappings)
 		if err != nil {
 			return err
 		}
@@ -124,7 +130,9 @@ func (h *helper) reconcileVolumePublishInfo(ctx context.Context) error {
 	return nil
 }
 
-func (h *helper) reconcileVolumePublishInfoFile(ctx context.Context, file string) error {
+func (h *helper) reconcileVolumePublishInfoFile(
+	ctx context.Context, file string, pvToDeviceMappings map[string]string,
+) error {
 	volumeId := strings.ReplaceAll(file, ".json", "")
 	paths, ok := h.publishedPaths[volumeId]
 	if !ok {
@@ -133,7 +141,7 @@ func (h *helper) reconcileVolumePublishInfoFile(ctx context.Context, file string
 		Log().Warningf("Could not determine determine published paths for volume: %s", volumeId)
 	}
 
-	shouldDelete, err := h.VolumePublishManager.UpgradeVolumeTrackingFile(ctx, volumeId, paths)
+	shouldDelete, err := h.VolumePublishManager.UpgradeVolumeTrackingFile(ctx, volumeId, paths, pvToDeviceMappings)
 	if err != nil {
 		Log().Infof("Volume tracking file upgrade failed for volume: %s .", volumeId)
 		return err

frontend/csi/node_helpers/kubernetes/plugin_test.go

@@ -102,7 +102,8 @@ func TestReconcileVolumeTrackingInfo(t *testing.T) {
 
 	_ = osFs.Mkdir("/pods", 0o777)
 	mockVolumePublishManager.EXPECT().GetVolumeTrackingFiles().Return([]os.FileInfo{fInfo}, nil)
-	mockVolumePublishManager.EXPECT().UpgradeVolumeTrackingFile(ctx, volId, make(map[string]struct{})).
+	mockVolumePublishManager.EXPECT().UpgradeVolumeTrackingFile(ctx, volId, make(map[string]struct{}),
+		make(map[string]string)).
 		Return(false, errors.New("foo"))
 	err = h.reconcileVolumePublishInfo(ctx)
 	assert.Error(t, err, "expected error if reconcile file fails")
@@ -127,27 +128,31 @@ func TestReconcileVolumeTrackingInfoFile(t *testing.T) {
 
 	validH := newValidHelper(orchestrator, volId, mockVolumePublishManager)
 
-	mockVolumePublishManager.EXPECT().UpgradeVolumeTrackingFile(ctx, volId, paths).Return(false, nil)
+	mockVolumePublishManager.EXPECT().UpgradeVolumeTrackingFile(ctx, volId, paths,
+		nil).Return(false, nil)
 	mockVolumePublishManager.EXPECT().ValidateTrackingFile(ctx, volId).Return(false, nil)
 
-	err := validH.reconcileVolumePublishInfoFile(ctx, fName)
+	err := validH.reconcileVolumePublishInfoFile(ctx, fName, nil)
 	assert.NoError(t, err, "did not expect an error during reconcile")
 
-	mockVolumePublishManager.EXPECT().UpgradeVolumeTrackingFile(ctx, volId, paths).Return(false, errors.New("foo"))
-	err = validH.reconcileVolumePublishInfoFile(ctx, fName)
+	mockVolumePublishManager.EXPECT().UpgradeVolumeTrackingFile(ctx, volId, paths,
+		nil).Return(false, errors.New("foo"))
+	err = validH.reconcileVolumePublishInfoFile(ctx, fName, nil)
 	assert.Error(t, err, "expected error if upgrade to tracking file occurred")
 
-	mockVolumePublishManager.EXPECT().UpgradeVolumeTrackingFile(gomock.Any(), volId, paths).Return(false, nil)
+	mockVolumePublishManager.EXPECT().UpgradeVolumeTrackingFile(gomock.Any(), volId, paths,
+		nil).Return(false, nil)
 	mockVolumePublishManager.EXPECT().ValidateTrackingFile(gomock.Any(), volId).Return(true, nil)
 	mockVolumePublishManager.EXPECT().DeleteTrackingInfo(gomock.Any(), volId).Return(errors.New("foo error"))
 
-	err = validH.reconcileVolumePublishInfoFile(ctx, fName)
+	err = validH.reconcileVolumePublishInfoFile(ctx, fName, nil)
 	assert.Error(t, err, "expected error if file delete failed")
 
-	mockVolumePublishManager.EXPECT().UpgradeVolumeTrackingFile(ctx, volId, paths).Return(false, nil)
+	mockVolumePublishManager.EXPECT().UpgradeVolumeTrackingFile(ctx, volId, paths,
+		nil).Return(false, nil)
 	mockVolumePublishManager.EXPECT().ValidateTrackingFile(ctx, volId).Return(false, errors.New("foo"))
 
-	err = validH.reconcileVolumePublishInfoFile(ctx, fName)
+	err = validH.reconcileVolumePublishInfoFile(ctx, fName, nil)
 	assert.Error(t, err, "expected error if validate tracking file error occurred")
 }
 

frontend/csi/node_helpers/types.go

@@ -30,7 +30,7 @@ type VolumePublishManager interface {
 	ReadTrackingInfo(context.Context, string) (*utils.VolumeTrackingInfo, error)
 	DeleteTrackingInfo(context.Context, string) error
 	ListVolumeTrackingInfo(context.Context) (map[string]*utils.VolumeTrackingInfo, error)
-	UpgradeVolumeTrackingFile(context.Context, string, map[string]struct{}) (bool, error)
+	UpgradeVolumeTrackingFile(context.Context, string, map[string]struct{}, map[string]string) (bool, error)
 	DeleteFailedUpgradeTrackingFile(context.Context, os.FileInfo)
 	ValidateTrackingFile(context.Context, string) (bool, error)
 	GetVolumeTrackingFiles() ([]os.FileInfo, error)

frontend/csi/volume_publish_manager.go

@@ -98,6 +98,15 @@ func (v *VolumePublishManager) readTrackingInfo(
 	}
 
 	Logc(ctx).WithField("volumeTrackingInfo", volumeTrackingInfo).Debug("Volume tracking info found.")
+
+	// Given upgrade logic in volume_publish_manager, this should not
+	// be the case but adding an extra check here to move the
+	// rawDevicePath value to devicePath.
+	if volumeTrackingInfo.RawDevicePath != "" && volumeTrackingInfo.DevicePath == "" {
+		volumeTrackingInfo.DevicePath = volumeTrackingInfo.RawDevicePath
+		volumeTrackingInfo.RawDevicePath = ""
+	}
+
 	return &volumeTrackingInfo, nil
 }
 
@@ -153,7 +162,7 @@ func (v *VolumePublishManager) DeleteTrackingInfo(ctx context.Context, volumeID
 // stagedDeviceInfo or legacy tracking file do not exist, or are unable to be unmarshalled, then an upgrade is not
 // possible, and we must delete the tracking file because it no longer has any value.
 func (v *VolumePublishManager) UpgradeVolumeTrackingFile(
-	ctx context.Context, volumeId string, publishedPaths map[string]struct{},
+	ctx context.Context, volumeId string, publishedPaths map[string]struct{}, pvToDeviceMappings map[string]string,
 ) (bool, error) {
 	var err error
 	fields := LogFields{"volumeId": volumeId}
@@ -178,8 +187,81 @@ func (v *VolumePublishManager) UpgradeVolumeTrackingFile(
 	// upon unmarshalling the json.
 	if volumeTrackingInfo.VolumePublishInfo.FilesystemType != "" {
 		Logc(ctx).Debug("Volume tracking method did not need to be upgraded.")
+
+		// For iSCSI case confirm iSCSI `devicePath` exists, if not check
+		// `rawDevicePath` exist, if yes then copy the value else log an
+		// error message in logs.
+		if volumeTrackingInfo.VolumePublishInfo.IscsiTargetPortal == "" {
+			Logc(ctx).Debug("IscsiTargetPortal was empty in volume publish info.")
+			return false, nil
+		}
+
+		Logc(ctx).Debug("Ensuring devicePath is present if not then attempting to recover it.")
+
+		// In 23.01 devicePath was changed to rawDevicePath, which led to missing devicePath for
+		// attached volumes from pre-23.01 and newly created volumes instead of devicePath would
+		// rawDevicePath. Starting 23.07, devicePath has been re-introduced, below effort ensures
+		// devicePath value is populated based on rawDevicePath, if both are missing Trident
+		// tries to identify the correct device based on the published paths.
+		if volumeTrackingInfo.DevicePath == "" {
+			if volumeTrackingInfo.RawDevicePath != "" {
+				volumeTrackingInfo.DevicePath = volumeTrackingInfo.RawDevicePath
+				volumeTrackingInfo.RawDevicePath = ""
+
+				Logc(ctx).WithFields(LogFields{
+					"volumeID":          volumeId,
+					"devicePath":        volumeTrackingInfo.DevicePath,
+					"iscsiTargetPortal": volumeTrackingInfo.IscsiTargetPortal,
+					"lun":               volumeTrackingInfo.IscsiLunNumber,
+				}).Debug("Updating new publish info records.")
+			} else if len(volumeTrackingInfo.PublishedPaths) > 0 {
+				// This is a best-effort to identify a missing device path
+				for publishedPath := range volumeTrackingInfo.PublishedPaths {
+					if device, ok := pvToDeviceMappings[publishedPath]; ok {
+						volumeTrackingInfo.DevicePath = device
+						Logc(ctx).WithFields(LogFields{
+							"volumeID":          volumeId,
+							"devicePath":        volumeTrackingInfo.DevicePath,
+							"iscsiTargetPortal": volumeTrackingInfo.IscsiTargetPortal,
+							"lun":               volumeTrackingInfo.IscsiLunNumber,
+						}).Debug("Updating new publish info records based on published paths.")
+					}
+				}
+			}
+
+			if volumeTrackingInfo.DevicePath != "" {
+				err = v.WriteTrackingInfo(ctx, volumeId, volumeTrackingInfo)
+				if err != nil {
+					Logc(ctx).WithFields(LogFields{
+						"volumeID":          volumeId,
+						"devicePath":        volumeTrackingInfo.DevicePath,
+						"iscsiTargetPortal": volumeTrackingInfo.IscsiTargetPortal,
+						"lun":               volumeTrackingInfo.IscsiLunNumber,
+					}).Error("Failed to update tracking file with device path information.")
+				}
+			} else {
+				Logc(ctx).WithFields(LogFields{
+					"volumeID":          volumeId,
+					"iscsiTargetPortal": volumeTrackingInfo.IscsiTargetPortal,
+					"lun":               volumeTrackingInfo.IscsiLunNumber,
+				}).Error("New publish info is missing device path.")
+			}
+		} else if volumeTrackingInfo.RawDevicePath != "" {
+			Logc(ctx).WithFields(LogFields{
+				"volumeID":          volumeId,
+				"iscsiTargetPortal": volumeTrackingInfo.IscsiTargetPortal,
+				"lun":               volumeTrackingInfo.IscsiLunNumber,
+				"devicePath":        volumeTrackingInfo.DevicePath,
+				"rawDevicePath":     volumeTrackingInfo.RawDevicePath,
+			}).Warn("Found both devices.")
+
+			// No need to have two sources of device path information
+			volumeTrackingInfo.RawDevicePath = ""
+		}
+
 		return false, nil
 	}
+
 	file = path.Join(volumeTrackingInfo.StagingTargetPath, volumePublishInfoFilename)
 	err = utils.JsonReaderWriter.ReadJSONFile(ctx, publishInfo, file, "publish info")
 	if err != nil {
@@ -202,6 +284,41 @@ func (v *VolumePublishManager) UpgradeVolumeTrackingFile(
 	volumeTrackingInfo.VolumePublishInfo = *publishInfo
 	volumeTrackingInfo.PublishedPaths = publishedPaths
 
+	// (arorar): I do not think this condition will ever be true since `rawDevicePath`
+	//           was introduced after this migration logic and `devicePath` has been re-introduced.
+	if volumeTrackingInfo.VolumePublishInfo.IscsiTargetPortal != "" {
+		if volumeTrackingInfo.DevicePath == "" {
+			if volumeTrackingInfo.RawDevicePath != "" {
+				volumeTrackingInfo.DevicePath = volumeTrackingInfo.RawDevicePath
+				volumeTrackingInfo.RawDevicePath = ""
+
+				Logc(ctx).WithFields(LogFields{
+					"volumeID":          volumeId,
+					"devicePath":        volumeTrackingInfo.DevicePath,
+					"iscsiTargetPortal": volumeTrackingInfo.IscsiTargetPortal,
+					"lun":               volumeTrackingInfo.IscsiLunNumber,
+				}).Debug("Updating publish info records.")
+			} else {
+				Logc(ctx).WithFields(LogFields{
+					"volumeID":          volumeId,
+					"iscsiTargetPortal": volumeTrackingInfo.IscsiTargetPortal,
+					"lun":               volumeTrackingInfo.IscsiLunNumber,
+				}).Errorf("Publish info is missing device path.")
+			}
+		} else if volumeTrackingInfo.RawDevicePath != "" {
+			Logc(ctx).WithFields(LogFields{
+				"volumeID":          volumeId,
+				"iscsiTargetPortal": volumeTrackingInfo.IscsiTargetPortal,
+				"lun":               volumeTrackingInfo.IscsiLunNumber,
+				"devicePath":        volumeTrackingInfo.DevicePath,
+				"rawDevicePath":     volumeTrackingInfo.RawDevicePath,
+			}).Warn("Found both devices.")
+
+			// No need to have two sources of device path information
+			volumeTrackingInfo.RawDevicePath = ""
+		}
+	}
+
 	Logc(ctx).WithField("publishInfoLocation", volumeTrackingInfo).Debug("Publish info location found.")
 	err = v.WriteTrackingInfo(ctx, volumeId, volumeTrackingInfo)
 	if err != nil {