From 7320d99a117ef3301d80c628b22b03c5f78d62be Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 21:39:36 +0200 Subject: [PATCH] Make nfs-server reconcile the kernel's exports with its file, so a failed reload or a withdrawn grant is not left exported while the module says it is healthy; warn on a stale or unknown backup list --- modules/nfs-server/README.md | 27 ++- modules/nfs-server/cmd/nfs-server/backup.go | 33 ++- .../nfs-server/cmd/nfs-server/backup_test.go | 61 +++++- modules/nfs-server/cmd/nfs-server/machine.go | 4 +- .../cmd/nfs-server/manifest_test.go | 14 ++ modules/nfs-server/cmd/nfs-server/process.go | 89 +++++++- .../cmd/nfs-server/reconcile_test.go | 201 ++++++++++++++++++ modules/nfs-server/cmd/nfs-server/shares.go | 1 + modules/nfs-server/cmd/nfs-server/tools.go | 11 +- 9 files changed, 418 insertions(+), 23 deletions(-) create mode 100644 modules/nfs-server/cmd/nfs-server/reconcile_test.go diff --git a/modules/nfs-server/README.md b/modules/nfs-server/README.md index faf96ea3..5c6632a0 100644 --- a/modules/nfs-server/README.md +++ b/modules/nfs-server/README.md @@ -35,9 +35,11 @@ another node requires. holds `node-backup` on this machine (`seat:mesh-controller.seats`) and every path that module keeps data in here (`seat:mesh-controller.data`; on the home server the restic repository and its state). They leave it in `/var/lib/nfs-server/from-bus/backup.json`, and every pass refuses a share at, inside or - holding one of those paths, with nothing kept. A failed ask keeps the last paths and says why. Until a - list has been learnt, what is exported stays exported and the status says the list is not known: a slow - bus is no reason to take the library away, and the look below still guards. + holding one of those paths, compared after resolving symbolic links, with nothing kept. A failed ask + 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. - **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. @@ -102,6 +104,12 @@ its config file, so on unassignment the config file is gone when the process is removes its export file — the one removal its code makes — and runs `exportfs -ra`, so the kernel lets the shares go. A plain restart, a push or a shutdown leaves the config file in place, and the exports stay. +**Two signs of being taken away.** The process takes a stop as an unassignment when its config file is +gone, or when its own unit (`nfs-server-exports.service`) is disabled: the node-engine stops a process it +removes with `systemctl disable --now`, which disables the unit first, while a restart, a push or a +shutdown leaves it enabled. The second sign covers a first apply that failed and recorded the process +after its config (mesh-host #62), when the config is still there at the stop. + **Within the stop timeout.** The service manager kills a process that has not stopped 90 s after SIGTERM (mesh-host #62), and a killed process cleans up nothing. So SIGTERM ends a running pass at once (its commands and its look below a share are cancelled, and it writes nothing), and the clean-up has its own @@ -109,6 +117,19 @@ bound of 10 s: the export file is removed first, which needs no command, then `e that bound. Whatever it could not finish is logged as `nfs-server: UNASSIGNED, NOT CLEAN: …`, with what a person must do. +**If it is killed anyway** (it did not stop within the service manager's 90 s), nothing cleans up and the +module never starts again. Then, on the home server as root: remove `/etc/exports.d/mesh.exports` and run +`exportfs -ra`; `exportfs -v` should list no client of the private network's range for the module's folders. + +## The kernel's table follows the file + +The process has the kernel read the export files again (`exportfs -ra`) on its first pass, after any +pass whose reload failed, when the file changed, when a share it exports is missing from the kernel's +table, and when the kernel exports one of the module's folders to a client the file no longer has — a +client inside the private network's range that no other export file names. While the kernel still +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. + ## 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/backup.go b/modules/nfs-server/cmd/nfs-server/backup.go index b9799bcd..8bf3d6fd 100644 --- a/modules/nfs-server/cmd/nfs-server/backup.go +++ b/modules/nfs-server/cmd/nfs-server/backup.go @@ -14,8 +14,8 @@ package main // been read, what is exported stays exported and the status says the list is not known: the bus being // slow is no reason to take the library away, and the look below still guards against a repository. // -// The parsing of mesh-controller.data's answer is a small copy of the mounts module's (DataFrom); shared -// code belongs in mesh-sdk, and moves there when a third module needs it. +// The parsing of mesh-controller.data's answer is a small copy of the mounts module's (DataFrom). Shared +// code belongs in mesh-sdk: moving both there is mesh-sdk issue 13. import ( "encoding/json" @@ -40,6 +40,8 @@ type BackupFromBus struct { Holder string `json:"holder"` Paths []string `json:"paths"` Looked string `json:"looked,omitempty"` + // Learnt is when an ask last succeeded: how old the holder and paths are. + Learnt string `json:"learnt,omitempty"` // Error is why the last ask got no answer; the holder and paths are then the last answer's. Error string `json:"error,omitempty"` } @@ -123,7 +125,7 @@ func (m *Machine) TakeBackupWith(machine string, ask func(key string, body any) var paths []string holder, paths, err = BackupFrom(machine, seats, data) if err == nil { - next.Holder, next.Paths = holder, paths + next.Holder, next.Paths, next.Learnt = holder, paths, next.Looked } } if err != nil { @@ -136,7 +138,8 @@ func (m *Machine) TakeBackupWith(machine string, ask func(key string, body any) } } -// Backup is what the tools last learnt, and a sentence when nothing usable has been learnt yet. +// Backup is what the tools last learnt, and a sentence when it cannot be relied on as it stands: nothing +// usable learnt yet, or a later ask failed and what is used is the last answer, with its age. func (m *Machine) Backup() (BackupFromBus, string) { text, err := m.Read(BackupFile) if err != nil { @@ -153,9 +156,31 @@ func (m *Machine) Backup() (BackupFromBus, string) { } return b, "where this machine's backup seat keeps its repository is not known (" + why + "); the shares are judged by the look below them alone until it is" } + if b.Error != "" { + age := "of unknown age" + if at, err := time.Parse(time.RFC3339, b.Learnt); err == nil { + age = fmt.Sprintf("learnt %s ago, at %s", m.Now().Sub(at).Round(time.Second), at.Local().Format("15:04")) + } + return b, fmt.Sprintf("where this machine's backup seat (%s) keeps its data is the last answer, %s: the "+ + "latest ask failed (%s)", b.Holder, age, b.Error) + } return b, "" } +// realPaths are paths as the kernel resolves them, so a backup path named through a symbolic link is +// compared where it really is; one that cannot be resolved is kept as named. +func (m *Machine) realPaths(paths []string) []string { + out := make([]string, 0, len(paths)) + for _, p := range paths { + if real, err := m.Real(p); err == nil { + out = append(out, real) + } else { + out = append(out, p) + } + } + return out +} + // nearBackup is the backup seat's path a real path is at, under or above, or "". func nearBackup(real string, b BackupFromBus) (string, string) { for _, p := range b.Paths { diff --git a/modules/nfs-server/cmd/nfs-server/backup_test.go b/modules/nfs-server/cmd/nfs-server/backup_test.go index 5e576fb4..7ae3298b 100644 --- a/modules/nfs-server/cmd/nfs-server/backup_test.go +++ b/modules/nfs-server/cmd/nfs-server/backup_test.go @@ -75,8 +75,8 @@ func TestWithoutTheBackupListTheExportsStayAndItIsSaid(t *testing.T) { if refusedWhy(s, "library") != "" || len(s.Shares) != 1 { t.Fatalf("the library is not exported while the backup list is unknown: %+v", s) } - if !strings.Contains(strings.Join(s.Said, "\n"), "backup seat") { - t.Errorf("the status does not say the backup seat's repository is not known: %v", s.Said) + if !strings.Contains(strings.Join(s.Warnings, "\n"), "backup seat") { + t.Errorf("the status does not warn that the backup seat's repository is not known: %v", s.Warnings) } } @@ -114,3 +114,60 @@ func TestAFailedAskKeepsTheLastBackupPaths(t *testing.T) { t.Errorf("a failed ask lost the last paths or did not say why: %+v", b) } } + +// The review of #142: the backup seat's path is compared as the kernel resolves it. A repository the data +// list names through a symbolic link is still found under the share that really holds it. +func TestTheBackupPathIsComparedAfterItsSymlinks(t *testing.T) { + f := newFake(t) + f.mountinfo("/") + f.dir("/srv/pool/restic") + f.symlink("pool/restic", "/srv/backups") // /srv/backups -> /srv/pool/restic + f.file(ConfigFile, grantsConfig("pool=/srv/pool", "pool=ws:rw")) + f.roster("ws 10.0.0.3") + f.wants(map[string][]Want{"ws": {{Share: "pool", Access: "rw"}}}) + f.backup("restic", "/srv/backups") + s := f.machine().Pass(context.Background(), nil) + if why := refusedWhy(s, "pool"); !strings.Contains(why, "/srv/pool/restic") { + t.Errorf("a share holding the backup seat's repository, named through a symlink, is not refused: %q", why) + } +} + +// The review of #142: a list the tools could not refresh is said in the status and the health with its +// age, and a list not known yet is a warning in the health — neither is a plain "healthy". +func TestAStaleOrUnknownBackupListIsSaidWithItsAge(t *testing.T) { + f := newFake(t) + f.mountinfo("/") + 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() + now := time.Date(2026, 10, 8, 20, 0, 0, 0, time.UTC) + m.Now = func() time.Time { return now } + + m.Pass(context.Background(), nil) + h := m.Health() + if !strings.Contains(h.Why, "Warning") || !strings.Contains(h.Why, "backup seat") { + t.Errorf("an unknown backup list is not a warning in the health: %+v", h) + } + + ok := func(key string, body any) (json.RawMessage, error) { + if strings.Contains(key, "seats") { + return json.RawMessage(`{"answer":{"seats":[{"seat":"node-backup","holders":[{"module":"restic","node":"hs"}]}]}}`), nil + } + return json.RawMessage(`{"answer":{"data":[{"machine":"hs","module":"restic","path":"/srv/restic"}]}}`), nil + } + m.TakeBackupWith("hs", ok) + now = now.Add(2 * time.Hour) + m.TakeBackupWith("hs", func(string, any) (json.RawMessage, error) { return nil, errors.New("the bus did not answer") }) + s := m.Pass(context.Background(), nil) + said := strings.Join(append(append([]string{}, s.Said...), s.Warnings...), "\n") + if !strings.Contains(said, "2h0m0s") || !strings.Contains(said, "the bus did not answer") { + t.Errorf("the status does not say the backup list is 2 h old and why: %q", said) + } + h = m.Health() + if !strings.Contains(h.Why, "2h0m0s") { + t.Errorf("the health does not say the backup list's age: %+v", h) + } +} diff --git a/modules/nfs-server/cmd/nfs-server/machine.go b/modules/nfs-server/cmd/nfs-server/machine.go index 4d302924..fdd948bf 100644 --- a/modules/nfs-server/cmd/nfs-server/machine.go +++ b/modules/nfs-server/cmd/nfs-server/machine.go @@ -250,8 +250,8 @@ func (m *Machine) Glob(pattern string) []string { var written = map[string]bool{ExportFile: true, StatusFile: true, WantsFile: true, BackupFile: true} // Write replaces one of the module's own files, atomically. Every other path is refused: the module owns -// these three files (the export file and the status, written as root; the wants, written by the tools) -// and nothing else on the machine. +// exactly these four files and nothing else on the machine: the export file and the status, written by the +// root process; the wants and what was learnt about the backup seat, written by the tools from the bus. func (m *Machine) Write(path, content string) error { if !written[path] { return fmt.Errorf("the module writes only %s, %s and %s, not %s", ExportFile, StatusFile, WantsFile, path) diff --git a/modules/nfs-server/cmd/nfs-server/manifest_test.go b/modules/nfs-server/cmd/nfs-server/manifest_test.go index 548371d3..f9e12319 100644 --- a/modules/nfs-server/cmd/nfs-server/manifest_test.go +++ b/modules/nfs-server/cmd/nfs-server/manifest_test.go @@ -162,3 +162,17 @@ func TestTheManifestCarriesGrantsTheRosterTheWantsAndTheRemovalOrder(t *testing. t.Errorf("the process must be declared before its config, so unassignment removes the config first: %v", order) } } + +// Stopped asks whether the process's own unit is disabled; that unit is the manifest's process name. +func TestTheProcessUnitIsTheManifestsProcess(t *testing.T) { + m, _ := readManifest(t) + for _, r := range m.Resources { + if r["type"] == "process" && r["id"] == "exports" { + if got := r["name"].(string) + ".service"; got != ProcessUnit { + t.Errorf("the exports process runs as %s, and Stopped asks about %s", got, ProcessUnit) + } + return + } + } + t.Error("the manifest has no exports process") +} diff --git a/modules/nfs-server/cmd/nfs-server/process.go b/modules/nfs-server/cmd/nfs-server/process.go index 81e2439c..c6c6172e 100644 --- a/modules/nfs-server/cmd/nfs-server/process.go +++ b/modules/nfs-server/cmd/nfs-server/process.go @@ -4,7 +4,9 @@ import ( "context" "encoding/json" "fmt" + "net/netip" "os" + "path" "strings" "time" ) @@ -26,6 +28,9 @@ type Status struct { // Said is what does not match between the grants and the wants: a node wanting a share it is not // granted, a grant nobody wants, a share exported to no node. Said []string `json:"said,omitempty"` + // Warnings make the module no less healthy but are said in its health: what it could not learn, and + // how old what it relies on is. + Warnings []string `json:"warnings,omitempty"` // Reloaded is when the process last had the kernel read the export files again, and why. Reloaded *time.Time `json:"reloaded,omitempty"` ReloadWhy string `json:"reload_why,omitempty"` @@ -54,7 +59,7 @@ func (m *Machine) Pass(ctx context.Context, last *Status) Status { s.Said = append(s.Said, note) } if _, note := m.Backup(); note != "" { - s.Said = append(s.Said, note) + s.Warnings = append(s.Warnings, note) } m.setWalk(ctx) exports, refused, said := m.Judge(c, roster, wants) @@ -78,6 +83,14 @@ func (m *Machine) Pass(ctx context.Context, last *Status) Status { } } live, note := m.Live() + foreign, _, _ := m.Foreign() + mine := ParseExports(want, ExportFile) + if reload == "" && last == nil { + reload = "the process started, and the kernel's table is made to match the file" + } + if reload == "" && last != nil && last.ReloadError != "" { + reload = "the last reload failed: " + last.ReloadError + } if reload == "" { for _, e := range exports { if !SharedLive(live, e) { @@ -86,6 +99,11 @@ func (m *Machine) Pass(ctx context.Context, last *Status) Status { } } } + if reload == "" { + if stray := Stray(live, mine, foreign, c.Range); len(stray) > 0 { + reload = fmt.Sprintf("the kernel exports %s to %s, which the export file no longer carries", stray[0].Path, stray[0].Client) + } + } if reload != "" { if _, err := m.AsRootWithin(ctx, "exportfs", "-ra"); err != nil { s.ReloadError = err.Error() @@ -98,6 +116,10 @@ func (m *Machine) Pass(ctx context.Context, last *Status) Status { if note != "" && len(exports) > 0 { s.Problems = append(s.Problems, note) } + for _, e := range Stray(live, mine, foreign, c.Range) { + s.Problems = append(s.Problems, fmt.Sprintf("the kernel still exports %s to %s, which the module's export file "+ + "no longer carries: a reload has to take it away (exportfs -ra as root)", e.Path, e.Client)) + } for i := range exports { exports[i].Live = SharedLive(live, exports[i]) } @@ -109,6 +131,9 @@ func (m *Machine) Pass(ctx context.Context, last *Status) Status { return s } +// ProcessUnit is the service the node-engine runs this process as: the manifest's process name, `.service`. +const ProcessUnit = "nfs-server-exports.service" + // StopTimeout bounds Stopped and the commands it runs: well under the service manager's stop timeout (90 s, // then SIGKILL), so the export file is gone before the process could be killed (mesh-host #62). var StopTimeout = 10 * time.Second @@ -186,17 +211,15 @@ func SharedLive(live []Entry, e Exported) bool { return true } -// Stopped is the process ending. When the module was unassigned — the node-engine removes the module's -// files in reverse declaration order, so the config file, declared after this process, is already gone -// when the process is stopped (mesh-host internal/apply: orphans removed in reverse order) — the -// module's export file goes and the kernel lets the shares go. A plain restart, a push or the machine -// shutting down leaves the config file, and the exports stay. +// Stopped is the process ending. When the module was unassigned (see unassigned) the module's export file +// goes and the kernel lets the shares go. A plain restart, a push or the machine shutting down leaves the +// config file and the unit enabled, and the exports stay. func (m *Machine) Stopped() { - if _, err := m.Read(ConfigFile); err == nil || !os.IsNotExist(err) { - return - } ctx, cancel := context.WithTimeout(context.Background(), StopTimeout) defer cancel() + if !m.unassigned(ctx) { + return + } // The file first: removing it needs no command, so a hanging sudo or exportfs cannot keep it. if err := m.unexport(); err != nil { loud("the module was unassigned and its export file is still there: " + err.Error() + @@ -212,8 +235,56 @@ func (m *Machine) Stopped() { fmt.Fprintln(os.Stderr, "nfs-server: unassigned: the kernel let the shares go (exportfs -ra)") } +// unassigned says whether this stop is the module being taken away: its config file is gone (the node-engine +// removes resources in reverse of their record, and the config is recorded after this process), or this +// process's own unit is disabled. The node-engine stops a process it removes with `systemctl disable +// --now`, which disables the unit before it stops it, while a restart, a push or a shutdown leaves it +// enabled — so the second sign holds even when a failed first apply recorded the process after its config +// (mesh-host #62) and the config is still there. +func (m *Machine) unassigned(ctx context.Context) bool { + if _, err := m.Read(ConfigFile); os.IsNotExist(err) { + return true + } + r := m.Run(ctx, "systemctl", "is-enabled", ProcessUnit) + return strings.TrimSpace(r.Out) == "disabled" +} + // loud says, on the process's log where the service manager's journal keeps it, what an unassignment left // undone, so a person finds it rather than an export that outlived its module. func loud(what string) { fmt.Fprintln(os.Stderr, "nfs-server: UNASSIGNED, NOT CLEAN: "+what) } + +// Stray is what the kernel exports that is the module's and that its file no longer carries: an entry to a +// client inside the private network's range — the only clients the module ever exports to — that neither +// the module's file nor any other export file (/etc/exports, ZFS's own) has. What another file exports is +// not the module's, whatever its client. +func Stray(live, mine, foreign []Entry, rng string) []Entry { + prefix, err := netip.ParsePrefix(rng) + if err != nil { + return nil + } + has := map[string]bool{} + for _, e := range append(append([]Entry{}, mine...), foreign...) { + has[path.Clean(e.Path)+" "+e.Client] = true + } + var out []Entry + for _, e := range live { + if has[path.Clean(e.Path)+" "+e.Client] || !insideRange(e.Client, prefix) { + continue + } + out = append(out, e) + } + return out +} + +// insideRange says whether a client — one address, or a network — lies wholly inside a range. +func insideRange(client string, prefix netip.Prefix) bool { + if a, err := netip.ParseAddr(client); err == nil { + return prefix.Contains(a) + } + if p, err := netip.ParsePrefix(client); err == nil { + return p.Bits() >= prefix.Bits() && prefix.Contains(p.Addr()) + } + return false +} diff --git a/modules/nfs-server/cmd/nfs-server/reconcile_test.go b/modules/nfs-server/cmd/nfs-server/reconcile_test.go new file mode 100644 index 00000000..d6be1ada --- /dev/null +++ b/modules/nfs-server/cmd/nfs-server/reconcile_test.go @@ -0,0 +1,201 @@ +package main + +import ( + "context" + "strings" + "testing" + "time" +) + +func countExportfs(ran []string) int { + n := 0 + for _, r := range ran { + if strings.Contains(r, "exportfs -ra") { + n++ + } + } + return n +} + +// The review of #142: pass 1 exports /srv/pool; pass 2 no longer exports it and exportfs fails; pass 3 ran nothing, +// and the kernel still exported the refused folder while the module said it was healthy. A failed reload +// is tried again on the next pass, and while the kernel exports anything of the module's that the file no +// longer carries, the module is unhealthy and says what. +func TestARefusalWhoseReloadFailedIsReloadedAgainAndIsUnhealthyUntilItIs(t *testing.T) { + f := newFake(t) + f.mountinfo("/", "/srv/pool") + f.dir("/srv/pool") + f.file(ConfigFile, grantsConfig("pool=/srv/pool", "pool=ws:rw")) + f.roster("ws 10.0.0.3") + f.wants(map[string][]Want{"ws": {{Share: "pool", Access: "rw"}}}) + f.kernelReads() + m := f.machine() + s := m.Pass(context.Background(), nil) + if !strings.Contains(f.read(Etab), "/srv/pool") { + t.Fatalf("pass 1 did not export the pool: %+v", s) + } + + f.file(ConfigFile, grantsConfig("pool=/srv/pool", "none")) // pass 2: the grant is taken away + f.onExportfs = nil + f.answers["exportfs -ra"] = Ran{Code: 1, Err: "exportfs: stuck"} + f.answers["sudo -n exportfs -ra"] = Ran{Code: 1, Err: "exportfs: stuck"} + s = m.Pass(context.Background(), &s) + if s.ReloadError == "" { + t.Fatalf("pass 2's reload did not fail as arranged: %+v", s) + } + + f.ran = nil + s = m.Pass(context.Background(), &s) + if countExportfs(f.ran) == 0 { + t.Errorf("pass 3 did not try the failed reload again: %v", f.ran) + } + h := m.Health() + if h.Healthy || !strings.Contains(h.Why, "/srv/pool") { + t.Errorf("the kernel still exports the refused /srv/pool and the module says: %+v", h) + } + + delete(f.answers, "exportfs -ra") + delete(f.answers, "sudo -n exportfs -ra") + f.kernelReads() + s = m.Pass(context.Background(), &s) + if strings.Contains(f.read(Etab), "/srv/pool") { + t.Errorf("a reload that works again did not let the refused pool go: %q", f.read(Etab)) + } + for _, p := range s.Problems { + if strings.Contains(p, "/srv/pool") { + t.Errorf("a problem stays after the kernel let the pool go: %q", p) + } + } +} + +// A grant taken away while the kernel still holds it — exported before the process started, or a reload +// that never happened — is reconciled: the first pass always reloads, a later pass reloads when the kernel +// exports one of the module's folders to a client the file no longer has, and the module is unhealthy +// while it cannot. +func TestAWithdrawnGrantIsTakenFromTheKernel(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", "laptop 10.0.0.4") + f.wants(map[string][]Want{"ws": {{Share: "library", Access: "rw"}}, "laptop": {{Share: "library", Access: "ro"}}}) + f.kernelReads() + m := f.machine() + s := m.Pass(context.Background(), nil) + // The file and the kernel agree on ws only; the laptop's old export is left in the kernel's table, as + // a grant withdrawn while the process was down would be. + f.file(Etab, f.read(Etab)+"/srv/library\t10.0.0.4(ro,sync)\n") + + f.ran = nil + m2 := f.machine() // a restarted process: its first pass + m2.Pass(context.Background(), nil) + if countExportfs(f.ran) == 0 { + t.Errorf("the first pass of a process did not reload: %v", f.ran) + } + if strings.Contains(f.read(Etab), "10.0.0.4") { + t.Errorf("the withdrawn grant stays in the kernel after the first pass: %q", f.read(Etab)) + } + + f.file(Etab, f.read(Etab)+"/srv/library\t10.0.0.4(ro,sync)\n") + f.ran = nil + s = m.Pass(context.Background(), &s) + if countExportfs(f.ran) == 0 { + t.Errorf("a later pass did not reload while the kernel exported a folder of the module's to a client the file no longer has: %v", f.ran) + } + if strings.Contains(f.read(Etab), "10.0.0.4") { + t.Errorf("the withdrawn grant stays in the kernel: %q", f.read(Etab)) + } + + // A client the module never exported to — the home network's range, from ZFS's own file — is not + // the module's, and makes no reload and no finding. + f.file("/etc/exports.d/zfs.exports", "/srv/library 192.0.2.0/24(rw)\n") + f.file(Etab, f.read(Etab)+"/srv/library\t192.0.2.0/24(rw)\n") + f.ran = nil + s = m.Pass(context.Background(), &s) + if n := countExportfs(f.ran); n != 0 { + t.Errorf("an export that is not the module's made a reload: %v", f.ran) + } + if h := m.Health(); !h.Healthy { + t.Errorf("an export that is not the module's made it unhealthy: %+v", h) + } +} + +// The review of #142: SIGTERM in the middle of a pass on a plain restart (the config is still there) ends +// the pass, and the exports stay: the export file is unchanged and the kernel's table keeps the share. +func TestSIGTERMDuringAPassOnARestartLeavesTheExports(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() + m.Pass(context.Background(), nil) + file, table := f.read(ExportFile), f.read(Etab) + if !strings.Contains(table, "/srv/library") { + t.Fatalf("the library was not exported: %q", table) + } + + inner := m.Run + hung := make(chan struct{}, 8) + m.Run = func(ctx context.Context, name string, args ...string) Ran { + if strings.Contains(strings.Join(append([]string{name}, args...), " "), "exportfs") { + hung <- struct{}{} + <-ctx.Done() + return Ran{Code: -1, Err: ctx.Err().Error()} + } + return inner(ctx, name, args...) + } + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan error, 1) + go func() { done <- m.RunExports(ctx) }() // its first pass reloads, and hangs there + select { + case <-hung: + case <-time.After(5 * time.Second): + t.Fatal("the pass never reached exportfs") + } + cancel() + select { + case <-done: + case <-time.After(3 * time.Second): + t.Fatal("the process did not stop after SIGTERM") + } + if got := f.read(ExportFile); got != file { + t.Errorf("a restart changed the export file:\nwas %q\nnow %q", file, got) + } + if got := f.read(Etab); got != table { + t.Errorf("a restart changed what the kernel exports:\nwas %q\nnow %q", table, got) + } +} + +// mesh-host #62's caveat: when the process's first apply failed, it can be recorded after its config, and +// unassignment then stops it with the config still there. The node-engine disables a process's unit before +// it stops it (`systemctl disable --now`), and a restart or a shutdown leaves it enabled, so a process +// whose own unit is disabled when it is stopped is being taken away, whatever the config says. +func TestAProcessWhoseUnitIsDisabledAtStopCleansUpWithTheConfigStillThere(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() + m.Pass(context.Background(), nil) + + f.answers["systemctl is-enabled "+ProcessUnit] = Ran{Out: "enabled\n"} + m.Stopped() + if !strings.Contains(f.read(ExportFile), "/srv/library") { + t.Fatalf("a stop with the unit enabled (a restart) removed the exports") + } + + f.answers["systemctl is-enabled "+ProcessUnit] = Ran{Code: 1, Out: "disabled\n"} + m.Stopped() + if strings.Contains(f.read(ExportFile), "/srv/library") { + t.Errorf("a stop with the unit disabled (unassigned, config still there) left the export file") + } + if strings.Contains(f.read(Etab), "/srv/library") { + t.Errorf("the kernel still exports the library after an unassignment with the config in place: %q", f.read(Etab)) + } +} diff --git a/modules/nfs-server/cmd/nfs-server/shares.go b/modules/nfs-server/cmd/nfs-server/shares.go index cf5b2d27..073ee0cb 100644 --- a/modules/nfs-server/cmd/nfs-server/shares.go +++ b/modules/nfs-server/cmd/nfs-server/shares.go @@ -343,6 +343,7 @@ func (m *Machine) Judge(c Config, roster map[string]string, wants map[string][]W } points := m.MountPoints() backup, _ := m.Backup() // judged on every pass, never kept: see backup.go + backup.Paths = m.realPaths(backup.Paths) shares := map[string]bool{} for _, s := range c.Shares { shares[s.Name] = true diff --git a/modules/nfs-server/cmd/nfs-server/tools.go b/modules/nfs-server/cmd/nfs-server/tools.go index 114fce40..596bc2f8 100644 --- a/modules/nfs-server/cmd/nfs-server/tools.go +++ b/modules/nfs-server/cmd/nfs-server/tools.go @@ -126,6 +126,7 @@ func (m *Machine) Exports() ExportsAnswer { a.Shares = append(a.Shares, e) } a.Notes = append(a.Notes, st.Problems...) + a.Notes = append(a.Notes, st.Warnings...) a.Unmatched = st.Said } entries, sets, notes := m.Foreign() @@ -437,8 +438,12 @@ func (m *Machine) Health() Health { } } sort.Strings(found) + warned := "" + for _, w := range st.Warnings { + warned += " Warning: " + sentence(w) + } if len(found) > 0 { - return Health{Healthy: false, Why: strings.Join(found, " "), Found: found} + return Health{Healthy: false, Why: strings.Join(found, " ") + warned, Found: found} } var parts []string for _, e := range st.Shares { @@ -447,9 +452,9 @@ func (m *Machine) Health() Health { } } if len(parts) == 0 { - return Health{Healthy: true, Why: "The NFS server is active, and the mesh exports nothing to any node here."} + return Health{Healthy: true, Why: "The NFS server is active, and the mesh exports nothing to any node here." + warned} } - return Health{Healthy: true, Why: fmt.Sprintf("The NFS server is active and exports %s.", strings.Join(parts, "; "))} + return Health{Healthy: true, Why: fmt.Sprintf("The NFS server is active and exports %s.", strings.Join(parts, "; ")) + warned} } func sentence(s string) string {