amadan

carlos / platform Public

instancectl: reset-failed a stopped generation unit; strict kernel test managers log (#272)

After a generation unit is stopped or killed with the unit proven gone
(stopGen, and abandonGen's successful killGen), run systemctl reset-failed
on it so it does not linger in systemctl --failed. Best-effort (Ruling 10):
"not loaded" is ignored, any other error is logged and the stop still
answers 204. An unproven stop or kill does not reset.

TestStrictKernel's second managers had no logf, so freshenOne panicked;
give both logf: t.Logf. No production change for that.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 496fb6ed63dedf61958cf3c2ffa4843c75273de8 parent 59b1895
3 files changed, +150 −4
  • internal/cgroup/strict_kernel_linux_test.go +2 −2
  • internal/instancectl/gen.go +14 −0
  • internal/instancectl/server_gen_test.go +134 −2
diff --git a/internal/cgroup/strict_kernel_linux_test.go b/internal/cgroup/strict_kernel_linux_test.go
index 922e09764..4d3274e2f 100644
--- a/internal/cgroup/strict_kernel_linux_test.go
+++ b/internal/cgroup/strict_kernel_linux_test.go
@@ -157,7 +157,7 @@ func TestStrictKernel(t *testing.T) {
for range 12 {
go func() {
<-begin
- p, err := (&Manager{root: m.root}).PrepareStrict("concurrent.example", TierDefault)
+ p, err := (&Manager{root: m.root, logf: t.Logf}).PrepareStrict("concurrent.example", TierDefault)
results <- result{p, err}
}()
}
@@ -187,7 +187,7 @@ func TestStrictKernel(t *testing.T) {
if e != nil {
t.Fatal(e)
}
- other := &Manager{root: m.root}
+ other := &Manager{root: m.root, logf: t.Logf}
if q, e := other.PrepareStrict(host, TierDefault); e != ErrStrictState || q != nil {
t.Fatal("second manager reserved same group", e)
}
diff --git a/internal/instancectl/gen.go b/internal/instancectl/gen.go
index 0d60d21fa..5adf3fb58 100644
--- a/internal/instancectl/gen.go
+++ b/internal/instancectl/gen.go
@@ -194,10 +194,23 @@ func (s *Server) abandonGen(ctx context.Context, w http.ResponseWriter, host, un
http.Error(w, msg+"; termination unproven: "+err.Error(), statusGenUnproven)
return
}
+ s.resetGenFailed(dctx, unit)
s.removeGenFiles(unit)
http.Error(w, msg, http.StatusBadGateway)
}
+// resetGenFailed clears a stopped generation unit's failed state, so a
+// generation that died on its own does not stay in systemctl --failed
+// once the control service has stopped it. It runs only after a stop or
+// kill that proved the unit gone. Best-effort: the stop already succeeded,
+// so a unit that is no longer loaded is ignored and any other failure is
+// logged, never turned into a failed stop.
+func (s *Server) resetGenFailed(ctx context.Context, unit string) {
+ if out, err := s.Systemctl(ctx, "reset-failed", unit); err != nil && !strings.Contains(out+err.Error(), "not loaded") {
+ s.logf("instance-control: reset-failed %s: %v", unit, err)
+ }
+}
+
// killGen kills a generation unit outright, stops it, and waits until its
// cgroup is empty (spec §8.2 step 1). nil is the proof that nothing of it
// runs. A kill that finds nothing to kill is not a failure: the stop and
@@ -233,6 +246,7 @@ func (s *Server) stopGen(w http.ResponseWriter, r *http.Request, req request) {
http.Error(w, "stop: "+strings.TrimSpace(out+" "+err.Error()), http.StatusBadGateway)
return
}
+ s.resetGenFailed(ctx, unit)
s.removeGenFiles(unit)
w.WriteHeader(http.StatusNoContent)
}
diff --git a/internal/instancectl/server_gen_test.go b/internal/instancectl/server_gen_test.go
index 86aefc0f4..3ec774378 100644
--- a/internal/instancectl/server_gen_test.go
+++ b/internal/instancectl/server_gen_test.go
@@ -182,7 +182,7 @@ func TestGenStopWithKillKillsStopsWaitsAndRemovesFiles(t *testing.T) {
if err := NewClient(sock).StopGen(context.Background(), genHost, 7, true); err != nil {
t.Fatal(err)
}
- if want := "kill --signal=SIGKILL " + genUnit + "\nstop " + genUnit; ctl.joined() != want {
+ if want := "kill --signal=SIGKILL " + genUnit + "\nstop " + genUnit + "\nreset-failed " + genUnit; ctl.joined() != want {
t.Errorf("calls =\n%s\nwant\n%s", ctl.joined(), want)
}
if _, err := os.Stat(filepath.Join(dropins, genUnit+".d")); !os.IsNotExist(err) {
@@ -325,7 +325,7 @@ func TestGenStartWhoseInvocationLookupFailsIsKilledBeforeItsFilesGo(t *testing.T
if err == nil || errors.Is(err, ErrGenUnproven) {
t.Fatalf("StartGen = %v, want an ordinary start failure", err)
}
- if want := "start " + genUnit + "\nshow -p InvocationID --value " + genUnit + "\nkill --signal=SIGKILL " + genUnit + "\nstop " + genUnit; !strings.HasSuffix(ctl.joined(), want) {
+ if want := "start " + genUnit + "\nshow -p InvocationID --value " + genUnit + "\nkill --signal=SIGKILL " + genUnit + "\nstop " + genUnit + "\nreset-failed " + genUnit; !strings.HasSuffix(ctl.joined(), want) {
t.Errorf("calls =\n%s\nwant them to end\n%s", ctl.joined(), want)
}
if dropIn, exe := genFilesPresent(t, s, dropins); dropIn || exe {
@@ -568,3 +568,135 @@ func TestSweepKeepsTheFilesOfAGenerationNotProvenGone(t *testing.T) {
})
}
}
+
+// genLogServer is genServer with the server's log captured.
+func genLogServer(t *testing.T, ctl *fakeCtl) (*Server, string, *syncLog) {
+ t.Helper()
+ logs := &syncLog{}
+ dropins, exe := t.TempDir(), t.TempDir()
+ s, sock := serveOnSocket(t, ctl, func(s *Server) { s.DropInDir = dropins; s.ExeRoot = exe; s.Logf = logs.logf })
+ return s, sock, logs
+}
+
+type syncLog struct {
+ mu sync.Mutex
+ lines []string
+}
+
+func (l *syncLog) logf(format string, args ...any) {
+ l.mu.Lock()
+ defer l.mu.Unlock()
+ l.lines = append(l.lines, fmt.Sprintf(format, args...))
+}
+
+func (l *syncLog) text() string {
+ l.mu.Lock()
+ defer l.mu.Unlock()
+ return strings.Join(l.lines, "\n")
+}
+
+func TestStopGenResetsTheFailedStateAfterAProvenStop(t *testing.T) {
+ for _, kill := range []bool{false, true} {
+ t.Run(fmt.Sprintf("kill=%v", kill), func(t *testing.T) {
+ ctl := genCtl()
+ _, sock, _ := genLogServer(t, ctl)
+ if err := NewClient(sock).StopGen(context.Background(), genHost, 7, kill); err != nil {
+ t.Fatal(err)
+ }
+ want := "stop " + genUnit + "\nreset-failed " + genUnit
+ if kill {
+ want = "kill --signal=SIGKILL " + genUnit + "\n" + want
+ }
+ if ctl.joined() != want {
+ t.Errorf("calls =\n%s\nwant\n%s", ctl.joined(), want)
+ }
+ })
+ }
+}
+
+// reset-failed is cosmetic: the stop succeeded, so the answer stays 204 and
+// the failure is logged; a unit that is no longer loaded is not logged.
+func TestStopGenAnswersNoContentWhenResetFailedFails(t *testing.T) {
+ for _, kill := range []bool{false, true} {
+ t.Run(fmt.Sprintf("kill=%v", kill), func(t *testing.T) {
+ ctl := genCtl()
+ ctl.fail = map[string]error{"reset-failed ": errors.New("bus timeout")}
+ s, sock, logs := genLogServer(t, ctl)
+ if err := s.writeGenDropIn(genHost, 7, cgroup.TierDefault); err != nil {
+ t.Fatal(err)
+ }
+ if err := NewClient(sock).StopGen(context.Background(), genHost, 7, kill); err != nil {
+ t.Fatalf("StopGen = %v, want success", err)
+ }
+ if want := "instance-control: reset-failed " + genUnit + ": bus timeout"; !strings.Contains(logs.text(), want) {
+ t.Errorf("log =\n%s\nwant it to contain %q", logs.text(), want)
+ }
+ if _, err := os.Stat(filepath.Join(s.DropInDir, genUnit+".d")); !os.IsNotExist(err) {
+ t.Errorf("drop-in survived the stop: %v", err)
+ }
+ })
+ }
+}
+
+func TestStopGenIgnoresResetFailedOnAnUnloadedUnit(t *testing.T) {
+ ctl := genCtl()
+ ctl.fail = map[string]error{"reset-failed ": errors.New("Unit " + genUnit + " not loaded.")}
+ _, sock, logs := genLogServer(t, ctl)
+ if err := NewClient(sock).StopGen(context.Background(), genHost, 7, false); err != nil {
+ t.Fatal(err)
+ }
+ if strings.Contains(logs.text(), "reset-failed") {
+ t.Errorf("an unloaded unit was logged: %s", logs.text())
+ }
+}
+
+func TestStopGenWhoseStopFailsDoesNotResetFailed(t *testing.T) {
+ for _, kill := range []bool{false, true} {
+ t.Run(fmt.Sprintf("kill=%v", kill), func(t *testing.T) {
+ ctl := genCtl()
+ ctl.fail = map[string]error{"stop ": errors.New("stop timed out")}
+ _, sock, _ := genLogServer(t, ctl)
+ if err := NewClient(sock).StopGen(context.Background(), genHost, 7, kill); err == nil {
+ t.Fatal("StopGen succeeded though the stop failed")
+ }
+ if strings.Contains(ctl.joined(), "reset-failed") {
+ t.Errorf("reset-failed after a failed stop:\n%s", ctl.joined())
+ }
+ })
+ }
+}
+
+func TestStopGenWhoseCgroupNeverEmptiesDoesNotResetFailed(t *testing.T) {
+ ctl := genCtl()
+ s, sock, _ := genLogServer(t, ctl)
+ s.cgroupWaitBudget = 100 * time.Millisecond
+ populateGenCgroup(t, s)
+ if err := NewClient(sock).StopGen(context.Background(), genHost, 7, true); err == nil {
+ t.Fatal("StopGen succeeded though the cgroup never emptied")
+ }
+ if strings.Contains(ctl.joined(), "reset-failed") {
+ t.Errorf("reset-failed after an unproven kill:\n%s", ctl.joined())
+ }
+}
+
+func TestAbandonedGenStartResetsFailedOnlyAfterAProvenKill(t *testing.T) {
+ ctl := genCtl()
+ ctl.fail = map[string]error{"start ": errors.New("boom")}
+ _, sock, _ := genLogServer(t, ctl)
+ if _, err := NewClient(sock).StartGen(context.Background(), genHost, 7, cgroup.TierDefault.MemoryMax); err == nil || errors.Is(err, ErrGenUnproven) {
+ t.Fatalf("StartGen = %v, want an ordinary start failure", err)
+ }
+ if want := "kill --signal=SIGKILL " + genUnit + "\nstop " + genUnit + "\nreset-failed " + genUnit; !strings.HasSuffix(ctl.joined(), want) {
+ t.Errorf("calls =\n%s\nwant them to end\n%s", ctl.joined(), want)
+ }
+
+ ctl = genCtl()
+ ctl.fail = map[string]error{"start ": errors.New("boom"), "stop ": errors.New("stop timed out")}
+ _, sock, _ = genLogServer(t, ctl)
+ if _, err := NewClient(sock).StartGen(context.Background(), genHost, 7, cgroup.TierDefault.MemoryMax); !errors.Is(err, ErrGenUnproven) {
+ t.Fatalf("StartGen = %v, want ErrGenUnproven", err)
+ }
+ if strings.Contains(ctl.joined(), "reset-failed") {
+ t.Errorf("reset-failed after a failed kill:\n%s", ctl.joined())
+ }
+}