podman: do not set rlimits to the default value

[giuseppe]

since the effect would be to lower the rlimits when their definition is higher than the default value.

The test doesn't fail on the previous version, unless the system is configured with a nofile ulimit higher than the default value.

Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2317721

Does this PR introduce a user-facing change?

None

[Luap99]

LGTM

[edsantiago]

I think we can do better in the test. Need some time to work on it.

[edsantiago]

This fails as root under old-podman, passes root+rootless with new-podman:

# #24228: podman used to set this to hardcoded value, ignoring current settings
# bats test_tags=ci:parallel
@test "podman run - can use maximum ulimit value" {
    skip_if_remote "cannot check local ulimits with podman remote"
    run -0 ulimit -n -H
    max=$output
    oldmax=$max

    # This test is more meaningful when root
    if ! is_rootless; then
        max=$((max + $RANDOM))
        ulimit -n -H $max
    fi
    run_podman run --rm $IMAGE sh -c 'ulimit -n -H'
    assert "$output" = "$max" "podman inherits parent ulimit"
    ulimit -n -H $oldmax
}

I removed the command-line --ulimit because (unless I read things very very wrong) that is entirely against the purpose of this PR. Again, I may be wrong.

I also noticed that, on my f40 system, this reduces the default root setting for podman:

$ for i in /usr/bin/podman bin/podman; do sudo $i run --rm quay.io/libpod/testimage:20241010 sh -c 'ulimit -n -H';done
1048576
524288

Could that be considered a breaking change?

[Luap99]

I removed the command-line --ulimit because (unless I read things very very wrong) that is entirely against the purpose of this PR. Again, I may be wrong.

The --ulimit is needed here, the point is podman has a default limit that it sets for rootless containers (up to 1048576) even when the host allows more. So the point here is that if your host has more than that --ulimit must be able to set that value and should not be capped at 1048576 (default). This did not work because the podman init code capped the limit at 1048576 thus the bug report. And this only fails as rootless with limit higher than 1048576 because rootful has CAP_SYS_RESOURCE and can increase the limit higher all it wants.

We would need to bump the ulimit for our rootless CI user > 1048576 in order for this to even be testable.

What I do not get is a we do not document this rootless default and rootful seems to inherit and not use the 1048576 number but I am not sure why?

[giuseppe]

This fails as root under old-podman, passes root+rootless with new-podman:

# #24228: podman used to set this to hardcoded value, ignoring current settings
# bats test_tags=ci:parallel
@test "podman run - can use maximum ulimit value" {
    skip_if_remote "cannot check local ulimits with podman remote"
    run -0 ulimit -n -H
    max=$output
    oldmax=$max

    # This test is more meaningful when root
    if ! is_rootless; then
        max=$((max + $RANDOM))
        ulimit -n -H $max
    fi
    run_podman run --rm $IMAGE sh -c 'ulimit -n -H'
    assert "$output" = "$max" "podman inherits parent ulimit"
    ulimit -n -H $oldmax
}

I removed the command-line --ulimit because (unless I read things very very wrong) that is entirely against the purpose of this PR. Again, I may be wrong.

I also noticed that, on my f40 system, this reduces the default root setting for podman:

$ for i in /usr/bin/podman bin/podman; do sudo $i run --rm quay.io/libpod/testimage:20241010 sh -c 'ulimit -n -H';done
1048576
524288

Could that be considered a breaking change?

Thanks for noticing it. Yes, I think this should not happen.

I'll take a look

[giuseppe]

/hold

[edsantiago]

Okay, I have a 1mt with

/etc/security/limits.conf:*             hard    nofile          12345678

and:

# ulimit -n -H
12345678
# for i in /usr/bin/podman bin/podman; do $i run --rm quay.io/libpod/testimage:20241010 sh -c 'ulimit -n -H';done
1048576     <<< expected
12345678    <<< expected

$ ulimit -n -H
12345678
$ for i in /usr/bin/podman bin/podman; do $i run --rm quay.io/libpod/testimage:20241010 sh -c 'ulimit -n -H';done
1048576     <<< expected
1048576     <<< UNEXPECTED but that's probably because Ed is clueless

With the command-line option, your PR seems to do the right thing:

$ for i in /usr/bin/podman bin/podman; do $i run --rm --ulimit=nofile=12345678:12345678 quay.io/libpod/testimage:20241010 sh -c 'ulimit -n -H';done
Error: crun: setrlimit `RLIMIT_NOFILE`: Operation not permitted: OCI permission denied
12345678

[giuseppe]

# ulimit -n -H
12345678
# for i in /usr/bin/podman bin/podman; do $i run --rm quay.io/libpod/testimage:20241010 sh -c 'ulimit -n -H';done
1048576     <<< expected
12345678    <<< expected

The first version of this PR was not correct, and this is wrong. It must be 1048576 in both cases to honor what the default is.

I've changed the behaviour of Podman in the last version to honor the default settings also with root.

[Luap99]

Please also document in the man page the the default nofile and nproc is 1048576 under the --ulimit option if we now also do this as root.

[giuseppe]

Please also document in the man page the the default nofile and nproc is 1048576 under the --ulimit option if we now also do this as root.

thanks. I've added a note to the documentation but I prefer to not specify their value (if you really want them I can change it again) to not have to worry about them getting out of sync

[mheon]

LGTM

[Luap99]

Please also document in the man page the the default nofile and nproc is 1048576 under the --ulimit option if we now also do this as root.

thanks. I've added a note to the documentation but I prefer to not specify their value (if you really want them I can change it again) to not have to worry about them getting out of sync

I think it somewhat important, it is not like we randomly will change the values. Saying look in containers.conf but then thee is nothing in the containers.conf docs is confusing. I think most users really want to know the defaults and we document them in most cases but I am fine with not blocking over this and to merge without it

[damaestro]

It would have been nice to have this default documented when I was working through figuring out what was going on here.

[giuseppe]

fine so I'll add it to the documentation.

Before that though, we need containers/common#2199

Not sure why we do that, but it is just wrong IMO: we set it to the maximum value possible, and we change the limits from a library

[giuseppe]

vendored the change in c/common and pushed a new version ⬆️

[Luap99]

LGTM

[giuseppe]

/hold cancel

@containers/podman-maintainers PTAL

[lsm5]

LGTM

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, lsm5, 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,lsm5]

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

[rhatdan]

/lgtm