fix ulimit issue #18721

Cydox · 2023-05-29T02:18:05Z

Does this PR introduce a user-facing change?

Fixed an issue where lowering the ulimit -u after a container got created will lead to the container failing to launch.

This touches a lot of code I'm not familiar with, so I'm playing it slow. Starting by adding tests that are supposed to fail on current main.

Cydox · 2023-05-29T03:32:40Z

Looks like the test is "failing successfully"

Cydox · 2023-05-29T05:11:04Z

I pushed the fix now.

There appear to be two different failures here.

The added Container inspect WIP test still fails for the rootfull configuration. (example)
For the rootless configurations the limits test is failing (example)

Cydox · 2023-05-29T05:20:45Z

For 2. (limits test): This test works on my system.

Is there a simple way to get the VM image used in the CI to test locally? The script in /hack/get_ci_vm.sh is asking me for GCP credentials (is it trying to provision a VM on GCP?).

Luap99

@giuseppe PTAL

Luap99 · 2023-05-30T08:49:24Z

pkg/specgen/container_validate.go

@@ -135,38 +133,6 @@ func (s *SpecGenerator) Validate() error {
 	// default:
 	//	return errors.New("unrecognized option for cgroups; supported are 'default', 'disabled', 'no-conmon'")
 	// }
-	invalidUlimitFormatError := errors.New("invalid default ulimit definition must be form of type=soft:hard")
-	// set ulimits if not rootless
-	if len(s.ContainerResourceConfig.Rlimits) < 1 && !rootless.IsRootless() {


Why did you remove this? This sets the limit based on containers.conf defaults so we need to keep it. Although doing this in Validate() looks wrong.

I'm under the impression that it's already set by

podman/pkg/specgenutil/specgen.go

Line 771 in 77bd041

s.Rlimits, err = GenRlimits(c.Ulimit)

Doing a quick test it looks like a ulimit from containers.conf still makes it into a container, so this was redundant. However it's in there twice right now. So I think the containers.conf defaults also make it to:

podman/pkg/specgen/generate/container_create.go

Line 34 in 77bd041

rlimits, err := specgenutil.GenRlimits(rtc.Ulimits())

The issue with the limits being in the list twice is fixed.

Luap99 · 2023-05-30T08:53:12Z

pkg/specgen/generate/oci.go

-	// If not explicitly overridden by the user, default number of open
-	// files and number of processes to the maximum they can be set to
-	// (without overriding a sysctl)
-	if !nofileSet {


I am pretty sure we actually want this. The problem is that we only set it once on create instead of for each start.

So I think this must be moved into func (c *Container) generateSpec() instead

Is there a problem with leaving the ulimits in the spec file empty? The container process should inherit the limits, right?

Is that generateSpec invoked on each run and the spec passed to crun, or is it also saved on disk for the next time the container is started?

Ok, so func (c *Container) generateSpec() from container_internal_common.go gets executed each time the container is started if it's in ContainerStateConfigured or ContainerStateExited. I'll put setting the max ulimits in there and see if that fixes the CI issues for rootless.

I'll also disable the new test for rootfull, as this issue is not really applicable to rootfull.

Looks like this approach works on my end.

Incorporated into latest version of this PR

Luap99 · 2023-05-30T09:00:04Z

For 2. (limits test): This test works on my system.

Is there a simple way to get the VM image used in the CI to test locally? The script in /hack/get_ci_vm.sh is asking me for GCP credentials (is it trying to provision a VM on GCP?).

Yes, the goal is to have a VM that matches the CI environment to 100%. It is only intended for us (RedHat maintainers) as it requires special GCE permission, however I can create a VM for you and add your public ssh key if you really need access.

Cydox · 2023-05-31T02:40:07Z

Alright, ulimits are now set to the maximum possible value (if not overriden) in the generateSpec() function each time the container is started instead of at creation time. Also fixed the issue this PR had with the limits test. The problem was that I was appending limits to the list instead of using g.AddProcessRlimits. If the resulting list of limits had a lower limit before a higher limit, this lead to permission errors.

Also added a test under test/system that starts a container, lowers the ulimit and starts the container a second time. Couldn't get the assert statement properly escaped. It's currently only matching against the number of the ulimit. I would prefer to match against "Soft": $nproc_limit. How do I escape this properly? Could also use jq alternatively.

Assuming all tests pass, this PR is ready from my perspective (except for the matching in the system test).

test/system/045-start.bats

Signed-off-by: Jan Hendrik Farr <[email protected]>

Cydox · 2023-05-31T10:34:07Z

/retest

Only changed the assert in the new system test. Unrelated stuff failed. Looks like a flake to me.

openshift-ci · 2023-05-31T10:34:22Z

@Cydox: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Luap99

LGTM

@giuseppe @umohnani8 PTAL

openshift-ci · 2023-05-31T12:38:41Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

umohnani8 · 2023-05-31T14:54:59Z

Changes LGTM but would like a review from @rhatdan @giuseppe

rhatdan · 2023-05-31T20:49:30Z

/lgtm
Thanks @Cydox

openshift-ci bot added release-note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 29, 2023

Cydox force-pushed the fix-ulimit-pr branch from 4dc5547 to 52fb3e0 Compare May 29, 2023 02:32

Cydox force-pushed the fix-ulimit-pr branch 2 times, most recently from 63f2459 to b282074 Compare May 29, 2023 03:54

Luap99 reviewed May 30, 2023

View reviewed changes

Cydox force-pushed the fix-ulimit-pr branch 2 times, most recently from 0df1936 to ba4776c Compare May 30, 2023 20:43

Cydox mentioned this pull request May 30, 2023

crun: setrlimit RLIMIT_NPROC: Operation not permitted: OCI permission denied (was: toolbox not working with existing containers after upgrade to 38.20230422.1) fedora-silverblue/issue-tracker#460

Closed

Cydox force-pushed the fix-ulimit-pr branch 3 times, most recently from 2d52c4b to 16a9720 Compare May 31, 2023 02:32

Cydox requested a review from Luap99 May 31, 2023 04:21

Luap99 reviewed May 31, 2023

View reviewed changes

test/system/045-start.bats Outdated Show resolved Hide resolved

Cydox force-pushed the fix-ulimit-pr branch from 16a9720 to 4ef102e Compare May 31, 2023 09:08

Cydox changed the title ~~[WIP] fix ulimit issue~~ fix ulimit issue May 31, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 31, 2023

set max ulimits for rootless on each start

f097728

Signed-off-by: Jan Hendrik Farr <[email protected]>

Cydox force-pushed the fix-ulimit-pr branch from 4ef102e to f097728 Compare May 31, 2023 09:21

Luap99 approved these changes May 31, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2023

Luap99 mentioned this pull request May 31, 2023

Add default ulimit test for gen kube #18733

Merged

openshift-ci bot assigned rhatdan May 31, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 31, 2023

openshift-merge-robot merged commit 249f046 into containers:main May 31, 2023

Luap99 mentioned this pull request Jun 1, 2023

Since kernel 6.1.28-rpi4, can't start pods or containers in rootless #18696

Closed

dustymabe mentioned this pull request Jun 5, 2023

Rootless containers cannot be started: RLIMIT_NPROC error coreos/fedora-coreos-tracker#1507

Closed

edsantiago mentioned this pull request Jun 7, 2023

unlinkat/EBUSY/hosed is back (Jan 2023) #17216

Closed

Luap99 mentioned this pull request Jun 30, 2023

Long running container becomes unusable/unable to start again: setrlimit 'RLIMIT_NPROC': Operation not permitted: OCI permission denied #19059

Closed

rugk mentioned this pull request Jul 23, 2023

Cannot enter container – crun: setrlimit RLIMIT_NPROC: Operation not permitted: OCI permission denied containers/toolbox#1312

Closed

Luap99 mentioned this pull request Aug 16, 2023

'podman start <toolbx-container>' fails with 'setrlimit RLIMIT_NPROC: Operation not permitted: OCI permission denied' #19634

Closed

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 Aug 30, 2023

github-actions bot locked as resolved and limited conversation to collaborators Aug 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix ulimit issue #18721

fix ulimit issue #18721

Cydox commented May 29, 2023 •

edited

Loading

Cydox commented May 29, 2023

Cydox commented May 29, 2023 •

edited

Loading

Cydox commented May 29, 2023 •

edited

Loading

Luap99 left a comment

Luap99 May 30, 2023

Cydox May 30, 2023

Cydox May 31, 2023

Luap99 May 30, 2023

Cydox May 30, 2023

Cydox May 30, 2023

Cydox May 30, 2023

Cydox May 31, 2023

Luap99 commented May 30, 2023

Cydox commented May 31, 2023 •

edited

Loading

Cydox commented May 31, 2023 •

edited

Loading

openshift-ci bot commented May 31, 2023

Luap99 left a comment

openshift-ci bot commented May 31, 2023

umohnani8 commented May 31, 2023

rhatdan commented May 31, 2023

fix ulimit issue #18721

fix ulimit issue #18721

Conversation

Cydox commented May 29, 2023 • edited Loading

Does this PR introduce a user-facing change?

Cydox commented May 29, 2023

Cydox commented May 29, 2023 • edited Loading

Cydox commented May 29, 2023 • edited Loading

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 commented May 30, 2023

Cydox commented May 31, 2023 • edited Loading

Cydox commented May 31, 2023 • edited Loading

openshift-ci bot commented May 31, 2023

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 31, 2023

umohnani8 commented May 31, 2023

rhatdan commented May 31, 2023

Cydox commented May 29, 2023 •

edited

Loading

Cydox commented May 29, 2023 •

edited

Loading

Cydox commented May 29, 2023 •

edited

Loading

Cydox commented May 31, 2023 •

edited

Loading

Cydox commented May 31, 2023 •

edited

Loading