Fix the issue with wrong FsType when the volume is created after retry

Author: VinayKumarHavanur

mocks/mock_storage_drivers/mock_ontap/mock_api.go

@@ -89,7 +89,7 @@ type OntapAPI interface {
 	LunGetGeometry(ctx context.Context, lunPath string) (uint64, error)
 	LunGetFSType(ctx context.Context, lunPath string) (string, error)
 	LunGetAttribute(ctx context.Context, lunPath, attributeName string) (string, error)
-	LunSetAttribute(ctx context.Context, lunPath, attribute, fstype, context, luks, formatOptions string) error
+	LunSetAttribute(ctx context.Context, lunPath, attribute, fstype, context, luks, formatOptions, poolName string) error
 	LunSetComment(ctx context.Context, lunPath, comment string) error
 	LunSetQosPolicyGroup(ctx context.Context, lunPath string, qosPolicyGroup QosPolicyGroup) error
 	LunGetByName(ctx context.Context, name string) (*Lun, error)

storage_drivers/ontap/api/abstraction.go

@@ -2164,7 +2164,7 @@ func (d OntapAPIREST) LunGetGeometry(ctx context.Context, lunPath string) (uint6
 }
 
 func (d OntapAPIREST) LunSetAttribute(
-	ctx context.Context, lunPath, attribute, fstype, context, luks, formatOptions string,
+	ctx context.Context, lunPath, attribute, fstype, context, luks, formatOptions, poolName string,
 ) error {
 	if strings.Contains(lunPath, failureLUNSetAttr) {
 		return errors.New("injected error")
@@ -2196,6 +2196,12 @@ func (d OntapAPIREST) LunSetAttribute(
 		}
 	}
 
+	// Save the pool name attribute at the end. Set new attribute as needed before this.
+	if err := d.api.LunSetAttribute(ctx, lunPath, "poolName", poolName); err != nil {
+		Logc(ctx).WithField("LUN", lunPath).Warning("Failed to save the pool name attribute for new LUN.")
+		return fmt.Errorf("failed to save the pool name attribute for new LUN: %w", err)
+	}
+
 	return nil
 }
 

storage_drivers/ontap/api/abstraction_rest.go

@@ -4003,18 +4003,19 @@ func TestLunSetAttribute(t *testing.T) {
 
 	// case 1: Positive test, update LUN attribute. context is empty.
 	rsi.EXPECT().LunSetAttribute(ctx, "/", "filesystem", "fake-FStype").Return(nil)
-	err := oapi.LunSetAttribute(ctx, "/", "filesystem", "fake-FStype", "", "", "")
+	rsi.EXPECT().LunSetAttribute(ctx, "/", "poolName", "").Return(nil)
+	err := oapi.LunSetAttribute(ctx, "/", "filesystem", "fake-FStype", "", "", "", "")
 	assert.NoError(t, err, "error returned while modifying a LUN attribute")
 
 	// case 2: Positive test, update LUN attribute. pass the value in context..
 	rsi.EXPECT().LunSetAttribute(ctx, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
 	err = oapi.LunSetAttribute(ctx, "/", "filesystem", "fake-FStype",
-		"context", "LUKS", "formatOptions")
+		"context", "LUKS", "formatOptions", "poolName")
 	assert.NoError(t, err, "error returned while modifying a LUN attribute")
 
 	// case 3 Negative test, update LUN attribute returned error..
 	err = oapi.LunSetAttribute(ctx, "failure_7c3a89e2_7d83_457b_9e29_bfdb082c1d8b",
-		"filesystem", "fake-FStype", "context", "LUKS", "formatOptions")
+		"filesystem", "fake-FStype", "context", "LUKS", "formatOptions", "poolName")
 	assert.Error(t, err, "no error returned while modifying a LUN attribute")
 }
 

storage_drivers/ontap/api/abstraction_rest_test.go

@@ -537,7 +537,7 @@ func (d OntapAPIZAPI) LunGetAttribute(ctx context.Context, lunPath, attributeNam
 }
 
 func (d OntapAPIZAPI) LunSetAttribute(
-	ctx context.Context, lunPath, attribute, fstype, context, luks, formatOptions string,
+	ctx context.Context, lunPath, attribute, fstype, context, luks, formatOptions, poolName string,
 ) error {
 	var attrResponse interface{}
 	var err error
@@ -573,6 +573,13 @@ func (d OntapAPIZAPI) LunSetAttribute(
 		}
 	}
 
