Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Running container is reported as stopped when cgroup v1 is used and freezer is disabled #1612

Closed
michalsieron opened this issue Dec 4, 2024 · 7 comments · Fixed by #1616
Closed

Comments

@michalsieron
Copy link
Contributor

Environment

  • crun: any
  • OS: Ubuntu 24.04, but any with systemd will do
  • Linux boot params: systemd.unified_cgroup_hierarchy=0 cgroup_no_v1=freezer

Steps to reproduce

  1. Boot Linux with systemd.unified_cgroup_hierarchy=0 cgroup_no_v1=freezer params
  2. Prepare minimal config.json:
{
	"ociVersion": "1.0.0",
	"process": {
		"user": { "uid": 0, "gid": 0 },
		"args": [ "/busybox", "sleep", "1000" ],
		"cwd": "/"
	},
	"root": { "path": "rootfs", "readonly": true },
	"hostname": "crun",
	"mounts": [
		{
			"destination": "/proc",
			"type": "proc",
			"source": "proc",
			"options": [
				"nosuid",
				"noexec",
				"nodev"
			]
		},
		{
			"destination": "/dev",
			"type": "tmpfs",
			"source": "tmpfs",
			"options": [
				"nosuid",
				"strictatime",
				"mode=755",
				"size=65536k"
			]
		}
	],
	"linux": {
		"resources": { },
		"namespaces": [ { "type": "uts" } ]
	}
}
  1. mkdir rootfs and put there a static busybox binary (I took one from EXALAB/Busybox-static)
  2. Run sudo crun --systemd-cgroup run -d test1234
  3. Run sudo crun state test1234

Expected results: Status of the test1234 container is running
Actual results: Status of the test1234 container is stopped

Notes

Even though crun state reports the container is stopped, crun ps confirms that there is a process running inside. Trying to use crun start on such a container results in an error about missing exec.fifo file.

The issue comes from this interpretation of the missing freezer.state file, which was originally merged in #474:

crun/src/libcrun/container.c

Lines 3130 to 3146 in 52ed588

