don't panic when /sys/fs/cgroup is missing for rootless#2634
don't panic when /sys/fs/cgroup is missing for rootless#2634cyphar merged 1 commit intoopencontainers:masterfrom
Conversation
| var st unix.Statfs_t | ||
| if err := unix.Statfs(unifiedMountpoint, &st); err != nil { | ||
| panic("cannot statfs cgroup root") | ||
| logrus.WithError(err).Debug("cannot statfs cgroup root") |
There was a problem hiding this comment.
- It should be
InfonotDebugI think as it's pretty major. - Amend the message to say "assuming cgroup v1".
There was a problem hiding this comment.
- Maybe add a TODO item to change the default to cgroupv2 somewhere in 2024 :)
There was a problem hiding this comment.
yeah, this is partially why I am leaning towards changing the signature to return an error, otherwise users will get this message 100% of the time exec'ing to a container via buildkit with rootless.
There was a problem hiding this comment.
Does something like this seem more reasonable to you?
err := unix.Statfs(unifiedMountpoint, &st)
if err != nil && os.IsNotExist(err) {
// /sys/fs/cgroup not found, likely rootless
logrus.WithError(err).Debugf("%s missing, assuming cgroup v1", unifiedMountpoint)
isUnified = false
return
} else if err != nil {
panic(fmt.Sprintf("cannot statfs cgroup root: %s", err))
}
This would preserve the panic behavior for all error scenarios except where the /sys/fs/cgroup path simply does not exist (which is a normal situation under rootless)
There was a problem hiding this comment.
Also better to panic when os.IsNotExist && !system.RunningInUserNS()
There was a problem hiding this comment.
updated the check to only ignore (w/ debug msg) errors for NotExists while RunningInUserNS
|
@AkihiroSuda PTAL |
coryb
force-pushed
the
issue-2573
branch
2 times, most recently
from
92a85a0 to
4a75408
Compare
|
The test failure (timeout) seems unrelated to my change, and Travis is not allowing me to retry the build. |
Signed-off-by: Cory Bennett <[email protected]>
|
I squashed and forced push, tests passed after the push this time. I think I have addressed all the feedback, so hopefully we can get this merged in. |
|
ping @kolyshkin @cyphar @mrunalp |
|
@kolyshkin @cyphar @mrunalp PTAL |
4 participants
Fixes issue #2573
Downgrade the panic to a debug message when /sys/fs/cgroup is missing. This issue is preventing runc exec from working within buildkit when running rootless (where /sys is not mounted at all).
Steps to reproduce:
now in separate shell:
A potentially better solution is to change the signature from:
to:
so that we can manage the error more directly in rootless mode, but the diff for this change would be much larger, since the function is used quite a lot. I can update is this direction is preferred.