Implement unique subsystem names for regular ONTAP NVMe driver

Author: VinayKumarHavanur

storage_drivers/ontap/api/abstraction_rest.go

@@ -3520,6 +3520,7 @@ func (d OntapAPIREST) NVMeEnsureNamespaceMapped(ctx context.Context, subsystemUU
 // NVMeEnsureNamespaceUnmapped first checks if a namespace is mapped to the subsystem and if it is mapped:
 // a) removes the namespace from the subsystem
 // b) deletes the subsystem if no more namespaces are attached to it
+// c) Handles the case where multiple hosts are mapped to single common subsystem arising out of lengthy node names.
 // If namespace is not mapped to subsystem, it is treated as success
 // The function also returns a bool value along with error. A true value denotes the subsystem is deleted
 // successfully and Published info can be removed for the NVMe volume
@@ -3549,8 +3550,15 @@ func (d OntapAPIREST) NVMeEnsureNamespaceUnmapped(ctx context.Context, hostNQN,
 		}
 	}
 
-	// In case of multiple hosts attached to a subsystem (e.g. in RWX case), do not delete the namespace,
-	// subsystem or the published info
+	// Below are the cases where multiple hosts are attached to a subsystem:
+	// case 1: RWX case - Multiple hosts are intentionally sharing the same namespace for ReadWriteMany access.
+	//         In this scenario, do not delete the namespace mapping, as other hosts may still be using it.
+	// case 2: Common subsystem due to lengthy host names - When host names exceed the allowed length,
+	//         multiple hosts may be mapped to a single "common" subsystem. In this scenario, the subsystem
+	//         is not truly shared for RWX purposes, but is a result of host name truncation. Therefore,
+	//         it is appropriate to remove the namespace mapping when detaching a host, as the mapping is
+	//         not required for other hosts. This differs from the RWX case, where the namespace mapping
+	//         must be preserved for legitimate multi-host access.
 	if len(subsystemHosts) > 1 {
 		if hostFound {
 			Logc(ctx).Infof("Multiple hosts are attached to this subsystem %v. Do not delete namespace or subsystem",
@@ -3560,6 +3568,21 @@ func (d OntapAPIREST) NVMeEnsureNamespaceUnmapped(ctx context.Context, hostNQN,
 				return false, err
 			}
 		}
+
+		// Check if there are multiple namespaces attached to the subsystem,
+		// This indicates case 2, so, remove the namespace only.
+		count, err := d.api.NVMeNamespaceCount(ctx, subsystemUUID)
+		if err != nil {
+			return false, fmt.Errorf("error getting namespace count for subsystem %s; %v", subsystemUUID, err)
+		}
+
+		if count > 1 {
+			err = d.api.NVMeSubsystemRemoveNamespace(ctx, subsystemUUID, namespaceUUID)
+			if err != nil {
+				return false, fmt.Errorf("error removing namespace %s from subsystem %s; %v", namespaceUUID, subsystemUUID, err)
+			}
+		}
+
 		return false, nil
 	}
 
@@ -3583,7 +3606,7 @@ func (d OntapAPIREST) NVMeEnsureNamespaceUnmapped(ctx context.Context, hostNQN,
 		return false, fmt.Errorf("error getting namespace count for subsystem %s; %v", subsystemUUID, err)
 	}
 
-	// Delete the subsystem if no. of namespaces is 0
+	// Delete the subsystem if the namespace count for this subsystem is 0 after removal.
 	if count == 0 {
 		if err := d.api.NVMeSubsystemDelete(ctx, subsystemUUID); err != nil {
 			return false, fmt.Errorf("error deleting subsystem %s; %v", subsystemUUID, err)

storage_drivers/ontap/api/abstraction_rest_test.go

@@ -964,6 +964,7 @@ func TestNVMeNamespaceUnmapped(t *testing.T) {
 	mock.EXPECT().NVMeIsNamespaceMapped(ctx, subsystemUUID, nsUUID).Return(true, nil).Times(1)
 	mock.EXPECT().NVMeGetHostsOfSubsystem(ctx, subsystemUUID).Return([]*models.NvmeSubsystemHost{host1, host2}, nil).Times(1)
 	mock.EXPECT().NVMeRemoveHostFromSubsystem(ctx, hostNQN, subsystemUUID).Return(nil).Times(1)
+	mock.EXPECT().NVMeNamespaceCount(ctx, subsystemUUID).Return(int64(1), nil).Times(1)
 
 	removePublishInfo, err = oapi.NVMeEnsureNamespaceUnmapped(ctx, hostNQN, subsystemUUID, nsUUID)
 
@@ -1035,11 +1036,84 @@ func TestNVMeNamespaceUnmapped(t *testing.T) {
 	mock.EXPECT().NVMeIsNamespaceMapped(ctx, subsystemUUID, nsUUID).Return(true, nil).Times(1)
 	mock.EXPECT().NVMeGetHostsOfSubsystem(ctx, subsystemUUID).Return([]*models.NvmeSubsystemHost{host1, host2},
 		nil).Times(1)
+	mock.EXPECT().NVMeNamespaceCount(ctx, subsystemUUID).Return(int64(1), nil).Times(1)
 
 	removePublishInfo, err = oapi.NVMeEnsureNamespaceUnmapped(ctx, nonExistentHostNQN, subsystemUUID, nsUUID)
 
 	assert.Equal(t, false, removePublishInfo, "nqn is unmapped")
 	assert.NoError(t, err)
+
+	// case 13: Multiple hosts with error getting namespace count
+	mock.EXPECT().ClientConfig().Return(clientConfig).AnyTimes()
+	mock.EXPECT().NVMeIsNamespaceMapped(ctx, subsystemUUID, nsUUID).Return(true, nil).Times(1)
+	mock.EXPECT().NVMeGetHostsOfSubsystem(ctx, subsystemUUID).Return([]*models.NvmeSubsystemHost{host1, host2}, nil).Times(1)
+	mock.EXPECT().NVMeRemoveHostFromSubsystem(ctx, hostNQN, subsystemUUID).Return(nil).Times(1)
+	mock.EXPECT().NVMeNamespaceCount(ctx, subsystemUUID).Return(int64(0), errors.New("Error getting namespace count")).Times(1)
+
+	removePublishInfo, err = oapi.NVMeEnsureNamespaceUnmapped(ctx, hostNQN, subsystemUUID, nsUUID)
+
+	assert.Equal(t, false, removePublishInfo, "subsystem removed")
+	assert.Error(t, err)
+
+	// case 14: Multiple hosts with namespace count > 1, error removing namespace
+	mock.EXPECT().ClientConfig().Return(clientConfig).AnyTimes()
+	mock.EXPECT().NVMeIsNamespaceMapped(ctx, subsystemUUID, nsUUID).Return(true, nil).Times(1)
+	mock.EXPECT().NVMeGetHostsOfSubsystem(ctx, subsystemUUID).Return([]*models.NvmeSubsystemHost{host1, host2}, nil).Times(1)
+	mock.EXPECT().NVMeRemoveHostFromSubsystem(ctx, hostNQN, subsystemUUID).Return(nil).Times(1)
+	mock.EXPECT().NVMeNamespaceCount(ctx, subsystemUUID).Return(int64(2), nil).Times(1)
+	mock.EXPECT().NVMeSubsystemRemoveNamespace(ctx, subsystemUUID, nsUUID).Return(errors.New("Error removing namespace")).Times(1)
+
+	removePublishInfo, err = oapi.NVMeEnsureNamespaceUnmapped(ctx, hostNQN, subsystemUUID, nsUUID)
+
+	assert.Equal(t, false, removePublishInfo, "subsystem removed")
+	assert.Error(t, err)
+
+	// case 15: Multiple hosts with namespace count > 1, success removing namespace
+	mock.EXPECT().ClientConfig().Return(clientConfig).AnyTimes()
+	mock.EXPECT().NVMeIsNamespaceMapped(ctx, subsystemUUID, nsUUID).Return(true, nil).Times(1)
+	mock.EXPECT().NVMeGetHostsOfSubsystem(ctx, subsystemUUID).Return([]*models.NvmeSubsystemHost{host1, host2}, nil).Times(1)
+	mock.EXPECT().NVMeRemoveHostFromSubsystem(ctx, hostNQN, subsystemUUID).Return(nil).Times(1)
+	mock.EXPECT().NVMeNamespaceCount(ctx, subsystemUUID).Return(int64(3), nil).Times(1)
+	mock.EXPECT().NVMeSubsystemRemoveNamespace(ctx, subsystemUUID, nsUUID).Return(nil).Times(1)
+
+	removePublishInfo, err = oapi.NVMeEnsureNamespaceUnmapped(ctx, hostNQN, subsystemUUID, nsUUID)
+
+	assert.Equal(t, false, removePublishInfo, "subsystem is not removed due to multiple namespaces")
+	assert.NoError(t, err)
+
+	// case 16: Multiple hosts with namespace count = 1 (RWX case), no removal of namespace or subsystem
+	mock.EXPECT().ClientConfig().Return(clientConfig).AnyTimes()
+	mock.EXPECT().NVMeIsNamespaceMapped(ctx, subsystemUUID, nsUUID).Return(true, nil).Times(1)
+	mock.EXPECT().NVMeGetHostsOfSubsystem(ctx, subsystemUUID).Return([]*models.NvmeSubsystemHost{host1, host2}, nil).Times(1)
+	mock.EXPECT().NVMeRemoveHostFromSubsystem(ctx, hostNQN, subsystemUUID).Return(nil).Times(1)
+	mock.EXPECT().NVMeNamespaceCount(ctx, subsystemUUID).Return(int64(1), nil).Times(1)
+
+	removePublishInfo, err = oapi.NVMeEnsureNamespaceUnmapped(ctx, hostNQN, subsystemUUID, nsUUID)
+
+	assert.Equal(t, false, removePublishInfo, "subsystem is not removed in RWX case")
+	assert.NoError(t, err)
+
+	// case 17: Single host but different from requested host (nonExistentHostNQN), subsystem and namespace not removed
+	mock.EXPECT().ClientConfig().Return(clientConfig).AnyTimes()
+	mock.EXPECT().NVMeIsNamespaceMapped(ctx, subsystemUUID, nsUUID).Return(true, nil).Times(1)
+	mock.EXPECT().NVMeGetHostsOfSubsystem(ctx, subsystemUUID).Return([]*models.NvmeSubsystemHost{host1}, nil).Times(1)
+
+	removePublishInfo, err = oapi.NVMeEnsureNamespaceUnmapped(ctx, nonExistentHostNQN, subsystemUUID, nsUUID)
+
+	assert.Equal(t, false, removePublishInfo, "subsystem is not removed when host doesn't match")
+	assert.NoError(t, err)
+
+	// case 18: Namespace count > 0 after removal, subsystem not deleted
+	mock.EXPECT().ClientConfig().Return(clientConfig).AnyTimes()
+	mock.EXPECT().NVMeIsNamespaceMapped(ctx, subsystemUUID, nsUUID).Return(true, nil).Times(1)
+	mock.EXPECT().NVMeGetHostsOfSubsystem(ctx, subsystemUUID).Return([]*models.NvmeSubsystemHost{host1}, nil).Times(1)
+	mock.EXPECT().NVMeSubsystemRemoveNamespace(ctx, subsystemUUID, nsUUID).Return(nil).Times(1)
+	mock.EXPECT().NVMeNamespaceCount(ctx, subsystemUUID).Return(int64(2), nil).Times(1)
+
+	removePublishInfo, err = oapi.NVMeEnsureNamespaceUnmapped(ctx, hostNQN, subsystemUUID, nsUUID)
+
+	assert.Equal(t, true, removePublishInfo, "subsystem has remaining namespaces")
+	assert.NoError(t, err)
 }
 
 func TestNVMeIsNamespaceMapped(t *testing.T) {

storage_drivers/ontap/ontap_common.go

@@ -5438,6 +5438,8 @@ func getUniqueNodeSpecificSubsystemName(
 
 	// Construct the subsystem name
 	completeSSName := fmt.Sprintf("%s_%s_%s", prefix, nodeName, tridentUUID)
+	// Skip any underscores at the beginning
+	completeSSName = strings.TrimLeft(completeSSName, "_")
 	finalSSName := completeSSName
 
 	// Ensure the final name does not exceed the maximum length
@@ -5451,6 +5453,7 @@ func getUniqueNodeSpecificSubsystemName(
 
 		base64Str := base64.StdEncoding.EncodeToString(u[:])
 		finalSSName = fmt.Sprintf("%s_%s_%s", prefix, nodeName, base64Str)
+		finalSSName = strings.TrimLeft(finalSSName, "_")
 
 		if len(finalSSName) > maxSubsystemLength {
 			// If even after Trident UUID reduction, the length is more than max length,

storage_drivers/ontap/ontap_common_test.go

@@ -9824,6 +9824,26 @@ func TestGetUniqueNodeSpecificSubsystemName(t *testing.T) {
 			expectedFinal:      "",
 			expectError:        true,
 		},
+		{
+			description:        "Valid input, with no prefix passed",
+			nodeName:           "node1",
+			tridentUUID:        tridentUUID,
+			prefix:             "",
+			maxSubsystemLength: 64,
+			expectedBestCase:   fmt.Sprintf("node1_%s", tridentUUID),
+			expectedFinal:      fmt.Sprintf("node1_%s", tridentUUID),
+			expectError:        false,
+		},
+		{
+			description:        "Even hash is more than limit, truncation needed with no prefix",
+			nodeName:           "averylongnodenameexceedingthelimit",
+			tridentUUID:        tridentUUID,
+			prefix:             "",
+			maxSubsystemLength: 32,
+			expectedBestCase:   fmt.Sprintf("averylongnodenameexceedingthelimit_%s", tridentUUID),
+			expectedFinal:      fmt.Sprintf("%x", sha256.Sum256([]byte(fmt.Sprintf("averylongnodenameexceedingthelimit_%s", tridentUUID))))[:32],
+			expectError:        false,
+		},
 	}
 
 	for _, test := range tests {

storage_drivers/ontap/ontap_san_nvme.go

@@ -58,6 +58,8 @@ const (
 	defaultNamespaceBlockSize = 4096
 	// maximumSubsystemNameLength represent the max length of subsystem name
 	maximumSubsystemNameLength = 64
+	// nvmeSubsystemPrefix Subsystem prefix for ONTAP NVMe driver (empty for legacy compatibility).
+	nvmeSubsystemPrefix = ""
 )
 
 // Namespace attributes stored in its comment field. These fields are useful for docker context.
@@ -923,6 +925,7 @@ func (d *NVMeStorageDriver) Publish(
 	// When FS type is RAW, we create a new subsystem per namespace,
 	// else we use the subsystem created for that particular node
 	var ssName string
+	var completeSSName string
 	if volConfig.FileSystem == filesystem.Raw {
 		ssName = getNamespaceSpecificSubsystemName(name, pvName)
 	} else {
@@ -931,13 +934,24 @@ func (d *NVMeStorageDriver) Publish(
 		if tridentconfig.CurrentDriverContext == tridentconfig.ContextDocker {
 			ssName = d.getStoragePrefixSubsystemName()
 		} else {
-			ssName = d.getNodeSpecificSubsystemName(publishInfo.HostName, publishInfo.TridentUUID)
+			if completeSSName, ssName, err = getUniqueNodeSpecificSubsystemName(
+				publishInfo.HostName, publishInfo.TridentUUID, nvmeSubsystemPrefix, maximumSubsystemNameLength); err != nil {
+				return fmt.Errorf("failed to create node specific subsystem name: %w", err)
+			}
 		}
 	}
 
+	// Update the subsystem comment
+	var ssComment string
+	if completeSSName == "" {
+		ssComment = ssName
+	} else {
+		ssComment = completeSSName
+	}
+
 	// If 2 concurrent requests try to create the same subsystem, one will succeed and ONTAP is guaranteed to return
 	// "already exists" error code for the other. So no need for locking around subsystem creation.
-	subsystem, err := d.createOrGetSubsystem(ctx, ssName)
+	subsystem, err := d.createOrGetSubsystem(ctx, ssName, ssComment)
 	if err != nil {
 		return err
 	}
@@ -1177,9 +1191,11 @@ func (d *NVMeStorageDriver) getStoragePoolAttributes(ctx context.Context) map[st
 	}
 }
 
-func (d *NVMeStorageDriver) createOrGetSubsystem(ctx context.Context, ssName string) (*api.NVMeSubsystem, error) {
+func (d *NVMeStorageDriver) createOrGetSubsystem(
+	ctx context.Context, ssName, comment string,
+) (*api.NVMeSubsystem, error) {
 	// This checks if subsystem exists and creates it if not.
-	ss, err := d.API.NVMeSubsystemCreate(ctx, ssName, ssName)
+	ss, err := d.API.NVMeSubsystemCreate(ctx, ssName, comment)
 	if err != nil {
 		Logc(ctx).Errorf("subsystem create failed, %v", err)
 		return nil, err
@@ -1743,17 +1759,6 @@ func (d *NVMeStorageDriver) ParseNVMeNamespaceCommentString(_ context.Context, c
 	return nil, fmt.Errorf("nsAttrs field not found in Namespace comment")
 }
 
-func (d *NVMeStorageDriver) getNodeSpecificSubsystemName(nodeName, tridentUUID string) string {
-	// For CSI mode, use per-node subsystems
-	subsystemName := fmt.Sprintf("%s-%s", nodeName, tridentUUID)
-	if len(subsystemName) > maximumSubsystemNameLength {
-		// If the new subsystem name is over the subsystem character limit, it means the host name is too long.
-		subsystemPrefixLength := maximumSubsystemNameLength - len(tridentUUID) - 1
-		subsystemName = fmt.Sprintf("%s-%s", nodeName[:subsystemPrefixLength], tridentUUID)
-	}
-	return subsystemName
-}
-
 // getStoragePrefixSubsystemName generates a stable subsystem name using storage prefix for Docker mode
 func (d *NVMeStorageDriver) getStoragePrefixSubsystemName() string {
 	// Default for Docker is "netappdvp_", users can customize for isolation/sharing