ret = libcrun_cgroup_is_container_paused (cgroup_status, &paused, err);
if (UNLIKELY (ret < 0))
{
/*
The cgroup might have been cleaned up by the time we try to read it, ignore both
ENOENT and ENODEV:
- ENOENT: if the open(CGROUP_PATH) fails because the cgroup was deleted.
- ENODEV: if the cgroup is deleted between the open and reading the freeze status.
*/
errno = crun_error_get_errno (err);
if (errno == ENOENT || errno == ENODEV)
{
crun_error_release (err);
*container_status = "stopped";
return 0;
}

When freezer is disabled, the freezer.state files don't exist and containers simply cannot be paused/resumed.

While writing this, I checked also with --cgroup-manager=cgroupfs and it reproduces as well. Only --cgroup-manager=disabled doesn't reproduce.

runc doesn't have this problem. I think it doesn't take race condition into consideration and when the freezer.state file is missing it just interprets it as container not being paused.

Proposed solutions

  1. Check if freezer is enabled at all and use that information to decide what missing freezer.state file means (or simply to exit early from "is container paused" check)
  2. Somehow reorganize the logic to run "is container running" check after "is container paused" check?
@giuseppe
Copy link
Member

giuseppe commented Dec 5, 2024

since you've already analyzed the root cause, would you mind opening a PR to fix it?

I think what we can do is to do a check access (CGROUP_ROOT "/freezer", X_OK) on ENOENT, and don't return stopped when that path is missing,

@michalsieron
Copy link
Contributor Author

The problem is that for some reason, that one path does exist, but there is nothing inside...
I am not sure why that one exists even with freezer disabled.
(I am assuming in my case access (CGROUP_ROOT "/freezer", X_OK) would check /sys/fs/cgroup/freezer)

@michalsieron
Copy link
Contributor Author

ok, I ran stat -f /sys/fs/cgroup/freezer/ and I see Type: tmpfs. With freezer enabled, it is Type: cgroupfs, so this kind of check could work. I will try to prepare PR soon.

@michalsieron
Copy link
Contributor Author

So far, I have something like this:

diff --git a/src/libcrun/cgroup.c b/src/libcrun/cgroup.c
index 4a9497d..a984f70 100644
--- a/src/libcrun/cgroup.c
+++ b/src/libcrun/cgroup.c
@@ -37,6 +37,7 @@
 #include <inttypes.h>
 #include <time.h>

+#include <linux/magic.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <fcntl.h>
@@ -173,7 +174,22 @@ libcrun_cgroup_is_container_paused (struct libcrun_cgroup_status *status, bool *

   ret = read_all_file (path, &content, NULL, err);
   if (UNLIKELY (ret < 0))
-    return ret;
+    {
+      errno = crun_error_get_errno (err);
+      if (errno != ENOENT || cgroup_mode == CGROUP_MODE_UNIFIED)
+        return ret;
+
+      struct statfs freezer_stat;
+      if ( statfs (CGROUP_ROOT "/freezer", &freezer_stat))
+          return crun_make_error (err, errno, "error getting stats of `%s`", CGROUP_ROOT "/freezer");
+
+      if (freezer_stat.f_type == CGROUP_SUPER_MAGIC)
+        return ret;
+
+      crun_error_release (err);
+      *paused = false;
+      return 0;
+    }

   *paused = strstr (content, state) != NULL;
   return 0;

I will need to add some comments and change error text, but the logic is more or less there. AFAIK, cgroupv2 cannot exist without the freeze ability, so no need to check that case. Once I do that, and I have a proper commit message, I will create the PR @giuseppe

@giuseppe
Copy link
Member

giuseppe commented Dec 5, 2024

thanks, the patch looks good to me

@rhatdan
Copy link
Member

rhatdan commented Dec 5, 2024

Please open a PR?

@giuseppe
Copy link
Member

giuseppe commented Dec 6, 2024

@michalsieron I am going to cut a new release today. If you hurry to open a PR, I'll merge your patch

michalsieron added a commit to michalsieron/crun that referenced this issue Dec 6, 2024
On cgroup v1 it is possible to disable freezer subsystem. In such case
freezer.state file won't be present. Due to the race condition handling
in libcrun_get_container_state_string, missing freezer.state would be
interpreted as cgroup being removed when check is being performed.

But as indicated earlier, that is not the case when it's cgroup v1 and
the freezer is disabled. Therefore introduce logic that checks for that
using type of the filesystem mounted under the freezer directory.

When freezer is disabled, container simply cannot be paused.

Fixes containers#1612

Signed-off-by: Michal Sieron <michalwsieron@gmail.com>
michalsieron added a commit to michalsieron/crun that referenced this issue Dec 6, 2024
On cgroup v1 it is possible to disable freezer subsystem. In such case
freezer.state file won't be present. Due to the race condition handling
in libcrun_get_container_state_string, missing freezer.state would be
interpreted as cgroup being removed when check is being performed.

But as indicated earlier, that is not the case when it's cgroup v1 and
the freezer is disabled. Therefore introduce logic that checks for that
using type of the filesystem mounted under the freezer directory.

When freezer is disabled, container simply cannot be paused.

Fixes containers#1612

Signed-off-by: Michal Sieron <michalwsieron@gmail.com>
michalsieron added a commit to michalsieron/crun that referenced this issue Dec 6, 2024
On cgroup v1 it is possible to disable freezer subsystem. In such case
freezer.state file won't be present. Due to the race condition handling
in libcrun_get_container_state_string, missing freezer.state would be
interpreted as cgroup being removed when check is being performed.

But as indicated earlier, that is not the case when it's cgroup v1 and
the freezer is disabled. Therefore introduce logic that checks for that
using type of the filesystem mounted under the freezer directory.

When freezer is disabled, container simply cannot be paused.

Fixes containers#1612

Signed-off-by: Michal Sieron <michalwsieron@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants