From e7842860b0b9a8a7367dfa475a0094b8b4abe328 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Tue, 7 Feb 2023 21:48:10 -0800 Subject: [PATCH] Simplify overlay2 self filestore file creation. No need to create a directory in each submount that is unique to the container. If the same submount is mounted multiple times within the same sandbox, then the overlay option doesn't work correctly. Because each sentry overlay mount is independent and changes to one are not visible to the other. In such a scenario the right thing to do would be to share a sentry overlay mount between such repeated submounts. Since each overlay-ed submount should really only have one sentry overlay mount per sandbox, this change simplifies the structure of filestore files by making them per-sandbox per-mount files, rather than having a per-container directory with multiple files. PiperOrigin-RevId: 507974578 --- runsc/boot/loader.go | 2 +- runsc/boot/vfs.go | 36 ++++++++++----------- runsc/container/container.go | 41 ++++++++++++------------ runsc/container/multi_container_test.go | 25 ++------------- test/e2e/integration_runtime_test.go | 42 ++----------------------- 5 files changed, 44 insertions(+), 102 deletions(-) diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go index 72a7b1017..49748fe95 100644 --- a/runsc/boot/loader.go +++ b/runsc/boot/loader.go @@ -822,7 +822,7 @@ func (l *Loader) createContainerProcess(root bool, cid string, info *containerIn } l.startGoferMonitor(cid, int32(info.goferFDs[0].FD())) - mntr := newContainerMounter(info, l.k, l.mountHints, l.productName, cid) + mntr := newContainerMounter(info, l.k, l.mountHints, l.productName, l.sandboxID) if root { if err := mntr.processHints(info.conf, info.procArgs.Credentials); err != nil { return nil, nil, err diff --git a/runsc/boot/vfs.go b/runsc/boot/vfs.go index f63e177ac..d6aa1b880 100644 --- a/runsc/boot/vfs.go +++ b/runsc/boot/vfs.go @@ -58,22 +58,22 @@ const ( Nonefs = "none" ) -// SelfOverlayFilestoreDirPrefix is the prefix in the directory name of the -// self overlay filestore directory. -const SelfOverlayFilestoreDirPrefix = ".gvisor.overlay.img." +// SelfOverlayFilestorePrefix is the prefix in the file name of the +// self overlay filestore file. +const SelfOverlayFilestorePrefix = ".gvisor.overlay.img." -// SelfOverlayFilestoreDir returns the directory path in which self overlay filestore -// files are stored for a given mount. -func SelfOverlayFilestoreDir(mountSrc, cid string) string { - // We will place filestore files in a gvisor specific hidden directory inside - // the mount being overlayed itself. The same volume can be overlay-ed by - // multiple containers. So make the filestore directory unique to container - // by suffixing the container ID. - return path.Join(mountSrc, selfOverlayFilestoreDirName(cid)) +// SelfOverlayFilestorePath returns the path at which the self overlay +// filestore file is stored for a given mount. +func SelfOverlayFilestorePath(mountSrc, sandboxID string) string { + // We will place the filestore file in a gVisor specific hidden file inside + // the mount being overlay-ed itself. The same volume can be overlay-ed by + // multiple sandboxes. So make the filestore file unique to a sandbox by + // suffixing the sandbox ID. + return path.Join(mountSrc, selfOverlayFilestoreName(sandboxID)) } -func selfOverlayFilestoreDirName(cid string) string { - return SelfOverlayFilestoreDirPrefix + cid +func selfOverlayFilestoreName(sandboxID string) string { + return SelfOverlayFilestorePrefix + sandboxID } // tmpfs has some extra supported options that we must pass through. @@ -350,11 +350,11 @@ type containerMounter struct { // /sys/devices/virtual/dmi/id/product_name. productName string - // cid is the container ID for the container. - cid string + // sandboxID is the ID for the whole sandbox. + sandboxID string } -func newContainerMounter(info *containerInfo, k *kernel.Kernel, hints *podMountHints, productName string, cid string) *containerMounter { +func newContainerMounter(info *containerInfo, k *kernel.Kernel, hints *podMountHints, productName string, sandboxID string) *containerMounter { return &containerMounter{ root: info.spec.Root, mounts: compileMounts(info.spec, info.conf), @@ -363,7 +363,7 @@ func newContainerMounter(info *containerInfo, k *kernel.Kernel, hints *podMountH k: k, hints: hints, productName: productName, - cid: cid, + sandboxID: sandboxID, } } @@ -572,7 +572,7 @@ func (c *containerMounter) configureOverlay(ctx context.Context, conf *config.Co if err := overlay.CreateWhiteout(ctx, c.k.VFS(), creds, &vfs.PathOperation{ Root: upperRootVD, Start: upperRootVD, - Path: fspath.Parse(selfOverlayFilestoreDirName(c.cid)), + Path: fspath.Parse(selfOverlayFilestoreName(c.sandboxID)), }); err != nil { return nil, nil, fmt.Errorf("failed to create whiteout to hide self overlay filestore: %w", err) } diff --git a/runsc/container/container.go b/runsc/container/container.go index 936d0f172..e68fdc2b3 100644 --- a/runsc/container/container.go +++ b/runsc/container/container.go @@ -766,13 +766,13 @@ func (c *Container) Destroy() error { errs = append(errs, err.Error()) return nil } - // mountSrc only contains the filestore directory if it is a directory. + // mountSrc only contains the filestore file if it is a directory. if !mountSrcInfo.IsDir() { return nil } - filestoreDir := c.SelfOverlayFilestoreDir(mountSrc) - if err := os.RemoveAll(filestoreDir); err != nil { - err = fmt.Errorf("failed to delete filestore directory %q: %v", filestoreDir, err) + filestorePath := boot.SelfOverlayFilestorePath(mountSrc, c.sandboxID()) + if err := os.Remove(filestorePath); err != nil { + err = fmt.Errorf("failed to delete filestore file %q: %v", filestorePath, err) log.Warningf("%v", err) errs = append(errs, err.Error()) } @@ -813,6 +813,10 @@ func (c *Container) Destroy() error { return fmt.Errorf(strings.Join(errs, "\n")) } +func (c *Container) sandboxID() string { + return c.Saver.ID.SandboxID +} + // createOverlayFilestores creates the regular files that will back the tmpfs // upper mount for overlay mounts. It may return (nil, nil) if overlay is not // configured to be backed by host files. @@ -840,12 +844,6 @@ func (c *Container) createOverlayFilestores() ([]*os.File, error) { return filestoreFiles, nil } -// SelfOverlayFilestoreDir returns the directory path in which self overlay filestore -// files are stored for a given mount. -func (c *Container) SelfOverlayFilestoreDir(mountSrc string) string { - return boot.SelfOverlayFilestoreDir(mountSrc, c.ID) -} - // Precondition: Overlay2.IsBackedByHostFile() && Overlay2.IsBackedBySelf(). func (c *Container) createOverlayFilestoreInSelf() ([]*os.File, error) { var filestoreFiles []*os.File @@ -858,16 +856,19 @@ func (c *Container) createOverlayFilestoreInSelf() ([]*os.File, error) { log.Warningf("overlay2 self medium is only supported for directory mounts, but mount %q is not a directory, falling back to memory", mountSrc) return nil } - // Create SelfFilestoreDir() directory. Note that it may already exist from - // previous iteration for the same mountSrc. - filestoreDir := c.SelfOverlayFilestoreDir(mountSrc) - if err := os.Mkdir(filestoreDir, 0755); err != nil && !os.IsExist(err) { - return fmt.Errorf("failed to create filestore directory %q: %v", filestoreDir, err) - } - // The same volume can be mounted at various places within the same - // container. So use os.CreateTemp to create a random filename. - filestoreFile, err := os.CreateTemp(filestoreDir, "filestore-") + // Create the self overlay filestore file. + filestorePath := boot.SelfOverlayFilestorePath(mountSrc, c.sandboxID()) + filestoreFD, err := unix.Open(filestorePath, unix.O_RDWR|unix.O_CREAT|unix.O_EXCL, 0666) if err != nil { + if err == unix.EEXIST { + // Note that if the same submount is mounted multiple times within the + // same sandbox, then the overlay option doesn't work correctly. + // Because each overlay mount is independent and changes to one are not + // visible to the other. Given "overlay on repeated submounts" is + // already broken, we don't support such a scenario with the self + // medium. The filestore file will already exist for such a case. + return fmt.Errorf("%q mount source already has a filestore file at %q; repeated submounts are not suppported with self medium", mountSrc, filestorePath) + } return fmt.Errorf("failed to create filestore file inside %q: %v", mountSrc, err) } // Filestore in self should be a named path because it needs to be @@ -875,7 +876,7 @@ func (c *Container) createOverlayFilestoreInSelf() ([]*os.File, error) { // and apply any limits appropriately (like local ephemeral storage // limits). So don't delte it. These files will be unlinked when the // container is destroyed. This makes self medium appropriate for k8s. - filestoreFiles = append(filestoreFiles, filestoreFile) + filestoreFiles = append(filestoreFiles, os.NewFile(uintptr(filestoreFD), filestorePath)) return nil }) return filestoreFiles, err diff --git a/runsc/container/multi_container_test.go b/runsc/container/multi_container_test.go index 942d8cd59..4fa717d5f 100644 --- a/runsc/container/multi_container_test.go +++ b/runsc/container/multi_container_test.go @@ -2274,6 +2274,7 @@ func TestMultiContainerOverlayLeaks(t *testing.T) { } defer cleanup() + sandboxID := conts[0].Sandbox.ID for i, c := range conts { if i == 0 { // Don't wait for the root container which just sleeps. @@ -2295,30 +2296,8 @@ func TestMultiContainerOverlayLeaks(t *testing.T) { continue } - // Fetch the overlay filestore directory. - filestoreDir := conts[i].SelfOverlayFilestoreDir(s.Root.Path) - dirFD, err := os.Open(filestoreDir) - if err != nil { - t.Fatalf("os.Open(%q) failed for filestore dir: %v", filestoreDir, err) - } - - // Find the filestore file. - names, err := dirFD.Readdirnames(0) - if err != nil { - t.Fatalf("dirFD.Readdirnames(0) failed for filestore dir: %v", err) - } - filestoreFile := "" - for _, name := range names { - if strings.HasPrefix(name, "filestore") { - filestoreFile = path.Join(filestoreDir, name) - break - } - } - if filestoreFile == "" { - t.Fatalf("could not find filestore file in root directory entries: %v", names) - } - // Stat filestoreFile to see its usage. It should have been cleaned up. + filestoreFile := boot.SelfOverlayFilestorePath(s.Root.Path, sandboxID) var stat unix.Stat_t if err := unix.Stat(filestoreFile, &stat); err != nil { t.Errorf("unix.Stat(%q) failed for rootfs filestore: %v", filestoreFile, err) diff --git a/test/e2e/integration_runtime_test.go b/test/e2e/integration_runtime_test.go index 28157c384..0010197da 100644 --- a/test/e2e/integration_runtime_test.go +++ b/test/e2e/integration_runtime_test.go @@ -197,44 +197,6 @@ func TestOverlayNameTooLong(t *testing.T) { } } -// TestMultipleOverlayMounts tests having multiple overlay mounts works -// correctly when using host file backed overlays. All overlay mount should -// have their own MemoryFile backed different host files. -func TestMultipleOverlayMounts(t *testing.T) { - ctx := context.Background() - d := dockerutil.MakeContainerWithRuntime(ctx, t, "-overlay") - defer d.CleanUp(ctx) - - tmpDir := testutil.TmpDir() - opts := dockerutil.RunOpts{ - Image: "basic/ubuntu", - Mounts: []mount.Mount{ - { - Type: mount.TypeBind, - Source: tmpDir, - Target: "/submount1", - }, - { - Type: mount.TypeBind, - Source: tmpDir, - Target: "/submount2", - }, - }, - } - if got, err := d.Run(ctx, opts, "bash", "-c", "echo one > /submount1/file && echo two > /submount2/file && grep -Fxq one /submount1/file && grep -Fxq two /submount2/file && echo success"); err != nil { - t.Fatalf("docker run failed: %v", err) - } else if want := "success"; !strings.Contains(got, want) { - t.Errorf("container output %q does not contain %q", got, want) - } - - // Ensure overlay was applied to both bind mounts and no changes were made - // to the host filesystem. - filePath := filepath.Join(tmpDir, "file") - if _, err := os.Stat(filePath); !os.IsNotExist(err) { - t.Errorf("overlay not applied to both bind mounts, %q file exists", filePath) - } -} - // Tests that the overlay backing host file inside the container's rootfs is // hidden from the application. func TestOverlayRootfsWhiteout(t *testing.T) { @@ -245,9 +207,9 @@ func TestOverlayRootfsWhiteout(t *testing.T) { opts := dockerutil.RunOpts{ Image: "basic/ubuntu", } - if got, err := d.Run(ctx, opts, "bash", "-c", fmt.Sprintf("ls -al / | grep %q || true", boot.SelfOverlayFilestoreDirPrefix)); err != nil { + if got, err := d.Run(ctx, opts, "bash", "-c", fmt.Sprintf("ls -al / | grep %q || true", boot.SelfOverlayFilestorePrefix)); err != nil { t.Fatalf("docker run failed: %s, %v", got, err) } else if got != "" { - t.Errorf("root directory contains a file/directory whose name contains %q: output = %q", boot.SelfOverlayFilestoreDirPrefix, got) + t.Errorf("root directory contains a file/directory whose name contains %q: output = %q", boot.SelfOverlayFilestorePrefix, got) } }