rootlessport: reduce memory usage of the process #11565

Luap99 · 2021-09-14T10:08:44Z

Don't use reexec for the rootlessport process, instead make it a
separate binary to reduce the memory usage. The problem with reexec is
that it will import all packages that podman uses and therefore loads a
lot of stuff into the heap. The rootlessport process however only needs
the rootlesskit library.
The memory usage is a concern since the rootlessport process will spawn
two process per container which has ports forwarded. The processes stay
until the container dies. On my laptop the current reexec version uses
47800 KB RSS. The new separate binary only uses 4540 KB RSS. This is
more than a 90% improvement.

The Makefile has been updated to compile the new binary and install it
to the libexec directory.

Fixes #10790

Makefile

libpod/networking_slirp4netns.go

rhatdan · 2021-10-06T20:23:46Z

Should we do the same for the pause process?

Luap99 · 2021-10-11T18:30:44Z

Should we do the same for the pause process?

If you want do it for the pause process then it should be a small c program to safe the unnecessary overhead from go.

Luap99 · 2021-10-11T18:34:16Z

contrib/cirrus/setup_environment.sh

@@ -236,7 +236,7 @@ case "$TEST_FLAVOR" in
        # Use existing host bits when testing is to happen inside a container
        # since this script will run again in that environment.
        # shellcheck disable=SC2154
-        if ((CONTAINER==0)) && [[ "$TEST_ENVIRON" == "host" ]]; then
+        if ((CONTAINER==0)); then


@cevich @edsantiago PTAL. This was required to build the new binary on the host and then I mount it in the container for the tests. I think this change is correct, otherwise this was never called for container tests since TEST_ENVIRON was set to container

Deferring to @cevich. I don't grok this code, and even tig blame doesn't help me understand the reasoning behind it. I can't even understand the comment. Sorry.

Right, this is because setup_environment.sh is called by runner.sh (a second time) when testing is to happen inside a container:

https://github.com/containers/podman/pull/11565/files#diff-db575befc56ec85f83b824430b9b62e20eb19d08b351dc743db26848639896a6R126

So the volume mount for /var/libexec/podman doesn't seem right either, normally the podman binary for testing would be compiled + installed inside the container environment (again, by way of calling setup_environment.sh again).

Note: I haven't seen the rest of this PR, so I'm not sure what it's all about. I'll take a look now...

TEST_ENVIRON is always set to container for all container test, regardless if we are outside or inside the container, thus it was never compiled in the container.

Also the container==0 condition means that this happens outside the container. If you want to compile only inside it should be container==1 AFAICT

Ya, there's a test setup bug here, it's entirely my fault 😢

cevich · 2021-10-12T18:51:01Z

contrib/cirrus/runner.sh

