rootless: make sure we only use a single pause process #18083

Luap99 · 2023-04-06T12:58:51Z

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. I do not see a better way.

This should fix the issues with namespace missmatches that we can see in CI as flakes.

[1] #18057

Fixes #17903

Does this PR introduce a user-facing change?

None

giuseppe

LGTM

vrothberg

/lgtm
/hold

edsantiago

LGTM on first pass, but tests need work.

edsantiago · 2023-04-06T13:14:29Z

test/system/550-pause-process.bats

+    # Use podman system migrate to stop the currently running pause process
+    run_podman system migrate
+
+    run pgrep "podman pause"


Should probably be pgrep -u $(id -u) ...

edsantiago · 2023-04-06T13:15:44Z

test/system/550-pause-process.bats

+    assert "$output" == "$tmpdir_userns" "podman should use the same userns"
+
+    run pgrep "podman pause"
+    assert "$output" == "$(cat $XDG_RUNTIME_DIR/libpod/tmp/pause.pid)" "pause pid written to correct location"


This won't work: system tests do not guarantee having XDG set, and ISTR that fedora gating tests run using su which leaves it unset.

The fact that the fedora tests are run in a different setup concerns me?! Using su to switch users without proper env vars and systemd login session is just asking for unnecessary trouble.

Could I just import the systemd helpers to define it?

podman/test/system/helpers.systemd.bash

Lines 6 to 11 in b39cdff

# podman initializes this if unset, but systemctl doesn't

if [ -z "$XDG_RUNTIME_DIR" ]; then

if is_rootless; then

export XDG_RUNTIME_DIR=/run/user/$(id -u)

fi

fi

It concerns me too, but this is a lose-lose situation: we need to make sure podman works in cases where XDG is unset, because that's a valid real-world situation and we need to catch those bugs before customers do. So, we test XDG environments in CI, and do almost-last-minute non-XDG tests in gating. If you can think of a better way to catch all conditions, I'd love to hear ideas.

so just wrap this check with if [ -n "$XDG_RUNTIME_DIR" ]; then so I only test it when it is defined?

[untested]

local xdg_default="/run/user/$(id -u)" local pidfile="${XDG_RUNTIME_DIR:-xdg_default}/libpod/tmp/pause.pid" assert "$output" = "$(<$pidfile)" ...

Could be done in one line but that makes my head hurt

Well the thing here is that podman will also fall back to /tmp/podman-run-$UID if /run/user/$UID is not writeable, which is often the case when you do not have a systemd session.

It is likely better to not assume much about podman and just do not check when we know it is set. In the end the test does not need to care about where the pid lives just that it works and we already verified this above.

Good point.

edsantiago · 2023-04-06T13:47:17Z

...but don't push yet. Tests are more than halfway complete, I think we should see how they do.

edsantiago · 2023-04-06T13:56:17Z

...such as, Debian. Maybe Debian doesn't set XDG? I think your fix would address that.

edsantiago · 2023-04-06T14:04:13Z

ok, tests complete. Timing results below, because I have a vague memory that you thought something-something might affect CI times but now I can't find that comment.

type	distro	user	DB	local	remote	container
int	debian-12	root		26:34	36:55
int	fedora-36	root		31:52	36:34	30:19
int	fedora-37	root		30:04	35:04	28:25
int	fedora-37	root	sqlite	30:50	34:01
int	debian-12	rootless		25:00
int	fedora-36	rootless		30:59
int	fedora-37	rootless		28:09
int	fedora-37	rootless	sqlite	28:22
sys	debian-12	root		26:28	18:13
sys	fedora-36	root		31:14	20:37
sys	fedora-37	root		29:47	20:58
sys	fedora-37-aarch64	root		32:35	22:03
sys	debian-12	rootless		!27:21
sys	fedora-36	rootless		29:23
sys	fedora-37	rootless		29:34	19:27

[EDIT: Debian was the only failure]

Luap99 · 2023-04-06T14:06:00Z

I asked for #18057, this PR should not be any slower than before

edsantiago · 2023-04-06T15:25:10Z

Weird: Debian is still failing.

