-
Notifications
You must be signed in to change notification settings - Fork 198
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
Quickstart tutorial #1044
Quickstart tutorial #1044
Conversation
c383c0e
to
adfe1f3
Compare
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 a lot!
Besides the idea of this tutorial, I also like the overall structure, its scope, length, and level of detail.
I mostly have suggestions and questions about tweaking the phrasing a little bit.
I guess we should add more cross references from the Documentation section of the readme, the doc INDEX, and runexec.md
?
ecafad6
to
3da2bf3
Compare
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.
Could I please ask you to go through the tutorial and replace "run" wherever it is used as a verb with "execute"? Because "run" occurs so often with different meanings, we try to avoid with as a verb.
Looks great for me now, all comments are minor editorial stuff.
Do you have it marked as draft because you want to add something or can I merge?
Comments should be resolved. I would really want to add a small "how to" for running in docker / podman but I couldn't get it to work.
and
I couldn't find anything else in the manuals ... |
Hm, seems I need to tweak the error-case detection and the error messages again... The actual problem is that in your tests, BenchExec is not the only process in the container (there is also the shell) and that the container is also not running some cgroup manager (like systemd). This means that all processes in the container are in the same cgroup, so there is no cgroup with BenchExec alone and neither can we ask someone to create a cgroup for us. This is why BenchExec fails. It should work if you ensure that only BenchExec is running in the container, i.e., start it directly as entry point of the container instead of spawning an interactive container. I would love to find a way to handle this in BenchExec, but I do not really want to mess with the cgroups of other processes, and this would be required. |
EDIT: I completely removed my writing-while-thinking comments, the results are now moved to separate issues. It seems that I think it would be really beneficial to have a tutorial of how to actually manage the cgroups in an interactive container. A lot of artifact evaluations want everything to be self-contained inside the image (i.e. include data processing etc.). But since I believe this requires more work, it is probably best if we delay this. |
Thanks. Continuing separately makes a lot of sense. Regarding the removed comments, I want to add that it should have worked with Podman already, it is just that after my last comment you seem to have switched to Docker only, and there got some arguments wrong. I also tried to improve the documentation in this regard in 901b50c. If you find something that does not work as documented now, this would be another issue.
It is tricky, because use cases and systems are so diverse. We can probably not hope to give a tutorial "this is how it works", but we can likely give an example "this is how you can do it in this particular case". But what needs to be done for example already differs between the case "interactive container, configure cgroups, start BenchExec" and "non-interactive container with script wrapping BenchExec". |
I needed that to be able to modify subtree_control easily. With --cgroupns=private, the docker container is put into a cgroup with empty subtree_control and this cannot be modified if there is a process in there. So, another solution is to create a sub-cgroup of the container's group, move the interactive shell there, set the subtree_control of the container's cgroup, create another group for benchexec, and then start benchexec inside that one. This seems kinda brittle. But being able to run easily in an interactive docker setting is (I think) really important.
Totally agree, but there should be a tutorial that works easily for a fairly recent setup (i.e. Ubuntu/Debian & docker). I think it is fine to strongly recommend podman, but docker should really work, too. EDIT: I found a somewhat reliable way for docker, I will add it to the quickstart |
Oh, I see now also from the explanation in the other issue. But this means that it works only because you escape the cgroups of the (outer) container completely. So for example any resource limits configured on the Docker container won't work. I won't recommend this.
Well, this is exactly how cgroups v2 are supposed to be used (and what BenchExec does internally for itself), so this is not brittle per se. It starts getting brittle once you think about "moving the interactive shell there", because if you have more than that shell process (e.g., background processes), then you also need to move them. And if someone starts
Not arguing against this.
Maybe discuss in a separate issue first? If we find something like this, I don't want to have it only inside the tutorial, but also in the main documentation. And if the final solution needs several iterations, it is nicer to have the discussion recorded in a specific issue. |
Yes, this is what I meant. But now that I understood what is the issue I think it is fine to just put this into the entrypoint of the docker container (which is what I did)
Done in #1048 |
80388ab
to
5ca96ee
Compare
Ok, I merged the two things and discovered an important bit: Even for non-interactive cases, we need the "complicated" setup due to permissions! If we only run So either we remove that part ("benchexec should not be run as root" -> "its fine inside a container for non-interactive setups") or simply say "this is the way to do it" |
Out of interest: How do you directly start a container as And are you talking about Docker here or about rootless Podman? I wasn't aware that there is support for this, in particular that you can use something like
Ok, so the following is true:
To be honest, for the case of a rootless Podman I hadn't thought much about whether root is acceptable or not. My first intuition would be that it is probably acceptable. After thinking more about this: If you managed to use In a setup with Podman and usable However, I don't want to say "you have to have What would be middle ground is to have a Podman container with just one user inside (no So, we would want to recommend If you are interested in even more details about this (feel free to ignore, but for cgroups you were quite interested, so I am leaving this here): There are several layers of isolation with regards to containers:
The main reason why I do not recommend 2. is that it is easy to overlook something during container setup, and then there is immediately an escape to being root at the host. I think there are also more things that a container tool needs to take care of if 2. is the target instead of 4., and BenchExec was written only for 4. And 2. inside 0. or 1. (BenchExec in Docker) is just the same as 2. so we recommend 4. in 0. (Docker+ |
Add
IDK :D My
Running as So I guess that confirms the suspicion + I think I understand the issue.
What will happen then is that people copy-paste the "unsafe" Dockerfile and use it with docker I think (we can be fine with that and explicitly say "this is at your own risk"; I'm just saying that this is probably what would happen). I would be happy with this variant. Especially because the su variant (a) looks confusing and (b) (as I understand it) breaks on a "recommended" setup (podman without newuidmap); and going into detail what needs to be done then just makes this more daunting. I think especially for a "basic guide" it is better if the approach is universal than being perfectly secure.
I think the working assumption is that docker images are trusted; but yeah. |
Hm, all this container handling by Podman seems to be even more complex. It seems that differently from what I thought and what I claimed above it does not use a user namespace per container, you need to request that explicitly with So although security-wise Podman containers are worse than I thought the complexity to explain all what is necessary for a proper solution is way beyond what we can and want to manage here. This is just to leave my latest knowledge here, I will now review the current state of the PR. |
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 a lot, looks great already! I have just minor comments.
All comments should be addressed |
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.
Great! Two small typos, otherwise ready to merge for me. Do you agree?
doc/benchexec-in-container.md
Outdated
purpose of the `init.sh` script. Only then can we create a separate, empty | ||
cgroup dedicated to BenchExec. | ||
|
||
Note that this also is the reason why non-interactive setups do not this. Then, |
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.
Note that this also is the reason why non-interactive setups do not this. Then, | |
Note that this also is the reason why non-interactive setups do not need this. Then, |
doc/benchexec-in-container.md
Outdated
containerized environments (such as Docker or Podman) and shows how you can | ||
create your own interactive Docker image (or adapt existing ones) to use | ||
BenchExec within it. We focus mainly on the (nowadays standard) cgroups v2, | ||
a brief guidline for the (outdated) cgroups v1 is provided |
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 brief guidline for the (outdated) cgroups v1 is provided | |
a brief guideline for the (outdated) cgroups v1 is provided |
Oh, I overlooked the conflicts. The changes from 901b50c and 29480b3 need to be merged here. One is the removal of |
5e5f8f7
to
84f2a9e
Compare
With this ( I merged the rest I think. |
Hm, maybe a version difference? I guess with the
Thank you very much for your extensive help with all this! It is a great improvement of our documentation. I will merge this now and of course also make sure to list you as a contributor for the next release. |
No problem, its helping me twofold, too. Esp. having runexec for my evaluation makes my life easier :)
thanks! |
First draft for the quickstart guide. It should include a short point how to add it for docker / what to consider there and maybe some basic troubleshooting without getting too large (I think this should remain at about 1-2 pages so it doesn't look more daunting than necessary).