Skip to content

Commit 3208d6d

Browse files
notsrchclaude
andcommitted
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-<backendUUID>, 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: #1179 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 parent 8ce1236 commit 3208d6d

7 files changed

Lines changed: 520 additions & 20 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
- **Kubernetes:** Fixed race conditions and state handling for concurrent publish/unpublish, clone, cache, and backend update operations.
1212
- **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.
13+
- **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)).
1314
- **Kubernetes:** Fixed ONTAP-NAS-Economy FlexVol discovery after MetroCluster failover when snapshot policies differ (Issue [#1082](https://github.com/NetApp/trident/issues/1082)).
1415
- **Kubernetes:** Fixed ONTAP-NAS-Economy volume delete retries that interrupted long-running qtree deletes (Issue [#1121](https://github.com/NetApp/trident/issues/1121)).
1516
- **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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,41 @@ func generateVolumePublication(volName string, publishInfo *models.VolumePublish
307307
return vp
308308
}
309309

310+
// hasPublicationsForBackend reports whether any publication belongs to backend b. A publication
311+
// records the UUID of the backend hosting its volume; records that predate that field are
312+
// attributed by checking whether their volume is in b's volume map.
313+
func hasPublicationsForBackend(b storage.Backend, publications []*models.VolumePublication) bool {
314+
backendUUID := b.BackendUUID()
315+
var volumes *sync.Map
316+
for _, pub := range publications {
317+
if pub.BackendUUID != "" {
318+
if pub.BackendUUID == backendUUID {
319+
return true
320+
}
321+
continue
322+
}
323+
if volumes == nil {
324+
volumes = b.Volumes()
325+
}
326+
if _, ok := volumes.Load(pub.VolumeName); ok {
327+
return true
328+
}
329+
}
330+
return false
331+
}
332+
333+
// logSkippedNodeAccessReconcile records that node-access reconciliation for b was deferred: its
334+
// computed node set was empty while publications still exist, so proceeding would remove every
335+
// export rule from a policy that is still in use. The backend stays marked as needing
336+
// reconciliation and the periodic loop retries once the caches agree.
337+
func logSkippedNodeAccessReconcile(ctx context.Context, b storage.Backend) {
338+
Logc(ctx).WithFields(LogFields{
339+
"backend": b.Name(),
340+
"backendUUID": b.BackendUUID(),
341+
}).Warn("Publications exist for this backend but none of their nodes could be resolved; " +
342+
"skipping node access reconciliation instead of removing every export rule.")
343+
}
344+
310345
// isDockerPluginMode returns true if the ENV variable config.DockerPluginModeEnvVariable is set
311346
func isDockerPluginMode() bool {
312347
return os.Getenv(config.DockerPluginModeEnvVariable) != ""

core/common_test.go

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1305,3 +1305,33 @@ func TestIsBackendBootstrapTimeout(t *testing.T) {
13051305
})
13061306
}
13071307
}
1308+
1309+
func TestHasPublicationsForBackend(t *testing.T) {
1310+
backend := getFakeBackend("backend1", "uuid1", nil)
1311+
backend.Volumes().Store("vol1", getFakeVolume("vol1", "uuid1"))
1312+
1313+
tests := []struct {
1314+
name string
1315+
publications []*models.VolumePublication
1316+
want bool
1317+
}{
1318+
{"NoPublications", nil, false},
1319+
{"PublicationForThisBackend", []*models.VolumePublication{
1320+
{VolumeName: "other", NodeName: "n1", BackendUUID: "uuid1"},
1321+
}, true},
1322+
{"PublicationForAnotherBackend", []*models.VolumePublication{
1323+
{VolumeName: "vol1", NodeName: "n1", BackendUUID: "uuid2"},
1324+
}, false},
1325+
{"LegacyPublicationForHostedVolume", []*models.VolumePublication{
1326+
{VolumeName: "vol1", NodeName: "n1"},
1327+
}, true},
1328+
{"LegacyPublicationForUnknownVolume", []*models.VolumePublication{
1329+
{VolumeName: "vol9", NodeName: "n1"},
1330+
}, false},
1331+
}
1332+
for _, tt := range tests {
1333+
t.Run(tt.name, func(t *testing.T) {
1334+
assert.Equal(t, tt.want, hasPublicationsForBackend(backend, tt.publications))
1335+
})
1336+
}
1337+
}

core/concurrent_core.go

Lines changed: 59 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1185,7 +1185,8 @@ func (o *ConcurrentTridentOrchestrator) reconcileNodeAccessOnAllBackends(ctx con
11851185
// just log it and continue.
11861186
err = func() error {
11871187
_, results, unlocker, dbErr := db.Lock(ctx, db.Query(
1188-
db.ListVolumePublications(), db.ListNodes(), db.UpsertBackend(backend.BackendUUID(), "", "")))
1188+
db.ListVolumePublications(), db.ListNodes(), db.ListSubordinateVolumes(),
1189+
db.UpsertBackend(backend.BackendUUID(), "", "")))
11891190
defer unlocker()
11901191
if dbErr != nil {
11911192
return dbErr
@@ -1196,8 +1197,8 @@ func (o *ConcurrentTridentOrchestrator) reconcileNodeAccessOnAllBackends(ctx con
11961197
return errors.NotFoundError("backend %s not found for reconcile", backend.BackendUUID())
11971198
}
11981199

1199-
if reconcileErr := o.reconcileNodeAccessOnBackend(
1200-
ctx, upsertBackend, results[0].VolumePublications, results[0].Nodes); reconcileErr != nil {
1200+
if reconcileErr := o.reconcileNodeAccessOnBackend(ctx, upsertBackend,
1201+
results[0].VolumePublications, results[0].Nodes, results[0].SubordinateVolumes); reconcileErr != nil {
12011202
return reconcileErr
12021203
}
12031204
results[0].Backend.Upsert(upsertBackend)
@@ -1217,7 +1218,7 @@ func (o *ConcurrentTridentOrchestrator) reconcileNodeAccessOnAllBackends(ctx con
12171218
}
12181219

12191220
func (o *ConcurrentTridentOrchestrator) reconcileNodeAccessOnBackend(ctx context.Context, b storage.Backend,
1220-
allVolumePublications []*models.VolumePublication, allNodes []*models.Node,
1221+
allVolumePublications []*models.VolumePublication, allNodes []*models.Node, subordinateVolumes []*storage.Volume,
12211222
) error {
12221223
if config.CurrentDriverContext != config.ContextCSI {
12231224
return nil
@@ -1226,11 +1227,23 @@ func (o *ConcurrentTridentOrchestrator) reconcileNodeAccessOnBackend(ctx context
12261227
var nodes []*models.Node
12271228

12281229
if b.CanEnablePublishEnforcement() {
1229-
nodes = publishedNodesForBackend(b, allVolumePublications, allNodes)
1230+
var unregisteredNodes []string
1231+
nodes, unregisteredNodes = publishedNodesForBackend(b, allVolumePublications, allNodes, subordinateVolumes)
1232+
if len(unregisteredNodes) > 0 {
1233+
Logc(ctx).WithFields(LogFields{
1234+
"backend": b.Name(),
1235+
"unregisteredNodes": unregisteredNodes,
1236+
}).Warn("Some publications name nodes that are not registered; reconciling node access without them.")
1237+
}
12301238
} else {
12311239
nodes = allNodes
12321240
}
12331241

1242+
if len(nodes) == 0 && hasPublicationsForBackend(b, allVolumePublications) {
1243+
logSkippedNodeAccessReconcile(ctx, b)
1244+
return nil
1245+
}
1246+
12341247
if err := b.ReconcileNodeAccess(ctx, nodes, o.uuid); err != nil {
12351248
return err
12361249
}
@@ -1289,28 +1302,48 @@ func (o *ConcurrentTridentOrchestrator) updateLastNodeRegistrationTime() {
12891302
o.lastNodeRegistrationTime = time.Now()
12901303
}
12911304

1292-
// publishedNodesForBackend returns the nodes that a backend has published volumes to
1305+
// publishedNodesForBackend returns the nodes that a backend has published volumes to, plus the
1306+
// names of published nodes that are absent from allNodes and therefore left out. Publications for
1307+
// subordinate volumes count toward the share-source volume that hosts them, since a backend's
1308+
// volume map only tracks the source.
12931309
func publishedNodesForBackend(b storage.Backend, allVolumePublications []*models.VolumePublication,
1294-
allNodes []*models.Node,
1295-
) []*models.Node {
1310+
allNodes []*models.Node, subordinateVolumes []*storage.Volume,
1311+
) (nodes []*models.Node, unregisteredNodes []string) {
12961312
nodesByName := make(map[string]*models.Node, len(allNodes))
12971313
for _, n := range allNodes {
12981314
nodesByName[n.Name] = n
12991315
}
13001316

1317+
shareSourceByName := make(map[string]string, len(subordinateVolumes))
1318+
for _, subordinate := range subordinateVolumes {
1319+
shareSourceByName[subordinate.Config.Name] = subordinate.Config.ShareSourceVolume
1320+
}
1321+
13011322
volumes := b.Volumes()
1302-
m := make(map[string]*models.Node)
1323+
seen := make(map[string]struct{})
13031324
for _, pub := range allVolumePublications {
1304-
if _, ok := volumes.Load(pub.VolumeName); ok {
1305-
m[pub.NodeName] = nodesByName[pub.NodeName]
1325+
hostingVolume := pub.VolumeName
1326+
if source, ok := shareSourceByName[hostingVolume]; ok {
1327+
hostingVolume = source
13061328
}
1307-
}
1329+
if _, ok := volumes.Load(hostingVolume); !ok {
1330+
continue
1331+
}
1332+
if _, done := seen[pub.NodeName]; done {
1333+
continue
1334+
}
1335+
seen[pub.NodeName] = struct{}{}
13081336

1309-
nodes := make([]*models.Node, 0, len(m))
1310-
for _, n := range m {
1311-
nodes = append(nodes, n)
1337+
// Drivers dereference every node they are given, so a publication whose node is not
1338+
// registered must be reported rather than passed through as a nil entry.
1339+
if node := nodesByName[pub.NodeName]; node != nil {
1340+
nodes = append(nodes, node)
1341+
} else {
1342+
unregisteredNodes = append(unregisteredNodes, pub.NodeName)
1343+
}
13121344
}
1313-
return nodes
1345+
sort.Strings(unregisteredNodes)
1346+
return nodes, unregisteredNodes
13141347
}
13151348

13161349
func (o *ConcurrentTridentOrchestrator) AddFrontend(ctx context.Context, f frontend.Plugin) {
@@ -1752,7 +1785,8 @@ func (o *ConcurrentTridentOrchestrator) upsertBackend(
17521785
Logc(ctx).Debug(">>>>>> upsertBackend")
17531786
defer Logc(ctx).Debug("<<<<<< upsertBackend")
17541787

1755-
_, results, unlocker, err := db.NestedLock(ctx, db.Query(db.ListVolumePublications(), db.ListNodes()))
1788+
_, results, unlocker, err := db.NestedLock(ctx, db.Query(
1789+
db.ListVolumePublications(), db.ListNodes(), db.ListSubordinateVolumes()))
17561790
defer unlocker()
17571791
if err != nil {
17581792
return nil, err
@@ -1777,7 +1811,8 @@ func (o *ConcurrentTridentOrchestrator) upsertBackend(
17771811

17781812
// Node access rules may have changed in the backend config
17791813
backend.InvalidateNodeAccess()
1780-
err = o.reconcileNodeAccessOnBackend(ctx, backend, results[0].VolumePublications, results[0].Nodes)
1814+
err = o.reconcileNodeAccessOnBackend(ctx, backend, results[0].VolumePublications, results[0].Nodes,
1815+
results[0].SubordinateVolumes)
17811816
if err != nil {
17821817
return nil, err
17831818
}
@@ -1973,6 +2008,12 @@ func (o *ConcurrentTridentOrchestrator) updateBackend(
19732008
}
19742009
}
19752010

2011+
// upsertBackend reconciles node access against this new object before updateBackendVolumes
2012+
// refreshes its volume map, and the AddBackend-on-existing-name path never refreshes it at
2013+
// all. Carry the volumes forward so reconciliation sees the backend's real consumers rather
2014+
// than an empty set, which the NAS drivers turn into "remove every export policy rule".
2015+
backend.SetVolumes(originalBackend.Volumes())
2016+
19762017
// The fake driver needs volumes copied forward
19772018
if originalFakeDriver, ok := originalBackend.Driver().(*fake.StorageDriver); ok {
19782019
Logc(ctx).Debug("Using fake driver, going to copy volumes forward...")

0 commit comments

Comments
 (0)