Also, this is weird too: in my hammer-at-sqlite PR, I see four "no logs from conmon" failures (#10927). That's very unusual; the most I've ever seen before is two in one run. I'm commenting on this PR, but it could just as easily be #18085.

edsantiago · 2023-04-06T15:44:43Z

Logs downloaded and checked. Lots of flakes, but none of them especially worrisome. (Mostly "no logs from conmon", and one "failed to unmount NS" (presumably handled in #18085)).

Luap99 · 2023-04-06T16:11:00Z

Ok I guess I need to hack/get_ci_vm to see what the debian env looks like.

Luap99 · 2023-04-06T16:44:05Z

/hold
The test is not working correctly because the podman pause process can also be catatonit -P so pgrep obviously didn't found it.

The question is also why is fedora using podman pause but debian catatonit -P?

podman/pkg/rootless/rootless_linux.c

Lines 280 to 285 in fb41410

    
           /* Attempt to execv catatonit to keep the pause process alive.  */ 
        
           execl (LIBEXECPODMAN "catatonit", "catatonit", "-P", NULL); 
        
           execl ("/usr/bin/catatonit", "catatonit", "-P", NULL); 
        
           /* and if the catatonit executable could not be found, fallback here... */ 
        
           prctl (PR_SET_NAME, "podman pause", NULL, NULL, NULL);

On debian we have /usr/bin/catatonit while on fedora it is /usr/libexec/podman/catatonit.
The LIBEXECPODMAN "catatonit" is suspicious, I do not see an extra / added between the dir and catatonit.

While this is a bug and can be fixed easily I am not so sure if my test would make much sense given that I would need to grep for catatonit -P and well this could also be run for whatever reasons by other commands, at least we have no direct way of controlling it.

Anyway it is end of the day here so I will continue next week.

edsantiago · 2023-04-06T19:38:06Z

Friendly suggestion for an alternative 550-pause-process.bats

A little safer in some ways. Tested with podman pause and with catatonit. Tested with podman@main (required disabling the pidfile check), and the using-same-namespace test failed as expected.

TomSweeneyRedHat · 2023-04-07T22:09:35Z

LGTM
once the tests get happier

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]>

The path was missing a slash between the libexec path and the binary name. This was never noticed because the code already falls back to a builtt-in pause process. Fixes: 71f96c2 ("rootless: define LIBEXECPODMAN") Signed-off-by: Paul Holzinger <[email protected]>

vrothberg

LGTM

openshift-ci · 2023-04-11T09:14:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, Luap99, vrothberg

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:

~~OWNERS~~ [Luap99,giuseppe,vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vrothberg · 2023-04-11T13:02:08Z

@giuseppe @edsantiago PTanotherL

edsantiago · 2023-04-11T14:19:04Z

LGTM

edsantiago · 2023-04-11T14:20:00Z

Also meant to say, I've had this running (cherry-picked) in #17831 and it doesn't cure the "stop" issue but I have seen a reduction in other flakes.

Luap99 · 2023-04-11T14:23:24Z

Yeah for now all I think #17903 should be fixed with it, if other flakes are fixed by this that is welcome too.

vrothberg

/lgtm

edsantiago · 2023-04-11T14:58:49Z

/hold cancel

edsantiago · 2023-04-11T16:36:26Z

Belated followup: I downloaded all CI logs, manually viewed all the int flakes, and saw no "FS magic" flakes.

In my no-retries PR, though, fully rebased against this, I hit this in f37 rootless:

$ podman [options] network disconnect aliasTest0d525e717e5b8341b3ec5d17a1ffd9884906485f7350245538d5e15c3aa161ab test
Error: getting rootless network namespace: unknown FS magic on "/run/user/1599/netns/rootless-netns-642818b009b6b2c8e81e": 1021994

Any suggestions on how to diagnose that?

Luap99 · 2023-04-11T16:53:18Z

OMG, sometimes I do not see the forest for the trees. While this fix is still correct the major problem is that we are killing the pause process in podman system reset.

podman/libpod/reset.go

Line 124 in 8c4838f

if err := r.stopPauseProcess(); err != nil {

We cannot do this when we run parallel testing. #18057 may have fixed this as well but there the problem would be that we leak 100/1000s of pause processes which I rather avoid.
I really see no sane way here, we can either
a) skip all reset tests for rootless or
b) make reset not kill the pause process

Regardless I will reopen the issue then, this is clearly not fixed.

openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 6, 2023

Luap99 mentioned this pull request Apr 6, 2023

WIP: pkg/rootless: do not use shortcut with --tmpdir #18057

Closed

giuseppe approved these changes Apr 6, 2023

View reviewed changes

vrothberg approved these changes Apr 6, 2023

View reviewed changes

openshift-ci bot assigned vrothberg Apr 6, 2023

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Apr 6, 2023

edsantiago reviewed Apr 6, 2023

View reviewed changes

Luap99 force-pushed the pause-single-process branch from f2b0cd6 to 3dd19bb Compare April 6, 2023 14:22

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2023

edsantiago mentioned this pull request Apr 6, 2023

test/e2e/systemd_activate_test.go: simplify test #18056

Merged

Luap99 added 2 commits April 11, 2023 10:57

Luap99 force-pushed the pause-single-process branch from 3dd19bb to 38c217a Compare April 11, 2023 09:06

vrothberg approved these changes Apr 11, 2023

View reviewed changes

vrothberg reviewed Apr 11, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 11, 2023

openshift-merge-robot merged commit 8c4838f into containers:main Apr 11, 2023

Luap99 deleted the pause-single-process branch April 11, 2023 15:08

edsantiago mentioned this pull request Apr 11, 2023

stop podman.service: inspecting object: no such object: "top_etc" #17904

Closed

Luap99 mentioned this pull request Apr 11, 2023

pod stats: unknown FS magic on "/run/user/4902/netns/netns-etc-etc" #17903

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 2, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rootless: make sure we only use a single pause process #18083

rootless: make sure we only use a single pause process #18083

Luap99 commented Apr 6, 2023 •

edited

Loading

giuseppe left a comment

vrothberg left a comment

edsantiago left a comment

edsantiago Apr 6, 2023

edsantiago Apr 6, 2023

Luap99 Apr 6, 2023

edsantiago Apr 6, 2023

Luap99 Apr 6, 2023

edsantiago Apr 6, 2023

Luap99 Apr 6, 2023

edsantiago Apr 6, 2023

edsantiago commented Apr 6, 2023

edsantiago commented Apr 6, 2023

edsantiago commented Apr 6, 2023 •

edited

Loading

Luap99 commented Apr 6, 2023

edsantiago commented Apr 6, 2023

edsantiago commented Apr 6, 2023

Luap99 commented Apr 6, 2023

Luap99 commented Apr 6, 2023

edsantiago commented Apr 6, 2023

TomSweeneyRedHat commented Apr 7, 2023

vrothberg left a comment

openshift-ci bot commented Apr 11, 2023

vrothberg commented Apr 11, 2023

edsantiago commented Apr 11, 2023

edsantiago commented Apr 11, 2023

Luap99 commented Apr 11, 2023

vrothberg left a comment

edsantiago commented Apr 11, 2023

edsantiago commented Apr 11, 2023

Luap99 commented Apr 11, 2023

	# podman initializes this if unset, but systemctl doesn't
	if [ -z "$XDG_RUNTIME_DIR" ]; then
	if is_rootless; then
	export XDG_RUNTIME_DIR=/run/user/$(id -u)
	fi
	fi

rootless: make sure we only use a single pause process #18083

rootless: make sure we only use a single pause process #18083

Conversation

Luap99 commented Apr 6, 2023 • edited Loading

Does this PR introduce a user-facing change?

giuseppe left a comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago commented Apr 6, 2023

edsantiago commented Apr 6, 2023

edsantiago commented Apr 6, 2023 • edited Loading

Luap99 commented Apr 6, 2023

edsantiago commented Apr 6, 2023

edsantiago commented Apr 6, 2023

Luap99 commented Apr 6, 2023

Luap99 commented Apr 6, 2023

edsantiago commented Apr 6, 2023

TomSweeneyRedHat commented Apr 7, 2023

vrothberg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 11, 2023

vrothberg commented Apr 11, 2023

edsantiago commented Apr 11, 2023

edsantiago commented Apr 11, 2023

Luap99 commented Apr 11, 2023

vrothberg left a comment

Choose a reason for hiding this comment

edsantiago commented Apr 11, 2023

edsantiago commented Apr 11, 2023

Luap99 commented Apr 11, 2023

Luap99 commented Apr 6, 2023 •

edited

Loading

edsantiago commented Apr 6, 2023 •

edited

Loading