@@ -119,6 +119,7 @@ exec_container() {
    exec podman run --rm --privileged --net=host --cgroupns=host \
        -v /dev/fuse:/dev/fuse \
        -v "$GOPATH:$GOPATH:Z" \
+        -v /usr/libexec/podman:/usr/libexec/podman \


This isn't right. For the TEST_ENVIRON=container case, we basically treat the container like a VM. Very little should leak into the container from the host.

cevich · 2021-10-12T19:25:42Z

Okay I think I follow what you're after, and there's a test-setup bug here. I believe the comment is correct, and the conditional is wrong. Which is why the comment is confusing 😕 Damn. This may expose a bunch of bugs...I believe the intended logic should be:

CONTAINER	TEST_ENVIRON	behavior
0	host	replace podman RPM + build/install from source
0	container	do nothing
1	host	Impossible, throw an error and panic.
1	container	replace podman RPM + build/install from source

So we want something like:

        if [[ "$TEST_ENVIRON" == "host" ]]; then
            if ((CONTAINER)); then
                die ("Refusing to config. host-test in container");
            fi
            remove_packaged_podman_files
            make install PREFIX=/usr ETCDIR=/etc
        elif [[ "$TEST_ENVIRON" == "container" ]]; then
            if ((CONTAINER)); then
                remove_packaged_podman_files
                make install PREFIX=/usr ETCDIR=/etc
            fi
        else
            die("Invalid value for $$TEST_ENVIRON=$TEST_ENVIRON")
        fi

I dunno if maybe there's a simpler/cleaner way to structure that or not. Just what I can think of off-hand.

Luap99 · 2021-10-12T19:32:16Z

@cevich That looks good to me. I will add this and remove the volume mount.

contrib/cirrus/setup_environment.sh

Don't use reexec for the rootlessport process, instead make it a separate binary to reduce the memory usage. The problem with reexec is that it will import all packages that podman uses and therefore loads a lot of stuff into the heap. The rootlessport process however only needs the rootlesskit library. The memory usage is a concern since the rootlessport process will spawn two process per container which has ports forwarded. The processes stay until the container dies. On my laptop the current reexec version uses 47800 KB RSS. The new separate binary only uses 4540 KB RSS. This is more than a 90% improvement. The Makefile has been updated to compile the new binary and install it to the libexec directory. Fixes containers#10790 [NO TESTS NEEDED] Signed-off-by: Paul Holzinger <[email protected]>

Luap99 · 2021-10-12T20:54:31Z

@containers/podman-maintainers PTAL. This is a good improvement.
Please note that this requires packaging changes. @lsm5 FYI

vrothberg

LGTM

Nice work, @Luap99 !

giuseppe · 2021-10-13T08:40:23Z

Should we do the same for the pause process?

the pause process doesn't load the go runtime, but still it keeps the podman executable in memory. So a custom binary would be better, and we could also re-use it for the pod init process to not require an image

giuseppe

/lgtm

openshift-ci · 2021-10-13T08:41:06Z

[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:

~~OWNERS~~ [Luap99,giuseppe]

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

vrothberg · 2021-10-13T08:42:15Z

Should we do the same for the pause process?

the pause process doesn't load the go runtime, but still it keeps the podman executable in memory. So a custom binary would be better, and we could also re-use it for the pod init process to not require an image

I started working on that yesterday :^)

vrothberg · 2021-10-13T09:44:10Z

FYI, @siretart for Debian / Ubuntu packaging

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2021

Luap99 force-pushed the rootlessport-bin branch from eb8e17e to 119cfbb Compare September 14, 2021 10:38

flouthoc reviewed Sep 14, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2021

Luap99 force-pushed the rootlessport-bin branch from 119cfbb to e15df9b Compare September 21, 2021 12:57

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2021

Luap99 force-pushed the rootlessport-bin branch from e15df9b to 1aeff92 Compare September 21, 2021 14:50

AkihiroSuda reviewed Sep 22, 2021

View reviewed changes

libpod/networking_slirp4netns.go Outdated Show resolved Hide resolved

Luap99 mentioned this pull request Oct 4, 2021

Cirrus: Bump Fedora to release 35 #11795

Merged

Luap99 force-pushed the rootlessport-bin branch 3 times, most recently from 2a55a97 to ce3e659 Compare October 6, 2021 19:51

Luap99 force-pushed the rootlessport-bin branch 2 times, most recently from c51864f to bc4e311 Compare October 11, 2021 15:53

Luap99 commented Oct 11, 2021

View reviewed changes

cevich reviewed Oct 12, 2021

View reviewed changes

Luap99 force-pushed the rootlessport-bin branch from bc4e311 to 6c24f4a Compare October 12, 2021 19:35

cevich reviewed Oct 12, 2021

View reviewed changes

contrib/cirrus/setup_environment.sh Outdated Show resolved Hide resolved

cevich reviewed Oct 12, 2021

View reviewed changes

contrib/cirrus/setup_environment.sh Outdated Show resolved Hide resolved

Luap99 force-pushed the rootlessport-bin branch from 6c24f4a to 3ba69dc Compare October 12, 2021 19:43

vrothberg reviewed Oct 13, 2021

View reviewed changes

giuseppe approved these changes Oct 13, 2021

View reviewed changes

openshift-ci bot assigned giuseppe Oct 13, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2021

openshift-merge-robot merged commit f12dc28 into containers:main Oct 13, 2021

Luap99 deleted the rootlessport-bin branch October 13, 2021 08:44

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 22, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rootlessport: reduce memory usage of the process #11565

rootlessport: reduce memory usage of the process #11565

Luap99 commented Sep 14, 2021 •

edited

Loading

rhatdan commented Oct 6, 2021

Luap99 commented Oct 11, 2021

Luap99 Oct 11, 2021

edsantiago Oct 11, 2021

cevich Oct 12, 2021

Luap99 Oct 12, 2021

Luap99 Oct 12, 2021

cevich Oct 12, 2021

cevich Oct 12, 2021

cevich commented Oct 12, 2021 •

edited

Loading

Luap99 commented Oct 12, 2021

Luap99 commented Oct 12, 2021

vrothberg left a comment

giuseppe commented Oct 13, 2021

giuseppe left a comment

openshift-ci bot commented Oct 13, 2021

vrothberg commented Oct 13, 2021

vrothberg commented Oct 13, 2021

rootlessport: reduce memory usage of the process #11565

rootlessport: reduce memory usage of the process #11565

Conversation

Luap99 commented Sep 14, 2021 • edited Loading

rhatdan commented Oct 6, 2021

Luap99 commented Oct 11, 2021

Luap99 Oct 11, 2021

Choose a reason for hiding this comment

edsantiago Oct 11, 2021

Choose a reason for hiding this comment

cevich Oct 12, 2021

Choose a reason for hiding this comment

Luap99 Oct 12, 2021

Choose a reason for hiding this comment

Luap99 Oct 12, 2021

Choose a reason for hiding this comment

cevich Oct 12, 2021

Choose a reason for hiding this comment

cevich Oct 12, 2021

Choose a reason for hiding this comment

cevich commented Oct 12, 2021 • edited Loading

Luap99 commented Oct 12, 2021

Luap99 commented Oct 12, 2021

vrothberg left a comment

Choose a reason for hiding this comment

giuseppe commented Oct 13, 2021

giuseppe left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 13, 2021

vrothberg commented Oct 13, 2021

vrothberg commented Oct 13, 2021

Luap99 commented Sep 14, 2021 •

edited

Loading

cevich commented Oct 12, 2021 •

edited

Loading