From 017d6b693f9a8bfc64f9ba2afa9526b47e03c871 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 13 May 2025 19:46:28 -0700 Subject: [PATCH 1/4] criu: improve prepareCriuRestoreMounts 1. Replace the big "if !" block with the if block and continue, simplifying the code flow. 2. Move comments closer to the code, improving readability. This commit is best reviewed with --ignore-all-space or similar. Signed-off-by: Kir Kolyshkin (cherry picked from commit 0c93d41c65b6a1055e945d1d3e56943b07b8405b) Signed-off-by: Kir Kolyshkin --- libcontainer/criu_linux.go | 45 +++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/libcontainer/criu_linux.go b/libcontainer/criu_linux.go index 8cd8fa5ae..86de1c2d4 100644 --- a/libcontainer/criu_linux.go +++ b/libcontainer/criu_linux.go @@ -572,9 +572,6 @@ func (c *Container) prepareCriuRestoreMounts(mounts []*configs.Mount) error { tmpfs = append(tmpfs, m.Destination) } } - // Now go through all mounts and create the mountpoints - // if the mountpoints are not on a tmpfs, as CRIU will - // restore the complete tmpfs content from its checkpoint. umounts := []string{} defer func() { for _, u := range umounts { @@ -590,28 +587,32 @@ func (c *Container) prepareCriuRestoreMounts(mounts []*configs.Mount) error { }) } }() + // Now go through all mounts and create the required mountpoints. for _, m := range mounts { - if !isPathInPrefixList(m.Destination, tmpfs) { - if err := c.makeCriuRestoreMountpoints(m); err != nil { + // If the mountpoint is on a tmpfs, skip it as CRIU will + // restore the complete tmpfs content from its checkpoint. + if isPathInPrefixList(m.Destination, tmpfs) { + continue + } + if err := c.makeCriuRestoreMountpoints(m); err != nil { + return err + } + // If the mount point is a bind mount, we need to mount + // it now so that runc can create the necessary mount + // points for mounts in bind mounts. + // This also happens during initial container creation. + // Without this CRIU restore will fail + // See: https://github.com/opencontainers/runc/issues/2748 + // It is also not necessary to order the mount points + // because during initial container creation mounts are + // set up in the order they are configured. + if m.Device == "bind" { + if err := utils.WithProcfd(c.config.Rootfs, m.Destination, func(dstFd string) error { + return mountViaFds(m.Source, nil, m.Destination, dstFd, "", unix.MS_BIND|unix.MS_REC, "") + }); err != nil { return err } - // If the mount point is a bind mount, we need to mount - // it now so that runc can create the necessary mount - // points for mounts in bind mounts. - // This also happens during initial container creation. - // Without this CRIU restore will fail - // See: https://github.com/opencontainers/runc/issues/2748 - // It is also not necessary to order the mount points - // because during initial container creation mounts are - // set up in the order they are configured. - if m.Device == "bind" { - if err := utils.WithProcfd(c.config.Rootfs, m.Destination, func(dstFd string) error { - return mountViaFds(m.Source, nil, m.Destination, dstFd, "", unix.MS_BIND|unix.MS_REC, "") - }); err != nil { - return err - } - umounts = append(umounts, m.Destination) - } + umounts = append(umounts, m.Destination) } } return nil From a97c49f96ed7d18ae721da86661d92fc30d522ee Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 13 May 2025 19:50:20 -0700 Subject: [PATCH 2/4] criu: ignore cgroup early in prepareCriuRestoreMounts It makes sense to ignore cgroup mounts much early in the code, saving some time on unnecessary operations. Signed-off-by: Kir Kolyshkin (cherry picked from commit b8aa5481db42b5222b1725e5af939bec829937c5) Signed-off-by: Kir Kolyshkin --- libcontainer/criu_linux.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/libcontainer/criu_linux.go b/libcontainer/criu_linux.go index 86de1c2d4..c96c2c42e 100644 --- a/libcontainer/criu_linux.go +++ b/libcontainer/criu_linux.go @@ -527,19 +527,8 @@ func (c *Container) restoreNetwork(req *criurpc.CriuReq, criuOpts *CriuOpts) { // restore using CRIU. This function is inspired from the code in // rootfs_linux.go. func (c *Container) makeCriuRestoreMountpoints(m *configs.Mount) error { - if m.Device == "cgroup" { - // No mount point(s) need to be created: - // - // * for v1, mount points are saved by CRIU because - // /sys/fs/cgroup is a tmpfs mount - // - // * for v2, /sys/fs/cgroup is a real mount, but - // the mountpoint appears as soon as /sys is mounted - return nil - } // TODO: pass srcFD? Not sure if criu is impacted by issue #2484. me := mountEntry{Mount: m} - // For all other filesystems, just make the target. if _, err := createMountpoint(c.config.Rootfs, me); err != nil { return fmt.Errorf("create criu restore mountpoint for %s mount: %w", me.Destination, err) } @@ -589,6 +578,14 @@ func (c *Container) prepareCriuRestoreMounts(mounts []*configs.Mount) error { }() // Now go through all mounts and create the required mountpoints. for _, m := range mounts { + // No cgroup mount point(s) need to be created: + // * for v1, mount points are saved by CRIU because + // /sys/fs/cgroup is a tmpfs mount; + // * for v2, /sys/fs/cgroup is a real mount, but + // the mountpoint appears as soon as /sys is mounted. + if m.Device == "cgroup" { + continue + } // If the mountpoint is on a tmpfs, skip it as CRIU will // restore the complete tmpfs content from its checkpoint. if isPathInPrefixList(m.Destination, tmpfs) { From 69a3439c31aabeb4e86c6c584736132863707b40 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 13 May 2025 19:52:18 -0700 Subject: [PATCH 3/4] criu: inline makeCriuRestoreMountpoints Since its code is now trivial, and it is only called from a single place, it does not make sense to have it as a separate function. Signed-off-by: Kir Kolyshkin (cherry picked from commit f91fbd34d9e819a833c7da00c6c88f5371a82ac5) Signed-off-by: Kir Kolyshkin --- libcontainer/criu_linux.go | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/libcontainer/criu_linux.go b/libcontainer/criu_linux.go index c96c2c42e..2af2f85b2 100644 --- a/libcontainer/criu_linux.go +++ b/libcontainer/criu_linux.go @@ -523,18 +523,6 @@ func (c *Container) restoreNetwork(req *criurpc.CriuReq, criuOpts *CriuOpts) { } } -// makeCriuRestoreMountpoints makes the actual mountpoints for the -// restore using CRIU. This function is inspired from the code in -// rootfs_linux.go. -func (c *Container) makeCriuRestoreMountpoints(m *configs.Mount) error { - // TODO: pass srcFD? Not sure if criu is impacted by issue #2484. - me := mountEntry{Mount: m} - if _, err := createMountpoint(c.config.Rootfs, me); err != nil { - return fmt.Errorf("create criu restore mountpoint for %s mount: %w", me.Destination, err) - } - return nil -} - // isPathInPrefixList is a small function for CRIU restore to make sure // mountpoints, which are on a tmpfs, are not created in the roofs. func isPathInPrefixList(path string, prefix []string) bool { @@ -591,8 +579,8 @@ func (c *Container) prepareCriuRestoreMounts(mounts []*configs.Mount) error { if isPathInPrefixList(m.Destination, tmpfs) { continue } - if err := c.makeCriuRestoreMountpoints(m); err != nil { - return err + if _, err := createMountpoint(c.config.Rootfs, mountEntry{Mount: m}); err != nil { + return fmt.Errorf("create criu restore mountpoint for %s mount: %w", m.Destination, err) } // If the mount point is a bind mount, we need to mount // it now so that runc can create the necessary mount From 02c412828817665cf008a40c5382486d8f0b7ce5 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 13 May 2025 20:00:06 -0700 Subject: [PATCH 4/4] criu: simplify isOnTmpfs check in prepareCriuRestoreMounts Instead of generating a list of tmpfs mount and have a special function to check whether the path is in the list, let's go over the list of mounts directly. This simplifies the code and improves readability. Signed-off-by: Kir Kolyshkin (cherry picked from commit ce3cd4234c9cd90f8109a33ab86f3456c2edf947) Signed-off-by: Kir Kolyshkin --- libcontainer/criu_linux.go | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/libcontainer/criu_linux.go b/libcontainer/criu_linux.go index 2af2f85b2..fd4b204a9 100644 --- a/libcontainer/criu_linux.go +++ b/libcontainer/criu_linux.go @@ -523,11 +523,9 @@ func (c *Container) restoreNetwork(req *criurpc.CriuReq, criuOpts *CriuOpts) { } } -// isPathInPrefixList is a small function for CRIU restore to make sure -// mountpoints, which are on a tmpfs, are not created in the roofs. -func isPathInPrefixList(path string, prefix []string) bool { - for _, p := range prefix { - if strings.HasPrefix(path, p+"/") { +func isOnTmpfs(path string, mounts []*configs.Mount) bool { + for _, m := range mounts { + if m.Device == "tmpfs" && strings.HasPrefix(path, m.Destination+"/") { return true } } @@ -541,14 +539,6 @@ func isPathInPrefixList(path string, prefix []string) bool { // This function also creates missing mountpoints as long as they // are not on top of a tmpfs, as CRIU will restore tmpfs content anyway. func (c *Container) prepareCriuRestoreMounts(mounts []*configs.Mount) error { - // First get a list of a all tmpfs mounts - tmpfs := []string{} - for _, m := range mounts { - switch m.Device { - case "tmpfs": - tmpfs = append(tmpfs, m.Destination) - } - } umounts := []string{} defer func() { for _, u := range umounts { @@ -576,7 +566,7 @@ func (c *Container) prepareCriuRestoreMounts(mounts []*configs.Mount) error { } // If the mountpoint is on a tmpfs, skip it as CRIU will // restore the complete tmpfs content from its checkpoint. - if isPathInPrefixList(m.Destination, tmpfs) { + if isOnTmpfs(m.Destination, mounts) { continue } if _, err := createMountpoint(c.config.Rootfs, mountEntry{Mount: m}); err != nil {