From 0c93d41c65b6a1055e945d1d3e56943b07b8405b 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 --- 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 dbeb722a2..1629ba5fc 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 b8aa5481db42b5222b1725e5af939bec829937c5 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 --- 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 1629ba5fc..af7e2eed2 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 f91fbd34d9e819a833c7da00c6c88f5371a82ac5 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 --- 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 af7e2eed2..92a93e75f 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 ce3cd4234c9cd90f8109a33ab86f3456c2edf947 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 --- 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 92a93e75f..a86e53fd7 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 {