WIP: pkg/rootless: do not use shortcut with --tmpdir #18057
Conversation
When using --tmpdir for the podman cli we use this as location for the pause.pid file. However the c shortcut code has no idea about this option and always assumes XDG_RUNTIME_DIR/libpod/tmp. This can cause us to join the wrong user namespace which leads to very weird issues as mounts are missing there. Fixes containers#17903 Signed-off-by: Paul Holzinger <[email protected]>
openshift-ci
bot
added
release-note-none
approved
|
@giuseppe PTAL @edsantiago It might be good to check the integration test runtime to see if it will be slower? Without the shortcut we need to rexec every time which may be noticeable. |
|
Could we move the |
|
That wasn’t a behavior change (just changed how we got the final path, the
intended destination did not change). I have no specific objections to
moving it, beyond that it needs to stay on tmpfs and that it could be a
breaking change to move it. We also need to make the final location Libpod
specific, I think? If you have two Libpods with different tmpdirs on the
same user, is it important that both have different user namespaces?
…On Wed, Apr 5, 2023 at 09:12 Paul Holzinger ***@***.***> wrote:
Could we move the pause.pid to another location (i.e., not under TMP)?
I would need to figure what commit ab88632
<ab88632>
really tried to fix with that.
cc @mheon <https://github.com/mheon>
—
Reply to this email directly, view it on GitHub
<#18057 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3AOCAM7YO2JVMVCFF2BBTW7VVVBANCNFSM6AAAAAAWUBKVXQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
No it changed the path, likely not intentionally but the previous path was But the big difference is that the new behavior now also respected --tmpdir while the previous one never did. So the fact is that this commit changed it from one pause process to one per tmpdir expect the the tmpdir thing never worked correctly with the c code.
I do not know, the only problem would be if one is using podman system migrate kill the pause process whikle other containers are still running but there should be a fallback to join a container namespace in any case so it likely won't matter. |
Luap99
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I am working on patch to only use a single pause process, this should be much better otherwise we leak many processes in CI. Also this sort of keeps the current behaviour assuming the |
|
The root issue is that we are trying to be too clever in how we compute would it be simpler if we use only |
|
Yes lets go with #18083 which should fix it in a better way, given that we never need more than a single pause process. |
Currently --tmpdir changes the location of the pause.pid file. this causes issues because the c code in pkg/rootless does not know about that. I tried to fix this[1] by fixing the c code to not use the shortcut. While this fix worked it will result in many pause processes leaking in the integrration tests. Commit ab88632 added this behavior but following the disccusion it was never the intention that we end up having more than one pause process. The issues that was trying to fix was caused by somthing else AFAICT, the main problem seems to be that the pause.pid file parent directory may not be created when we try to create the pid file so it failed with ENOENT. This patch fixes it by creating this directory always and revert the change to no longer depend on the tmpdir value. With this commit we now always use XDG_RUNTIME_DIR/libpod/tmp/pause.pid for all podman processes. This allows the c shortcut to work reliably and should therefore improve perfomance over my other approach. A system test is added to ensure we see the right behavior and that podman system migrate actually stops the pause process. Thanks to Ed Santiago for the improved test to make it work for both `catatonit` and `podman pause`. This should fix the issues with namespace missmatches that we can see in CI as flakes. [1] containers#18057 Fixes containers#18057 Signed-off-by: Paul Holzinger <[email protected]>
github-actions
bot
added
the
locked - please file new issue/PR
When using --tmpdir for the podman cli we use this as location for the pause.pid file. However the c shortcut code has no idea about this option and always assumes XDG_RUNTIME_DIR/libpod/tmp. This can cause us to join the wrong user namesapce which leads to very weird issues as mounts are missing there.
Fixes #17903
Does this PR introduce a user-facing change?