From 4024c736aec122c8d5a57ae80361c225e96395d3 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Wed, 18 Oct 2023 11:13:06 -0700 Subject: [PATCH] Create shared mounts on demand. Earlier we were creating them upfront when the root container started. However, later some of these shared mounts might require additional configuration that is more easily available when the sub-container using that shared mount is being started. So move to an on-demand model. This also has the benefit that mounts will not be created until needed (hence resource utilization will be more optimal). Consequently, failures in setting up shared mounts will not cause the sandbox from failing to start up. It will instead cause the impacted containers from failing to start up. PiperOrigin-RevId: 574533960 --- runsc/boot/loader.go | 25 +++++---- runsc/boot/loader_test.go | 4 -- runsc/boot/vfs.go | 106 ++++++++++++++++---------------------- 3 files changed, 55 insertions(+), 80 deletions(-) diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go index 6cb3cd282..04b64fc58 100644 --- a/runsc/boot/loader.go +++ b/runsc/boot/loader.go @@ -159,10 +159,6 @@ type Loader struct { // apply to the entire pod. mountHints *PodMountHints - // sharedMountKey holds VFS mounts that may be shared between containers - // within the same pod. It is mapped by mount source. - sharedMounts map[string]*vfs.Mount - // productName is the value to show in // /sys/devices/virtual/dmi/id/product_name. productName string @@ -170,9 +166,13 @@ type Loader struct { // nvidiaUVMDevMajor is the device major number used for nvidia-uvm. nvidiaUVMDevMajor uint32 - // mu guards processes and porForwardProxies. + // mu guards the fields below. mu sync.Mutex + // sharedMounts holds VFS mounts that may be shared between containers within + // the same pod. It is mapped by mount source. + sharedMounts map[string]*vfs.Mount + // processes maps containers init process and invocation of exec. Root // processes are keyed with container ID and pid=0, while exec invocations // have the corresponding pid set. @@ -508,6 +508,7 @@ func New(args Args) (*Loader, error) { sandboxID: args.ID, processes: map[execID]*execProcess{eid: {}}, mountHints: mountHints, + sharedMounts: make(map[string]*vfs.Mount), root: info, stopProfiling: stopProfiling, productName: args.ProductName, @@ -720,7 +721,7 @@ func (l *Loader) run() error { tg *kernel.ThreadGroup err error ) - tg, ep.tty, err = l.createContainerProcess(true, l.sandboxID, &l.root) + tg, ep.tty, err = l.createContainerProcess(l.sandboxID, &l.root) if err != nil { return err } @@ -873,7 +874,7 @@ func (l *Loader) startSubcontainer(spec *specs.Spec, conf *config.Config, cid st info.stdioFDs = stdioFDs } - ep.tg, ep.tty, err = l.createContainerProcess(false, cid, info) + ep.tg, ep.tty, err = l.createContainerProcess(cid, info) if err != nil { return err } @@ -902,7 +903,8 @@ func (l *Loader) startSubcontainer(spec *specs.Spec, conf *config.Config, cid st return nil } -func (l *Loader) createContainerProcess(root bool, cid string, info *containerInfo) (*kernel.ThreadGroup, *host.TTYFileDescription, error) { +// +checklocks:l.mu +func (l *Loader) createContainerProcess(cid string, info *containerInfo) (*kernel.ThreadGroup, *host.TTYFileDescription, error) { // Create the FD map, which will set stdin, stdout, and stderr. ctx := info.procArgs.NewContext(l.k) fdTable, ttyFile, err := createFDTable(ctx, info.spec.Process.Terminal, info.stdioFDs, info.passFDs, info.spec.Process.User) @@ -939,11 +941,8 @@ func (l *Loader) createContainerProcess(root bool, cid string, info *containerIn } l.startGoferMonitor(cid, int32(info.goferFDs[0].FD())) - if root { - if err := l.processHints(info.conf, info.procArgs.Credentials); err != nil { - return nil, nil, err - } - } + // We can share l.sharedMounts with containerMounter since l.mu is locked. + // Hence, mntr must only be used within this function (while l.mu is locked). mntr := newContainerMounter(info, l.k, l.mountHints, l.sharedMounts, l.productName, l.sandboxID) if err := setupContainerVFS(ctx, info, mntr, &info.procArgs); err != nil { return nil, nil, err diff --git a/runsc/boot/loader_test.go b/runsc/boot/loader_test.go index 304b1c156..395513727 100644 --- a/runsc/boot/loader_test.go +++ b/runsc/boot/loader_test.go @@ -475,11 +475,7 @@ func TestCreateMountNamespace(t *testing.T) { defer l.Destroy() defer loaderCleanup() - if err := l.processHints(l.root.conf, l.root.procArgs.Credentials); err != nil { - t.Fatalf("failed process hints: %v", err) - } mntr := newContainerMounter(&l.root, l.k, l.mountHints, l.sharedMounts, "", l.sandboxID) - ctx := l.k.SupervisorContext() creds := auth.NewRootCredentials(l.root.procArgs.Credentials.UserNamespace) mns, err := mntr.mountAll(ctx, creds, l.root.conf, &l.root.procArgs) diff --git a/runsc/boot/vfs.go b/runsc/boot/vfs.go index 84b157bf5..1e56dd198 100644 --- a/runsc/boot/vfs.go +++ b/runsc/boot/vfs.go @@ -678,11 +678,11 @@ func (c *containerMounter) mountSubmounts(ctx context.Context, conf *config.Conf ) if submount.hint != nil && submount.hint.ShouldShareMount() { - sharedMount, ok := c.sharedMounts[submount.hint.Mount.Source] - if !ok { - return fmt.Errorf("shared mount %q not found", submount.hint.Name) + sharedMount, err := c.getSharedMount(ctx, conf, submount, creds) + if err != nil { + return fmt.Errorf("getting shared mount %q: %w", submount.hint.Name, err) } - mnt, err = c.mountSharedSubmount(ctx, conf, mns, creds, submount.mount, submount.hint, sharedMount) + mnt, err = c.mountSharedSubmount(ctx, conf, mns, creds, submount, sharedMount) if err != nil { return fmt.Errorf("mount shared mount %q to %q: %v", submount.hint.Name, submount.mount.Destination, err) } @@ -715,16 +715,12 @@ func (c *containerMounter) mountSubmounts(ctx context.Context, conf *config.Conf type mountInfo struct { mount *specs.Mount - goferFD int + goferFD *fd.FD hint *MountHint goferMountConf GoferMountConf filestoreFD *fd.FD } -func newNonGoferMountInfo(mount *specs.Mount) *mountInfo { - return &mountInfo{mount: mount, goferFD: -1} -} - func (c *containerMounter) prepareMounts() ([]mountInfo, error) { // Associate bind mounts with their FDs before sorting since there is an // undocumented assumption that FDs are dispensed in the order in which @@ -738,12 +734,11 @@ func (c *containerMounter) prepareMounts() ([]mountInfo, error) { // Only bind mounts use host FDs; see // containerMounter.getMountNameAndOptions. info := mountInfo{ - mount: m, - goferFD: -1, - hint: c.hints.FindMount(m.Source), + mount: m, + hint: c.hints.FindMount(m.Source), } if specutils.IsGoferMount(*m) { - info.goferFD = c.goferFDs.remove() + info.goferFD = c.goferFDs.removeAsFD() info.goferMountConf = c.goferMountConfs[goferMntIdx] if info.goferMountConf.IsFilestorePresent() { info.filestoreFD = c.goferFilestoreFDs.removeAsFD() @@ -837,11 +832,11 @@ func getMountNameAndOptions(conf *config.Config, m *mountInfo, productName strin case Bind: fsName = gofer.Name - if m.goferFD < 0 { + if m.goferFD == nil { // Check that an FD was provided to fails fast. return "", nil, fmt.Errorf("gofer mount requires a connection FD") } - data = goferMountData(m.goferFD, getMountAccessType(conf, m.hint), conf) + data = goferMountData(m.goferFD.Release(), getMountAccessType(conf, m.hint), conf) internalData = gofer.InternalFilesystemOptions{ UniqueID: m.mount.Destination, } @@ -959,7 +954,7 @@ func (c *containerMounter) mountTmp(ctx context.Context, conf *config.Config, cr // another user. This is normally done for /tmp. Options: []string{"mode=01777"}, } - if _, err := c.mountSubmount(ctx, conf, mns, creds, newNonGoferMountInfo(&tmpMount)); err != nil { + if _, err := c.mountSubmount(ctx, conf, mns, creds, &mountInfo{mount: &tmpMount}); err != nil { return fmt.Errorf("mountSubmount failed: %v", err) } return nil @@ -973,63 +968,48 @@ func (c *containerMounter) mountTmp(ctx context.Context, conf *config.Config, cr } } -// processHints processes annotations that container hints about how volumes -// should be mounted (e.g. a volume shared between containers). -// Precondition: Must be only called once during the loader sequence -// for the root container. -// Postcondition: Initialized l.sharedMounts on success. -func (l *Loader) processHints(conf *config.Config, creds *auth.Credentials) error { - ctx := l.k.SupervisorContext() - var sharedMounts map[string]*vfs.Mount - for _, hint := range l.mountHints.Mounts { - if !hint.ShouldShareMount() { - continue - } - - log.Infof("Mounting master of shared mount %q from %q type %q", hint.Name, hint.Mount.Source, hint.Mount.Type) - mnt, err := l.mountSharedMaster(ctx, conf, hint, creds) - if err != nil { - return fmt.Errorf("mounting shared master %q: %v", hint.Name, err) - } - if sharedMounts == nil { - sharedMounts = make(map[string]*vfs.Mount) - } - sharedMounts[hint.Mount.Source] = mnt +func (c *containerMounter) getSharedMount(ctx context.Context, conf *config.Config, mount *mountInfo, creds *auth.Credentials) (*vfs.Mount, error) { + sharedMount, ok := c.sharedMounts[mount.hint.Mount.Source] + if ok { + log.Infof("Using existing shared mount %q from %q type %q", mount.hint.Name, mount.hint.Mount.Source, mount.hint.Mount.Type) + return sharedMount, nil } - l.sharedMounts = sharedMounts - return nil + log.Infof("Mounting master of shared mount %q from %q type %q", mount.hint.Name, mount.hint.Mount.Source, mount.hint.Mount.Type) + sharedMount, err := c.mountSharedMaster(ctx, conf, mount, creds) + if err != nil { + return nil, fmt.Errorf("mounting shared master %q: %v", mount.hint.Name, err) + } + c.sharedMounts[mount.hint.Mount.Source] = sharedMount + return sharedMount, nil } // mountSharedMaster mounts the master of a volume that is shared among // containers in a pod. -func (l *Loader) mountSharedMaster(ctx context.Context, conf *config.Config, hint *MountHint, creds *auth.Credentials) (*vfs.Mount, error) { - // Map mount type to filesystem name, and parse out the options that we are - // capable of dealing with. - mntInfo := newNonGoferMountInfo(&hint.Mount) - fsName, opts, err := getMountNameAndOptions(conf, mntInfo, l.productName) +func (c *containerMounter) mountSharedMaster(ctx context.Context, conf *config.Config, mntInfo *mountInfo, creds *auth.Credentials) (*vfs.Mount, error) { + // Mount the master using the options from the hint (mount annotations). + origOpts := mntInfo.mount.Options + mntInfo.mount.Options = mntInfo.hint.Mount.Options + fsName, opts, err := getMountNameAndOptions(conf, mntInfo, c.productName) + mntInfo.mount.Options = origOpts if err != nil { return nil, err } if len(fsName) == 0 { - return nil, fmt.Errorf("mount type not supported %q", hint.Mount.Type) + return nil, fmt.Errorf("mount type not supported %q", mntInfo.hint.Mount.Type) } - return l.k.VFS().MountDisconnected(ctx, creds, "", fsName, opts) + return c.k.VFS().MountDisconnected(ctx, creds, "", fsName, opts) } // mountSharedSubmount binds mount to a previously mounted volume that is shared // among containers in the same pod. -func (c *containerMounter) mountSharedSubmount(ctx context.Context, conf *config.Config, mns *vfs.MountNamespace, creds *auth.Credentials, mount *specs.Mount, srcHint *MountHint, srcMount *vfs.Mount) (*vfs.Mount, error) { - if err := srcHint.checkCompatible(mount); err != nil { +func (c *containerMounter) mountSharedSubmount(ctx context.Context, conf *config.Config, mns *vfs.MountNamespace, creds *auth.Credentials, mntInfo *mountInfo, sharedMount *vfs.Mount) (*vfs.Mount, error) { + if err := mntInfo.hint.checkCompatible(mntInfo.mount); err != nil { return nil, err } - // Ignore data and useOverlay because these were already applied to - // the master mount. - _, opts, err := getMountNameAndOptions(conf, newNonGoferMountInfo(mount), c.productName) - if err != nil { - return nil, err - } - newMnt := c.k.VFS().NewDisconnectedMount(srcMount.Filesystem(), srcMount.Root(), opts) + // Generate mount point specific opts using mntInfo.mount. + opts := ParseMountOptions(mntInfo.mount.Options) + newMnt := c.k.VFS().NewDisconnectedMount(sharedMount.Filesystem(), sharedMount.Root(), opts) defer newMnt.DecRef(ctx) root := mns.Root(ctx) @@ -1037,17 +1017,17 @@ func (c *containerMounter) mountSharedSubmount(ctx context.Context, conf *config target := &vfs.PathOperation{ Root: root, Start: root, - Path: fspath.Parse(mount.Destination), + Path: fspath.Parse(mntInfo.mount.Destination), } - if err := c.makeMountPoint(ctx, creds, mns, mount.Destination); err != nil { - return nil, fmt.Errorf("creating mount point %q: %w", mount.Destination, err) + if err := c.makeMountPoint(ctx, creds, mns, mntInfo.mount.Destination); err != nil { + return nil, fmt.Errorf("creating mount point %q: %w", mntInfo.mount.Destination, err) } if err := c.k.VFS().ConnectMountAt(ctx, creds, newMnt, target); err != nil { return nil, err } - log.Infof("Mounted %q type shared bind to %q", mount.Destination, srcHint.Name) + log.Infof("Mounted %q type shared bind to %q", mntInfo.mount.Destination, mntInfo.hint.Name) return newMnt, nil } @@ -1080,10 +1060,10 @@ func (c *containerMounter) configureRestore(ctx context.Context) (context.Contex if err != nil { return ctx, err } - for i := range c.mounts { + for i := range mounts { submount := &mounts[i] - if submount.goferFD >= 0 { - fdmap[submount.mount.Destination] = submount.goferFD + if submount.goferFD != nil { + fdmap[submount.mount.Destination] = submount.goferFD.Release() } } return context.WithValue(ctx, gofer.CtxRestoreServerFDMap, fdmap), nil