diff --git a/go/buildstep/isolate.go b/go/buildstep/isolate.go index 497f4bb..c87ad81 100644 --- a/go/buildstep/isolate.go +++ b/go/buildstep/isolate.go @@ -7,6 +7,7 @@ import ( "os" "path" "path/filepath" + "slices" "strconv" "strings" "sync" @@ -33,20 +34,26 @@ const Bubblewrap = "bwrap" // downloaded) read-only; // - the shared warm Caches read-only under a throwaway writable layer (S18), or read-write for a trunk build alone // (WriteCaches), so a candidate's code never changes what a later build reads; -// - with a CacheLayer, what one command writes into the caches kept for the later commands of the same ask (issue 1921, -// the try speed-up): the vet, the unit tests and the integration compile of one ask compile each package once. +// - with a CacheLayer, what one command writes into the LayerCaches kept for the later commands of the same ask (issue +// 1921, the try speed-up): the vet, the unit tests and the integration compile of one ask compile each package once. type Isolation struct { Workspace string Writable []string ReadOnly []string Caches []string WriteCaches bool - // CacheLayer is a directory of the ask's own, in its workspace and gone with it: each command writes the caches into - // a layer of its own beneath it (overlayfs refuses an upper layer two mounts share, and an ask's commands run side by - // side), which Isolated moves into the ask's kept layer when the command ends; every later command reads the kept - // layer above the shared cache. The shared cache stays unchanged. Empty: what a command writes is thrown away. + // CacheLayer is a directory of the ask's own, gone with its workspace (Workspaces.CacheLayer): each command writes + // the LayerCaches into a layer of its own beneath it (overlayfs refuses an upper layer two mounts share, and an + // ask's commands run side by side), which Isolated moves into the ask's kept layer when the command ends; every + // later command reads the kept layer above the shared cache. The shared cache stays unchanged. It must lie outside + // the Workspace and every Writable directory, so no command reaches it (a command that could would make the move, + // which runs outside the sandbox, write where it points). Empty: what a command writes is thrown away. CacheLayer string - Env []string + // LayerCaches are the Caches kept in the CacheLayer: a build cache, whose entries are whole files written by + // rename. Every other cache (a module cache, with read-only directories and half-written modules) keeps a + // throwaway layer. + LayerCaches []string + Env []string // Hosts is the file bound as /etc/hosts; Isolated writes one naming only the loopback. Hosts string // run is the directory of this command's own writable layers under CacheLayer; Isolated makes one per command. @@ -110,6 +117,9 @@ func (iso Isolation) command(env []string, name string, args ...string) ([]strin } } } + if err := iso.checkLayer(); err != nil { + return nil, err + } words := append(append([]string{}, iso.Env...), env...) home := false for _, w := range words { @@ -146,16 +156,16 @@ func (iso Isolation) command(env []string, name string, args ...string) ([]strin // cacheArgs binds the shared warm caches (S18): for a trunk build read-write, so the next build is warm; for any other // read-only beneath an overlay whose writes go to a tmpfs of the step's own, so the step writes as a build does and -// nothing it wrote outlives it (bubblewrap's --tmp-overlay, an overlay mount in the step's user namespace). With a -// CacheLayer the ask's kept layer lies above the shared cache and the command's writes go to its own layer on disk -// (--overlay), which Isolated keeps for the ask when the command ends. +// nothing it wrote outlives it (bubblewrap's --tmp-overlay, an overlay mount in the step's user namespace). A cache of +// LayerCaches, with a CacheLayer, has the ask's kept layer above the shared cache and the command's writes go to its own +// layer on disk (--overlay), which Isolated keeps for the ask when the command ends. func (iso Isolation) cacheArgs() []string { var argv []string for i, c := range iso.Caches { switch { case iso.WriteCaches: argv = append(argv, "--bind", c, c) - case iso.CacheLayer != "": + case iso.layered(c): upper, work := iso.runLayer(i) argv = append(argv, "--overlay-src", c, "--overlay-src", keptLayer(iso.CacheLayer, i), "--overlay", upper, work, c) default: @@ -165,6 +175,33 @@ func (iso Isolation) cacheArgs() []string { return argv } +// layered says whether the cache c is kept in the ask's cache layer. +func (iso Isolation) layered(c string) bool { + return iso.CacheLayer != "" && !iso.WriteCaches && slices.Contains(iso.LayerCaches, c) +} + +// checkLayer refuses a cache layer a command could reach (in or above the workspace or a writable directory, or a +// cache), and a layered cache that is not one of the caches. +func (iso Isolation) checkLayer() error { + if iso.CacheLayer == "" { + return nil + } + for _, d := range append(append([]string{iso.Workspace}, iso.Writable...), iso.Caches...) { + if within(iso.CacheLayer, d) || within(d, iso.CacheLayer) { + return fmt.Errorf("buildstep: the cache layer %s is within reach of the step's %s", iso.CacheLayer, d) + } + } + for _, c := range iso.LayerCaches { + if !slices.Contains(iso.Caches, c) { + return fmt.Errorf("buildstep: the layered cache %s is not one of the caches", c) + } + } + return nil +} + +// within says whether p is dir or lies beneath it. +func within(p, dir string) bool { return p == dir || strings.HasPrefix(p, dir+"/") } + func layerDirs(layer string) []string { if layer == "" { return nil @@ -172,8 +209,9 @@ func layerDirs(layer string) []string { return []string{layer} } -// keptLayer is where the ask keeps what its commands wrote into the cache i. -func keptLayer(layer string, i int) string { return filepath.Join(layer, "kept", strconv.Itoa(i)) } +// keptRel is where, in the cache layer, the ask keeps what its commands wrote into the cache i; keptLayer is its path. +func keptRel(i int) string { return filepath.Join("kept", strconv.Itoa(i)) } +func keptLayer(layer string, i int) string { return filepath.Join(layer, keptRel(i)) } // runLayer is the upper and work directory of this command's writes into the cache i. Command, which makes no // directory, names the layer "run"; Isolated gives each command one of its own. @@ -190,9 +228,12 @@ var layerLocks sync.Map // prepareLayer makes the kept layers and a run directory of the command's own under the cache layer. func (iso *Isolation) prepareLayer() error { - if iso.CacheLayer == "" || iso.WriteCaches || len(iso.Caches) == 0 { + if iso.CacheLayer == "" || iso.WriteCaches || len(iso.LayerCaches) == 0 { return nil } + if err := iso.checkLayer(); err != nil { + return err + } if err := os.MkdirAll(filepath.Join(iso.CacheLayer, "runs"), 0o700); err != nil { return err } @@ -201,7 +242,10 @@ func (iso *Isolation) prepareLayer() error { return err } iso.run = run - for i := range iso.Caches { + for i, c := range iso.Caches { + if !iso.layered(c) { + continue + } upper, work := iso.runLayer(i) for _, d := range []string{keptLayer(iso.CacheLayer, i), upper, work} { if err := os.MkdirAll(d, 0o700); err != nil { @@ -212,10 +256,11 @@ func (iso *Isolation) prepareLayer() error { return nil } -// keepLayer moves what the command wrote into the caches into the ask's kept layers, and removes its run directory. -// A file the command wrote replaces the kept one (a cache's newest write wins); a whiteout (a file the command -// deleted) is not kept, since a cache entry deleted by one command is no fault of a later one. A write that cannot be -// kept only means a later command builds it again. +// keepLayer moves what the command wrote into the layered caches into the ask's kept layers, and removes its run +// directory. A file the command wrote replaces the kept one (a cache's newest write wins); a whiteout (a file the +// command deleted), a symlink or any other entry that is not a directory or a regular file is not kept. Every move goes +// through an os.Root on the cache layer, so no symlink leads it outside the layer. A write that cannot be kept only +// means a later command builds it again. func (iso Isolation) keepLayer() { if iso.run == "" { return @@ -224,23 +269,37 @@ func (iso Isolation) keepLayer() { mu, _ := layerLocks.LoadOrStore(iso.CacheLayer, &sync.Mutex{}) mu.(*sync.Mutex).Lock() defer mu.(*sync.Mutex).Unlock() - for i := range iso.Caches { - upper, _ := iso.runLayer(i) - kept := keptLayer(iso.CacheLayer, i) - filepath.WalkDir(upper, func(p string, d fs.DirEntry, err error) error { + root, err := os.OpenRoot(iso.CacheLayer) + if err != nil { + return + } + defer root.Close() + runRel, err := filepath.Rel(iso.CacheLayer, iso.run) + if err != nil { + return + } + for i, c := range iso.Caches { + if !iso.layered(c) { + continue + } + upper := filepath.Join(runRel, strconv.Itoa(i), "upper") + fs.WalkDir(root.FS(), upper, func(p string, d fs.DirEntry, err error) error { if err != nil || p == upper { return nil } rel, _ := filepath.Rel(upper, p) - to := filepath.Join(kept, rel) + to := filepath.Join(keptRel(i), rel) switch { case d.IsDir(): - if fi, err := os.Lstat(to); err == nil && !fi.IsDir() { - os.Remove(to) + if fi, err := root.Lstat(to); err == nil && !fi.IsDir() { + root.Remove(to) } - os.MkdirAll(to, 0o700) + root.MkdirAll(to, 0o700) case d.Type().IsRegular(): - os.Rename(p, to) + if fi, err := root.Lstat(to); err == nil && !fi.Mode().IsRegular() { + return nil // never replace what is not a file the layer kept + } + root.Rename(p, to) } return nil }) diff --git a/go/buildstep/isolate_caches_test.go b/go/buildstep/isolate_caches_test.go index de2d816..b30e397 100644 --- a/go/buildstep/isolate_caches_test.go +++ b/go/buildstep/isolate_caches_test.go @@ -32,24 +32,35 @@ func TestACandidatesStepWritesTheCachesOnlyIntoAThrowawayLayer(t *testing.T) { } // 1921/S2: with a cache layer, a command reads the ask's kept layer above the shared cache and writes into a layer // on disk of the ask's own, never into the shared cache and never into a throwaway tmpfs. - iso = Isolation{Workspace: "/w", Caches: []string{"/cache/gomod", "/cache/gobuild"}, CacheLayer: "/w/cachelayer"} + // Only the layered caches (the build cache) are; the module cache keeps its throwaway layer. + iso = Isolation{Workspace: "/w/ws1", Caches: []string{"/cache/gomod", "/cache/gobuild"}, CacheLayer: "/w/ws1.cachelayer", + LayerCaches: []string{"/cache/gobuild"}} argv, err = iso.Command("go", "vet") if err != nil { t.Fatal(err) } - for i, c := range iso.Caches { - if !holds(argv, "--overlay-src", c, "--overlay-src", keptLayer(iso.CacheLayer, i), "--overlay") { - t.Errorf("the cache %s is not read beneath the ask's kept layer: %v", c, argv) - } - if up := overlayOf(argv, c); up == "" || !strings.HasPrefix(up, "/w/cachelayer/") { - t.Errorf("the cache %s is written into %q, not a layer of the ask's own: %v", c, up, argv) - } - if holds(argv, "--tmp-overlay", c) || holds(argv, "--bind", c, c) { - t.Errorf("the cache %s is thrown away or written: %v", c, argv) - } + if !holds(argv, "--overlay-src", "/cache/gobuild", "--overlay-src", keptLayer(iso.CacheLayer, 1), "--overlay") { + t.Errorf("the build cache is not read beneath the ask's kept layer: %v", argv) } - if _, err := (Isolation{Workspace: "/w", CacheLayer: "cachelayer"}).Command("go"); err == nil { - t.Error("a relative cache layer was taken") + if up := overlayOf(argv, "/cache/gobuild"); up == "" || !strings.HasPrefix(up, "/w/ws1.cachelayer/") { + t.Errorf("the build cache is written into %q, not a layer of the ask's own: %v", up, argv) + } + if !holds(argv, "--overlay-src", "/cache/gomod", "--tmp-overlay", "/cache/gomod") || overlayOf(argv, "/cache/gomod") != "" { + t.Errorf("the module cache is not under a throwaway layer: %v", argv) + } + // A layer a command could reach is refused: in the workspace (bound read-write), in a writable directory, in a + // cache, or above one of them; so is a relative one, and a layered cache that is no cache. + for _, bad := range []Isolation{ + {Workspace: "/w/ws1", CacheLayer: "/w/ws1/cachelayer"}, + {Workspace: "/w/ws1", CacheLayer: "/w"}, + {Workspace: "/w/ws1", Writable: []string{"/out"}, CacheLayer: "/out/layer"}, + {Workspace: "/w/ws1", Caches: []string{"/cache/gobuild"}, CacheLayer: "/cache/gobuild/layer"}, + {Workspace: "/w/ws1", CacheLayer: "cachelayer"}, + {Workspace: "/w/ws1", Caches: []string{"/cache/gobuild"}, CacheLayer: "/w/ws1.cachelayer", LayerCaches: []string{"/cache/other"}}, + } { + if _, err := bad.Command("go"); err == nil { + t.Errorf("the cache layer %s of %+v was taken", bad.CacheLayer, bad) + } } } diff --git a/go/buildstep/isolate_integration_test.go b/go/buildstep/isolate_integration_test.go index 75c0c9b..818507e 100644 --- a/go/buildstep/isolate_integration_test.go +++ b/go/buildstep/isolate_integration_test.go @@ -66,8 +66,9 @@ func TestARunStepReachesNothingAndLeavesTheCachesUnchanged(t *testing.T) { // 1921/S2: with the ask's cache layer, what one command writes into the cache the next command of the ask reads, // above what the shared cache holds, and the shared cache stays unchanged. + layer := filepath.Join(filepath.Dir(workspace), "ws1.cachelayer") asked := Isolated(forgekey.ExecRunnerFor(0), Isolation{Workspace: workspace, Caches: []string{cacheDir}, - CacheLayer: filepath.Join(workspace, "cachelayer")}) + CacheLayer: layer, LayerCaches: []string{cacheDir}}) r = asked(context.Background(), []string{"PATH=/usr/bin:/bin"}, "/bin/sh", "-c", "cat "+filepath.Join(cacheDir, "warm")+" && mkdir -p "+filepath.Join(cacheDir, "ab")+" && echo compiled > "+filepath.Join(cacheDir, "ab", "one-d")) if r.Status != 0 || r.Err != "" || strings.TrimSpace(r.Stdout) != "warm" { t.Fatalf("the first command of the ask did not read the shared cache and write its own (status %d, %s): %s%s", r.Status, r.Err, r.Stdout, r.Stderr) @@ -76,10 +77,23 @@ func TestARunStepReachesNothingAndLeavesTheCachesUnchanged(t *testing.T) { if r.Status != 0 || r.Err != "" || r.Stdout != "compiled\nwarm\n" { t.Fatalf("the next command of the ask did not find what the first wrote (status %d, %s): %q %s", r.Status, r.Err, r.Stdout, r.Stderr) } - if _, err := os.Stat(filepath.Join(cacheDir, "ab")); !os.IsNotExist(err) { - t.Fatalf("the ask's commands wrote the shared cache: %v", err) + // A command cannot reach the layer (it lies outside every directory the step binds), and a symlink it writes into + // the cache is not kept, so nothing it does makes the keeping write outside the layer. + r = asked(context.Background(), []string{"PATH=/usr/bin:/bin"}, "/bin/sh", "-c", + "ls "+layer+" >/dev/null 2>&1 && echo reached; ln -s "+cacheDir+" "+filepath.Join(cacheDir, "esc")+" && mkdir "+filepath.Join(cacheDir, "cd")+" && echo x > "+filepath.Join(cacheDir, "cd", "f")) + if r.Status != 0 || r.Err != "" || strings.Contains(r.Stdout, "reached") { + t.Fatalf("the command reached the cache layer or could not write the cache (status %d, %s): %q %s", r.Status, r.Err, r.Stdout, r.Stderr) } - if err := os.RemoveAll(filepath.Join(workspace, "cachelayer")); err != nil { - t.Fatalf("the ask's cache layer cannot be removed with its workspace: %v", err) + if fi, err := os.Lstat(filepath.Join(keptLayer(layer, 0), "esc")); err == nil { + t.Fatalf("a symlink the command wrote was kept: %v", fi.Mode()) + } + if b, err := os.ReadFile(filepath.Join(keptLayer(layer, 0), "cd", "f")); err != nil || string(b) != "x\n" { + t.Fatalf("the command's file was not kept: %q %v", b, err) + } + if entries, _ := os.ReadDir(cacheDir); len(entries) != 1 || entries[0].Name() != "warm" { + t.Fatalf("the ask's commands wrote the shared cache: %v", entries) + } + if err := os.RemoveAll(layer); err != nil { + t.Fatalf("the ask's cache layer cannot be removed: %v", err) } } diff --git a/go/buildstep/isolate_test.go b/go/buildstep/isolate_test.go index 7a1eda3..906c0a0 100644 --- a/go/buildstep/isolate_test.go +++ b/go/buildstep/isolate_test.go @@ -124,7 +124,7 @@ func TestIsolatedRunsEachCommandThroughBubblewrap(t *testing.T) { if err := os.MkdirAll(shared, 0o700); err != nil { t.Fatal(err) } - layer := filepath.Join(root, "ws", "cachelayer") + layer := filepath.Join(root, "ws1.cachelayer") var mu sync.Mutex uppers := map[string]bool{} gate := make(chan struct{}) @@ -141,7 +141,15 @@ func TestIsolatedRunsEachCommandThroughBubblewrap(t *testing.T) { os.WriteFile(filepath.Join(up, "ab", args[len(args)-1]+"-d"), []byte("compiled"), 0o600) return forgekey.Ran{} } - run := Isolated(writer, Isolation{Workspace: filepath.Join(root, "ws"), Caches: []string{shared}, CacheLayer: layer}) + // A symlink in the kept layer (none is ever kept; planted here) leads no move outside the layer: the kept directory + // replaces it, and the shared cache it points to stays unchanged. + if err := os.MkdirAll(keptLayer(layer, 0), 0o700); err != nil { + t.Fatal(err) + } + if err := os.Symlink(shared, filepath.Join(keptLayer(layer, 0), "ab")); err != nil { + t.Fatal(err) + } + run := Isolated(writer, Isolation{Workspace: filepath.Join(root, "ws1"), Caches: []string{shared}, CacheLayer: layer, LayerCaches: []string{shared}}) var wg sync.WaitGroup for _, pkg := range []string{"one", "two"} { wg.Add(1) diff --git a/go/buildstep/workspaces.go b/go/buildstep/workspaces.go index eefdee3..23d408d 100644 --- a/go/buildstep/workspaces.go +++ b/go/buildstep/workspaces.go @@ -10,6 +10,7 @@ import ( "path/filepath" "regexp" "sort" + "strings" "sync" "syscall" "time" @@ -116,6 +117,14 @@ func (w *Workspaces) touch(id string) { _ = os.Chtimes(filepath.Join(w.Dir, id), now, now) // a workspace not made yet has nothing to mark } +// cacheLayerSuffix names a workspace's cache layer beside it (Isolation.CacheLayer): never inside it, which every step +// of the workspace binds read-write. +const cacheLayerSuffix = ".cachelayer" + +// CacheLayer is the directory of the workspace id's cache layer: beside the workspace, out of every step's reach, and +// removed with it by Release and Sweep. +func (w *Workspaces) CacheLayer(id string) string { return filepath.Join(w.Dir, id+cacheLayerSuffix) } + // Release removes the workspace at once: its caller's chain of steps has ended. One already gone is no error; one a // running step holds is refused and kept; an id that is not a workspace's is refused. func (w *Workspaces) Release(id string) error { @@ -130,6 +139,9 @@ func (w *Workspaces) Release(id string) error { if err := os.RemoveAll(filepath.Join(w.Dir, id)); err != nil { return fmt.Errorf("the workspace %s could not be removed: %w", id, err) } + if err := os.RemoveAll(w.CacheLayer(id)); err != nil { + return fmt.Errorf("the cache layer of the workspace %s could not be removed: %w", id, err) + } return nil } @@ -144,6 +156,13 @@ func (w *Workspaces) Sweep(now time.Time, bound time.Duration) []string { w.mu.Lock() defer w.mu.Unlock() for _, e := range entries { + if id, ok := strings.CutSuffix(e.Name(), cacheLayerSuffix); ok && workspaceID.MatchString(id) && w.held[id] == 0 { + // a cache layer whose workspace is gone (its removal failed or was cut short) goes too + if _, err := os.Stat(filepath.Join(w.Dir, id)); os.IsNotExist(err) { + os.RemoveAll(filepath.Join(w.Dir, e.Name())) + } + continue + } if !e.IsDir() || !workspaceID.MatchString(e.Name()) || w.held[e.Name()] > 0 { continue } @@ -152,6 +171,7 @@ func (w *Workspaces) Sweep(now time.Time, bound time.Duration) []string { continue } if os.RemoveAll(filepath.Join(w.Dir, e.Name())) == nil { + os.RemoveAll(w.CacheLayer(e.Name())) removed = append(removed, e.Name()) } } diff --git a/go/buildstep/workspaces_test.go b/go/buildstep/workspaces_test.go index 8badfa2..cf95117 100644 --- a/go/buildstep/workspaces_test.go +++ b/go/buildstep/workspaces_test.go @@ -4,6 +4,7 @@ import ( "errors" "os" "path/filepath" + "strings" "testing" "time" ) @@ -68,8 +69,15 @@ func gone(p string) bool { _, err := os.Stat(p); return os.IsNotExist(err) } func TestReleaseRemovesAWorkspaceAtOnce(t *testing.T) { w := &Workspaces{Dir: t.TempDir()} p := makeWorkspace(t, w, "abc1", 0) - if err := w.Release("abc1"); err != nil || !gone(p) { - t.Fatalf("Release = %v, gone %v", err, gone(p)) + layer := w.CacheLayer("abc1") // issue 1921/S2: the cache layer lies beside the workspace and goes with it + if filepath.Dir(layer) != w.Dir || strings.HasPrefix(layer, p+"/") { + t.Fatalf("the cache layer %s does not lie beside the workspace %s", layer, p) + } + if err := os.MkdirAll(filepath.Join(layer, "kept", "0"), 0o700); err != nil { + t.Fatal(err) + } + if err := w.Release("abc1"); err != nil || !gone(p) || !gone(layer) { + t.Fatalf("Release = %v, gone %v, its cache layer gone %v", err, gone(p), gone(layer)) } if err := w.Release("abc1"); err != nil { t.Errorf("a second release = %v, want nil", err) @@ -98,11 +106,21 @@ func TestSweepRemovesUnheldWorkspacesPastTheBound(t *testing.T) { if err := os.WriteFile(filepath.Join(w.Dir, "not-a-workspace.txt"), nil, 0o600); err != nil { t.Fatal(err) } + // issue 1921/S2: a swept workspace's cache layer goes with it, and so does a layer whose workspace is gone; a held + // workspace's stays + for _, id := range []string{"old1", "held1", "orphan1"} { + if err := os.MkdirAll(w.CacheLayer(id), 0o700); err != nil { + t.Fatal(err) + } + } done := w.Hold("held1") removed := w.Sweep(time.Now(), 2*time.Hour) if !gone(old) || gone(heldOld) || gone(recent) { t.Errorf("after the sweep: old gone %v, held kept %v, recent kept %v", gone(old), !gone(heldOld), !gone(recent)) } + if !gone(w.CacheLayer("old1")) || !gone(w.CacheLayer("orphan1")) || gone(w.CacheLayer("held1")) { + t.Errorf("after the sweep: old1's layer gone %v, orphan1's gone %v, held1's kept %v", gone(w.CacheLayer("old1")), gone(w.CacheLayer("orphan1")), !gone(w.CacheLayer("held1"))) + } if len(removed) != 1 || removed[0] != "old1" { t.Errorf("Sweep answered %v, want [old1]", removed) }