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)