+	// Save the pool name attribute at the end. Set new attribute as needed before this.
+	attrResponse, err = d.api.LunSetAttribute(lunPath, "poolName", poolName)
+	if err = azgo.GetError(ctx, attrResponse, err); err != nil {
+		Logc(ctx).WithField("LUN", lunPath).Warning("Failed to save the pool name attribute for new LUN.")
+		return fmt.Errorf("failed to save the pool name attribute for new LUN: %w", err)
+	}
+
 	return nil
 }
 

storage_drivers/ontap/api/abstraction_zapi.go

@@ -125,18 +125,19 @@ func TestLunSetAttributeZapi(t *testing.T) {
 
 	// case 1a: Positive test, update LUN attribute - fsType.
 	zapi.EXPECT().LunSetAttribute(tempLunPath, tempAttribute, "fake-FStype").Return(&response, nil).Times(1)
-	err = oapi.LunSetAttribute(ctx, tempLunPath, tempAttribute, "fake-FStype", "", "", "")
+	zapi.EXPECT().LunSetAttribute(tempLunPath, "poolName", "").Return(&response, nil).Times(1)
+	err = oapi.LunSetAttribute(ctx, tempLunPath, tempAttribute, "fake-FStype", "", "", "", "")
 	assert.NoError(t, err, "error returned while modifying a LUN attribute")
 
 	// case 1b: Negative test, d.api.LunSetAttribute for fsType return error
 	zapi.EXPECT().LunSetAttribute(tempLunPath, tempAttribute, "fake-FStype").Return(nil, fmt.Errorf("error")).Times(1)
-	err = oapi.LunSetAttribute(ctx, tempLunPath, tempAttribute, "fake-FStype", "", "", "")
+	err = oapi.LunSetAttribute(ctx, tempLunPath, tempAttribute, "fake-FStype", "", "", "", "")
 	assert.Error(t, err)
 
-	// case 2: Positive test, update LUN attributes those are: context, luks, formatOptions.
+	// case 2: Positive test, update LUN attributes those are: context, luks, formatOptions, poolName.
 	zapi.EXPECT().LunSetAttribute(tempLunPath, gomock.Any(), gomock.Any()).Return(&response, nil).AnyTimes()
 	err = oapi.LunSetAttribute(ctx, tempLunPath, "filesystem", "",
-		"context", "LUKS", "formatOptions")
+		"context", "LUKS", "formatOptions", "poolName")
 	assert.NoError(t, err, "error returned while modifying a LUN attribute")
 }
 

storage_drivers/ontap/api/abstraction_zapi_test.go

