Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Don't bump RLIMIT_NOFILE in exec sessions with '--ulimit host' #24243

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Commits on Oct 13, 2024

  1. Don't bump RLIMIT_NOFILE in exec sessions with '--ulimit host'

    Starting from commit 9126b45 ("Up default Podman rlimits to
    avoid max open files"), Podman started bumping its soft limit for the
    maximum number of open file descriptors (RLIMIT_NOFILE or ulimit -n) to
    permit exposing a large number of ports to a container.  This was later
    fine-tuned in commit a2c1a2d ("podman: bump RLIMIT_NOFILE also
    without CAP_SYS_RESOURCE").
    
    Unfortunately, this also increases the limits for 'podman exec' sessions
    running in containers created with:
      $ podman create --network host --ulimit host ...
    
    This is what Toolbx uses to provide a containerized interactive command
    line environment for software development and troubleshooting the host
    operating system.
    
    It confuses developers and system administrators debugging a process
    that's leaking file descriptors and crashing on the host OS.  The
    crashes either don't reproduce inside the container or they take a lot
    longer to reproduce, both of which are frustrating.
    
    Therefore, it will be good to retain the limits, at least for this
    specific scenario.
    
    It turns out that since this code was written, the Go runtime has had
    two interesting changes.
    
    Starting from Go 1.19 [1], the Go runtime bumps the soft limit for
    RLIMIT_NOFILE for all Go programs [2].  This means that there's no
    longer any need for Podman to bump it's own limits, because it switched
    from requiring Go 1.18 to 1.20 in commit 4dd58f2 ("Move golang
    requirement from 1.18 to 1.20").  It's probably good to still log the
    detected limits, in case Go's behaviour changes.
    
    Not everybody was happy with this [3], because the higher limits got
    propagated to child processes spawned by Go programs.  Among other
    things, this can break old programs using select(2) [4].  So, Go's
    behaviour was fine-tuned to restore the original soft limit for
    RLIMIT_NOFILE when forking a child process [5].
    
    With these two changes in Go, which Podman already uses, if the bumping
    of RLIMIT_NOFILE is left to the Go runtime, then the limits are no
    longer increased for 'podman exec' sessions.  Otherwise, if Podman
    continues to bump the soft limit for RLIMIT_NOFILE on its own, then it
    prevents the Go runtime from restoring the original limits when forking,
    and leads to the higher limits in 'podman exec' sessions.
    
    The existing 'podman run --ulimit host ... ulimit -Hn' test in
    test/e2e/run_test.go was extended to also check the soft limit.  The
    similar test for 'podman exec' was moved from test/e2e/toolbox_test.go
    to test/e2e/exec_test.go for consistency and because there's nothing
    Toolbx specific about it.  The test was similarly extended, and updated
    to be more idiomatic.
    
    Due to the behaviour of the Go runtime noted above, and since the tests
    are written in Go, the current or soft limit for RLIMIT_NOFILE returned
    by syscall.Getrlimit() is the same as the hard limit.
    
    The Alpine Linux image doesn't have a standalone binary for 'ulimit' and
    it's picky about the order in which the options are listed.  The -H or
    -S must come first, followed by a space, and then the -n.
    
    [1] https://go.dev/doc/go1.19#runtime
    
    [2] Go commit 8427429c592588af ("os: raise open file rlimit at startup")
        golang/go@8427429c592588af
        golang/go#46279
    
    [3] containerd/containerd#8249
    
    [4] http://0pointer.net/blog/file-descriptor-limits.html
    
    [5] Go commit f5eef58e4381259c ("syscall: restore original NOFILE ...")
        golang/go@f5eef58e4381259c
        golang/go#46279
    
    Fixes: containers#17681
    
    Signed-off-by: Debarshi Ray <[email protected]>
    debarshiray committed Oct 13, 2024
    Configuration menu
    Copy the full SHA
    302d638 View commit details
    Browse the repository at this point in the history