diff --git a/modules/mesh-build-image/cmd/mesh-build-image/module.go b/modules/mesh-build-image/cmd/mesh-build-image/module.go index 819654c1..4fb2a146 100644 --- a/modules/mesh-build-image/cmd/mesh-build-image/module.go +++ b/modules/mesh-build-image/cmd/mesh-build-image/module.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "fmt" "net/http" "sync" @@ -97,11 +98,25 @@ func (m *Module) look(ctx context.Context) (Look, error) { return l, &buildstep.Refusal{Text: err.Error(), Have: l.Buildah} } if err := m.tunDevice(); err != nil { - return l, &buildstep.Refusal{Text: err.Error(), Have: l.Buildah} + return l, ¬Eligible{&buildstep.Refusal{Text: err.Error(), Have: l.Buildah}} } return l, nil } +// notEligible is a refusal of a machine that runs the module fine but cannot build here now (no tun device: its asks go +// to another node). It is a *buildstep.Refusal for a step, and the health stays healthy for it (issue 1126): the +// assigner reads unhealthy as a failed delivery and holds the build for every node. What cannot run (buildah missing, +// no ranges, no namespace, no runtime directory, no room) stays unhealthy. +type notEligible struct{ *buildstep.Refusal } + +func (e *notEligible) Unwrap() error { return e.Refusal } + +// isNotEligible says whether err is a refusal that leaves the module healthy. +func isNotEligible(err error) bool { + var n *notEligible + return errors.As(err, &n) +} + // Check is the guard: nil when the machine can build, else the refusal. Every look is kept for Eligible, and a change // wakes the puller, so a node that can build again takes asks at once. func (m *Module) Check(ctx context.Context) error { @@ -120,6 +135,13 @@ func (m *Module) keep(err error) { changed := m.last != why m.last = why m.lastMu.Unlock() + if changed && m.Say != nil { + if why == "" { + m.Say("this machine is eligible to build images again") + } else { + m.Say("this machine takes no image build now: %s", why) + } + } if changed && m.Held != nil { m.Held.Wake() } @@ -155,12 +177,16 @@ func (m *Module) Start(ctx context.Context) { func (m *Module) Health(ctx context.Context) health.Answer { l, err := m.look(ctx) m.keep(err) - if err != nil { + if err != nil && !isNotEligible(err) { return health.Unhealthy(err.Error()) } if err := m.room(); err != nil { return health.Unhealthy(err.Error()) } + if err != nil { + // Not eligible: the module runs, it takes no image ask here now, and says why; the capacity record says it too. + return health.Healthy("not eligible to build images, so it takes no image ask: " + err.Error()) + } return health.Healthy(fmt.Sprintf("buildah %s installed; %s holds %d subordinate uids and %d gids; a user namespace can be made; %s opens", l.Buildah, l.Account, l.UIDs, l.GIDs, m.tunPath())) } @@ -168,7 +194,7 @@ func (m *Module) Health(ctx context.Context) health.Answer { func (m *Module) Status(ctx context.Context) map[string]any { l, err := m.look(ctx) out := map[string]any{"node": m.Node, "account": m.Account, "buildah": l.Buildah, "subuids": l.UIDs, "subgids": l.GIDs, "refusing": err != nil, - "cpu": m.cpuStatus()} + "eligible": err == nil, "cpu": m.cpuStatus()} if err != nil { out["why"] = err.Error() } diff --git a/modules/mesh-build-image/cmd/mesh-build-image/module_test.go b/modules/mesh-build-image/cmd/mesh-build-image/module_test.go index cd26685b..fe2c5f2f 100644 --- a/modules/mesh-build-image/cmd/mesh-build-image/module_test.go +++ b/modules/mesh-build-image/cmd/mesh-build-image/module_test.go @@ -2,7 +2,9 @@ package main import ( "context" + "fmt" "os" + "path/filepath" "strings" "testing" @@ -54,7 +56,6 @@ func TestHealthIsUnhealthyWithoutWhatABuildNeeds(t *testing.T) { }{ {"subuid", func(m *Module, p string) { m.SubUIDFile = p }, []string{"/subuid", "mesh-build-image"}}, {"subgid", func(m *Module, p string) { m.SubGIDFile = p }, []string{"/subgid", "mesh-build-image"}}, - {"tun", func(m *Module, p string) { m.Tun = p + "/dev/net/tun" }, []string{"not eligible: ", "/dev/net/tun cannot be opened", "tun"}}, } { f := newFake(t) m := moduleWith(t, f, newStates()) @@ -74,6 +75,61 @@ func TestHealthIsUnhealthyWithoutWhatABuildNeeds(t *testing.T) { } } +// 1126: a node that cannot build here now (no tun device) stays healthy, saying "not eligible" and why; the assigner reads +// unhealthy as a failed delivery and held the new build for every node. The eligibility, the status and the log say it too. +func TestANodeWithoutTunIsHealthyAndSaysItIsNotEligible(t *testing.T) { + f := newFake(t) + m := moduleWith(t, f, newStates()) + var said []string + m.Say = func(format string, a ...any) { said = append(said, fmt.Sprintf(format, a...)) } + m.Tun = t.TempDir() + "/dev/net/tun" + a := m.Health(context.Background()) + if a.State != health.StateHealthy || !a.Healthy { + t.Fatalf("health = %+v, want healthy", a) + } + for _, want := range []string{"not eligible", "/dev/net/tun cannot be opened", "tun"} { + if !strings.Contains(a.Why, want) { + t.Errorf("health says %q, lacks %q", a.Why, want) + } + } + if got := m.Eligible(); !strings.Contains(got, "not eligible: ") { + t.Errorf("eligible says %q", got) + } + if s := m.Status(context.Background()); s["eligible"] != false || s["refusing"] != true { + t.Errorf("status = %v, want not eligible", s) + } + // One log line for the change, none for the same look again; one when it can build again. + m.Health(context.Background()) + if len(said) != 1 || !strings.Contains(said[0], "takes no image build now") { + t.Errorf("said = %q, want one line for the change", said) + } + m.Tun = filepath.Join(t.TempDir(), "tun") + os.WriteFile(m.Tun, nil, 0o644) + if a := m.Health(context.Background()); !a.Healthy || strings.Contains(a.Why, "not eligible") { + t.Errorf("health = %+v, want plain healthy", a) + } + if len(said) != 2 || !strings.Contains(said[1], "eligible to build images again") { + t.Errorf("said = %q, want the return said", said) + } + // A step still ends refused with the cause named. + m.Tun = t.TempDir() + "/none" + if err := m.Check(context.Background()); err == nil || !strings.Contains(err.Error(), "not eligible") { + t.Errorf("check = %v", err) + } +} + +// 1126: what cannot run stays unhealthy even beside a missing tun: buildah missing is not "not eligible". +func TestWithoutBuildahTheNodeIsUnhealthyNotIneligible(t *testing.T) { + m := moduleWith(t, newFake(t), newStates()) + m.Tun = t.TempDir() + "/none" + m.Run = func(ctx context.Context, env []string, name string, args ...string) forgekey.Ran { + return forgekey.Ran{Status: 127, Err: "exec: \"buildah\": executable file not found in $PATH"} + } + if a := m.Health(context.Background()); a.Healthy { + t.Errorf("health = %+v, want unhealthy", a) + } +} + func TestHealthIsUnhealthyWhenANamespaceCannotBeMade(t *testing.T) { f := newFake(t) m := moduleWith(t, f, newStates()) diff --git a/modules/mesh-build-image/cmd/mesh-build-image/tools.go b/modules/mesh-build-image/cmd/mesh-build-image/tools.go index 8455637c..6bd15a55 100644 --- a/modules/mesh-build-image/cmd/mesh-build-image/tools.go +++ b/modules/mesh-build-image/cmd/mesh-build-image/tools.go @@ -54,7 +54,7 @@ func (m *Module) Tools(steps []StepTool) []stdio.Tool { Run: func(map[string]any) (any, error) { return m.Status(m.Ctx), nil }}, stdio.Tool{Name: HealthTool, Description: "How the image build module is: unhealthy while buildah cannot be run, while its account holds no " + - "subordinate user or group range, or while a user namespace cannot be made, saying which; healthy otherwise. Read-only.", + "subordinate user or group range, or while a user namespace cannot be made, saying which; healthy otherwise. A machine that cannot build here now (no tun device) stays healthy and says in its words \"not eligible\", with the cause; it takes no image ask meanwhile (issue 1126). Read-only.", Run: func(map[string]any) (any, error) { return m.Health(m.Ctx), nil }}, ) }