From a5a2655ea0e2050e4377b39ccf063ca43c54d0fd Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Fri, 14 Jun 2024 13:27:25 +0100 Subject: [PATCH 1/4] Make buildah handling of ulimits match podman There was a bug in #5275 which meant that in some cases (not rootless, with low limits) buildah tried to set limits to more than the hard limit. Fixing that issue made some of the tests fail as they only passed due to this bug. This was resolved by more accurately reflecting podman's handling of ulmits. Signed-off-by: Chris Reeves --- run_linux.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/run_linux.go b/run_linux.go index d0b31f6daa6..009a41bdfdf 100644 --- a/run_linux.go +++ b/run_linux.go @@ -1003,10 +1003,12 @@ func addRlimits(ulimit []string, g *generate.Generator, defaultUlimits []string) g.AddProcessRlimits("RLIMIT_"+strings.ToUpper(ul.Name), uint64(ul.Hard), uint64(ul.Soft)) } if !nofileSet { + // For nofile, podman sets both hard and soft limits to min(hard limit, RLimitDefaultValue) + // regardless of rootlessness (see cmd/podman/early_init_linux.go). max := define.RLimitDefaultValue var rlimit unix.Rlimit if err := unix.Getrlimit(unix.RLIMIT_NOFILE, &rlimit); err == nil { - if max < rlimit.Max || unshare.IsRootless() { + if rlimit.Max < max { max = rlimit.Max } } else { @@ -1014,17 +1016,23 @@ func addRlimits(ulimit []string, g *generate.Generator, defaultUlimits []string) } g.AddProcessRlimits("RLIMIT_NOFILE", max, max) } - if !nprocSet { + if !nprocSet && unshare.IsRootless() { + // For nproc, podman only sets limits for rootless containers (handling hard and soft limits + // independently). As before, these are set to min(soft/hard limit, RLimitDefaultValue). + current := define.RLimitDefaultValue max := define.RLimitDefaultValue var rlimit unix.Rlimit if err := unix.Getrlimit(unix.RLIMIT_NPROC, &rlimit); err == nil { - if max < rlimit.Max || unshare.IsRootless() { + if rlimit.Max < max { max = rlimit.Max } + if rlimit.Cur < current { + current = rlimit.Cur + } } else { logrus.Warnf("Failed to return RLIMIT_NPROC ulimit %q", err) } - g.AddProcessRlimits("RLIMIT_NPROC", max, max) + g.AddProcessRlimits("RLIMIT_NPROC", max, current) } return nil From 1036cfdd3228eec966ccf11b477fd04fc8b1b85a Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Sat, 15 Jun 2024 22:44:17 +0100 Subject: [PATCH 2/4] Make limits tests have consistent wording This is really a no-op to keep the validate_test check happy... Signed-off-by: Chris Reeves --- tests/run.bats | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/run.bats b/tests/run.bats index 454351048f5..773a040bb12 100644 --- a/tests/run.bats +++ b/tests/run.bats @@ -524,18 +524,17 @@ function configure_and_check_user() { _prefetch alpine - run podman run --rm alpine sh -c "awk '/open files/{print \$4 \"/\" \$5}' /proc/self/limits" - podman_files=$output - run_buildah from --quiet --pull=false $WITH_POLICY_JSON alpine cid=$output + run podman run --rm alpine sh -c "awk '/open files/{print \$4 \"/\" \$5}' /proc/self/limits" + podman_files=$output run_buildah run $cid awk '/open files/{print $4 "/" $5}' /proc/self/limits expect_output "${podman_files}" "limits: podman and buildah should agree on open files" run podman run --rm alpine sh -c "awk '/processes/{print \$3 \"/\" \$4}' /proc/self/limits" podman_processes=$output run_buildah run $cid awk '/processes/{print $3 "/" $4}' /proc/self/limits - expect_output ${podman_processes} "processes should match podman" + expect_output "${podman_processes}" "limits: podman and buildah should agree on processes" run_buildah rm $cid run_buildah from --quiet --ulimit nofile=300:400 --pull=false $WITH_POLICY_JSON alpine From 5241c9255e750f5c2654a5c6d2d4fe2adbe81761 Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Mon, 17 Jun 2024 00:27:23 +0100 Subject: [PATCH 3/4] Always try to set nofile limit Try to set nofile limit to RLimitDefaultValue - this could potentially increase the limit past the current hard limit in non-rootless environments. This makes buildah behaviour match podman when a non-rootless environment has lower limits set. Signed-off-by: Chris Reeves --- run_linux.go | 17 ++++++++++++++--- tests/run.bats | 5 +++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/run_linux.go b/run_linux.go index 009a41bdfdf..a617629eeb3 100644 --- a/run_linux.go +++ b/run_linux.go @@ -1003,10 +1003,21 @@ func addRlimits(ulimit []string, g *generate.Generator, defaultUlimits []string) g.AddProcessRlimits("RLIMIT_"+strings.ToUpper(ul.Name), uint64(ul.Hard), uint64(ul.Soft)) } if !nofileSet { - // For nofile, podman sets both hard and soft limits to min(hard limit, RLimitDefaultValue) - // regardless of rootlessness (see cmd/podman/early_init_linux.go). - max := define.RLimitDefaultValue + // For nofile, podman first tries to set both the hard and soft limits for the current + // process to RLimitDefaultValue - this will be successful in most (but not all) + // non-rootless environments. If this fails (e.g. in a rootless environment) it will ensure + // that the soft limit for the current process is increased to match the hard limit (see + // cmd/podman/early_init_linux.go). We simply fire and forget the call to Setrlimit() here, + // because if it fails we effectively handle setting soft to hard in the call to + // AddProcessRlimits() later on. var rlimit unix.Rlimit + rlimit.Cur = define.RLimitDefaultValue + rlimit.Max = define.RLimitDefaultValue + unix.Setrlimit(unix.RLIMIT_NOFILE, &rlimit) + + // Set both hard and soft limits to min(hard limit, RLimitDefaultValue) regardless of + // rootlessness. + max := define.RLimitDefaultValue if err := unix.Getrlimit(unix.RLIMIT_NOFILE, &rlimit); err == nil { if rlimit.Max < max { max = rlimit.Max diff --git a/tests/run.bats b/tests/run.bats index 773a040bb12..876b3287218 100644 --- a/tests/run.bats +++ b/tests/run.bats @@ -524,6 +524,11 @@ function configure_and_check_user() { _prefetch alpine + # drop limits prior to tests - this tests the ability of non-rootless containers to increase + # file limits to match those of podman + ulimit -S -n 1024 + ulimit -H -n 1024 + run_buildah from --quiet --pull=false $WITH_POLICY_JSON alpine cid=$output run podman run --rm alpine sh -c "awk '/open files/{print \$4 \"/\" \$5}' /proc/self/limits" From 6646b573d651e8c8e9027bf7a7906979ee143591 Mon Sep 17 00:00:00 2001 From: Chris Reeves Date: Mon, 17 Jun 2024 21:34:15 +0100 Subject: [PATCH 4/4] Check return value of Setrlimit() to keep linter happy Signed-off-by: Chris Reeves --- run_linux.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/run_linux.go b/run_linux.go index a617629eeb3..79a9a324a60 100644 --- a/run_linux.go +++ b/run_linux.go @@ -1007,13 +1007,16 @@ func addRlimits(ulimit []string, g *generate.Generator, defaultUlimits []string) // process to RLimitDefaultValue - this will be successful in most (but not all) // non-rootless environments. If this fails (e.g. in a rootless environment) it will ensure // that the soft limit for the current process is increased to match the hard limit (see - // cmd/podman/early_init_linux.go). We simply fire and forget the call to Setrlimit() here, - // because if it fails we effectively handle setting soft to hard in the call to - // AddProcessRlimits() later on. + // cmd/podman/early_init_linux.go). var rlimit unix.Rlimit rlimit.Cur = define.RLimitDefaultValue rlimit.Max = define.RLimitDefaultValue - unix.Setrlimit(unix.RLIMIT_NOFILE, &rlimit) + err := unix.Setrlimit(unix.RLIMIT_NOFILE, &rlimit) + if err != nil { + // We don't really care whether there's an error here, because if it fails we + // effectively handle setting soft to hard in the call to AddProcessRlimits() later on. + logrus.Debugf("Failed to set RLIMIT_NOFILE ulimit %q", err) + } // Set both hard and soft limits to min(hard limit, RLimitDefaultValue) regardless of // rootlessness.