From 995f8237ce2545bdbe8d7bba22ac131bbc8e373b Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 13:14:00 +0200 Subject: [PATCH 1/2] Free a build node's disk after every build and refuse work under a floor of free space (issue 1834) A full / made every check on shanks end 'store never became ready' with the reason erased by --rm. Builds now remove their local image tag, narrowed source and context clone; a build or check refuses under 20 GiB free on / and the workspace, saying so; the throwaway store is no longer started with --rm and a store that does not come up reports its exit code and last 20 log lines. --- internal/builder/build_source_test.go | 9 +- internal/builder/builder.go | 45 ++++++- internal/builder/builder_test.go | 13 ++ internal/builder/check.go | 42 ++++++- internal/builder/disk_1834_test.go | 174 ++++++++++++++++++++++++++ internal/builder/floor.go | 46 +++++++ 6 files changed, 322 insertions(+), 7 deletions(-) create mode 100644 internal/builder/disk_1834_test.go create mode 100644 internal/builder/floor.go diff --git a/internal/builder/build_source_test.go b/internal/builder/build_source_test.go index 87ae28fe..c3433dcc 100644 --- a/internal/builder/build_source_test.go +++ b/internal/builder/build_source_test.go @@ -86,11 +86,14 @@ func TestAnImageCompilingGoIsHandedItsBuildSourceAndSaysIt(t *testing.T) { } for file, want := range map[string]bool{"proxy/main.go": true, "internal/shared/s.go": true, "go.mod": true, "cmd/ctl/main.go": false, "internal/only/o.go": false, "README.md": false} { - _, err := os.Stat(filepath.Join(workspace, "narrow-server", filepath.FromSlash(file))) - if (err == nil) != want { - t.Errorf("%s handed to the recipe: %v, wanted %v", file, err == nil, want) + held := slices.Contains(r.builtFrom, file) + if held != want { + t.Errorf("%s handed to the recipe: %v, wanted %v", file, held, want) } } + if _, err := os.Stat(filepath.Join(workspace, "narrow-server")); err == nil { + t.Error("the narrowed build source was left in the workspace") + } // Said per repository: its own, the manifest and the recipe; the context's, the closure. if len(got.Sources) != 2 { diff --git a/internal/builder/builder.go b/internal/builder/builder.go index d4997009..83d0f9a9 100644 --- a/internal/builder/builder.go +++ b/internal/builder/builder.go @@ -219,6 +219,11 @@ func build(ctx context.Context, run Runner, publish Publisher, if err := os.MkdirAll(workspace, 0o755); err != nil { return Result{}, err } + // **A node with no room takes no build** (novox/hq issue 1834). + if err := RoomToWork("/", workspace); err != nil { + say("clone", "refused: %v", err) + return Result{}, err + } // The credential is a file git reads, never an argument: a URL carrying a password in argv // would be readable by anything that can list processes for as long as a clone runs. credentials, forget, err := storeCredential(workspace, forge) @@ -738,11 +743,34 @@ func logging(log Log) func(step, format string, args ...any) { } } +// keepsImageLocally says the publisher leaves the image in the machine's runtime as the artifact itself. +func keepsImageLocally(publish Publisher) bool { + switch publish.(type) { + case Local, *Local: + return true + } + return false +} + +// releaseImage removes the local image tag a build made, with a context of its own: the build's may be cancelled. A +// tag that was never made (the build failed before it) is no fault; one that cannot be removed is said. +func releaseImage(ctx context.Context, run Runner, tag string, say func(step, format string, args ...any)) { + removing, done := context.WithTimeout(context.WithoutCancel(ctx), time.Minute) + defer done() + if _, err := run(removing, "", "docker", "rmi", tag); err != nil { + if !strings.Contains(err.Error(), "No such image") { + say("image", "the local image %s could not be removed, and goes with the week's sweep: %v", tag, err) + } + return + } + say("image", "the local image %s is removed: the registry holds it", tag) +} + // contextFrom clones an image artifact's own build context, when it names one apart from this // module's own repository — a fresh tree, the same way the module's own is, keyed by artifact // name so two artifacts of one module naming different contexts do not collide. func contextFrom(ctx context.Context, run Runner, workspace, artifact, credentials string, - from catalogue.ArtifactContext, seats map[string]string, say func(step, format string, args ...any)) (string, error) { + from catalogue.ArtifactContext, seats map[string]string, say func(step, format string, args ...any)) (_ string, err error) { url, err := contextURL(from, seats) if err != nil { return "", err @@ -752,6 +780,12 @@ func contextFrom(ctx context.Context, run Runner, workspace, artifact, credentia if err := os.RemoveAll(dir); err != nil { return "", err } + // A clone that failed half-way is no context to leave behind (novox/hq issue 1834). + defer func() { + if err != nil { + os.RemoveAll(dir) + } + }() if _, err := run(ctx, workspace, "git", cloneWith(credentials, "clone", "--quiet", url, dir)...); err != nil { return "", fmt.Errorf("cannot clone %s: %w", url, err) } @@ -1143,6 +1177,8 @@ func one(ctx context.Context, run Runner, publish Publisher, src.contexts[a.Name] = t } buildDir = cloned + // Let go of when the image is built, whatever became of it (novox/hq issue 1834). + defer os.RemoveAll(cloned) if t, _ := trunkOf(ctx, run, cloned, "HEAD"); t == "" || !atTrunkHead(ctx, run, cloned, "HEAD", t) { src.contextOffHead() } @@ -1180,6 +1216,7 @@ func one(ctx context.Context, run Runner, publish Publisher, src.ownHas(paths...) } buildDir = narrowed + defer os.RemoveAll(narrowed) } else if a.Context != nil { src.readWhole(*a.Context) } @@ -1205,6 +1242,12 @@ func one(ctx context.Context, run Runner, publish Publisher, } invocation = append(invocation, ".") say("image", "docker build -f %s", recipePath) + // **The local tag goes with the build** (novox/hq issue 1834): an image published to a registry is held there, and + // a tag per module and commit left on the node was the 96 GB of a full disk. Only a node publishing nowhere (a + // mesh being raised) keeps it, since the local image is then the artifact. + if !keepsImageLocally(publish) { + defer releaseImage(ctx, run, local, say) + } if _, err := run(ctx, buildDir, "docker", invocation...); err != nil { return catalogue.Built{}, fmt.Errorf("%s: building %s failed: %w", module, a.Name, err) } diff --git a/internal/builder/builder_test.go b/internal/builder/builder_test.go index 47efb4f4..0151585d 100644 --- a/internal/builder/builder_test.go +++ b/internal/builder/builder_test.go @@ -38,6 +38,9 @@ type recorded struct { stamped time.Time // tree is what git names as the tree of the module's directory; empty answers as for a commit. tree string + // builtFrom is the files (slash-separated, relative) of the folder the last `docker build` ran in, read as it ran: + // the build removes that folder when it ends (novox/hq issue 1834). + builtFrom []string } func (r *recorded) run(_ context.Context, dir, name string, args ...string) (string, error) { @@ -53,6 +56,16 @@ func (r *recorded) run(_ context.Context, dir, name string, args ...string) (str break } } + if name == "docker" && len(args) > 0 && args[0] == "build" { + r.builtFrom = nil + _ = filepath.WalkDir(dir, func(p string, d os.DirEntry, err error) error { + if err == nil && !d.IsDir() { + rel, _ := filepath.Rel(dir, p) + r.builtFrom = append(r.builtFrom, filepath.ToSlash(rel)) + } + return nil + }) + } switch { case name == "git" && isClone: repository := args[len(args)-2] diff --git a/internal/builder/check.go b/internal/builder/check.go index 9dc3f110..57d86925 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -392,6 +392,12 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry ctx, stop := context.WithTimeout(ctx, CheckTimeout) defer stop() + // **A node with no room takes no check** (novox/hq issue 1834): said as the check's verdict, never a throwaway store + // that dies at once on a full disk. + if err := RoomToWork("/", workspace); err != nil { + say("check", "refused: %v", err) + return CheckVerdict{}, err + } root := filepath.Join(workspace, "check") if err := os.RemoveAll(root); err != nil { return CheckVerdict{}, err @@ -1631,7 +1637,9 @@ func BusImage(version string) string { // throwaway starts a container publishing one port on loopback, and answers where it is reached. func throwaway(ctx context.Context, run, plain Runner, name, image string, port int, opts, args []string) (string, error) { - invocation := []string{"run", "-d", "--rm", "--name", name, "-p", fmt.Sprintf("127.0.0.1::%d", port)} + // Never with --rm (novox/hq issue 1834): a store that dies at once took its log with it. The check removes what it + // raised, with its volumes, after it has read why one stopped (RemoveContainersOf). + invocation := []string{"run", "-d", "--name", name, "-p", fmt.Sprintf("127.0.0.1::%d", port)} invocation = append(invocation, opts...) invocation = append(invocation, image) invocation = append(invocation, args...) @@ -1652,17 +1660,45 @@ func throwaway(ctx context.Context, run, plain Runner, name, image string, port // waitFor runs a readiness command in a container until it answers, for a minute. func waitFor(ctx context.Context, run Runner, name string, ready []string) error { - for i := 0; i < 60; i++ { + return waitReady(ctx, run, name, ready, 60) +} + +// waitReady is waitFor for a number of tries, a second apart. A container that never answers is not left a mystery +// (novox/hq issue 1834): its state and exit code and the last 20 lines of its log are in the error, read before the +// check removes it. +func waitReady(ctx context.Context, run Runner, name string, ready []string, attempts int) error { + for i := 0; i < attempts; i++ { if _, err := run(ctx, "", "docker", append([]string{"exec", name}, ready...)...); err == nil { return nil } + if i == attempts-1 { + break + } select { case <-ctx.Done(): return ctx.Err() case <-time.After(time.Second): } } - return fmt.Errorf("%s never became ready", name) + return fmt.Errorf("%s never became ready%s", name, whyItStopped(ctx, run, name)) +} + +// whyItStopped is ": , exit code N; its last log lines: …" for a container, or "" when docker says nothing. +func whyItStopped(ctx context.Context, run Runner, name string) string { + reading, done := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second) + defer done() + var said []string + if out, err := run(reading, "", "docker", "inspect", "--format", + "{{.State.Status}}, exit code {{.State.ExitCode}}", name); err == nil && strings.TrimSpace(out) != "" { + said = append(said, strings.TrimSpace(out)) + } + if out, err := run(reading, "", "docker", "logs", "--tail", "20", name); err == nil && strings.TrimSpace(out) != "" { + said = append(said, "its last log lines: "+strings.TrimSpace(out)) + } + if len(said) == 0 { + return "" + } + return ": " + strings.Join(said, "; ") } // dialable waits for an address to take a connection, for a minute. diff --git a/internal/builder/disk_1834_test.go b/internal/builder/disk_1834_test.go new file mode 100644 index 00000000..0c5fcbf1 --- /dev/null +++ b/internal/builder/disk_1834_test.go @@ -0,0 +1,174 @@ +package builder + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +// Issue 1834: a build node's root disk filled and every check there ended "the store never became ready". A node +// frees what each build and check leaves, refuses to take work under a floor of free space, and a store that does +// not come up says why. + +func init() { + // The disk the tests run on is none of their business; the tests of the floor say their own. + FreeBytes = func(string) (uint64, error) { return 500 << 30, nil } +} + +func withFree(t *testing.T, bytes uint64) { + t.Helper() + was := FreeBytes + FreeBytes = func(string) (uint64, error) { return bytes, nil } + t.Cleanup(func() { FreeBytes = was }) +} + +// 1834/AC2: given 3 GiB free, when a check or a build is asked, then it refuses at once, saying so, running nothing. +func TestACheckUnderTheFloorOfFreeSpaceRefusesSayingSo(t *testing.T) { + withFree(t, 3<<30) + var ran []string + run := func(_ context.Context, _ string, name string, args ...string) (string, error) { + ran = append(ran, name+" "+strings.Join(args, " ")) + return "", nil + } + _, err := Check(context.Background(), run, CheckSpec{ID: "build-1"}, t.TempDir(), "registry.invalid", GitCredential{}, nil) + if err == nil || !strings.Contains(err.Error(), "the build node has 3 GiB free on /, under the floor of 20 GiB") { + t.Fatalf("a check on a full disk said %v", err) + } + if len(ran) != 0 { + t.Errorf("a refused check ran %v", ran) + } +} + +func TestABuildUnderTheFloorOfFreeSpaceRefusesSayingSo(t *testing.T) { + withFree(t, 3<<30) + r := &recorded{} + _, err := Build(context.Background(), r.run, r, "https://forge.invalid/x.git", "", "", t.TempDir(), nil, Npmrc{}, GitCredential{}, nil) + if err == nil || !strings.Contains(err.Error(), "under the floor of 20 GiB") { + t.Fatalf("a build on a full disk said %v", err) + } + if len(r.ran) != 0 { + t.Errorf("a refused build ran %v", r.ran) + } +} + +// The workspace's filesystem is read too, and a workspace not made yet is read at its nearest folder. +func TestTheWorkspacesFilesystemIsReadToo(t *testing.T) { + was := FreeBytes + t.Cleanup(func() { FreeBytes = was }) + missing := filepath.Join(t.TempDir(), "not", "yet") + FreeBytes = func(p string) (uint64, error) { + if p == "/" { + return 100 << 30, nil + } + if _, err := os.Stat(p); err != nil { + t.Errorf("read the free space of %s, which is not there", p) + } + return 1 << 30, nil + } + err := RoomToWork("/", missing) + if err == nil || !strings.Contains(err.Error(), "1 GiB free on "+missing) { + t.Errorf("the workspace's disk, under the floor, said %v", err) + } + FreeBytes = func(string) (uint64, error) { return 0, errors.New("no") } + if RoomToWork("/") == nil { + t.Error("a disk that cannot be read was taken as having room") + } +} + +// 1834/AC3: given a store that stops at once, when the check waits for it, then its exit code and last log lines are +// in the error and the container is removed afterwards, not by --rm before anyone could read it. +func TestAStoreThatDiesSaysWhyItDied(t *testing.T) { + var ran []string + run := func(_ context.Context, _ string, name string, args ...string) (string, error) { + line := name + " " + strings.Join(args, " ") + ran = append(ran, line) + switch { + case args[0] == "exec": + return "", errors.New("Error response from daemon: container is not running") + case args[0] == "inspect": + return "exited exit code 1\n", nil + case args[0] == "logs": + return "initdb: error: could not create directory: No space left on device\n", nil + } + return "", nil + } + err := waitReady(context.Background(), run, "build-1-store", []string{"pg_isready"}, 1) + for _, want := range []string{"build-1-store never became ready", "exit code 1", "No space left on device"} { + if err == nil || !strings.Contains(err.Error(), want) { + t.Errorf("the verdict %q lacks %q", err, want) + } + } + logs := "" + for _, l := range ran { + if strings.HasPrefix(l, "docker logs") { + logs = l + } + } + if logs != "docker logs --tail 20 build-1-store" { + t.Errorf("the log was read as %q", logs) + } +} + +func TestTheThrowawayStoreIsNotStartedWithRm(t *testing.T) { + var started []string + run := func(_ context.Context, _ string, name string, args ...string) (string, error) { + if args[0] == "run" { + started = args + } + return "127.0.0.1:40000\n", nil + } + if _, err := throwaway(context.Background(), run, run, "build-1-store", "postgres:17-alpine", 5432, nil, nil); err != nil { + t.Fatal(err) + } + for _, a := range started { + if a == "--rm" { + t.Errorf("started with --rm, which erases why it died: %v", started) + } + } +} + +// 1834/AC1: given an image built for a check or a walk and published to the registry, when the build ends, then the +// local image tag, the narrowed source and the context clone it left are gone; an image published nowhere (a mesh +// being raised) is kept, since it is the artifact. +func TestABuildRemovesWhatItLeftOnTheNode(t *testing.T) { + r := &recorded{ + contents: map[string]string{"modules/m/" + ManifestName: anImageWithAContext, "modules/m/Dockerfile": "FROM scratch\n"}, + secondary: map[string]map[string]string{"https://forge.invalid/ctl.git": {"go.mod": "module x\n"}}, + } + workspace := t.TempDir() + if _, err := Build(context.Background(), r.run, r, "https://forge.invalid/c.git", "modules/m", "", workspace, nil, Npmrc{}, + GitCredential{}, nil); err != nil { + t.Fatal(err) + } + if !strings.Contains(strings.Join(r.ran, "\n"), "docker rmi m-server:c0ffeec0") { + t.Errorf("the image built for it was not removed: %v", r.ran) + } + left, _ := filepath.Glob(filepath.Join(workspace, "*")) + if len(left) != 0 { + t.Errorf("the build left %v in its workspace", left) + } +} + +const anImageWithAContext = `{"module":"m","version":"1", + "build":{"artifacts":[{"name":"server","kind":"image","from":"Dockerfile", + "context":{"repository":"https://forge.invalid/ctl.git","ref":"main"}}]}}` + +func TestAnImageKeptLocallyIsNotRemoved(t *testing.T) { + r := &recorded{contents: map[string]string{"modules/m/" + ManifestName: anImageWithAContext, "modules/m/Dockerfile": "FROM scratch\n"}} + local := Local{Run: func(ctx context.Context, dir, name string, args ...string) (string, error) { + if args[0] == "image" { + return "sha256:" + strings.Repeat("a", 64) + "\n", nil + } + return r.run(ctx, dir, name, args...) + }} + if _, err := Build(context.Background(), local.Run, local, "https://forge.invalid/c.git", "modules/m", "", t.TempDir(), nil, Npmrc{}, + GitCredential{}, nil); err != nil { + t.Fatal(err) + } + if strings.Contains(strings.Join(r.ran, "\n"), "docker rmi") { + t.Errorf("the artifact of a mesh being raised was removed: %v", r.ran) + } +} diff --git a/internal/builder/floor.go b/internal/builder/floor.go new file mode 100644 index 00000000..f13d4e93 --- /dev/null +++ b/internal/builder/floor.go @@ -0,0 +1,46 @@ +package builder + +import ( + "fmt" + "os" + "path/filepath" + "syscall" +) + +// FreeSpaceFloor is the free space a build node must have, on / and on its workspace's filesystem, before it takes +// a build or a check (novox/hq issue 1834): under it, a throwaway store cannot initialise its data directory and +// dies at once, and the check ended "never became ready" instead of saying the disk was full. +const FreeSpaceFloor = 20 << 30 + +// FreeBytes reads the bytes an unprivileged writer can still use on the filesystem holding path. A variable so a +// test answers for a disk it does not have. +var FreeBytes = func(path string) (uint64, error) { + var s syscall.Statfs_t + if err := syscall.Statfs(path, &s); err != nil { + return 0, err + } + return s.Bavail * uint64(s.Bsize), nil +} + +// RoomToWork refuses, saying how much is free, when any of the paths' filesystems is under FreeSpaceFloor. A path +// that does not exist yet (a workspace not made) is read at its nearest folder that does. +func RoomToWork(paths ...string) error { + for _, p := range paths { + at := p + for { + if _, err := os.Stat(at); err == nil || filepath.Dir(at) == at { + break + } + at = filepath.Dir(at) + } + free, err := FreeBytes(at) + if err != nil { + return fmt.Errorf("the free space on %s cannot be read: %w", p, err) + } + if free < FreeSpaceFloor { + return fmt.Errorf("the build node has %d GiB free on %s, under the floor of %d GiB", + free>>30, p, FreeSpaceFloor>>30) + } + } + return nil +} From f501eaca430541240e804ef89ba9664db96e75fb Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 13:19:43 +0200 Subject: [PATCH 2/2] Remove an image's pushed names with its build tag, and take no work under the free-space floor (issue 1834) The registry push tags the image a second time, so removing only the build tag freed nothing. The held image a comparison pulls goes after it is read, and a node under the floor leaves the ask to another build node instead of taking it and failing. --- cmd/mesh-builder/busy.go | 22 ++++++++++++++ cmd/mesh-builder/busy_test.go | 16 ++++++++++ cmd/mesh-builder/main.go | 2 +- internal/builder/builder.go | 25 +++++++++++----- internal/builder/disk_1834_test.go | 48 ++++++++++++++++++++++++++++-- internal/builder/registry.go | 5 ++++ internal/builder/reuse.go | 7 +++++ internal/builder/same_program.go | 3 ++ 8 files changed, 117 insertions(+), 11 deletions(-) diff --git a/cmd/mesh-builder/busy.go b/cmd/mesh-builder/busy.go index b5db4f37..049e97b2 100644 --- a/cmd/mesh-builder/busy.go +++ b/cmd/mesh-builder/busy.go @@ -9,6 +9,8 @@ import ( "sync" "sync/atomic" "time" + + "github.com/novox/mesh-controller/internal/builder" ) // **The build agent says whether it is busy, and being asked drains it** (novox/hq ADR 0335 §1, §5). Its @@ -211,3 +213,23 @@ func busy() (string, error) { } return fmt.Sprintf("busy: %s, in hand since %s; taking no new build", what, since), nil } + +var ( + roomMu sync.Mutex + roomSaid bool +) + +// noRoom says this node is under the floor of free space (novox/hq issue 1834): it takes no work, so another build node +// takes the ask instead of this one taking it and ending it in an error. Said once when it starts and again when it ends. +func noRoom(workspace string) bool { + err := builder.RoomToWork("/", workspace) + roomMu.Lock() + defer roomMu.Unlock() + if err != nil && !roomSaid { + fmt.Fprintf(os.Stderr, "taking no builds: %v\n", err) + } else if err == nil && roomSaid { + fmt.Fprintf(os.Stderr, "there is room again: taking builds\n") + } + roomSaid = err != nil + return err != nil +} diff --git a/cmd/mesh-builder/busy_test.go b/cmd/mesh-builder/busy_test.go index e44da2a4..d5f89b7f 100644 --- a/cmd/mesh-builder/busy_test.go +++ b/cmd/mesh-builder/busy_test.go @@ -1,6 +1,7 @@ package main import ( + "github.com/novox/mesh-controller/internal/builder" "os" "strings" "sync" @@ -221,3 +222,18 @@ func TestAnAgentThatCannotMarkABuildTakesNoWorkUntilItCan(t *testing.T) { } released() } + +// 1834/AC4: given a node under the floor of free space, when the taking loop asks, then it takes nothing, and takes again +// once there is room. +func TestANodeUnderTheFloorOfFreeSpaceTakesNoWork(t *testing.T) { + was := builder.FreeBytes + t.Cleanup(func() { builder.FreeBytes = was }) + builder.FreeBytes = func(string) (uint64, error) { return 2 << 30, nil } + if !noRoom(t.TempDir()) { + t.Error("a node with 2 GiB free took work") + } + builder.FreeBytes = func(string) (uint64, error) { return 200 << 30, nil } + if noRoom(t.TempDir()) { + t.Error("a node with 200 GiB free refused work") + } +} diff --git a/cmd/mesh-builder/main.go b/cmd/mesh-builder/main.go index 3ea5810b..2945e462 100644 --- a/cmd/mesh-builder/main.go +++ b/cmd/mesh-builder/main.go @@ -190,7 +190,7 @@ func run() error { // Paused or drained, it takes nothing new (novox/hq ADR 0219, ADR 0335): a drained agent finishes the build // in hand and then says idle, and its container is replaced between builds. machine := link.MachineOverNATSWith(js, on, seat, link.MachineOptions{ - Paused: func() bool { return h.Paused() || drained() || markFaulty(time.Now()) }, + Paused: func() bool { return h.Paused() || drained() || markFaulty(time.Now()) || noRoom(workspace) }, Taking: func() { taking(seat) }, Claim: claim, }) diff --git a/internal/builder/builder.go b/internal/builder/builder.go index 83d0f9a9..eb5b09e3 100644 --- a/internal/builder/builder.go +++ b/internal/builder/builder.go @@ -752,18 +752,25 @@ func keepsImageLocally(publish Publisher) bool { return false } -// releaseImage removes the local image tag a build made, with a context of its own: the build's may be cancelled. A -// tag that was never made (the build failed before it) is no fault; one that cannot be removed is said. -func releaseImage(ctx context.Context, run Runner, tag string, say func(step, format string, args ...any)) { +// PushedNames is implemented by a publisher that tags the image under other names to push it: the names it leaves in the +// machine's runtime for a repository, which go with the build's own tag (novox/hq issue 1834). +type PushedNames interface { + PushedNames(repository string) []string +} + +// releaseImage removes the names an image was given on the node, in one command: its layers go only with the last. It runs +// with a context of its own, the build's may be cancelled. A name never made (the build failed before it) is no fault; +// one that cannot be removed is said, and removal is said only when the image is gone. +func releaseImage(ctx context.Context, run Runner, names []string, say func(step, format string, args ...any)) { removing, done := context.WithTimeout(context.WithoutCancel(ctx), time.Minute) defer done() - if _, err := run(removing, "", "docker", "rmi", tag); err != nil { + if _, err := run(removing, "", "docker", append([]string{"rmi"}, names...)...); err != nil { if !strings.Contains(err.Error(), "No such image") { - say("image", "the local image %s could not be removed, and goes with the week's sweep: %v", tag, err) + say("image", "the local image %s could not be removed, and goes with the week's sweep: %v", strings.Join(names, ", "), err) } return } - say("image", "the local image %s is removed: the registry holds it", tag) + say("image", "the local image %s is removed: the registry holds it", strings.Join(names, ", ")) } // contextFrom clones an image artifact's own build context, when it names one apart from this @@ -1246,7 +1253,11 @@ func one(ctx context.Context, run Runner, publish Publisher, // a tag per module and commit left on the node was the 96 GB of a full disk. Only a node publishing nowhere (a // mesh being raised) keeps it, since the local image is then the artifact. if !keepsImageLocally(publish) { - defer releaseImage(ctx, run, local, say) + names := []string{local} + if p, ok := publish.(PushedNames); ok { + names = append(names, p.PushedNames(module+"/"+a.Name)...) + } + defer releaseImage(ctx, run, names, say) } if _, err := run(ctx, buildDir, "docker", invocation...); err != nil { return catalogue.Built{}, fmt.Errorf("%s: building %s failed: %w", module, a.Name, err) diff --git a/internal/builder/disk_1834_test.go b/internal/builder/disk_1834_test.go index 0c5fcbf1..63c0a17b 100644 --- a/internal/builder/disk_1834_test.go +++ b/internal/builder/disk_1834_test.go @@ -133,18 +133,26 @@ func TestTheThrowawayStoreIsNotStartedWithRm(t *testing.T) { // 1834/AC1: given an image built for a check or a walk and published to the registry, when the build ends, then the // local image tag, the narrowed source and the context clone it left are gone; an image published nowhere (a mesh // being raised) is kept, since it is the artifact. +// pushedNamed is a publisher that tags the image as / to push it, as the registry one does. +type pushedNamed struct{ *recorded } + +func (pushedNamed) PushedNames(repository string) []string { + return []string{"registry.invalid/" + repository} +} + func TestABuildRemovesWhatItLeftOnTheNode(t *testing.T) { r := &recorded{ contents: map[string]string{"modules/m/" + ManifestName: anImageWithAContext, "modules/m/Dockerfile": "FROM scratch\n"}, secondary: map[string]map[string]string{"https://forge.invalid/ctl.git": {"go.mod": "module x\n"}}, } workspace := t.TempDir() - if _, err := Build(context.Background(), r.run, r, "https://forge.invalid/c.git", "modules/m", "", workspace, nil, Npmrc{}, + if _, err := Build(context.Background(), r.run, pushedNamed{r}, "https://forge.invalid/c.git", "modules/m", "", workspace, nil, Npmrc{}, GitCredential{}, nil); err != nil { t.Fatal(err) } - if !strings.Contains(strings.Join(r.ran, "\n"), "docker rmi m-server:c0ffeec0") { - t.Errorf("the image built for it was not removed: %v", r.ran) + // Both names go in one rmi: the layers stay while either is left (the push tagged the image with the registry's). + if !strings.Contains(strings.Join(r.ran, "\n"), "docker rmi m-server:c0ffeec0 registry.invalid/m/server") { + t.Errorf("the image built for it and its pushed name were not removed: %v", r.ran) } left, _ := filepath.Glob(filepath.Join(workspace, "*")) if len(left) != 0 { @@ -172,3 +180,37 @@ func TestAnImageKeptLocallyIsNotRemoved(t *testing.T) { t.Errorf("the artifact of a mesh being raised was removed: %v", r.ran) } } + +// The held image the comparison pulls to read is let go of once read. +func TestTheHeldImageAComparisonPullsIsRemovedAfterReading(t *testing.T) { + var ran []string + run := func(_ context.Context, _ string, name string, args ...string) (string, error) { + ran = append(ran, name+" "+strings.Join(args, " ")) + if args[0] == "create" { + return "abcdef0123456789\n", nil + } + if args[0] == "image" { + return "", errors.New("stop here") + } + return "", nil + } + _, _ = imageContent(context.Background(), run, t.TempDir(), "reg.invalid/m/server@sha256:"+strings.Repeat("a", 64)) + if got := ran[len(ran)-1]; !strings.HasPrefix(got, "docker rmi reg.invalid/m/server@sha256:") { + t.Errorf("the last command was %q, not the removal of the pulled image: %v", got, ran) + } +} + +func TestAnImageThatCannotBeRemovedIsNotSaidRemoved(t *testing.T) { + var said []string + run := func(context.Context, string, string, ...string) (string, error) { + return "", errors.New("image is being used") + } + releaseImage(context.Background(), run, []string{"a:1"}, func(step, format string, args ...any) { + said = append(said, format) + }) + for _, s := range said { + if strings.Contains(s, "is removed") { + t.Errorf("said removed: %q", s) + } + } +} diff --git a/internal/builder/registry.go b/internal/builder/registry.go index 95918550..92d428d7 100644 --- a/internal/builder/registry.go +++ b/internal/builder/registry.go @@ -43,6 +43,11 @@ type Registry struct { HTTP *http.Client } +// PushedNames are the names PublishImage leaves the image under on the node. +func (r Registry) PushedNames(repository string) []string { + return []string{r.Address + "/" + repository} +} + // PublishImage pushes a locally built image and returns a reference pinned by digest. // // The digest is read back from the registry's own answer rather than computed here. What matters diff --git a/internal/builder/reuse.go b/internal/builder/reuse.go index 8c1b5ba1..b0f9ca8e 100644 --- a/internal/builder/reuse.go +++ b/internal/builder/reuse.go @@ -253,6 +253,13 @@ func (s scratchPublisher) PublishImage(ctx context.Context, localTag, repository return s.to.PublishImage(ctx, localTag, ScratchPrefix+repository) } +func (s scratchPublisher) PushedNames(repository string) []string { + if p, ok := s.to.(PushedNames); ok { + return p.PushedNames(ScratchPrefix + repository) + } + return nil +} + func (s scratchPublisher) PublishArchive(ctx context.Context, repository string, body []byte, digest string) (string, error) { return s.to.PublishArchive(ctx, ScratchPrefix+repository, body, digest) } diff --git a/internal/builder/same_program.go b/internal/builder/same_program.go index 11ce2f2e..b744e983 100644 --- a/internal/builder/same_program.go +++ b/internal/builder/same_program.go @@ -88,6 +88,9 @@ func imageContent(ctx context.Context, run Runner, dir, ref string) (string, err if _, err := run(ctx, dir, "docker", "pull", "--quiet", ref); err != nil { return "", fmt.Errorf("pulling %s: %w", ref, err) } + // The image is on the node only for the reading (novox/hq issue 1834); one a container still runs is refused by + // docker and stays. + defer func() { _, _ = run(context.WithoutCancel(ctx), dir, "docker", "rmi", ref) }() config, err := run(ctx, dir, "docker", "image", "inspect", "--format", "{{.Os}}/{{.Architecture}}/{{.Variant}} {{json .Config}}", ref) if err != nil {