sclorg / container-common-scripts

Apache License 2.0
21 stars 45 forks source link

Make cgroup-limits script work for non-root containers #317

Closed hhorak closed 1 year ago

hhorak commented 1 year ago

Resource limits detection was enhanced via: https://github.com/sclorg/container-common-scripts/pull/237 While this is not directly connected, mentioning to connect both issues that touch resource limits detection.

Per podman-run(1) man page, on some systems, changing the resource limits may not be allowed for non-root users. For more details, see https://github.com/containers/podman/blob/main/troubleshooting.md#26-running-containers-with-resource-limits-fails-with-a-permissions-error.

This caused the cgroup-limits script to fail with python traceback:

$ podman run -ti --rm quay.io/fedora/s2i-core:37 /usr/bin/cgroup-limits Warning: Can't detect cpu quota from cgroups
Warning: Can't detect cpuset size from cgroups
Traceback (most recent call last):
  File /usr/bin/cgroup-limits, line 143, in <module>
    NUMBER_OF_CORES: get_number_of_cores()
                       ^^^^^^^^^^^^^^^^^^^^^
  File /usr/bin/cgroup-limits, line 76, in get_number_of_cores
    return min([l for l in limits if l])
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: min() arg is an empty sequence

This change implements another way to see number of CPUs (without cgroups), using nproc tool.

This might work as the last resort when running a container as non-root, so we get the same output as when the same container is run as a root on the same machine.

Also tracked internally: https://issues.redhat.com/browse/RHELPLAN-141599

hhorak commented 1 year ago

[test]

phracek commented 1 year ago

[test-all]

hhorak commented 1 year ago

Thanks, I've re-implemented the nproc call to something that should work on both, Python 2 and 3.

hhorak commented 1 year ago

[test-all]

hhorak commented 1 year ago

[test-all]

phracek commented 1 year ago

[test]

hhorak commented 1 year ago

I don't understand why, but some commits from this PR are already merged in master. The remaining fix that fixes #319 is is now separately in #322, so let's close this PR that is not needed in its most part.