From 26f9c6e10debe3fd561048001e85b30d95406a1b Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 14:45:03 +0200 Subject: [PATCH] nfs-server: one test per guard (each stray guard, the failed-reload retry, the empty data list, the walk's stop, a stopping pass); say that a start drops hand-made exports and that unknown backup paths are a deliberate warning (review of #142) --- modules/nfs-server/README.md | 10 +- .../nfs-server/cmd/nfs-server/guards_test.go | 111 ++++++++++++++++++ 2 files changed, 120 insertions(+), 1 deletion(-) create mode 100644 modules/nfs-server/cmd/nfs-server/guards_test.go diff --git a/modules/nfs-server/README.md b/modules/nfs-server/README.md index 5c6632a0..3c17f47a 100644 --- a/modules/nfs-server/README.md +++ b/modules/nfs-server/README.md @@ -39,7 +39,10 @@ another node requires. keeps the last paths, and the status and the health warn that they are the last answer, with its age and why. Until a list has been learnt, what is exported stays exported and the health warns that the list is not known: a slow bus is no reason to take the library away, and the look below still guards. - A warning is said in the health without making the module unhealthy. + A warning is said in the health without making the module unhealthy. **This is a deliberate choice**: + while the backup seat's paths are unknown the module stays healthy with a warning, rather than + unhealthy or refusing, because the look below every share still refuses one that holds a repository, + and taking the library away from every node because the bus was slow would be the larger harm. - **Only while its filesystem is mounted.** A share that is a filesystem of its own is exported with `mp`, a folder inside one with `mp=`, so an unmounted dataset never exports the empty folder under it. A folder of the root filesystem carries neither. @@ -130,6 +133,11 @@ client inside the private network's range that no other export file names. While exports such an entry, the module is unhealthy and says which. An export another file makes (ZFS's own, `/etc/exports`) is not the module's and is left alone. +**A hand-made export does not survive.** `exportfs -ra` makes the kernel's table exactly what the export +files say, so an export a person added by hand with `exportfs -o …` (in no file) is dropped by the first +pass of every start of the process, and by any later reload. An export meant to last belongs in a file: +the module's settings for a share of the mesh, or `/etc/exports` for anything else. + ## What it leaves - **A subfolder export shares its filesystem.** A share that is a folder inside a larger filesystem (not a filesystem of its own) is exported with `no_subtree_check`, so a client that forges file handles could diff --git a/modules/nfs-server/cmd/nfs-server/guards_test.go b/modules/nfs-server/cmd/nfs-server/guards_test.go new file mode 100644 index 00000000..add729ff --- /dev/null +++ b/modules/nfs-server/cmd/nfs-server/guards_test.go @@ -0,0 +1,111 @@ +package main + +// One test per guard of the review of #142, each failing when its guard alone is taken away. + +import ( + "context" + "encoding/json" + "strings" + "testing" +) + +func TestStrayFindsAnExportOfTheModulesThatNoFileCarries(t *testing.T) { + live := []Entry{{Path: "/srv/library", Client: "10.0.0.4"}} + if got := Stray(live, nil, nil, meshRange); len(got) != 1 { + t.Errorf("an export inside the mesh range that no file carries is the module's leftover: %+v", got) + } +} + +// The foreign-file guard alone: the client is inside the mesh's range, so only the other file tells it apart. +func TestStrayLeavesAnExportAnotherFileCarriesEvenInsideTheMeshRange(t *testing.T) { + // ZFS's own file exports the library to a client inside the mesh's range. + foreign := ParseExports("/srv/library 10.0.0.9(rw,no_subtree_check)\n", "/etc/exports.d/zfs.exports") + live := []Entry{{Path: "/srv/library", Client: "10.0.0.9", Options: "rw"}} + if got := Stray(live, nil, foreign, meshRange); len(got) != 0 { + t.Errorf("what ZFS's file exports is not the module's, whatever its client: %+v", got) + } +} + +// The range guard alone: no file carries the client, so only the range tells it apart. +func TestStrayLeavesAClientOutsideTheMeshRange(t *testing.T) { + live := []Entry{{Path: "/srv/library", Client: "192.0.2.0/24"}, {Path: "/srv/library", Client: "192.0.2.7"}, + {Path: "/srv/library", Client: "10.0.0.0/16"}} + if got := Stray(live, nil, nil, meshRange); len(got) != 0 { + t.Errorf("a client the module never exports to (outside or wider than the mesh's range) is not its leftover: %+v", got) + } +} + +// The failed-reload retry alone: the file is unchanged, every share is live and nothing is stray, so the +// last pass's failed reload is the only reason to reload. +func TestAFailedReloadIsTriedAgainWhenNothingElseAsks(t *testing.T) { + f := newFake(t) + f.mountinfo("/", "/srv/library") + f.dir("/srv/library") + f.file(ConfigFile, grantsConfig("library=/srv/library", "library=ws:rw")) + f.roster("ws 10.0.0.3") + f.wants(map[string][]Want{"ws": {{Share: "library", Access: "rw"}}}) + f.kernelReads() + m := f.machine() + s := m.Pass(context.Background(), nil) + if !s.Shares[0].Live { + t.Fatalf("the library was not exported: %+v", s) + } + + f.ran = nil + if s = m.Pass(context.Background(), &s); countExportfs(f.ran) != 0 { + t.Fatalf("with nothing to do a pass reloads: %v", f.ran) + } + s.ReloadError = "exportfs: stuck" + f.ran = nil + m.Pass(context.Background(), &s) + if countExportfs(f.ran) == 0 { + t.Errorf("the last pass's failed reload was not tried again: %v", f.ran) + } +} + +func TestBackupFromRefusesAnAnswerWithoutADataList(t *testing.T) { + seats := json.RawMessage(`{"answer":{"seats":[{"seat":"node-backup","holders":[{"module":"restic","node":"ace"}]}]}}`) + for _, data := range []string{`{"answer":{}}`, `{"answer":{"data":null}}`} { + if _, paths, err := BackupFrom("ace", seats, json.RawMessage(data)); err == nil { + t.Errorf("%s was read as the empty list %v: never \"nothing to fear\"", data, paths) + } + } +} + +func TestTheLookBelowStopsWhenTheProcessStops(t *testing.T) { + f := newFake(t) + f.dir("/srv/library/movies/a") + m := f.machine() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + m.setWalk(ctx) + if found, complete := m.walkBelow("/srv/library"); complete || found != "" { + t.Errorf("a look the process ended is unfinished, so the share is refused rather than exported: %q %v", found, complete) + } +} + +func TestAPassEndedByStoppingWritesAndReloadsNothing(t *testing.T) { + f := newFake(t) + f.mountinfo("/", "/srv/library") + f.dir("/srv/library") + f.file(ConfigFile, grantsConfig("library=/srv/library", "library=ws:rw")) + f.roster("ws 10.0.0.3") + f.wants(map[string][]Want{"ws": {{Share: "library", Access: "rw"}}}) + f.kernelReads() + m := f.machine() + s := m.Pass(context.Background(), nil) + file, status := f.read(ExportFile), f.read(StatusFile) + + // The grant is withdrawn, and the process is stopping as the next pass runs. + f.file(ConfigFile, grantsConfig("library=/srv/library", "none")) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + f.ran = nil + got := m.Pass(ctx, &s) + if f.read(ExportFile) != file || f.read(StatusFile) != status || countExportfs(f.ran) != 0 { + t.Errorf("a pass ended by stopping wrote or reloaded: file %q, ran %v", f.read(ExportFile), f.ran) + } + if !strings.Contains(strings.Join(got.Problems, ";"), "nothing was written") { + t.Errorf("it says so: %+v", got.Problems) + } +}