From 3208d6dbc384d75db51e6878c0f3ea36cf597a1a Mon Sep 17 00:00:00 2001 From: notsrch <57882515+notsrch@users.noreply.github.com> Date: Thu, 3 Sep 2026 20:03:08 -0500 Subject: [PATCH] Guard node-access reconciliation against an empty backend view (#1179) Code changes of patches/patch-26.06.0 from Euronet-OneEuronet/trident-patched, branch patch/1179-node-access-reconcile-guard, rebased onto this fork's master, plus the CHANGELOG entry from fix/1179-backend-volume-view. The builder's Dockerfile hunk is not included; it is a build environment change for a private builder, not for upstream. Problem: on a TridentBackendConfig update or controller start the concurrent core builds a new backend object with an empty volume map and reconciles node access on it before the volumes are stored. The desired node set comes out empty and every rule is deleted from the shared export policy trident-, so new NFS mounts are denied on every node until each node happens to publish again. Fix, from fix/1179-backend-volume-view: - Carry the volume map forward when the concurrent core replaces a backend object (Backend.SetVolumes; master already declared this method, so only the call sites needed reconciling during the rebase). - Count publications of subordinate volumes toward the share-source volume that hosts them. - In both cores, when the computed node set is empty while publications still exist for the backend, log a warning, leave the backend marked as needing reconciliation, and return so the periodic loop retries. A backend with no publications still reconciles to an empty set. An image of this change on top of v26.06.0 passed the live reproduction test on 2026-09-04: three backend reconciles, zero export policy rule deletions, fresh NFS mounts succeeded on nodes with an existing VolumeAttachment. go build, go vet, gofmt, and go test ./core/ ./storage/ are clean. Refs: https://github.com/NetApp/trident/issues/1179 Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 1 + core/common.go | 35 ++++++ core/common_test.go | 30 +++++ core/concurrent_core.go | 77 +++++++++--- core/concurrent_core_test.go | 220 +++++++++++++++++++++++++++++++++ core/orchestrator_core.go | 22 +++- core/orchestrator_core_test.go | 155 +++++++++++++++++++++++ 7 files changed, 520 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4913decf3..a00752ac0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ - **Kubernetes:** Fixed race conditions and state handling for concurrent publish/unpublish, clone, cache, and backend update operations. - **Kubernetes:** Fixed export policy race conditions and concurrent publish/unpublish handling for subordinate volumes and read-only clones in ONTAP-NAS and ONTAP-NAS-Economy drivers. +- **Kubernetes:** Fixed node-access reconciliation removing every rule from the shared backend export policy of ONTAP-NAS drivers with `autoExportPolicy` during backend updates, controller restarts, and for backends published only through subordinate volumes (Issue [#1179](https://github.com/NetApp/trident/issues/1179)). - **Kubernetes:** Fixed ONTAP-NAS-Economy FlexVol discovery after MetroCluster failover when snapshot policies differ (Issue [#1082](https://github.com/NetApp/trident/issues/1082)). - **Kubernetes:** Fixed ONTAP-NAS-Economy volume delete retries that interrupted long-running qtree deletes (Issue [#1121](https://github.com/NetApp/trident/issues/1121)). - **Kubernetes:** Fixed ONTAP-SAN and ONTAP-SAN-Economy import and resize behavior, including `fsType` validation, volume metadata handling, and autogrow mode behavior. diff --git a/core/common.go b/core/common.go index 9905ab4c3..ebe376d9c 100644 --- a/core/common.go +++ b/core/common.go @@ -307,6 +307,41 @@ func generateVolumePublication(volName string, publishInfo *models.VolumePublish return vp } +// hasPublicationsForBackend reports whether any publication belongs to backend b. A publication +// records the UUID of the backend hosting its volume; records that predate that field are +// attributed by checking whether their volume is in b's volume map. +func hasPublicationsForBackend(b storage.Backend, publications []*models.VolumePublication) bool { + backendUUID := b.BackendUUID() + var volumes *sync.Map + for _, pub := range publications { + if pub.BackendUUID != "" { + if pub.BackendUUID == backendUUID { + return true + } + continue + } + if volumes == nil { + volumes = b.Volumes() + } + if _, ok := volumes.Load(pub.VolumeName); ok { + return true + } + } + return false +} + +// logSkippedNodeAccessReconcile records that node-access reconciliation for b was deferred: its +// computed node set was empty while publications still exist, so proceeding would remove every +// export rule from a policy that is still in use. The backend stays marked as needing +// reconciliation and the periodic loop retries once the caches agree. +func logSkippedNodeAccessReconcile(ctx context.Context, b storage.Backend) { + Logc(ctx).WithFields(LogFields{ + "backend": b.Name(), + "backendUUID": b.BackendUUID(), + }).Warn("Publications exist for this backend but none of their nodes could be resolved; " + + "skipping node access reconciliation instead of removing every export rule.") +} + // isDockerPluginMode returns true if the ENV variable config.DockerPluginModeEnvVariable is set func isDockerPluginMode() bool { return os.Getenv(config.DockerPluginModeEnvVariable) != "" diff --git a/core/common_test.go b/core/common_test.go index 0a0607ab7..3de6ae98f 100644 --- a/core/common_test.go +++ b/core/common_test.go @@ -1305,3 +1305,33 @@ func TestIsBackendBootstrapTimeout(t *testing.T) { }) } } + +func TestHasPublicationsForBackend(t *testing.T) { + backend := getFakeBackend("backend1", "uuid1", nil) + backend.Volumes().Store("vol1", getFakeVolume("vol1", "uuid1")) + + tests := []struct { + name string + publications []*models.VolumePublication + want bool + }{ + {"NoPublications", nil, false}, + {"PublicationForThisBackend", []*models.VolumePublication{ + {VolumeName: "other", NodeName: "n1", BackendUUID: "uuid1"}, + }, true}, + {"PublicationForAnotherBackend", []*models.VolumePublication{ + {VolumeName: "vol1", NodeName: "n1", BackendUUID: "uuid2"}, + }, false}, + {"LegacyPublicationForHostedVolume", []*models.VolumePublication{ + {VolumeName: "vol1", NodeName: "n1"}, + }, true}, + {"LegacyPublicationForUnknownVolume", []*models.VolumePublication{ + {VolumeName: "vol9", NodeName: "n1"}, + }, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, hasPublicationsForBackend(backend, tt.publications)) + }) + } +} diff --git a/core/concurrent_core.go b/core/concurrent_core.go index 30f2b6663..aa2b046ba 100644 --- a/core/concurrent_core.go +++ b/core/concurrent_core.go @@ -1185,7 +1185,8 @@ func (o *ConcurrentTridentOrchestrator) reconcileNodeAccessOnAllBackends(ctx con // just log it and continue. err = func() error { _, results, unlocker, dbErr := db.Lock(ctx, db.Query( - db.ListVolumePublications(), db.ListNodes(), db.UpsertBackend(backend.BackendUUID(), "", ""))) + db.ListVolumePublications(), db.ListNodes(), db.ListSubordinateVolumes(), + db.UpsertBackend(backend.BackendUUID(), "", ""))) defer unlocker() if dbErr != nil { return dbErr @@ -1196,8 +1197,8 @@ func (o *ConcurrentTridentOrchestrator) reconcileNodeAccessOnAllBackends(ctx con return errors.NotFoundError("backend %s not found for reconcile", backend.BackendUUID()) } - if reconcileErr := o.reconcileNodeAccessOnBackend( - ctx, upsertBackend, results[0].VolumePublications, results[0].Nodes); reconcileErr != nil { + if reconcileErr := o.reconcileNodeAccessOnBackend(ctx, upsertBackend, + results[0].VolumePublications, results[0].Nodes, results[0].SubordinateVolumes); reconcileErr != nil { return reconcileErr } results[0].Backend.Upsert(upsertBackend) @@ -1217,7 +1218,7 @@ func (o *ConcurrentTridentOrchestrator) reconcileNodeAccessOnAllBackends(ctx con } func (o *ConcurrentTridentOrchestrator) reconcileNodeAccessOnBackend(ctx context.Context, b storage.Backend, - allVolumePublications []*models.VolumePublication, allNodes []*models.Node, + allVolumePublications []*models.VolumePublication, allNodes []*models.Node, subordinateVolumes []*storage.Volume, ) error { if config.CurrentDriverContext != config.ContextCSI { return nil @@ -1226,11 +1227,23 @@ func (o *ConcurrentTridentOrchestrator) reconcileNodeAccessOnBackend(ctx context var nodes []*models.Node if b.CanEnablePublishEnforcement() { - nodes = publishedNodesForBackend(b, allVolumePublications, allNodes) + var unregisteredNodes []string + nodes, unregisteredNodes = publishedNodesForBackend(b, allVolumePublications, allNodes, subordinateVolumes) + if len(unregisteredNodes) > 0 { + Logc(ctx).WithFields(LogFields{ + "backend": b.Name(), + "unregisteredNodes": unregisteredNodes, + }).Warn("Some publications name nodes that are not registered; reconciling node access without them.") + } } else { nodes = allNodes } + if len(nodes) == 0 && hasPublicationsForBackend(b, allVolumePublications) { + logSkippedNodeAccessReconcile(ctx, b) + return nil + } + if err := b.ReconcileNodeAccess(ctx, nodes, o.uuid); err != nil { return err } @@ -1289,28 +1302,48 @@ func (o *ConcurrentTridentOrchestrator) updateLastNodeRegistrationTime() { o.lastNodeRegistrationTime = time.Now() } -// publishedNodesForBackend returns the nodes that a backend has published volumes to +// publishedNodesForBackend returns the nodes that a backend has published volumes to, plus the +// names of published nodes that are absent from allNodes and therefore left out. Publications for +// subordinate volumes count toward the share-source volume that hosts them, since a backend's +// volume map only tracks the source. func publishedNodesForBackend(b storage.Backend, allVolumePublications []*models.VolumePublication, - allNodes []*models.Node, -) []*models.Node { + allNodes []*models.Node, subordinateVolumes []*storage.Volume, +) (nodes []*models.Node, unregisteredNodes []string) { nodesByName := make(map[string]*models.Node, len(allNodes)) for _, n := range allNodes { nodesByName[n.Name] = n } + shareSourceByName := make(map[string]string, len(subordinateVolumes)) + for _, subordinate := range subordinateVolumes { + shareSourceByName[subordinate.Config.Name] = subordinate.Config.ShareSourceVolume + } + volumes := b.Volumes() - m := make(map[string]*models.Node) + seen := make(map[string]struct{}) for _, pub := range allVolumePublications { - if _, ok := volumes.Load(pub.VolumeName); ok { - m[pub.NodeName] = nodesByName[pub.NodeName] + hostingVolume := pub.VolumeName + if source, ok := shareSourceByName[hostingVolume]; ok { + hostingVolume = source } - } + if _, ok := volumes.Load(hostingVolume); !ok { + continue + } + if _, done := seen[pub.NodeName]; done { + continue + } + seen[pub.NodeName] = struct{}{} - nodes := make([]*models.Node, 0, len(m)) - for _, n := range m { - nodes = append(nodes, n) + // Drivers dereference every node they are given, so a publication whose node is not + // registered must be reported rather than passed through as a nil entry. + if node := nodesByName[pub.NodeName]; node != nil { + nodes = append(nodes, node) + } else { + unregisteredNodes = append(unregisteredNodes, pub.NodeName) + } } - return nodes + sort.Strings(unregisteredNodes) + return nodes, unregisteredNodes } func (o *ConcurrentTridentOrchestrator) AddFrontend(ctx context.Context, f frontend.Plugin) { @@ -1752,7 +1785,8 @@ func (o *ConcurrentTridentOrchestrator) upsertBackend( Logc(ctx).Debug(">>>>>> upsertBackend") defer Logc(ctx).Debug("<<<<<< upsertBackend") - _, results, unlocker, err := db.NestedLock(ctx, db.Query(db.ListVolumePublications(), db.ListNodes())) + _, results, unlocker, err := db.NestedLock(ctx, db.Query( + db.ListVolumePublications(), db.ListNodes(), db.ListSubordinateVolumes())) defer unlocker() if err != nil { return nil, err @@ -1777,7 +1811,8 @@ func (o *ConcurrentTridentOrchestrator) upsertBackend( // Node access rules may have changed in the backend config backend.InvalidateNodeAccess() - err = o.reconcileNodeAccessOnBackend(ctx, backend, results[0].VolumePublications, results[0].Nodes) + err = o.reconcileNodeAccessOnBackend(ctx, backend, results[0].VolumePublications, results[0].Nodes, + results[0].SubordinateVolumes) if err != nil { return nil, err } @@ -1973,6 +2008,12 @@ func (o *ConcurrentTridentOrchestrator) updateBackend( } } + // upsertBackend reconciles node access against this new object before updateBackendVolumes + // refreshes its volume map, and the AddBackend-on-existing-name path never refreshes it at + // all. Carry the volumes forward so reconciliation sees the backend's real consumers rather + // than an empty set, which the NAS drivers turn into "remove every export policy rule". + backend.SetVolumes(originalBackend.Volumes()) + // The fake driver needs volumes copied forward if originalFakeDriver, ok := originalBackend.Driver().(*fake.StorageDriver); ok { Logc(ctx).Debug("Using fake driver, going to copy volumes forward...") diff --git a/core/concurrent_core_test.go b/core/concurrent_core_test.go index dd950af69..640f3e038 100644 --- a/core/concurrent_core_test.go +++ b/core/concurrent_core_test.go @@ -4213,6 +4213,35 @@ func TestAddBackendConcurrentCore(t *testing.T) { } } +// AddBackend on an existing backend name replaces the backend object without ever calling +// updateBackendVolumes, so the volumes it hosts must survive the replacement on their own. +func TestAddBackendConcurrentCore_ExistingBackendKeepsVolumes(t *testing.T) { + db.Initialize() + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + mockStoreClient := mockpersistentstore.NewMockStoreClient(mockCtrl) + o := getConcurrentOrchestrator() + o.storeClient = mockStoreClient + + existing := getFakeBackend("existingBackend", "uuid1", nil) + vol := getFakeVolume("vol1", "uuid1") + existing.Volumes().Store(vol.Config.Name, vol) + addBackendsToCache(t, existing) + mockStoreClient.EXPECT().UpdateBackend(gomock.Any(), gomock.Any()).Return(nil) + + configJSON := `{"backendName": "existingBackend", "storageDriverName": "fake", "version": 1, "protocol": "file", "volumeAccess": "1.0.0.1"}` + _, err := o.AddBackend(context.TODO(), configJSON, "") + require.NoError(t, err) + + replaced := getBackendByNameFromCache(t, "existingBackend") + require.NotNil(t, replaced) + assert.NotSame(t, existing, replaced, "AddBackend on an existing name must build a new backend object") + _, found := replaced.Volumes().Load("vol1") + assert.True(t, found, "volumes must be carried forward into the replacement backend object") +} + // TestUpdateBackendStateConcurrentCore covers public UpdateBackendState API. func TestUpdateBackendStateConcurrentCore(t *testing.T) { tests := []struct { @@ -14197,6 +14226,196 @@ func TestDeleteNodeConcurrentCore(t *testing.T) { } } +func TestPublishedNodesForBackendConcurrentCore(t *testing.T) { + nodeA, nodeB := getFakeNode("nodeA"), getFakeNode("nodeB") + allNodes := []*models.Node{nodeA, nodeB} + subordinate := &storage.Volume{Config: &storage.VolumeConfig{Name: "sub1", ShareSourceVolume: "vol1"}} + + tests := []struct { + name string + publications []*models.VolumePublication + subordinates []*storage.Volume + expectedNodes []*models.Node + }{ + { + name: "PublicationForHostedVolume", + publications: []*models.VolumePublication{getFakeVolumePublication("vol1", "nodeA")}, + expectedNodes: []*models.Node{nodeA}, + }, + { + name: "PublicationForVolumeOnAnotherBackend", + publications: []*models.VolumePublication{getFakeVolumePublication("vol9", "nodeA")}, + expectedNodes: []*models.Node{}, + }, + { + // Recorded under the subordinate's name, which the backend's volume map never holds. + name: "SubordinatePublicationCountsForSourceVolume", + publications: []*models.VolumePublication{getFakeVolumePublication("sub1", "nodeB")}, + subordinates: []*storage.Volume{subordinate}, + expectedNodes: []*models.Node{nodeB}, + }, + { + name: "MixedPublicationsDeduplicateNodes", + publications: []*models.VolumePublication{ + getFakeVolumePublication("vol1", "nodeA"), + getFakeVolumePublication("sub1", "nodeA"), + getFakeVolumePublication("sub1", "nodeB"), + }, + subordinates: []*storage.Volume{subordinate}, + expectedNodes: []*models.Node{nodeA, nodeB}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + backend := getFakeBackend("backend1", "uuid1", nil) + backend.Volumes().Store("vol1", getFakeVolume("vol1", "uuid1")) + + nodes, unregistered := publishedNodesForBackend(backend, tt.publications, allNodes, tt.subordinates) + + assert.ElementsMatch(t, tt.expectedNodes, nodes) + assert.Empty(t, unregistered) + }) + } +} + +// A publication may outlive its node's registration. The driver dereferences every node it is +// handed, so such a node must be reported and left out rather than passed through as nil. +func TestPublishedNodesForBackendConcurrentCore_UnregisteredNode(t *testing.T) { + backend := getFakeBackend("backend1", "uuid1", nil) + backend.Volumes().Store("vol1", getFakeVolume("vol1", "uuid1")) + nodeA := getFakeNode("nodeA") + publications := []*models.VolumePublication{ + getFakeVolumePublication("vol1", "nodeA"), + getFakeVolumePublication("vol1", "goneNode"), + getFakeVolumePublication("vol1", "goneNode"), + } + + nodes, unregistered := publishedNodesForBackend(backend, publications, []*models.Node{nodeA}, nil) + + assert.Equal(t, []*models.Node{nodeA}, nodes) + assert.Equal(t, []string{"goneNode"}, unregistered) +} + +// If the computed node set is empty while publications still exist for the backend, the driver +// would be told to remove every export rule from an in-use policy; reconciliation must be deferred +// and the backend left marked for retry instead. +func TestReconcileNodeAccessOnBackendConcurrentCore_EmptyNodeSet(t *testing.T) { + const backendUUID = "uuid1" + vol1 := getFakeVolume("vol1", backendUUID) + nodeA := getFakeNode("nodeA") + + tests := []struct { + name string + enforcement bool + backendVolumes map[string]*storage.Volume + nodes []*models.Node + publications []*models.VolumePublication + expectReconcile bool + expectedNodeNames []string + }{ + { + name: "EnforcedVolumeMissingFromBackendMap", + enforcement: true, + backendVolumes: map[string]*storage.Volume{}, + nodes: []*models.Node{nodeA}, + publications: []*models.VolumePublication{ + {VolumeName: "vol1", NodeName: "nodeA", BackendUUID: backendUUID}, + }, + expectReconcile: false, + }, + { + name: "EnforcedPublishedNodeUnregistered", + enforcement: true, + backendVolumes: map[string]*storage.Volume{"vol1": vol1}, + nodes: nil, + publications: []*models.VolumePublication{ + {VolumeName: "vol1", NodeName: "nodeA", BackendUUID: backendUUID}, + }, + expectReconcile: false, + }, + { + // Only another backend's volume is published: this backend legitimately has no + // consumers, so reconciling to an empty set (stripping its rules) must still happen. + name: "EnforcedNoPublicationsStripsRules", + enforcement: true, + backendVolumes: map[string]*storage.Volume{"vol1": vol1}, + nodes: []*models.Node{nodeA}, + publications: []*models.VolumePublication{ + {VolumeName: "vol9", NodeName: "nodeA", BackendUUID: "some-other-backend"}, + }, + expectReconcile: true, + expectedNodeNames: []string{}, + }, + { + name: "EnforcedConsistentView", + enforcement: true, + backendVolumes: map[string]*storage.Volume{"vol1": vol1}, + nodes: []*models.Node{nodeA}, + publications: []*models.VolumePublication{ + {VolumeName: "vol1", NodeName: "nodeA", BackendUUID: backendUUID}, + }, + expectReconcile: true, + expectedNodeNames: []string{"nodeA"}, + }, + { + name: "UnenforcedNoNodesWhilePublished", + enforcement: false, + backendVolumes: map[string]*storage.Volume{"vol1": vol1}, + nodes: nil, + publications: []*models.VolumePublication{ + {VolumeName: "vol1", NodeName: "nodeA", BackendUUID: backendUUID}, + }, + expectReconcile: false, + }, + { + name: "UnenforcedNoNodesNoPublications", + enforcement: false, + backendVolumes: map[string]*storage.Volume{"vol1": vol1}, + nodes: nil, + publications: nil, + expectReconcile: true, + expectedNodeNames: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + savedContext := config.CurrentDriverContext + config.CurrentDriverContext = config.ContextCSI + defer func() { config.CurrentDriverContext = savedContext }() + + mockCtrl := gomock.NewController(t) + mockBackend := mockstorage.NewMockBackend(mockCtrl) + mockBackend.EXPECT().Name().Return("backend1").AnyTimes() + mockBackend.EXPECT().BackendUUID().Return(backendUUID).AnyTimes() + mockBackend.EXPECT().CanEnablePublishEnforcement().Return(tt.enforcement).AnyTimes() + mockBackend.EXPECT().Volumes().Return(makeSyncMapFromMap(tt.backendVolumes)).AnyTimes() + + if tt.expectReconcile { + mockBackend.EXPECT().ReconcileNodeAccess(gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, nodes []*models.Node, _ string) error { + names := make([]string, 0, len(nodes)) + for _, n := range nodes { + names = append(names, n.Name) + } + assert.ElementsMatch(t, tt.expectedNodeNames, names) + return nil + }) + mockBackend.EXPECT().SetNodeAccessUpToDate() + } else { + mockBackend.EXPECT().ReconcileNodeAccess(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + mockBackend.EXPECT().SetNodeAccessUpToDate().Times(0) + } + + o := getConcurrentOrchestrator() + err := o.reconcileNodeAccessOnBackend(context.Background(), mockBackend, tt.publications, tt.nodes, nil) + + assert.NoError(t, err) + }) + } +} + func TestPeriodicallyReconcileNodeAccessOnBackendsConcurrentCore(t *testing.T) { tests := []struct { name string @@ -14609,6 +14828,7 @@ func TestReconcileBackendStateConcurrentCore(t *testing.T) { mockBackend.EXPECT().SmartCopy().Return(mockBackend).AnyTimes() mockBackend.EXPECT().ConstructPersistent(gomock.Any()).Return(&storage.BackendPersistent{Name: "backend1", BackendUUID: "uuid1"}).AnyTimes() mockBackend.EXPECT().Terminate(gomock.Any()).Times(1) + mockBackend.EXPECT().Volumes().Return(&sync.Map{}).AnyTimes() mockStoreClient.EXPECT().UpdateBackend(gomock.Any(), gomock.Any()).Return(nil).Times(1) // Add backend to cache diff --git a/core/orchestrator_core.go b/core/orchestrator_core.go index 91933aa6a..3e46c007a 100644 --- a/core/orchestrator_core.go +++ b/core/orchestrator_core.go @@ -6611,6 +6611,10 @@ func (o *TridentOrchestrator) reconcileNodeAccessOnBackend(ctx context.Context, if b.CanEnablePublishEnforcement() { nodes = o.publishedNodesForBackend(b) + if len(nodes) == 0 && hasPublicationsForBackend(b, o.volumePublications.ListPublications()) { + logSkippedNodeAccessReconcile(ctx, b) + return nil + } volToNodePublications := o.volumePublicationsForBackend(b) nodeMap := make(map[string]*models.Node) @@ -6643,6 +6647,10 @@ func (o *TridentOrchestrator) reconcileNodeAccessOnBackend(ctx context.Context, } else { nodes = o.nodes.List() + if len(nodes) == 0 && hasPublicationsForBackend(b, o.volumePublications.ListPublications()) { + logSkippedNodeAccessReconcile(ctx, b) + return nil + } } if err := b.ReconcileNodeAccess(ctx, nodes, o.uuid); err != nil { @@ -6653,14 +6661,15 @@ func (o *TridentOrchestrator) reconcileNodeAccessOnBackend(ctx context.Context, return nil } -// publishedNodesForBackend returns the nodes that a backend has published volumes to +// publishedNodesForBackend returns the nodes that a backend has published volumes to. Publications +// for subordinate volumes count toward the share-source volume that hosts them. func (o *TridentOrchestrator) publishedNodesForBackend(b storage.Backend) []*models.Node { pubs := o.volumePublications.ListPublications() volumes := b.Volumes() m := make(map[string]struct{}) for _, pub := range pubs { - if _, ok := volumes.Load(pub.VolumeName); ok { + if _, ok := volumes.Load(o.hostingVolumeName(pub.VolumeName)); ok { m[pub.NodeName] = struct{}{} } } @@ -6675,6 +6684,15 @@ func (o *TridentOrchestrator) publishedNodesForBackend(b storage.Backend) []*mod return nodes } +// hostingVolumeName maps a subordinate volume to the share-source volume whose storage it uses; +// a backend's volume map only tracks the source. Any other name maps to itself. +func (o *TridentOrchestrator) hostingVolumeName(volumeName string) string { + if subordinate, ok := o.subordinateVolumes[volumeName]; ok { + return subordinate.Config.ShareSourceVolume + } + return volumeName +} + func (o *TridentOrchestrator) volumePublicationsForBackend(b storage.Backend) map[string][]*models.VolumePublication { volumes := b.Volumes() volumeToNodePublications := make(map[string][]*models.VolumePublication) diff --git a/core/orchestrator_core_test.go b/core/orchestrator_core_test.go index a5510c303..5beef1d96 100644 --- a/core/orchestrator_core_test.go +++ b/core/orchestrator_core_test.go @@ -10860,6 +10860,161 @@ func TestPublishedNodesForBackend(t *testing.T) { assert.Equal(t, expectedNodes, actualNodes) } +// A subordinate volume's publication is recorded under the subordinate's name, which a backend's +// volume map never contains; the node must still count toward the share-source volume's backend. +func TestPublishedNodesForBackend_SubordinatePublication(t *testing.T) { + mockCtrl := gomock.NewController(t) + mockBackend := mockstorage.NewMockBackend(mockCtrl) + o := getOrchestrator(t, false) + + o.nodes.Set("nodeA", &models.Node{Name: "nodeA"}) + o.nodes.Set("nodeB", &models.Node{Name: "nodeB"}) + o.subordinateVolumes["sub1"] = &storage.Volume{ + Config: &storage.VolumeConfig{Name: "sub1", ShareSourceVolume: "vol1"}, + } + o.volumePublications.Set("sub1", "nodeB", &models.VolumePublication{NodeName: "nodeB", VolumeName: "sub1"}) + o.volumePublications.Set("sub9", "nodeA", &models.VolumePublication{NodeName: "nodeA", VolumeName: "sub9"}) + mockBackend.EXPECT().Volumes().Return(makeSyncMap(map[string]*storage.Volume{ + "vol1": {Config: &storage.VolumeConfig{Name: "vol1"}}, + })) + + actualNodes := o.publishedNodesForBackend(mockBackend) + + assert.Equal(t, []*models.Node{o.nodes.Get("nodeB")}, actualNodes, + "the subordinate's node counts for the source's backend; an unknown subordinate does not") +} + +// A publish-enforcement backend derives its node set from publications intersected with its own +// volume map and the node cache. If that intersection is empty while publications still exist, +// the driver would be told to remove every export rule from an in-use policy, so reconciliation +// must be deferred instead and the backend left marked for retry. +func TestReconcileNodeAccessOnBackend_EmptyNodeSet(t *testing.T) { + const backendUUID = "uuid1" + vol1 := &storage.Volume{Config: &storage.VolumeConfig{Name: "vol1"}, BackendUUID: backendUUID} + + tests := []struct { + name string + enforcement bool + backendVolumes map[string]*storage.Volume + nodes []string + publications []*models.VolumePublication + expectReconcile bool + expectedNodeNames []string + expectVolumeRecons int + }{ + { + name: "EnforcedVolumeMissingFromBackendMap", + enforcement: true, + backendVolumes: map[string]*storage.Volume{}, + nodes: []string{"nodeA"}, + publications: []*models.VolumePublication{ + {VolumeName: "vol1", NodeName: "nodeA", BackendUUID: backendUUID}, + }, + expectReconcile: false, + }, + { + name: "EnforcedLegacyPublicationNodeMissingFromCache", + enforcement: true, + backendVolumes: map[string]*storage.Volume{"vol1": vol1}, + nodes: nil, + publications: []*models.VolumePublication{ + {VolumeName: "vol1", NodeName: "nodeA"}, + }, + expectReconcile: false, + }, + { + // Only another backend's volume is published: this backend legitimately has no + // consumers, so reconciling to an empty set (stripping its rules) must still happen. + name: "EnforcedNoPublicationsStripsRules", + enforcement: true, + backendVolumes: map[string]*storage.Volume{"vol1": vol1}, + nodes: []string{"nodeA"}, + publications: []*models.VolumePublication{ + {VolumeName: "vol9", NodeName: "nodeA", BackendUUID: "some-other-backend"}, + }, + expectReconcile: true, + expectedNodeNames: []string{}, + expectVolumeRecons: 1, + }, + { + name: "EnforcedConsistentView", + enforcement: true, + backendVolumes: map[string]*storage.Volume{"vol1": vol1}, + nodes: []string{"nodeA"}, + publications: []*models.VolumePublication{ + {VolumeName: "vol1", NodeName: "nodeA", BackendUUID: backendUUID}, + }, + expectReconcile: true, + expectedNodeNames: []string{"nodeA"}, + expectVolumeRecons: 1, + }, + { + name: "UnenforcedNodeCacheEmptyWhilePublished", + enforcement: false, + backendVolumes: map[string]*storage.Volume{"vol1": vol1}, + nodes: nil, + publications: []*models.VolumePublication{ + {VolumeName: "vol1", NodeName: "nodeA", BackendUUID: backendUUID}, + }, + expectReconcile: false, + }, + { + name: "UnenforcedNodeCacheEmptyNoPublications", + enforcement: false, + backendVolumes: map[string]*storage.Volume{"vol1": vol1}, + nodes: nil, + publications: nil, + expectReconcile: true, + expectedNodeNames: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + savedContext := config.CurrentDriverContext + config.CurrentDriverContext = config.ContextCSI + defer func() { config.CurrentDriverContext = savedContext }() + + mockCtrl := gomock.NewController(t) + mockBackend := mockstorage.NewMockBackend(mockCtrl) + mockBackend.EXPECT().Name().Return("backend1").AnyTimes() + mockBackend.EXPECT().BackendUUID().Return(backendUUID).AnyTimes() + mockBackend.EXPECT().CanEnablePublishEnforcement().Return(tt.enforcement).AnyTimes() + mockBackend.EXPECT().Volumes().Return(makeSyncMap(tt.backendVolumes)).AnyTimes() + + o := getOrchestrator(t, false) + o.volumes["vol1"] = vol1 + for _, n := range tt.nodes { + o.nodes.Set(n, &models.Node{Name: n}) + } + for _, pub := range tt.publications { + o.volumePublications.Set(pub.VolumeName, pub.NodeName, pub) + } + + if tt.expectReconcile { + mockBackend.EXPECT().ReconcileVolumeNodeAccess(coreCtx, gomock.Any(), gomock.Any()). + Return(nil).Times(tt.expectVolumeRecons) + mockBackend.EXPECT().ReconcileNodeAccess(coreCtx, gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, nodes []*models.Node, _ string) error { + names := make([]string, 0, len(nodes)) + for _, n := range nodes { + names = append(names, n.Name) + } + assert.ElementsMatch(t, tt.expectedNodeNames, names) + return nil + }) + mockBackend.EXPECT().SetNodeAccessUpToDate() + } else { + mockBackend.EXPECT().ReconcileVolumeNodeAccess(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + mockBackend.EXPECT().ReconcileNodeAccess(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + mockBackend.EXPECT().SetNodeAccessUpToDate().Times(0) + } + + assert.NoError(t, o.reconcileNodeAccessOnBackend(coreCtx, mockBackend)) + }) + } +} + func TestVolumePublicationsForBackend(t *testing.T) { mockCtrl := gomock.NewController(t) mockBackend := mockstorage.NewMockBackend(mockCtrl)