Implement unique subsystem names for regular ONTAP NVMe driver

VinayKumarHavanur · web-flow · commit 5d2664c762f0 · 2025-11-06T12:03:02.000+05:30
diff --git a/storage_drivers/ontap/api/abstraction_rest.go b/storage_drivers/ontap/api/abstraction_rest.go
@@ -3430,6 +3430,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
@@ -3459,8 +3460,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",
@@ -3470,6 +3478,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
 	}
 
@@ -3493,7 +3516,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)
diff --git a/storage_drivers/ontap/api/abstraction_rest_test.go b/storage_drivers/ontap/api/abstraction_rest_test.go
@@ -928,6 +928,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)
 
@@ -999,11 +1000,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) {
diff --git a/storage_drivers/ontap/ontap_common.go b/storage_drivers/ontap/ontap_common.go
@@ -5085,6 +5085,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
@@ -5098,6 +5100,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,
diff --git a/storage_drivers/ontap/ontap_common_test.go b/storage_drivers/ontap/ontap_common_test.go
@@ -9729,6 +9729,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 {
diff --git a/storage_drivers/ontap/ontap_san_nvme.go b/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.
@@ -912,14 +914,26 @@ 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 {
-		ssName = 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
 	}
 
 	// Checks if subsystem exists and creates a new one if not
-	subsystem, err := d.API.NVMeSubsystemCreate(ctx, ssName, ssName)
+	subsystem, err := d.API.NVMeSubsystemCreate(ctx, ssName, ssComment)
 	if err != nil {
 		Logc(ctx).Errorf("subsystem create failed, %v", err)
 		return err
@@ -1705,16 +1719,6 @@ func (d *NVMeStorageDriver) ParseNVMeNamespaceCommentString(_ context.Context, c
 	return nil, fmt.Errorf("nsAttrs field not found in Namespace comment")
 }
 
-func getNodeSpecificSubsystemName(nodeName, tridentUUID string) string {
-	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
-}
-
 // getNamespaceSpecificSubsystemName constructs the subsystem name using the name passed.
 func getNamespaceSpecificSubsystemName(name, pvName string) string {
 	subSystemPrefix := "s"
diff --git a/storage_drivers/ontap/ontap_san_nvme_test.go b/storage_drivers/ontap/ontap_san_nvme_test.go
@@ -1510,25 +1510,6 @@ func TestGetNamespaceSpecificSubsystemName(t *testing.T) {
 	assert.Equal(t, got_name, expected_name)
 }
 
-func TestGetNodeSpecificSubsystemName(t *testing.T) {
-	// case 1: subsystem, name is shorter than 64 char
-	nodeName := "fakeNodeName"
-	tridentUUID := "fakeUUID"
-	expected := "fakeNodeName-fakeUUID"
-
-	got := getNodeSpecificSubsystemName(nodeName, tridentUUID)
-
-	assert.Equal(t, got, expected)
-
-	// case 2: subsystem name is longer than 64 char
-	nodeName = "fakeNodeNamefakeNodeNamefakeNodeNamefakeNodeNamefakeNodeNamefakeNodeNamefakeNodeNamefakeNodeName"
-	expected = "fakeNodeNamefakeNodeNamefakeNodeNamefakeNodeNamefakeNod-fakeUUID"
-
-	got = getNodeSpecificSubsystemName(nodeName, tridentUUID)
-
-	assert.Equal(t, got, expected)
-}
-
 func TestPublish(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	mock := mockapi.NewMockOntapAPI(mockCtrl)
@@ -1599,7 +1580,7 @@ func TestPublish(t *testing.T) {
 	publishInfo.HostNQN = "fakeHostNQN"
 	mock.EXPECT().VolumeInfo(ctx, volConfig.InternalName).Return(flexVol, nil).Times(1)
 	mock.EXPECT().NVMeNamespaceGetByName(ctx, gomock.Any()).Return(namespace, nil).Times(1)
-	mock.EXPECT().NVMeSubsystemCreate(ctx, "fakeHostName-fakeUUID", "fakeHostName-fakeUUID").Return(subsystem, fmt.Errorf("Error creating subsystem")).Times(1)
+	mock.EXPECT().NVMeSubsystemCreate(ctx, "fakeHostName_fakeUUID", "fakeHostName_fakeUUID").Return(subsystem, fmt.Errorf("Error creating subsystem")).Times(1)
 
 	err = d.Publish(ctx, volConfig, publishInfo)
 
@@ -1608,7 +1589,7 @@ func TestPublish(t *testing.T) {
 	// case 6: Error creating subsystem in CSI Context
 	d.Config.DriverContext = tridentconfig.ContextCSI
 	mock.EXPECT().VolumeInfo(ctx, volConfig.InternalName).Return(flexVol, nil).Times(1)
-	mock.EXPECT().NVMeSubsystemCreate(ctx, "fakeHostName-fakeUUID", "fakeHostName-fakeUUID").Return(subsystem, fmt.Errorf("Error creating subsystem")).Times(1)
+	mock.EXPECT().NVMeSubsystemCreate(ctx, "fakeHostName_fakeUUID", "fakeHostName_fakeUUID").Return(subsystem, fmt.Errorf("Error creating subsystem")).Times(1)
 
 	err = d.Publish(ctx, volConfig, publishInfo)
 
@@ -1626,7 +1607,7 @@ func TestPublish(t *testing.T) {
 	// case 8: Error while adding host nqn to subsystem
 	publishInfo.HostNQN = "fakeHostNQN"
 	mock.EXPECT().VolumeInfo(ctx, volConfig.InternalName).Return(flexVol, nil).Times(1)
-	mock.EXPECT().NVMeSubsystemCreate(ctx, "fakeHostName-fakeUUID", "fakeHostName-fakeUUID").Return(subsystem, nil).Times(1)
+	mock.EXPECT().NVMeSubsystemCreate(ctx, "fakeHostName_fakeUUID", "fakeHostName_fakeUUID").Return(subsystem, nil).Times(1)
 	mock.EXPECT().NVMeAddHostToSubsystem(ctx, publishInfo.HostNQN, subsystem.UUID).Return(fmt.Errorf("Error adding host nqnq to subsystem")).Times(1)
 
 	err = d.Publish(ctx, volConfig, publishInfo)
@@ -1635,7 +1616,7 @@ func TestPublish(t *testing.T) {
 
 	// case 9: Error returned by NVMeEnsureNamespaceMapped
 	mock.EXPECT().VolumeInfo(ctx, volConfig.InternalName).Return(flexVol, nil).Times(1)
-	mock.EXPECT().NVMeSubsystemCreate(ctx, "fakeHostName-fakeUUID", "fakeHostName-fakeUUID").Return(subsystem, nil).Times(1)
+	mock.EXPECT().NVMeSubsystemCreate(ctx, "fakeHostName_fakeUUID", "fakeHostName_fakeUUID").Return(subsystem, nil).Times(1)
 	mock.EXPECT().NVMeAddHostToSubsystem(ctx, publishInfo.HostNQN, subsystem.UUID).Return(nil).Times(1)
 	mock.EXPECT().NVMeEnsureNamespaceMapped(ctx, gomock.Any(), gomock.Any()).Return(fmt.Errorf("Error returned by NVMeEnsureNamespaceMapped")).Times(1)
 
@@ -1645,7 +1626,7 @@ func TestPublish(t *testing.T) {
 
 	// case 10: Success
 	mock.EXPECT().VolumeInfo(ctx, volConfig.InternalName).Return(flexVol, nil).Times(1)
-	mock.EXPECT().NVMeSubsystemCreate(ctx, "fakeHostName-fakeUUID", "fakeHostName-fakeUUID").Return(subsystem, nil).Times(1)
+	mock.EXPECT().NVMeSubsystemCreate(ctx, "fakeHostName_fakeUUID", "fakeHostName_fakeUUID").Return(subsystem, nil).Times(1)
 	mock.EXPECT().NVMeAddHostToSubsystem(ctx, publishInfo.HostNQN, subsystem.UUID).Return(nil).Times(1)
 	mock.EXPECT().NVMeEnsureNamespaceMapped(ctx, gomock.Any(), gomock.Any()).Return(nil).Times(1)
 
