From db59489b680104319541b0614c30229c8fa0270f Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 11 Nov 2024 23:12:32 -0800 Subject: [PATCH 1/3] runc delete: fix for rootless cgroup + ro cgroupfs An issue with runc 1.2.0 was reported to buildkit, in which runc delete returns with an error, with the log saying: > unable to destroy container: unable to remove container's cgroup: open /sys/fs/cgroup/snschvixiy3s74w74fjantrdg: no such file or directory Apparently, what happens is runc is running with no cgroup access (because /sys/fs/cgroup is mounted read-only). In this case error to create a cgroup path (in runc create/run) is ignored, but cgroup removal (in runc delete) is not. This is caused by commit d3d7f7d, which changes the cgroup removal logic in RemovePath. In the current code, if the initial rmdir has failed (in this case with EROFS), but the subsequent os.ReadDir returns ENOENT, it is returned (instead of being ignored -- as the path does not exist and so there is nothing to remove). Here is the minimal fix for the issue. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/utils.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index a05945cba6c..f699ed709a6 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -257,7 +257,10 @@ func RemovePath(path string) error { } infos, err := os.ReadDir(path) - if err != nil && !os.IsNotExist(err) { + if err != nil { + if os.IsNotExist(err) { + return nil + } return err } for _, info := range infos { From 12e06a7c4f081fc6fb4347741bf054f7ee7c256b Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 11 Nov 2024 23:13:27 -0800 Subject: [PATCH 2/3] libct/cg: RemovePath: simplify logic If the sub-cgroup RemovePath has failed for any reason, return the error right away. This way, we don't have to check for err != nil before retrying rmdir. This is a cosmetic change and should not change any functionality. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/utils.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index f699ed709a6..e3605856568 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -267,14 +267,11 @@ func RemovePath(path string) error { if info.IsDir() { // We should remove subcgroup first. if err = RemovePath(filepath.Join(path, info.Name())); err != nil { - break + return err } } } - if err == nil { - err = rmdir(path, true) - } - return err + return rmdir(path, true) } // RemovePaths iterates over the provided paths removing them. From ba3d026e5267c3b456ad946cc30877e303479f84 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 11 Nov 2024 23:17:42 -0800 Subject: [PATCH 3/3] libct/cg: RemovePath: improve comments Let's explain in greater details what's happening here and why. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/utils.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index e3605856568..d404647c8c0 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -251,11 +251,21 @@ again: // RemovePath aims to remove cgroup path. It does so recursively, // by removing any subdirectories (sub-cgroups) first. func RemovePath(path string) error { - // Try the fast path first. + // Try the fast path first; don't retry on EBUSY yet. if err := rmdir(path, false); err == nil { return nil } + // There are many reasons why rmdir can fail, including: + // 1. cgroup have existing sub-cgroups; + // 2. cgroup (still) have some processes (that are about to vanish); + // 3. lack of permission (one example is read-only /sys/fs/cgroup mount, + // in which case rmdir returns EROFS even for for a non-existent path, + // see issue 4518). + // + // Using os.ReadDir here kills two birds with one stone: check if + // the directory exists (handling scenario 3 above), and use + // directory contents to remove sub-cgroups (handling scenario 1). infos, err := os.ReadDir(path) if err != nil { if os.IsNotExist(err) { @@ -263,14 +273,16 @@ func RemovePath(path string) error { } return err } + // Let's remove sub-cgroups, if any. for _, info := range infos { if info.IsDir() { - // We should remove subcgroup first. if err = RemovePath(filepath.Join(path, info.Name())); err != nil { return err } } } + // Finally, try rmdir again, this time with retries on EBUSY, + // which may help with scenario 2 above. return rmdir(path, true) }