-
Notifications
You must be signed in to change notification settings - Fork 202
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
fix: Add error message output to respond to the impact of Ubuntu 24.04 #1042
fix: Add error message output to respond to the impact of Ubuntu 24.04 #1042
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A general question:
Does the container mode work at all in Ubuntu 24.04 with the default settings?
In #1041 it fails due to network configuration, does it work with --network-access
?
If it does not work at all due to further problems, we don't want to print several warnings just to fail with a different error in the end. Instead, it would be easier for the user to understand if we just detect the situation once the first problem occurs and abort immediately with an appropriate error message that includes the explanation.
If container mode works in principle, but several features are missing, then it would probably be appropriate to print a warning with the explanation and continue. But then we should also handle the problem with the networking.
After adding
So, should we let child |
Thanks for trying out! So this means that we cannot mount anything in the container, but mounting stuff is a hard requirement for our container mode. So there is no way to make container mode work on Ubuntu 24.04 except by letting the user change the system config.
I am not 100% sure how to best implement this. Let's think about this together. I see the following goals:
Could you help thinking about how to implement this? |
ce27511
to
81b5d5b
Compare
I have pushed a new version that meets a part of our vision.
Having a global variable in
After catching
Now, if |
Ah sorry, this is a misunderstanding because I didn't write it clearly.
Yes,
I see, so we get all of these warnings. Could you post an example of a such a failing run of |
Oh I see. However, I think it would be cooler to place this check at the import time, similar to how
I think maybe we can do it this way:
Yes, I tested
|
Well, we do not need to check for it if everythings works. But overhead is not my concern, I am mostly wondering whether this has the potential to break something. For example, we had problems in the past where modules did stuff during import time, and this caused problems for situations where the module was not actually used but still imported, e.g., when using other parts of BenchExec on Windows, or in tests, or by type checkers. The use of
Like this, yes. But the comments are not correct, because it is not guaranteed that
Hm, I find the explanation quite hidden. It is in between other log messages, and it is even a warning where the rest is marked critical. Ideal would be a separate error message, and I thought we have that in similar cases. But I realize now that maybe this was only done for For comparison, consider for example the case if BenchExec cannot create a cgroup because pystemd is missing or so. |
That's correct. Using such a global variable does indeed carry unpredictable risks. But should we wrap this check into a function and perhaps place it in As I mentioned before, if an error message is printed at Additionally, I noticed that in
I'll remove them in later commits.
In fact, after installing I've tested
I tested
When handling the missing pystemd case, the warning messages are somewhat repetitive, such as |
You mean such that it will typically abort early without lots of confusing error messages because of the check in But why can't we handle this more generally, such that if there is some permission error and the problematic sysctl is set, then we terminate BenchExec and show a final error message. For example, if cgroups are unusable but necessary we get something like this:
This is something that would be nice to have here as well.
Hm, probably yes, but I would like to not mess with the container details just for the sake of getting some nice message. It is easy to imagine that sometime later someone brings the setup steps into a more logical and consistent order and thus breaks the message again, if it relies on tricky ordering details.
Warning is for cases where BenchExec can continue, critical is for cases where BenchExec can not continue. If we cannot change the hostname, we can still continue, so it is a warning.
Yes, because several different cgroup subsystems are missing, and they are independent from each other (it can happen that we have one of them but not the others). Furthermore, if cgroups are actually required and not just optional, there will be a final error message with an explanation what to do. So this is not perfect but good enough. |
In previous discussions, you have also mentioned this idea. However, implementing it seems to require quite a few changes, including adding other special return codes for the child. Errors caused by the restriction of user-ns are caught as exceptions within the The problem here is evident: we cannot delay the output of the error message until # containerexecutor.py > ContainerExecutor > _start_execution_in_container > check_chile_exit_code
# Line 914 - 919
if child_exitcode.value == CHILD_OSERROR:
# This was an OSError in the child,
# details were already logged
raise BenchExecException(
"execution in container failed, check log for details"
) I am now feeling a bit confused. Should we continue to implement the same way as in the previous commits, where we only check if the problematic sysctl is set and print an error message when errors occur in |
I see. Thanks for digging through this and the summary! Might be somewhat awkward in the code to handle this specific case so differently. So now I tend to say we can proceed as in the previous commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Would you be willing to do some polishing?
fb3ef0b
to
fb6a54a
Compare
I squashed the commits of this PR. Usually we do not do this, but in this case the back-and-forth just clutters the history and does not provide a benefit. I also added some slight refactoring and will now merge this PR. Thanks! |
Very honored that these codes has been merged into the main branch! |
On Ubuntu since 24.04, user namespaces are forbidden for regular users (cf. #1041 and #1042). There is a global sysctl switch to enable them again, but applications whose AppArmor profile allows this can also use it. (Typically, AppArmor only restricts application, but in this case an AppArmor profile can actually provide a privilege than an unconfined application does not have.) More explanations are at https://ubuntu.com/blog/ubuntu-23-10-restricted-unprivileged-user-namespaces In order to make BenchExec usable out-of-the-box after installing the .deb package we want to ship such an AppArmor profile. This is made complicated by the fact that the AppArmor profile that is necessary on Ubuntu 24.04+ breaks AppArmor on previous Ubuntu versions. So we have to install this profile conditionally. I found a way to do so using ucf (a tool for handling config files) and this seems to work in my tests on Ubuntu 22.04 (old AppArmor), Ubuntu 24.04 (new AppArmor), and Debian 12 (old AppArmor), as well as installation without AppArmor present. There are two known remaining problems: - If one upgrades from Ubuntu 22.04 to Ubuntu 24.04 while having BenchExec installed, the AppArmor profile will not be installed, so BenchExec will not work. Upgrading or reinstalling the BenchExec package makes it work. - The command "python3 -m benchexec.test_tool_info" will not work, because the AppArmor profile won't match it. One has to either disable container mode or temporarily allow the use of user namespaces for the whole system. If we implement #1053 this would just work. Part of #1041.
On Ubuntu since 24.04, user namespaces are forbidden for regular users (cf. sosy-lab#1041 and sosy-lab#1042). There is a global sysctl switch to enable them again, but applications whose AppArmor profile allows this can also use it. (Typically, AppArmor only restricts application, but in this case an AppArmor profile can actually provide a privilege than an unconfined application does not have.) More explanations are at https://ubuntu.com/blog/ubuntu-23-10-restricted-unprivileged-user-namespaces In order to make BenchExec usable out-of-the-box after installing the .deb package we want to ship such an AppArmor profile. This is made complicated by the fact that the AppArmor profile that is necessary on Ubuntu 24.04+ breaks AppArmor on previous Ubuntu versions. So we have to install this profile conditionally. I found a way to do so using ucf (a tool for handling config files) and this seems to work in my tests on Ubuntu 22.04 (old AppArmor), Ubuntu 24.04 (new AppArmor), and Debian 12 (old AppArmor), as well as installation without AppArmor present. There are two known remaining problems: - If one upgrades from Ubuntu 22.04 to Ubuntu 24.04 while having BenchExec installed, the AppArmor profile will not be installed, so BenchExec will not work. Upgrading or reinstalling the BenchExec package makes it work. - The command "python3 -m benchexec.test_tool_info" will not work, because the AppArmor profile won't match it. One has to either disable container mode or temporarily allow the use of user namespaces for the whole system. If we implement sosy-lab#1053 this would just work. Part of sosy-lab#1041.
container.py
andcontainerexecutor.py
to detect whether BenchExec is being used with container mode enabled on Ubuntu 24.04 and the use of unprivileged user namespaces is restricted in system configuration, output the corresponding error message if so.Part of #1041