@@ -406,9 +406,9 @@ func (d *ASAStorageDriver) Create(
 
 	// Save the fstype in a LUN attribute so we know what to do in Attach.  If this fails, clean up and
 	// move on to the next pool.
-	// Save the context, fstype, and LUKS value in LUN comment
+	// Save the context, fstype, LUKS value, and pool name in LUN comment
 	err = d.API.LunSetAttribute(ctx, name, LUNAttributeFSType, fstype, string(d.Config.DriverContext),
-		luksEncryption, formatOptions)
+		luksEncryption, formatOptions, storagePool.Name())
 	if err != nil {
 
 		errMessage := fmt.Sprintf("error saving file system type for LUN %s: %v", name, err)

storage_drivers/ontap/ontap_asa.go

@@ -663,7 +663,7 @@ func TestCreateASA(t *testing.T) {
 				mockAPI.EXPECT().TieringPolicyValue(ctx).Return("fake").Times(1)
 				mockAPI.EXPECT().LunCreate(ctx, gomock.Any()).Return(nil).Times(1)
 				mockAPI.EXPECT().LunSetComment(ctx, volumeName, labels).Return(nil).Times(1)
-				mockAPI.EXPECT().LunSetAttribute(ctx, volumeName, LUNAttributeFSType, storagePool.InternalAttributes()[FileSystemType], string(driver.Config.DriverContext), storagePool.InternalAttributes()[LUKSEncryption], storagePool.InternalAttributes()[FormatOptions]).Return(nil).Times(1)
+				mockAPI.EXPECT().LunSetAttribute(ctx, volumeName, LUNAttributeFSType, storagePool.InternalAttributes()[FileSystemType], string(driver.Config.DriverContext), storagePool.InternalAttributes()[LUKSEncryption], storagePool.InternalAttributes()[FormatOptions], storagePool.Name()).Return(nil).Times(1)
 			},
 			verify: func(t *testing.T, err error) {
 				assert.NoError(t, err, "Should not be an error")
@@ -689,7 +689,7 @@ func TestCreateASA(t *testing.T) {
 				mockAPI.EXPECT().TieringPolicyValue(ctx).Return("fake").Times(1)
 				mockAPI.EXPECT().LunCreate(ctx, gomock.Any()).Return(nil).Times(1)
 				mockAPI.EXPECT().LunSetComment(ctx, volumeName, labels).Return(nil).Times(1)
-				mockAPI.EXPECT().LunSetAttribute(ctx, volumeName, LUNAttributeFSType, storagePool.InternalAttributes()[FileSystemType], string(driver.Config.DriverContext), storagePool.InternalAttributes()[LUKSEncryption], storagePool.InternalAttributes()[FormatOptions]).Return(nil).Times(1)
+				mockAPI.EXPECT().LunSetAttribute(ctx, volumeName, LUNAttributeFSType, storagePool.InternalAttributes()[FileSystemType], string(driver.Config.DriverContext), storagePool.InternalAttributes()[LUKSEncryption], storagePool.InternalAttributes()[FormatOptions], storagePool.Name()).Return(nil).Times(1)
 			},
 			verify: func(t *testing.T, err error) {
 				assert.NoError(t, err, "Should not be an error")
@@ -708,7 +708,7 @@ func TestCreateASA(t *testing.T) {
 						assert.Equal(t, expectedLabels, labels, "Labels should match the expected value")
 						return nil
 					}).Times(1)
-				mockAPI.EXPECT().LunSetAttribute(ctx, volumeName, LUNAttributeFSType, storagePool.InternalAttributes()[FileSystemType], string(driver.Config.DriverContext), storagePool.InternalAttributes()[LUKSEncryption], storagePool.InternalAttributes()[FormatOptions]).Return(nil).Times(1)
+				mockAPI.EXPECT().LunSetAttribute(ctx, volumeName, LUNAttributeFSType, storagePool.InternalAttributes()[FileSystemType], string(driver.Config.DriverContext), storagePool.InternalAttributes()[LUKSEncryption], storagePool.InternalAttributes()[FormatOptions], storagePool.Name()).Return(nil).Times(1)
 			},
 			verify: func(t *testing.T, err error) {
 				assert.NoError(t, err, "Should not be an error")
@@ -783,7 +783,8 @@ func TestCreateASA(t *testing.T) {
 					storagePool.InternalAttributes()[FileSystemType],
 					string(driver.Config.DriverContext),
 					storagePool.InternalAttributes()[LUKSEncryption],
-					storagePool.InternalAttributes()[FormatOptions]).
+					storagePool.InternalAttributes()[FormatOptions],
+					storagePool.Name()).
 					Return(fmt.Errorf("api-error")).Times(1)
 				mockAPI.EXPECT().LunDestroy(ctx, volumeName).Return(nil).Times(1)
 			},

storage_drivers/ontap/ontap_asa_test.go

@@ -259,8 +259,11 @@ func (d *SANStorageDriver) validate(ctx context.Context) error {
 	return nil
 }
 
-// destroyVolumeIfNoLUN attempts to destroy volume if there exists a volume with no associated LUN.
-// This is used to make Create() idempotent by cleaning up a Flexvol with no LUN.
+// cleanupIncompleteLUN attempts to destroy volume if there exists a volume with no associated LUN.
+// This is used to make Create() idempotent by cleaning up a Flexvol with no LUN or
+// the LUN exists but is associated with a different pool as it was not cleaned up properly and creation is retried
+// with different pool.
+// This can happen if volume creation succeeded but LUN creation failed in a previous Create() call.
 // Returns (Volume State, error)
 //
 //	Caller can check for:
