Stop node-access reconciliation from emptying an in-use export policy

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.

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) != ""

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))
+		})
+	}
+}

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...")