From 0f330fc57d6bb23ed26b71b2c9df24acc258eff6 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 14:26:01 +0200 Subject: [PATCH 1/3] mounts: clear a failed mount of its own whose source was away, and the failure of a unit it takes over, so neither is said as the machine's own failed unit (hq ADR 0263 rules 4 and 6) --- modules/mounts/README.md | 8 ++ modules/mounts/cmd/mounts/apply.go | 90 ++++++++++++++++++-- modules/mounts/cmd/mounts/failed_test.go | 101 +++++++++++++++++++++++ modules/mounts/cmd/mounts/write_test.go | 5 ++ 4 files changed, 199 insertions(+), 5 deletions(-) create mode 100644 modules/mounts/cmd/mounts/failed_test.go diff --git a/modules/mounts/README.md b/modules/mounts/README.md index 0e1a1911..92cdc4bb 100644 --- a/modules/mounts/README.md +++ b/modules/mounts/README.md @@ -42,6 +42,14 @@ an autofs mount, so nothing writes into the empty directory under it. | an SMB source | `///`, `Type=cifs` | `credentials=,iocharset=utf8,_netdev,nofail,uid=…,gid=…` | 10 | | a disk | `/dev/disk/by-uuid/`; the mount `BindsTo=` the disk's device | `nofail` | 10 | +A mount of the module's own that failed because its source was not there — an occasional source that is +off, or a node's share whose server is away, tried when someone opened the mount point — is cleared +(`systemctl reset-failed`) by `apply` within five seconds, and so is the failure a unit of the same name +carried before the module wrote it (the unit fstab's generator makes until the line is adopted). The module +says such a share as `absent` or `unreachable`; left failed, the unit would also be said as a failed unit of +the machine's own, with a notification each time it is touched. A mount that failed while its source answers +is left failed: that is a fault, and the share is said `failed`. + A share is read-only when the setting says `:ro` or the sharing machine exports it read-only. A read-write share is mounted hard, so a write is never dropped while the server is away. diff --git a/modules/mounts/cmd/mounts/apply.go b/modules/mounts/cmd/mounts/apply.go index c2df2630..19910c01 100644 --- a/modules/mounts/cmd/mounts/apply.go +++ b/modules/mounts/cmd/mounts/apply.go @@ -33,6 +33,11 @@ import ( // LookEvery is how often the process looks. const LookEvery = 30 * time.Second +// ClearEvery is how often the acting process clears a failed mount of its own whose source does not +// answer (clearFailed). Shorter than the node-engine's two looks of 15 s, so a mount that failed because +// its source is away is never read as a failed unit of the machine's own. +const ClearEvery = 5 * time.Second + // ProbeTimeout bounds one try of a source. const ProbeTimeout = 2 * time.Second @@ -89,6 +94,9 @@ type Applier struct { Dial func(addr string, timeout time.Duration) error Now func() time.Time Lookup func(account string) (int, int, error) + // written is every pair the last pass wrote and armed, by its mount unit: what clearFailed looks at + // between passes. The acting process's own memory; nothing else reads it. + written map[string]Pair } // NewApplier is the process on this machine. @@ -326,6 +334,7 @@ func (a *Applier) Look(ctx context.Context) Status { own := a.ownUnits() mounted := ParseMountinfo(func() string { t, _ := a.M.read("/proc/self/mountinfo"); return t }()) changed := false + fresh := map[string]bool{} // the mount units this pass wrote anew var written []Pair for _, p := range pairs { ours := own[p.MountUnit] && own[p.AutoUnit] @@ -364,6 +373,7 @@ func (a *Applier) Look(ctx context.Context) Status { } if c1 || c2 { changed = true + fresh[p.MountUnit] = true st.Changed = append(st.Changed, "the units of "+p.Name) } // A missing mount point is made; one that is there is left exactly as it is. An automount point @@ -448,6 +458,23 @@ func (a *Applier) Look(ctx context.Context) Status { } } } + if a.Act { + // A mount unit this pass wrote anew over one that had failed — the unit fstab's generator made for + // the same mount point, before the line was adopted — carries a failure that was not the module's + // pair's. It is cleared, so the pair starts armed and is judged on its own mounts (ADR 0263 rule 4). + for _, p := range written { + if fresh[p.MountUnit] && states[p.MountUnit]["ActiveState"] == "failed" { + if _, err := a.M.Run(ctx, "systemctl", "reset-failed", p.MountUnit); err == nil { + st.Changed = append(st.Changed, "cleared "+p.MountUnit+", which had failed before the module wrote it") + } + } + } + a.written = map[string]Pair{} + for _, p := range written { + a.written[p.MountUnit] = p + } + st.Changed = append(st.Changed, a.clearFailed(ctx)...) + } if !a.Judge { a.finish(&st, notSet) return st @@ -569,17 +596,70 @@ func (a *Applier) writeStatus(st *Status) { } } -// RunApply looks every LookEvery until the context ends, saying on stderr what it changed. +// clearFailed clears every failed mount of the module's own pairs whose source does not answer now: an +// occasional source that is away, or a node's share whose server is. Such a mount failed because its +// source was not there, which the module already says as the share's state (absent, or unreachable, and +// for a share that should always be there its health after five minutes, ADR 0263 rule 8); left failed, +// the unit would be said a second time as a failed unit of the machine's own, with a notification each +// time it is touched — what rules 4 and 6 promise never happens. A mount that failed while its source +// answers is left failed: that is a fault, and the share is said failed until a person looks. +func (a *Applier) clearFailed(ctx context.Context) []string { + if len(a.written) == 0 { + return nil + } + var units []string + for unit := range a.written { + units = append(units, unit) + } + sort.Strings(units) + states := a.unitStates(ctx, units) + var cleared []string + for _, unit := range units { + if states[unit]["ActiveState"] != "failed" { + continue + } + p := a.written[unit] + if a.sourceAnswers(p) { + continue + } + if _, err := a.M.Run(ctx, "systemctl", "reset-failed", unit); err == nil { + cleared = append(cleared, "cleared "+unit+": "+p.Name+" was not there to mount") + } + } + return cleared +} + +// sourceAnswers tries a pair's source the way the judging process does, without touching its mount point. +func (a *Applier) sourceAnswers(p Pair) bool { + if p.Kind == "disk" { + return a.M.present(p.Probe) + } + return a.Dial != nil && a.Dial(p.Probe, ProbeTimeout) == nil +} + +// RunApply looks every LookEvery until the context ends, saying on stderr what it changed. Between looks +// the acting process clears, every ClearEvery, a mount of its own that failed because its source was away. func RunApply(ctx context.Context, a *Applier) { for { st := a.Look(ctx) for _, c := range st.Changed { fmt.Fprintln(os.Stderr, "mounts:", c) } - select { - case <-ctx.Done(): - return - case <-time.After(LookEvery): + next := time.After(LookEvery) + between: + for { + select { + case <-ctx.Done(): + return + case <-next: + break between + case <-time.After(ClearEvery): + if a.Act { + for _, c := range a.clearFailed(ctx) { + fmt.Fprintln(os.Stderr, "mounts:", c) + } + } + } } } } diff --git a/modules/mounts/cmd/mounts/failed_test.go b/modules/mounts/cmd/mounts/failed_test.go new file mode 100644 index 00000000..2762d5c3 --- /dev/null +++ b/modules/mounts/cmd/mounts/failed_test.go @@ -0,0 +1,101 @@ +package main + +import ( + "context" + "strings" + "testing" + "time" +) + +// A mount the module took over, or one whose source was away when it was touched, is never left as a +// failed unit: the node-engine would say it as a failed unit of the machine's own, with a notification each +// time (hq ADR 0263 rules 4 and 6). A mount that failed while its source answers stays failed: a fault. + +func failedLook(t *testing.T, f *fakeSystem) map[string]ShareStatus { + t.Helper() + st := f.applier(time.Date(2026, 10, 9, 14, 0, 0, 0, time.UTC)).Look(context.Background()) + out := map[string]ShareStatus{} + for _, s := range st.Shares { + out[s.Name] = s + } + return out +} + +func TestTheFailureOfTheUnitTheModuleTakesOverIsCleared(t *testing.T) { + f := newSystem(t) + f.write(ConfigFile, "shares=library=/srv/library\nserver=homeserver\nat=192.0.2.10\nnode=workstation\n") + f.write(ExportsFile, exportsAnswer(t, library)) + f.reachable["192.0.2.10:2049"] = true + // fstab's generator made srv-library.mount for the same mount point, and it timed out at boot. + f.units["srv-library.mount"] = map[string]string{"ActiveState": "failed", "Result": "timeout"} + + states := failedLook(t, f) + + if !f.called("systemctl reset-failed srv-library.mount") { + t.Fatalf("the generator's failure is cleared once the module's unit is written: %q", f.calls) + } + if s := states["library"]; s.State != StateArmed { + t.Errorf("the share is judged on its own mounts, not on the generator's: %+v", s) + } +} + +func TestAMountThatFailedBecauseItsSourceWasAwayIsCleared(t *testing.T) { + f := newSystem(t) + f.write(ConfigFile, "shares=library=/srv/library\nsources=games=smb://192.0.2.76/Share@/mnt/games\n"+ + "server=homeserver\nat=192.0.2.10\nnode=workstation\naccount=operator\n"+ + "secret=/var/lib/mesh/mounts/smb-credentials.secret\nstate=/var/lib/mesh/mounts\n") + f.write(ExportsFile, exportsAnswer(t, library)) + f.write("/var/lib/mesh/mounts/smb-credentials.secret", "[games]\nusername=root\npassword="+testPassword+"\n") + a := f.applier(time.Date(2026, 10, 9, 14, 0, 0, 0, time.UTC)) + a.Look(context.Background()) // writes and arms both pairs; neither source answers + f.calls = nil + + // Someone opened both mount points: the gaming device is off and the server is away, so both mounts failed. + f.units["mnt-games.mount"] = map[string]string{"ActiveState": "failed", "Result": "timeout"} + f.units["srv-library.mount"] = map[string]string{"ActiveState": "failed", "Result": "timeout"} + cleared := a.clearFailed(context.Background()) + + for _, unit := range []string{"mnt-games.mount", "srv-library.mount"} { + if !f.called("systemctl reset-failed " + unit) { + t.Errorf("%s failed because its source was away and is cleared: %q", unit, f.calls) + } + } + if len(cleared) != 2 || !strings.Contains(strings.Join(cleared, "; "), "games was not there to mount") { + t.Errorf("each clearing is said: %q", cleared) + } + states := failedLook(t, f) + if states["games"].State != StateAbsent || states["library"].State != StateUnreachable { + t.Errorf("the module still says each as absent or unreachable: %+v", states) + } +} + +func TestAMountThatFailedWhileItsSourceAnswersStaysFailed(t *testing.T) { + f := newSystem(t) + f.write(ConfigFile, "shares=library=/srv/library\nserver=homeserver\nat=192.0.2.10\nnode=workstation\n") + f.write(ExportsFile, exportsAnswer(t, library)) + f.reachable["192.0.2.10:2049"] = true + a := f.applier(time.Date(2026, 10, 9, 14, 0, 0, 0, time.UTC)) + a.Look(context.Background()) + f.calls = nil + + f.units["srv-library.mount"] = map[string]string{"ActiveState": "failed", "Result": "exit-code"} + if cleared := a.clearFailed(context.Background()); len(cleared) != 0 || f.called("systemctl reset-failed") { + t.Fatalf("a mount refused by a server that answers is a fault and is left failed: %q %q", cleared, f.calls) + } + if s := failedLook(t, f)["library"]; s.State != StateFailed { + t.Errorf("it is said failed: %+v", s) + } +} + +func TestOnlyTheActingProcessClears(t *testing.T) { + f := newSystem(t) + f.write(ConfigFile, "shares=library=/srv/library\nserver=homeserver\nat=192.0.2.10\nnode=workstation\n") + f.write(ExportsFile, exportsAnswer(t, library)) + f.units["srv-library.mount"] = map[string]string{"ActiveState": "failed", "Result": "timeout"} + a := f.applier(time.Now()) + a.Act = false + a.Look(context.Background()) + if f.called("systemctl reset-failed") { + t.Errorf("the judging process changes nothing: %q", f.calls) + } +} diff --git a/modules/mounts/cmd/mounts/write_test.go b/modules/mounts/cmd/mounts/write_test.go index bf06d3d4..7d6768c1 100644 --- a/modules/mounts/cmd/mounts/write_test.go +++ b/modules/mounts/cmd/mounts/write_test.go @@ -77,6 +77,11 @@ func (f *fakeSystem) run(_ context.Context, name string, args ...string) (string case "enable": u := args[len(args)-1] f.units[u] = map[string]string{"ActiveState": "active", "UnitFileState": "enabled"} + case "reset-failed": + u := args[len(args)-1] + if f.units[u]["ActiveState"] == "failed" { + f.units[u]["ActiveState"], f.units[u]["Result"] = "inactive", "success" + } } return "", nil } From 02eab5aa8e76747f0c42ec109e721b8e35cbb38c Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 14:43:16 +0200 Subject: [PATCH 2/3] mounts: test the clearing loop, the look's own clearing and a disk's presence; clear only a taken-over unit's earlier failure; clear and re-arm an automount that gave up; refuse a transient unit of the same name (review of #162) --- modules/mounts/README.md | 7 +- modules/mounts/cmd/mounts/apply.go | 71 +++++++++--- modules/mounts/cmd/mounts/failed_test.go | 137 +++++++++++++++++++++++ modules/mounts/cmd/mounts/write_test.go | 22 ++++ 4 files changed, 219 insertions(+), 18 deletions(-) diff --git a/modules/mounts/README.md b/modules/mounts/README.md index 92cdc4bb..b9c3cc9f 100644 --- a/modules/mounts/README.md +++ b/modules/mounts/README.md @@ -48,7 +48,10 @@ off, or a node's share whose server is away, tried when someone opened the mount carried before the module wrote it (the unit fstab's generator makes until the line is adopted). The module says such a share as `absent` or `unreachable`; left failed, the unit would also be said as a failed unit of the machine's own, with a notification each time it is touched. A mount that failed while its source answers -is left failed: that is a fault, and the share is said `failed`. +is left failed: that is a fault, and the share is said `failed`. An automount that gave up after repeated +failed mounts (`mount-start-limit-hit`) while its source was away is cleared and armed again the same way. +Only a unit the module takes over has its earlier failure cleared regardless of the source; a pair of its +own that it rewrites keeps its failure. A share is read-only when the setting says `:ro` or the sharing machine exports it read-only. A read-write share is mounted hard, so a write is never dropped while the server is away. @@ -110,6 +113,8 @@ Each refusal is said as the share's state `failed`, with why, and nothing is wri - a mount point the system owns (`/`, `/etc`, `/usr`, `/boot`, `/proc`, `/sys`, `/dev`, `/run` and what is under them, and `/var`, `/home`, `/root`, `/tmp` themselves); - a mount point that something else is mounted at, or an automount that is not the module's; +- a mount point whose unit someone made at run time (`/run/systemd/transient`, from `systemd-mount` or the + like): that is somebody's mount now; - a folder that holds files: a mount would hide them. Only a folder not in the kernel's mount table is opened, so a server that is away cannot hold the look; - a share that this machine serves itself. diff --git a/modules/mounts/cmd/mounts/apply.go b/modules/mounts/cmd/mounts/apply.go index 19910c01..3376f26a 100644 --- a/modules/mounts/cmd/mounts/apply.go +++ b/modules/mounts/cmd/mounts/apply.go @@ -97,6 +97,8 @@ type Applier struct { // written is every pair the last pass wrote and armed, by its mount unit: what clearFailed looks at // between passes. The acting process's own memory; nothing else reads it. written map[string]Pair + // LookInterval and ClearInterval are LookEvery and ClearEvery, or a test's own when set. + LookInterval, ClearInterval time.Duration } // NewApplier is the process on this machine. @@ -334,7 +336,9 @@ func (a *Applier) Look(ctx context.Context) Status { own := a.ownUnits() mounted := ParseMountinfo(func() string { t, _ := a.M.read("/proc/self/mountinfo"); return t }()) changed := false - fresh := map[string]bool{} // the mount units this pass wrote anew + // The mount units this pass wrote where no unit file of the module's was before: a unit of the same + // name that was there — the one fstab's generator makes until the line is adopted — is taken over. + takenOver := map[string]bool{} var written []Pair for _, p := range pairs { ours := own[p.MountUnit] && own[p.AutoUnit] @@ -371,9 +375,11 @@ func (a *Applier) Look(ctx context.Context) Status { State: StateFailed, Why: fmt.Sprintf("its units could not be written: %v %v", err1, err2)} continue } + if c1 && !own[p.MountUnit] { + takenOver[p.MountUnit] = true + } if c1 || c2 { changed = true - fresh[p.MountUnit] = true st.Changed = append(st.Changed, "the units of "+p.Name) } // A missing mount point is made; one that is there is left exactly as it is. An automount point @@ -459,11 +465,12 @@ func (a *Applier) Look(ctx context.Context) Status { } } if a.Act { - // A mount unit this pass wrote anew over one that had failed — the unit fstab's generator made for - // the same mount point, before the line was adopted — carries a failure that was not the module's - // pair's. It is cleared, so the pair starts armed and is judged on its own mounts (ADR 0263 rule 4). + // A mount unit the module wrote for the first time, over a unit of the same name that had already + // failed — the one fstab's generator makes until the line is adopted — carries a failure that is not + // the module's pair's. It is cleared, so the pair starts armed and is judged on its own mounts (ADR + // 0263 rule 4). A pair the module already had and rewrote keeps its failure: that one is its own. for _, p := range written { - if fresh[p.MountUnit] && states[p.MountUnit]["ActiveState"] == "failed" { + if takenOver[p.MountUnit] && states[p.MountUnit]["ActiveState"] == "failed" { if _, err := a.M.Run(ctx, "systemctl", "reset-failed", p.MountUnit); err == nil { st.Changed = append(st.Changed, "cleared "+p.MountUnit+", which had failed before the module wrote it") } @@ -549,6 +556,12 @@ func (a *Applier) finish(st *Status, notSet map[string]ShareStatus) { // foreign refuses a pair whose unit file exists and is not the module's. func (a *Applier) foreign(p Pair, own map[string]bool) error { for _, unit := range []string{p.MountUnit, p.AutoUnit} { + // A transient unit of the same name — made by `systemd-mount` or `mount` on the command line — is + // someone's mount now, whatever the module's own file says. + transient := filepath.Join(TransientDir, unit) + if _, err := os.Lstat(a.path(transient)); err == nil { + return fmt.Errorf("%s has a transient unit that is not the module's (%s); it is left as it is, and this share is not mounted here", p.Mountpoint, transient) + } if own[unit] { continue } @@ -559,6 +572,9 @@ func (a *Applier) foreign(p Pair, own map[string]bool) error { return nil } +// TransientDir is where the service manager keeps the units made at run time rather than from a file. +const TransientDir = "/run/systemd/transient" + // judge is one pair's state. The source is tried first, without touching the mount point. func (a *Applier) judge(p Pair, states map[string]map[string]string, mounted map[string]Mounted) (string, string) { there := true @@ -607,23 +623,37 @@ func (a *Applier) clearFailed(ctx context.Context) []string { if len(a.written) == 0 { return nil } - var units []string - for unit := range a.written { - units = append(units, unit) + var mounts, units []string + for unit, p := range a.written { + mounts = append(mounts, unit) + units = append(units, unit, p.AutoUnit) } - sort.Strings(units) + sort.Strings(mounts) states := a.unitStates(ctx, units) var cleared []string - for _, unit := range units { - if states[unit]["ActiveState"] != "failed" { + for _, unit := range mounts { + p := a.written[unit] + mountFailed := states[unit]["ActiveState"] == "failed" + // After repeated failed mounts the automount gives up too (mount-start-limit-hit) and stops + // answering its point: cleared and armed again, or the share stays dead until the next pass. + autoFailed := states[p.AutoUnit]["ActiveState"] == "failed" + if !mountFailed && !autoFailed { continue } - p := a.written[unit] if a.sourceAnswers(p) { continue } - if _, err := a.M.Run(ctx, "systemctl", "reset-failed", unit); err == nil { - cleared = append(cleared, "cleared "+unit+": "+p.Name+" was not there to mount") + if mountFailed { + if _, err := a.M.Run(ctx, "systemctl", "reset-failed", unit); err == nil { + cleared = append(cleared, "cleared "+unit+": "+p.Name+" was not there to mount") + } + } + if autoFailed { + if _, err := a.M.Run(ctx, "systemctl", "reset-failed", p.AutoUnit); err == nil { + if _, err := a.M.Run(ctx, "systemctl", "start", p.AutoUnit); err == nil { + cleared = append(cleared, "cleared and armed "+p.AutoUnit+" again ("+orDefault(states[p.AutoUnit]["Result"], "failed")+"): "+p.Name+" was not there to mount") + } + } } } return cleared @@ -645,7 +675,7 @@ func RunApply(ctx context.Context, a *Applier) { for _, c := range st.Changed { fmt.Fprintln(os.Stderr, "mounts:", c) } - next := time.After(LookEvery) + next := time.After(orInterval(a.LookInterval, LookEvery)) between: for { select { @@ -653,7 +683,7 @@ func RunApply(ctx context.Context, a *Applier) { return case <-next: break between - case <-time.After(ClearEvery): + case <-time.After(orInterval(a.ClearInterval, ClearEvery)): if a.Act { for _, c := range a.clearFailed(ctx) { fmt.Fprintln(os.Stderr, "mounts:", c) @@ -663,3 +693,10 @@ func RunApply(ctx context.Context, a *Applier) { } } } + +func orInterval(d, otherwise time.Duration) time.Duration { + if d > 0 { + return d + } + return otherwise +} diff --git a/modules/mounts/cmd/mounts/failed_test.go b/modules/mounts/cmd/mounts/failed_test.go index 2762d5c3..5f8c17f3 100644 --- a/modules/mounts/cmd/mounts/failed_test.go +++ b/modules/mounts/cmd/mounts/failed_test.go @@ -99,3 +99,140 @@ func TestOnlyTheActingProcessClears(t *testing.T) { t.Errorf("the judging process changes nothing: %q", f.calls) } } + + +// waitFor polls until cond holds or the deadline passes. +func waitFor(t *testing.T, within time.Duration, cond func() bool) bool { + t.Helper() + deadline := time.Now().Add(within) + for time.Now().Before(deadline) { + if cond() { + return true + } + time.Sleep(5 * time.Millisecond) + } + return cond() +} + +func TestTheActingProcessClearsBetweenItsLooks(t *testing.T) { + f := newSystem(t) + f.write(ConfigFile, libraryOnly) + f.write(ExportsFile, exportsAnswer(t, library)) + a := f.applier(time.Date(2026, 10, 9, 14, 0, 0, 0, time.UTC)) + a.Judge = false + // The next look is an hour away: only the loop between looks can clear what fails now. + a.LookInterval, a.ClearInterval = time.Hour, 10*time.Millisecond + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { RunApply(ctx, a); close(done) }() + defer func() { cancel(); <-done }() + + if !waitFor(t, 2*time.Second, func() bool { return f.called("systemctl enable --now srv-library.automount") }) { + t.Fatal("the first look did not arm the pair") + } + // The server is away and someone opened the mount point: the mount failed after the look. + f.setUnit("srv-library.mount", map[string]string{"ActiveState": "failed", "Result": "timeout"}) + if !waitFor(t, 2*time.Second, func() bool { return f.called("systemctl reset-failed srv-library.mount") }) { + t.Fatal("a mount that failed between looks, its server away, is cleared before the next look") + } +} + +func TestALookClearsAMountOfItsOwnThatFailedWhileItsSourceWasAway(t *testing.T) { + f := newSystem(t) + f.write(ConfigFile, libraryOnly) + f.write(ExportsFile, exportsAnswer(t, library)) + a := f.applier(time.Date(2026, 10, 9, 14, 0, 0, 0, time.UTC)) + a.Look(context.Background()) + f.setUnit("srv-library.mount", map[string]string{"ActiveState": "failed", "Result": "timeout"}) + f.calls = nil + + // A second look, with nothing to write: the pair is the module's own already, so this is no takeover. + a.Look(context.Background()) + if !f.called("systemctl reset-failed srv-library.mount") { + t.Errorf("the look itself clears it: %q", f.calls) + } +} + +func TestAnAbsentDiskIsClearedAndAPresentOneIsNot(t *testing.T) { + f := newSystem(t) + f.write(ConfigFile, "sources=usb=disk:1234-ABCD@/mnt/usb\nnode=workstation\n") + a := f.applier(time.Date(2026, 10, 9, 14, 0, 0, 0, time.UTC)) + a.Look(context.Background()) + + f.setUnit("mnt-usb.mount", map[string]string{"ActiveState": "failed", "Result": "timeout"}) + f.calls = nil + if a.clearFailed(context.Background()); !f.called("systemctl reset-failed mnt-usb.mount") { + t.Errorf("the disk is not plugged in: its failed mount is cleared: %q", f.calls) + } + + // Plugged in, and the mount still failed: a fault, left as it is. + f.write("/dev/disk/by-uuid/1234-ABCD", "") + f.setUnit("mnt-usb.mount", map[string]string{"ActiveState": "failed", "Result": "exit-code"}) + f.calls = nil + if a.clearFailed(context.Background()); f.called("systemctl reset-failed") { + t.Errorf("a disk that is there and failed to mount is left failed: %q", f.calls) + } +} + +func TestARewrittenPairOfItsOwnKeepsItsFailure(t *testing.T) { + f := newSystem(t) + f.write(ConfigFile, libraryOnly) + f.write(ExportsFile, exportsAnswer(t, library)) + f.reachable["192.0.2.10:2049"] = true + a := f.applier(time.Date(2026, 10, 9, 14, 0, 0, 0, time.UTC)) + a.Look(context.Background()) + + // The server moved the library and refuses the mount while it answers: the module's own failure. + f.setUnit("srv-library.mount", map[string]string{"ActiveState": "failed", "Result": "exit-code"}) + moved := library + moved.Path = "/tank/library" + f.write(ExportsFile, exportsAnswer(t, moved)) + f.calls = nil + a.Look(context.Background()) + if !strings.Contains(f.read(UnitDir+"/srv-library.mount"), "What=192.0.2.10:/tank/library") { + t.Fatal("the pair was not rewritten") + } + if f.called("systemctl reset-failed") { + t.Errorf("only a unit the module takes over has its failure cleared, not one of its own it rewrites: %q", f.calls) + } +} + +func TestAnAutomountThatGaveUpIsClearedAndArmedAgain(t *testing.T) { + f := newSystem(t) + f.write(ConfigFile, libraryOnly) + f.write(ExportsFile, exportsAnswer(t, library)) + a := f.applier(time.Date(2026, 10, 9, 14, 0, 0, 0, time.UTC)) + a.Look(context.Background()) + + f.setUnit("srv-library.automount", map[string]string{"ActiveState": "failed", "Result": "mount-start-limit-hit", "UnitFileState": "enabled"}) + f.calls = nil + cleared := a.clearFailed(context.Background()) + if !f.called("systemctl reset-failed srv-library.automount") || !f.called("systemctl start srv-library.automount") { + t.Fatalf("the automount is cleared and started: %q", f.calls) + } + if !strings.Contains(strings.Join(cleared, ";"), "mount-start-limit-hit") { + t.Errorf("said with its result: %q", cleared) + } + + // With the server answering, an automount that gave up is a fault: left as it is. + f.reachable["192.0.2.10:2049"] = true + f.setUnit("srv-library.automount", map[string]string{"ActiveState": "failed", "Result": "mount-start-limit-hit", "UnitFileState": "enabled"}) + f.calls = nil + if a.clearFailed(context.Background()); f.called("systemctl reset-failed") || f.called("systemctl start") { + t.Errorf("left failed while the server answers: %q", f.calls) + } +} + +func TestATransientUnitOfTheSameNameIsNotTheModules(t *testing.T) { + f := newSystem(t) + f.write(ConfigFile, libraryOnly) + f.write(ExportsFile, exportsAnswer(t, library)) + f.write(TransientDir+"/srv-library.mount", "[Mount]\nWhat=elsewhere\n") + states := failedLook(t, f) + if s := states["library"]; s.State != StateFailed || !strings.Contains(s.Why, "transient") { + t.Errorf("a share whose unit someone made at run time is refused: %+v", s) + } + if f.called("systemctl enable --now srv-library.automount") { + t.Error("and not armed") + } +} diff --git a/modules/mounts/cmd/mounts/write_test.go b/modules/mounts/cmd/mounts/write_test.go index 7d6768c1..1446ff6f 100644 --- a/modules/mounts/cmd/mounts/write_test.go +++ b/modules/mounts/cmd/mounts/write_test.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" "time" ) @@ -16,6 +17,8 @@ const testPassword = "s3cr3t-Pa55" // fakeSystem is a machine under a temporary root with a recorded service manager. type fakeSystem struct { + // mu guards calls, units and reachable, for a test that runs RunApply beside itself. + mu sync.Mutex t *testing.T root string calls []string @@ -55,6 +58,8 @@ func (f *fakeSystem) read(p string) string { } func (f *fakeSystem) run(_ context.Context, name string, args ...string) (string, error) { + f.mu.Lock() + defer f.mu.Unlock() f.calls = append(f.calls, name+" "+strings.Join(args, " ")) if name != "systemctl" || len(args) == 0 { return "", nil @@ -77,6 +82,12 @@ func (f *fakeSystem) run(_ context.Context, name string, args ...string) (string case "enable": u := args[len(args)-1] f.units[u] = map[string]string{"ActiveState": "active", "UnitFileState": "enabled"} + case "start": + u := args[len(args)-1] + if f.units[u] == nil { + f.units[u] = map[string]string{} + } + f.units[u]["ActiveState"], f.units[u]["Result"] = "active", "success" case "reset-failed": u := args[len(args)-1] if f.units[u]["ActiveState"] == "failed" { @@ -93,6 +104,8 @@ func (f *fakeSystem) machine() *Machine { func (f *fakeSystem) applier(now time.Time) *Applier { return &Applier{M: f.machine(), Act: true, Judge: true, Dial: func(addr string, _ time.Duration) error { + f.mu.Lock() + defer f.mu.Unlock() if f.reachable[addr] { return nil } @@ -104,6 +117,8 @@ func (f *fakeSystem) applier(now time.Time) *Applier { } func (f *fakeSystem) called(prefix string) bool { + f.mu.Lock() + defer f.mu.Unlock() for _, c := range f.calls { if strings.HasPrefix(c, prefix) { return true @@ -565,3 +580,10 @@ func TestApplyActsAndWatchJudgesAndNeitherDoesTheOthersWork(t *testing.T) { t.Error("the password leaked") } } + +// setUnit sets one unit's state as the service manager answers it, under the lock. +func (f *fakeSystem) setUnit(unit string, state map[string]string) { + f.mu.Lock() + defer f.mu.Unlock() + f.units[unit] = state +} From 6d3cb73e3b28c997457df8698f242955ec3016df Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 14:43:25 +0200 Subject: [PATCH 3/3] mounts: gofmt the clearing tests --- modules/mounts/cmd/mounts/failed_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/mounts/cmd/mounts/failed_test.go b/modules/mounts/cmd/mounts/failed_test.go index 5f8c17f3..2b03ae2e 100644 --- a/modules/mounts/cmd/mounts/failed_test.go +++ b/modules/mounts/cmd/mounts/failed_test.go @@ -100,7 +100,6 @@ func TestOnlyTheActingProcessClears(t *testing.T) { } } - // waitFor polls until cond holds or the deadline passes. func waitFor(t *testing.T, within time.Duration, cond func() bool) bool { t.Helper()