@@ -270,15 +273,18 @@ func (d *SANStorageDriver) validate(ctx context.Context) error {
 //	   - Could not destroy the required volume for an error.
 //	Volume state:true indicating both volume and required LUN exist.
 //	Volume state:false indicating no volume existed or cleaned up now.
-func (d *SANStorageDriver) destroyVolumeIfNoLUN(ctx context.Context, volConfig *storage.VolumeConfig) (bool, error) {
+func (d *SANStorageDriver) cleanupIncompleteLUN(
+	ctx context.Context, volConfig *storage.VolumeConfig, poolName string,
+) (bool, error) {
 	name := volConfig.InternalName
 	fields := LogFields{
-		"Method": "destroyVolumeIfNoLUN",
-		"Type":   "SANStorageDriver",
-		"name":   name,
+		"Method":   "cleanupIncompleteLUN",
+		"Type":     "SANStorageDriver",
+		"name":     name,
+		"poolName": poolName,
 	}
-	Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> destroyVolumeIfNoLUN")
-	defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< destroyVolumeIfNoLUN")
+	Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> cleanupIncompleteLUN")
+	defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< cleanupIncompleteLUN")
 
 	volExists, err := d.API.VolumeExists(ctx, name)
 	if err != nil {
@@ -296,20 +302,44 @@ func (d *SANStorageDriver) destroyVolumeIfNoLUN(ctx context.Context, volConfig *
 	// Verify if LUN exists.
 	newLUNPath := lunPath(name)
 	extantLUN, err := d.API.LunGetByName(ctx, newLUNPath)
-	if extantLUN != nil {
-		// Volume and LUN both exist. No clean up needed.
-		return true, nil
-	}
-	if !errors.IsNotFoundError(err) {
+	if err != nil && !errors.IsNotFoundError(err) {
 		// Could not verify if LUN exists. Clean up pending.
 		return false, fmt.Errorf("error checking for existing LUN %s: %v", newLUNPath, err)
 	}
-	// LUN does not exist, but volume. Initiate clean-up.
-	if err = d.API.VolumeDestroy(ctx, name, true, true); err != nil {
-		Logc(ctx).WithField("volume", name).Errorf("Could not clean up volume: %v", err)
-		return true, fmt.Errorf("could not clean up partial create of vol/lun: %v", err)
+
+	var destroyReason string
+	var destroyErrorMsg string
+
+	if extantLUN != nil {
+		// Both the volume and LUN exist. Check if the last attribute set on the LUN, i.e., the pool name, matches.
+		lunPoolName, err := d.API.LunGetAttribute(ctx, newLUNPath, "poolName")
+		if err != nil || lunPoolName != poolName {
+			// If there is an error getting pool name or pool name doesn't match
+			Logc(ctx).WithFields(LogFields{
+				"LUN":           newLUNPath,
+				"lunPoolName":   lunPoolName,
+				"inputPoolName": poolName,
+				"error":         err,
+			}).Info("Pool name not found or mismatch detected. Destroying volume.")
+
+			destroyReason = "Destroyed volume with mismatched pool name."
+			destroyErrorMsg = "could not destroy volume with mismatched pool"
+		} else {
+			// Pool name matches or no pool name attribute. No clean up needed.
+			return true, nil
+		}
+	} else {
+		// LUN does not exist, but volume does. Initiate clean-up.
+		destroyReason = "Cleaned up volume since LUN create failed."
+		destroyErrorMsg = "could not clean up partial create of vol/lun"
 	}
-	Logc(ctx).WithField("volume", name).Debug("Cleaned up volume since LUN create failed.")
+
+	if err := d.API.VolumeDestroy(ctx, name, true, true); err != nil {
+		Logc(ctx).WithError(err).WithField("volume", name).Errorf("Could not clean up volume")
+		return true, fmt.Errorf("%s: %v", destroyErrorMsg, err)
+	}
+	Logc(ctx).WithField("volume", name).Debug(destroyReason)
+
 	return false, nil
 }
 
@@ -331,7 +361,7 @@ func (d *SANStorageDriver) Create(
 	defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< Create")
 
 	// Early exit if volume+LUN exist. Clean up volume if no LUN exists.
-	volExists, err := d.destroyVolumeIfNoLUN(ctx, volConfig)
+	volExists, err := d.cleanupIncompleteLUN(ctx, volConfig, storagePool.Name())
 	if err != nil {
 		return fmt.Errorf("failure checking for existence of volume and cleaning if any: %v", err)
 	}
@@ -573,9 +603,9 @@ func (d *SANStorageDriver) Create(
 
 			// Save the fstype in a LUN attribute so we know what to do in Attach.  If this fails, clean up and
 			// move on to the next pool.
-			// Save the context, fstype, and LUKS value in LUN comment
+			// Save the context, fstype, LUKS value, and pool name in LUN comment
 			err = d.API.LunSetAttribute(ctx, lunPath, LUNAttributeFSType, fstype, string(d.Config.DriverContext),
-				luksEncryption, formatOptions)
+				luksEncryption, formatOptions, storagePool.Name())
 			if err != nil {
 
 				errMessage := fmt.Sprintf("ONTAP-SAN pool %s/%s; error saving file system type for LUN %s: %v",

storage_drivers/ontap/ontap_san.go

@@ -693,7 +693,7 @@ func (d *SANEconomyStorageDriver) Create(
 
 		// Save the fstype in a LUN attribute so we know what to do in Attach
 		err = d.API.LunSetAttribute(ctx, lunPathEco, LUNAttributeFSType, fstype, string(d.Config.DriverContext),
-			luksEncryption, formatOptions)
+			luksEncryption, formatOptions, storagePool.Name())
 		if err != nil {
 
 			errMessage := fmt.Sprintf(