diff --git a/Makefile b/Makefile index fce59ecf..f957467f 100644 --- a/Makefile +++ b/Makefile @@ -117,7 +117,7 @@ proxy-image: # as the script runs them: a bus per test, of the release the mesh runs, makes the suite the same in any # order, and the timeout bounds a hang to a failure with a stack, never a stalled gate. check: postgres - @sh merge-check.sh ; status=$$? ; docker rm -f $(PG_CONTAINER) >/dev/null 2>&1 || true ; exit $$status + @sh merge-check.sh ; status=$$? ; docker rm -f -v $(PG_CONTAINER) >/dev/null 2>&1 || true ; exit $$status # The unit level alone (untagged: no bus, no store, no other process), which a developer runs continuously. The tests # that need a bus or the store carry `//go:build integration` and run in `check`, not here; without a database the @@ -135,7 +135,7 @@ fmt: if [ -n "$$unformatted" ] ; then echo "not gofmt'd:" ; echo "$$unformatted" ; exit 1 ; fi postgres: - @docker rm -f $(PG_CONTAINER) >/dev/null 2>&1 || true + @docker rm -f -v $(PG_CONTAINER) >/dev/null 2>&1 || true @docker run -d --name $(PG_CONTAINER) -e POSTGRES_PASSWORD=check \ -p 127.0.0.1:$(PG_PORT):5432 $(PG_IMAGE) >/dev/null @printf 'waiting for postgres' @@ -147,7 +147,7 @@ postgres: echo ' — never came up' ; docker logs $(PG_CONTAINER) | tail -20 ; exit 1 postgres-stop: - @docker rm -f $(PG_CONTAINER) >/dev/null 2>&1 || true + @docker rm -f -v $(PG_CONTAINER) >/dev/null 2>&1 || true clean: rm -rf build/ diff --git a/cmd/mesh-builder/holder_test.go b/cmd/mesh-builder/holder_test.go index d5ac76d1..c89c6b3d 100644 --- a/cmd/mesh-builder/holder_test.go +++ b/cmd/mesh-builder/holder_test.go @@ -91,7 +91,7 @@ func TestKillEndsTheBuildRunningHereAndNoOther(t *testing.T) { t.Errorf("kill said %q", said) } // Removed at the kill and again once the build had ended: a container made in between is caught. - if len(removed) != 4 || !strings.Contains(removed[0], "label=mesh.build=build-1") || !strings.Contains(removed[2], "label=mesh.build=build-1") { + if len(removed) != 6 || !strings.Contains(removed[0], "label=mesh.build=build-1") || !strings.Contains(removed[3], "label=mesh.build=build-1") { t.Errorf("removed %v", removed) } if c := h.current(); c.Running != nil { @@ -160,7 +160,7 @@ func TestAHolderStartingRemovesWhatEarlierBuildsLeftHere(t *testing.T) { } return "", nil } - if n := h.removeLeftBehind(); n != 2 || len(removed) != 2 || removed[1] != "docker rm -f c1 c2" { + if n := h.removeLeftBehind(); n != 2 || len(removed) != 3 || removed[2] != "docker rm -f -v c1 c2" { t.Fatalf("removed %d: %v", n, removed) } h.remove = func(context.Context, string, string, ...string) (string, error) { diff --git a/internal/builder/kill.go b/internal/builder/kill.go index 9200da5b..19bec371 100644 --- a/internal/builder/kill.go +++ b/internal/builder/kill.go @@ -3,6 +3,7 @@ package builder import ( "context" "os/exec" + "regexp" "strings" "syscall" "time" @@ -71,12 +72,45 @@ func RemoveContainersOf(ctx context.Context, run Runner, id string) (int, error) if len(ids) == 0 { return 0, nil } - if _, err := run(ctx, "", "docker", append([]string{"rm", "-f"}, ids...)...); err != nil { + if err := removeWithVolumes(ctx, run, ids); err != nil { return 0, err } return len(ids), nil } +// MountedVolumes is the template `docker inspect --format` reads the names of a container's volumes with. +const MountedVolumes = `{{range .Mounts}}{{if eq .Type "volume"}}{{println .Name}}{{end}}{{end}}` + +// anonymousVolume is the name docker gives a volume it makes for an image's VOLUME line: 64 hex digits. +var anonymousVolume = regexp.MustCompile(`^[0-9a-f]{64}$`) + +// removeWithVolumes removes containers together with their anonymous volumes (novox/hq issue 1772). A throwaway +// store declares its data directory a VOLUME, docker makes an anonymous volume for it, and `docker rm -f` +// without -v leaves that volume for ever: 1,629 of them, 107 GB, on one build node. So the volumes the +// containers mount are read first, the containers are removed with -v, and an anonymous volume still there +// afterwards is removed by name. Only a volume one of these containers mounted, and only one with an anonymous +// volume's name, is touched: a named volume — an operator's own data — never is. A volume already gone is +// no failure. +func removeWithVolumes(ctx context.Context, run Runner, ids []string) error { + var volumes []string + if out, err := run(ctx, "", "docker", append([]string{"inspect", "--format", MountedVolumes}, ids...)...); err == nil { + seen := map[string]bool{} + for _, name := range strings.Fields(out) { + if anonymousVolume.MatchString(name) && !seen[name] { + seen[name] = true + volumes = append(volumes, name) + } + } + } + if _, err := run(ctx, "", "docker", append([]string{"rm", "-f", "-v"}, ids...)...); err != nil { + return err + } + if len(volumes) > 0 { + _, _ = run(ctx, "", "docker", append([]string{"volume", "rm"}, volumes...)...) + } + return nil +} + // A holder that stops mid-build leaves its containers running (novox/hq issue 306). // // A build's containers are removed by the build itself, by a kill, or by the next delivery of the same @@ -106,7 +140,7 @@ func RemoveLeftBehind(ctx context.Context, run Runner) (int, error) { if len(ids) == 0 { return 0, nil } - if _, err := run(ctx, "", "docker", append([]string{"rm", "-f"}, ids...)...); err != nil { + if err := removeWithVolumes(ctx, run, ids); err != nil { return 0, err } return len(ids), nil diff --git a/internal/builder/kill_test.go b/internal/builder/kill_test.go index b3779f65..52ba7c46 100644 --- a/internal/builder/kill_test.go +++ b/internal/builder/kill_test.go @@ -2,6 +2,7 @@ package builder import ( "context" + "errors" "reflect" "strings" "testing" @@ -49,7 +50,8 @@ func TestAKillRemovesTheContainersLabelledWithTheBuild(t *testing.T) { if err != nil || n != 2 { t.Fatalf("%d %v", n, err) } - if want := []string{"docker ps -aq --filter label=mesh.build=build-3", "docker rm -f c1 c2"}; !reflect.DeepEqual(ran, want) { + if want := []string{"docker ps -aq --filter label=mesh.build=build-3", "docker inspect --format " + MountedVolumes + " c1 c2", + "docker rm -f -v c1 c2"}; !reflect.DeepEqual(ran, want) { t.Errorf("ran %v", ran) } } @@ -73,7 +75,7 @@ func TestAStartingHolderRemovesWhatEarlierHoldersLeftAndNothingElse(t *testing.T if n != 3 { t.Errorf("removed %d", n) } - if len(ran) != 2 || !reflect.DeepEqual(ran[1], []string{"docker", "rm", "-f", "c1", "c2", "c7"}) { + if len(ran) != 3 || !reflect.DeepEqual(ran[2], []string{"docker", "rm", "-f", "-v", "c1", "c2", "c7"}) { t.Fatalf("ran %v", ran) } if !strings.Contains(strings.Join(ran[0], " "), "--filter label=mesh.build") { @@ -90,3 +92,54 @@ func TestAStartingHolderRemovesWhatEarlierHoldersLeftAndNothingElse(t *testing.T t.Errorf("with nothing of the seat's: %d %v %v", n, err, ran) } } + +// **Issue 1772**: a throwaway store declares its data directory a VOLUME, so docker makes an anonymous volume for +// it, and `docker rm -f` without -v leaves that volume behind (1,629 of them, 107 GB, on one build node). Every +// container a build or check removes is removed with its anonymous volumes (-v), and any volume the container +// mounted that is still there afterwards is removed by name — those, and only those, never a named volume. +func TestARemovedContainerTakesItsAnonymousVolumesWithIt(t *testing.T) { + anon := strings.Repeat("a", 64) + other := strings.Repeat("b", 64) + var ran []string + run := func(_ context.Context, _ string, name string, args ...string) (string, error) { + ran = append(ran, name+" "+strings.Join(args, " ")) + switch args[0] { + case "ps": + return "c1\n", nil + case "inspect": + return anon + "\n" + "compose_data\n" + other + "\n", nil + } + return "", nil + } + if n, err := RemoveContainersOf(context.Background(), run, "build-4"); err != nil || n != 1 { + t.Fatalf("%d %v", n, err) + } + last := ran[len(ran)-1] + if want := "docker volume rm " + anon + " " + other; last != want { + t.Errorf("last command %q, want %q", last, want) + } + if !strings.Contains(strings.Join(ran, "\n"), "docker rm -f -v c1") { + t.Errorf("ran %v", ran) + } + if strings.Contains(last, "compose_data") { + t.Errorf("a named volume was removed: %q", last) + } +} + +// A volume already gone when the sweep looks (rm -v took it) is no failure. +func TestAVolumeAlreadyGoneIsNoFailure(t *testing.T) { + run := func(_ context.Context, _ string, name string, args ...string) (string, error) { + switch args[0] { + case "ps": + return "c1\n", nil + case "inspect": + return strings.Repeat("c", 64) + "\n", nil + case "volume": + return "", errors.New("no such volume") + } + return "", nil + } + if n, err := RemoveContainersOf(context.Background(), run, "build-5"); err != nil || n != 1 { + t.Fatalf("%d %v", n, err) + } +} diff --git a/internal/builder/same_program.go b/internal/builder/same_program.go index dfef2001..11ce2f2e 100644 --- a/internal/builder/same_program.go +++ b/internal/builder/same_program.go @@ -105,7 +105,7 @@ func imageContent(ctx context.Context, run Runner, dir, ref string) (string, err if id == "" { return "", errors.New("the container engine named no container for " + ref) } - defer func() { _, _ = run(context.WithoutCancel(ctx), dir, "docker", "rm", "--force", id) }() + defer func() { _, _ = run(context.WithoutCancel(ctx), dir, "docker", "rm", "--force", "--volumes", id) }() if err := os.MkdirAll(dir, 0o700); err != nil { return "", err }