Fix stale tridentNode CR not cleaned up after soft-deleted node's vol…

Author: vivakeram

core/orchestrator_core.go

@@ -6552,6 +6552,8 @@ func (o *TridentOrchestrator) DeleteNode(ctx context.Context, nodeName string) (
 		if err = o.storeClient.AddOrUpdateNode(ctx, node); err != nil {
 			return err
 		}
+
+		o.nodes.Set(nodeName, node)
 	}
 
 	return nil

core/orchestrator_core_test.go

@@ -4484,6 +4484,140 @@ func TestDeleteNode(t *testing.T) {
 	}
 }
 
+// TestDeleteNode_SoftDeleteUpdatesCacheDeletedFlag verifies that when a node is
+// soft-deleted (marked Deleted=true because it still has volume publications),
+// the in-memory cache is updated with the Deleted flag.
+func TestDeleteNode_SoftDeleteUpdatesCacheDeletedFlag(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	mockStoreClient := mockpersistentstore.NewMockStoreClient(mockCtrl)
+
+	o := getOrchestrator(t, false)
+	o.storeClient = mockStoreClient
+
+	nodeName := "test-node-1"
+	volumeName := "test-vol-1"
+
+	// Set up a node.
+	o.nodes.Set(nodeName, &models.Node{
+		Name:    nodeName,
+		IPs:     []string{"10.0.0.1"},
+		Deleted: false,
+	})
+
+	// Set up a volume publication so DeleteNode takes the soft-delete path.
+	err := o.volumePublications.Set(volumeName, nodeName, &models.VolumePublication{
+		NodeName:   nodeName,
+		VolumeName: volumeName,
+	})
+	assert.NoError(t, err)
+
+	// Mock: AddOrUpdateNode is called to persist the soft-delete.
+	mockStoreClient.EXPECT().AddOrUpdateNode(gomock.Any(), gomock.Any()).Return(nil)
+
+	// Act: Delete the node while it still has publications -> soft-delete.
+	err = o.DeleteNode(ctx(), nodeName)
+	assert.NoError(t, err)
+
+	// Assert: Node should still be in cache (not hard-deleted).
+	node := o.nodes.Get(nodeName)
+	assert.NotNil(t, node, "Node should still be in cache after soft-delete")
+
+	// Assert: The Deleted flag in the cache MUST be true.
+	// NodeCache.Get() returns a deep copy.
+	// DeleteNode was modifying the copy and persisting to store, but never
+	// calling o.nodes.Set() to update the cache. So the cache still had Deleted=false.
+	assert.True(t, node.Deleted,
+		"Node in cache should have Deleted=true after soft-delete; "+
+			"if this fails, the cache was not updated")
+}
+
+// TestDeleteNode_SoftDeletedNodeCleanedUpAfterLastUnpublish verifies the full
+// end-to-end flow: when a node is soft-deleted and then all its volumes are
+// unpublished, the tridentNode CR (and cache entry) should be cleaned up.
+func TestDeleteNode_SoftDeletedNodeCleanedUpAfterLastUnpublish(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	backendUUID := "backend-uuid-1"
+	nodeName := "test-node-1"
+	volumeName := "test-vol-1"
+
+	mockBackend := mockstorage.NewMockBackend(mockCtrl)
+	mockBackend.EXPECT().BackendUUID().Return(backendUUID).AnyTimes()
+	mockStoreClient := mockpersistentstore.NewMockStoreClient(mockCtrl)
+
+	o := getOrchestrator(t, false)
+	o.storeClient = mockStoreClient
+
+	// Set up a node.
+	o.nodes.Set(nodeName, &models.Node{
+		Name:    nodeName,
+		IPs:     []string{"10.0.0.1"},
+		Deleted: false,
+	})
+
+	// Set up a volume.
+	o.volumes = map[string]*storage.Volume{
+		volumeName: {
+			BackendUUID: backendUUID,
+			Config:      &storage.VolumeConfig{Name: volumeName},
+		},
+	}
+
+	// Set up a single volume publication.
+	err := o.volumePublications.Set(volumeName, nodeName, &models.VolumePublication{
+		NodeName:   nodeName,
+		VolumeName: volumeName,
+	})
+	assert.NoError(t, err)
+
+	// --- Step 1: Delete the node (soft-delete because publication exists) ---
+	// Note: Don't add the mock backend to o.backends yet, because DeleteNode calls
+	// updateMetrics() which would call backend methods we don't want to mock here.
+	mockStoreClient.EXPECT().AddOrUpdateNode(gomock.Any(), gomock.Any()).Return(nil)
+
+	err = o.DeleteNode(ctx(), nodeName)
+	assert.NoError(t, err)
+
+	// Node should still exist in cache (soft-deleted).
+	node := o.nodes.Get(nodeName)
+	assert.NotNil(t, node, "Node should still be in cache after soft-delete")
+
+	// --- Step 2: Unpublish the last volume from the soft-deleted node ---
+	// Now add the backend (needed for the unpublish flow).
+	o.backends[backendUUID] = mockBackend
+
+	// Mock expectations for the unpublish flow.
+	mockBackend.EXPECT().UnpublishVolume(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
+	mockStoreClient.EXPECT().UpdateVolume(gomock.Any(), gomock.Any()).Return(nil)
+	mockStoreClient.EXPECT().DeleteVolumePublication(gomock.Any(), gomock.Any()).Return(nil)
+
+	// These are called by deleteNode() when the soft-deleted node is finally cleaned up.
+	// We require exactly one call each to verify the persistent-store cleanup and
+	// backend invalidation actually occur on the success path.
+	nodeDeleted := false
+	mockStoreClient.EXPECT().DeleteNode(gomock.Any(), gomock.Any()).
+		DoAndReturn(func(_ context.Context, _ *models.Node) error {
+			nodeDeleted = true
+			return nil
+		}).Times(1)
+	mockBackend.EXPECT().InvalidateNodeAccess().Times(1)
+
+	err = o.unpublishVolume(coreCtx, volumeName, nodeName)
+	assert.NoError(t, err)
+
+	// Assert: deleteNode was called to remove the soft-deleted node from the persistent store.
+	assert.True(t, nodeDeleted,
+		"storeClient.DeleteNode should have been called to clean up the soft-deleted node")
+
+	// Assert: The node should now be fully removed from cache.
+	node = o.nodes.Get(nodeName)
+	assert.Nil(t, node,
+		"Node should be removed from cache after last volume is unpublished from soft-deleted node")
+}
+
 func TestSnapshotVolumes(t *testing.T) {
 	mockPools := tu.GetFakePools()
 	orchestrator := getOrchestrator(t, false)

storage_drivers/ontap/ontap_common_test.go

@@ -3369,6 +3369,116 @@ func TestRemoveExportPolicyRules_CNVA_ContinueOnPartialErrors(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+// TestRemoveExportPolicyRules_SameIPOnDepartingAndActiveNode reproduces the customer bug
+// where a volume migrates from an old node to a new node that received the same IP address (DHCP/IP reuse).
+// The publish for the new node arrives first (no-op since IP already in policy), then the unpublish for the old
+// node arrives and must NOT remove the IP because the new node still needs it.
+func TestRemoveExportPolicyRules_SameIPOnDepartingAndActiveNode(t *testing.T) {
+	ctx := context.TODO()
+	mockAPI := newMockOntapAPI(t)
+	exportPolicy := "trident_pvc_test"
+
+	// Scenario: NodeA (old) is being unpublished. NodeB (new) remains active.
+	// Both nodes share the same IP 10.10.1.1 due to DHCP/IP reuse after migration.
+	sharedIP := "10.10.1.1"
+
+	// publishInfo as constructed by the orchestrator during unpublish:
+	// - HostIP: departing node's IPs (NodeA)
+	// - Nodes:  remaining active nodes (NodeB), which has the same IP
+	publishInfo := &tridentmodels.VolumePublishInfo{
+		HostName: "node-a",
+		HostIP:   []string{sharedIP, "10.10.1.2"},
+		Nodes: []*tridentmodels.Node{
+			createTestNode("node-b", []string{sharedIP}),
+		},
+	}
+
+	// Export policy currently has rules for both of NodeA's IPs
+	existingRules := map[int]string{
+		1: sharedIP,    // Shared IP - must be PRESERVED (NodeB still needs it)
+		2: "10.10.1.2", // NodeA-only IP - should be removed
+	}
+	finalRules := map[int]string{
+		1: sharedIP, // Preserved because NodeB is still active with this IP
+	}
+
+	mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(existingRules, nil)
+	mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, 2).Return(nil) // Only remove NodeA-only IP
+	// ExportRuleDestroy for rule 1 (shared IP) must NOT be called
+	mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(finalRules, nil)
+
+	err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
+	assert.NoError(t, err)
+}
+
+// TestRemoveExportPolicyRules_SameIPOnDepartingAndActiveNode_OnlyIPIsShared tests the edge case
+// where the departing node's only IP is the same as the active node's IP.
+// No export policy rules should be removed since the active node still needs the rule.
+func TestRemoveExportPolicyRules_SameIPOnDepartingAndActiveNode_OnlyIPIsShared(t *testing.T) {
+	ctx := context.TODO()
+	mockAPI := newMockOntapAPI(t)
+	exportPolicy := "trident_pvc_test"
+
+	sharedIP := "10.10.1.1"
+
+	publishInfo := &tridentmodels.VolumePublishInfo{
+		HostName: "node-a",
+		HostIP:   []string{sharedIP},
+		Nodes: []*tridentmodels.Node{
+			createTestNode("node-b", []string{sharedIP}),
+		},
+	}
+
+	existingRules := map[int]string{
+		1: sharedIP,
+	}
+
+	mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(existingRules, nil)
+	// No ExportRuleDestroy calls expected - the shared IP must be preserved
+	mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(existingRules, nil)
+
+	err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
+	assert.NoError(t, err)
+}
+
+// TestRemoveExportPolicyRules_SameIPOnDepartingAndMultipleActiveNodes tests IP reuse across
+// multiple active nodes where the departing node shares IPs with more than one remaining node.
+func TestRemoveExportPolicyRules_SameIPOnDepartingAndMultipleActiveNodes(t *testing.T) {
+	ctx := context.TODO()
+	mockAPI := newMockOntapAPI(t)
+	exportPolicy := "trident_pvc_test"
+
+	sharedIP := "10.10.1.1"
+
+	publishInfo := &tridentmodels.VolumePublishInfo{
+		HostName: "node-a",
+		HostIP:   []string{sharedIP, "10.10.1.2"},
+		Nodes: []*tridentmodels.Node{
+			createTestNode("node-b", []string{sharedIP}),
+			createTestNode("node-c", []string{"10.10.1.3"}),
+		},
+	}
+
+	existingRules := map[int]string{
+		1: sharedIP,    // Keep - NodeB still needs it
+		2: "10.10.1.2", // Remove - only NodeA had this
+		3: "10.10.1.3", // Keep - NodeC still needs it
+		4: "10.10.1.4", // Remove - not in any active node
+	}
+	finalRules := map[int]string{
+		1: sharedIP,
+		3: "10.10.1.3",
+	}
+
+	mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(existingRules, nil)
+	mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, 2).Return(nil)
+	mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, 4).Return(nil)
+	mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(finalRules, nil)
+
+	err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
+	assert.NoError(t, err)
+}
+
 func TestDeleteExportPolicy(t *testing.T) {
 	// Test-1: Positive flow