-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[1.1] libct/cg/sd: use systemd version when generating dev props #3845
Merged
AkihiroSuda
merged 1 commit into
opencontainers:release-1.1
from
kolyshkin:1.1-rm-warning
Apr 26, 2023
Merged
[1.1] libct/cg/sd: use systemd version when generating dev props #3845
AkihiroSuda
merged 1 commit into
opencontainers:release-1.1
from
kolyshkin:1.1-rm-warning
Apr 26, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Commit 343951a added a call to os.Stat for the device path when generating systemd device properties, to avoid systemd warning for non-existing devices. The idea was, since systemd uses stat(2) to look up device properties for a given path, it will fail anyway. In addition, this allowed to suppress a warning like this from systemd: > Couldn't stat device /dev/char/10:200 NOTE that this was done because: - systemd could not add the rule anyway; - runs puts its own set of rules on top of what systemd does. Apparently, the above change broke some setups, resulting in inability to use e.g. /dev/null inside a container. My guess is this is because in cgroup v2 we add a second eBPF program, which is not used if the first one (added by systemd) returns "access denied". Next, commit 3b95828 fixed that by adding a call to os.Stat for "/sys/"+path (meaning, if "/dev/char/10:200" does not exist, we retry with "/sys/dev/char/10:200", and if it exists, proceed with adding a device rule with the original (non-"/sys") path). How that second fix ever worked was a mystery, because the path we gave to systemd still doesn't exist. Well, I think now I know. Since systemd v240 (commit 74c48bf5a8005f20) device access rules specified as /dev/{block|char}/MM:mm are no longer looked up on the filesystem, instead, if possible, those are parsed from the string. So, we need to do different things, depending on systemd version: - for systemd >= v240, use the /dev/{char,block}/MM:mm as is, without doing stat() -- since systemd doesn't do stat() either; - for older version, check if the path exists, and skip passing it on to systemd otherwise. - the check for /sys/dev/{block,char}/MM:mm is not needed in either case. Pass the systemd version to the function that generates the rules, and fix it accordingly. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit d7208f5) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
mrunalp
approved these changes
Apr 25, 2023
@evanphx can you test this? |
@kolyshkin Sure, let me spin it up today. |
@kolyshkin Tested and works fine still! Here is the systemd version info in case you need it:
|
Merged
AkihiroSuda
approved these changes
Apr 26, 2023
Closed
7 tasks
This was referenced May 3, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a backport of #3842 to release-1.1 branch.
Essentially, it reverts PR #3620 (which made its way into 1.1.5), and makes a check added by PR #3504 conditional, depending on systemd version used.
Due to cgroup devices refactoring in main branch, this is a manual cherry-pick.
Fixes: #3671
Fixes: #3708
Also fixes an issue of printing an extra warning (which breaks some k8s tests).
Original description follows.
Commit 343951a added a call to os.Stat for the device path when generating systemd device properties, to avoid systemd warning for non-existing devices. The idea was, since systemd uses stat(2) to look up device properties for a given path, it will fail anyway. In addition, this allowed to suppress a warning like this from systemd:
NOTE that this was done because:
Apparently, the above change broke some setups, resulting in inability to use e.g. /dev/null inside a container. My guess is this is because in cgroup v2 we add a second eBPF program, which is not used if the first one (added by systemd) returns "access denied".
Next, commit 3b95828 fixed that by adding a call to os.Stat for "/sys/"+path (meaning, if "/dev/char/10:200" does not exist, we retry with "/sys/dev/char/10:200", and if it exists, proceed with adding a device rule with the original (non-"/sys") path).
How that second fix ever worked was a mystery, because the path we gave to systemd still doesn't exist.
Well, I think now I know.
Since systemd v240 (commit 74c48bf5a8005f20) device access rules specified as /dev/{block|char}/MM:mm are no longer looked up on the filesystem, instead, if possible, those are parsed from the string.
So, we need to do different things, depending on systemd version:
Pass the systemd version to the function that generates the rules, and fix it accordingly.
(cherry picked from commit d7208f5)