diff --git a/modules/nfs-server/README.md b/modules/nfs-server/README.md index 6f9c9987..faf96ea3 100644 --- a/modules/nfs-server/README.md +++ b/modules/nfs-server/README.md @@ -102,6 +102,13 @@ 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. +**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 +bound of 10 s: the export file is removed first, which needs no command, then `exportfs -ra` runs within +that bound. Whatever it could not finish is logged as `nfs-server: UNASSIGNED, NOT CLEAN: …`, with what a +person must do. + ## 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/machine.go b/modules/nfs-server/cmd/nfs-server/machine.go index 34476e06..4d302924 100644 --- a/modules/nfs-server/cmd/nfs-server/machine.go +++ b/modules/nfs-server/cmd/nfs-server/machine.go @@ -108,6 +108,21 @@ type Machine struct { // clean is when a look below each real path last found no backup repository (LookKept). clean map[string]time.Time cleanMu sync.Mutex + // walk is the running pass's context, which ends the look below a share when the process stops. + walk context.Context + walkMu sync.Mutex +} + +func (m *Machine) setWalk(ctx context.Context) { + m.walkMu.Lock() + m.walk = ctx + m.walkMu.Unlock() +} + +func (m *Machine) walking() context.Context { + m.walkMu.Lock() + defer m.walkMu.Unlock() + return m.walk } // Here is the machine this process runs on. @@ -272,8 +287,17 @@ func (m *Machine) Escalated(name string, args ...string) (string, []string) { // AsRoot runs what only root may do, and names a refusal by how it failed. func (m *Machine) AsRoot(name string, args ...string) (Ran, error) { + return m.AsRootWithin(context.Background(), name, args...) +} + +// AsRootWithin is AsRoot ended with its context: the process's pass is cancelled by SIGTERM, and Stopped +// runs within StopTimeout. +func (m *Machine) AsRootWithin(ctx context.Context, name string, args ...string) (Ran, error) { program, argv := m.Escalated(name, args...) - r := m.Run(context.Background(), program, argv...) + r := m.Run(ctx, program, argv...) + if err := ctx.Err(); err != nil && r.Code != 0 { + return r, fmt.Errorf("%s %s did not finish: %v", name, strings.Join(args, " "), err) + } if r.Code != 0 { why := r.Err if why == "" { diff --git a/modules/nfs-server/cmd/nfs-server/process.go b/modules/nfs-server/cmd/nfs-server/process.go index 114677ad..81e2439c 100644 --- a/modules/nfs-server/cmd/nfs-server/process.go +++ b/modules/nfs-server/cmd/nfs-server/process.go @@ -56,7 +56,14 @@ func (m *Machine) Pass(ctx context.Context, last *Status) Status { if _, note := m.Backup(); note != "" { s.Said = append(s.Said, note) } + m.setWalk(ctx) exports, refused, said := m.Judge(c, roster, wants) + m.setWalk(nil) + if ctx.Err() != nil { + // Stopping: write nothing and reload nothing, so Stopped starts at once. + s.Problems = append(s.Problems, "the pass was ended by the process stopping; nothing was written") + return s + } s.Refused = refused s.Said = append(s.Said, said...) @@ -80,7 +87,7 @@ func (m *Machine) Pass(ctx context.Context, last *Status) Status { } } if reload != "" { - if _, err := m.AsRoot("exportfs", "-ra"); err != nil { + if _, err := m.AsRootWithin(ctx, "exportfs", "-ra"); err != nil { s.ReloadError = err.Error() } else { at := m.Now().UTC() @@ -102,6 +109,10 @@ func (m *Machine) Pass(ctx context.Context, last *Status) Status { return s } +// 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 + // RunExports is the process: a pass now and every Every until the context ends, then Stopped. func (m *Machine) RunExports(ctx context.Context) error { last := m.Pass(ctx, nil) @@ -184,11 +195,25 @@ func (m *Machine) Stopped() { if _, err := m.Read(ConfigFile); err == nil || !os.IsNotExist(err) { return } + ctx, cancel := context.WithTimeout(context.Background(), StopTimeout) + defer cancel() + // The file first: removing it needs no command, so a hanging sudo or exportfs cannot keep it. if err := m.unexport(); err != nil { - fmt.Fprintln(os.Stderr, "nfs-server:", err) + loud("the module was unassigned and its export file is still there: " + err.Error() + + "; remove " + ExportFile + " and run exportfs -ra as root") return } - if _, err := m.AsRoot("exportfs", "-ra"); err != nil { - fmt.Fprintln(os.Stderr, "nfs-server: the export file is gone, and exportfs -ra failed:", err) + fmt.Fprintln(os.Stderr, "nfs-server: unassigned: the export file "+ExportFile+" is removed") + if _, err := m.AsRootWithin(ctx, "exportfs", "-ra"); err != nil { + loud("the module was unassigned and its export file is removed, but the kernel may still export its " + + "shares until exportfs -ra runs as root: " + err.Error()) + return } + fmt.Fprintln(os.Stderr, "nfs-server: unassigned: the kernel let the shares go (exportfs -ra)") +} + +// 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) } diff --git a/modules/nfs-server/cmd/nfs-server/review_test.go b/modules/nfs-server/cmd/nfs-server/review_test.go index c0893d11..86128e7a 100644 --- a/modules/nfs-server/cmd/nfs-server/review_test.go +++ b/modules/nfs-server/cmd/nfs-server/review_test.go @@ -379,3 +379,56 @@ func TestTheLookBelowIsKeptForAWhileAndThenTakenAgain(t *testing.T) { t.Errorf("a repository put there later was not found once the kept look expired: %+v", s.Refused) } } + +// mesh-host #62 found that a stop the service manager ends with SIGKILL after 90 s leaves the export file +// behind. A pass that hangs in a command (sudo, exportfs: 20 s each) must not hold the cleanup: SIGTERM +// cancels the running pass, and Stopped has its own short bound, so the file is gone well inside the stop +// timeout even when every command hangs. +func TestAHangingCommandStillLetsStoppedRemoveTheFileInTime(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"}}}) + m := f.machine() + inner := m.Run + hung := make(chan struct{}, 8) + m.Run = func(ctx context.Context, name string, args ...string) Ran { + line := strings.Join(append([]string{name}, args...), " ") + if strings.Contains(line, "exportfs") { + hung <- struct{}{} + <-ctx.Done() // hangs until its context ends, as a stuck sudo or exportfs would + return Ran{Code: -1, Err: ctx.Err().Error()} + } + return inner(ctx, name, args...) + } + saved := StopTimeout + StopTimeout = 300 * time.Millisecond + defer func() { StopTimeout = saved }() + + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan error, 1) + go func() { done <- m.RunExports(ctx) }() + select { + case <-hung: // the first pass is stuck in exportfs + case <-time.After(5 * time.Second): + t.Fatal("the first pass never reached exportfs") + } + if err := os.Remove(filepath.Join(f.root, ConfigFile)); err != nil { // unassigned: config removed first + t.Fatal(err) + } + start := time.Now() + cancel() // SIGTERM + select { + case <-done: + case <-time.After(3 * time.Second): + t.Fatal("the process did not stop within 3 s of SIGTERM while a command hung") + } + if took := time.Since(start); took > 2*time.Second { + t.Errorf("stopping took %s with a hanging command; the bound is StopTimeout (%s)", took, StopTimeout) + } + if _, err := os.Stat(filepath.Join(f.root, ExportFile)); err == nil { + t.Errorf("the export file stays after an unassignment whose commands hung") + } +} diff --git a/modules/nfs-server/cmd/nfs-server/shares.go b/modules/nfs-server/cmd/nfs-server/shares.go index 3beef7d8..cf5b2d27 100644 --- a/modules/nfs-server/cmd/nfs-server/shares.go +++ b/modules/nfs-server/cmd/nfs-server/shares.go @@ -281,9 +281,13 @@ func (m *Machine) walkBelow(dir string) (found string, complete bool) { } seen := 0 level := []string{dir} + walk := m.walking() for depth := 1; depth <= WalkDepth && len(level) > 0; depth++ { var next []string for _, d := range level { + if walk != nil && walk.Err() != nil { + return "", false // ended by the process stopping: unfinished, so refused + } for _, child := range m.Children(d) { seen++ if seen > limit {