Add ulimits to `podman update` #26445

aaron-ang · 2025-06-17T04:38:39Z

Does this PR introduce a user-facing change?

Added `--ulimit` flag to `podman update` allowing users to edit container ulimits.

openshift-ci · 2025-06-17T04:38:45Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aaron-ang
Once this PR has been reviewed and has the lgtm label, please assign tomsweeneyredhat for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

packit-as-a-service · 2025-06-17T04:43:55Z

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

packit-as-a-service · 2025-06-17T04:43:55Z

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

packit-as-a-service · 2025-06-17T04:43:55Z

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

Honny1 · 2025-06-17T08:43:55Z

Hi, I did a quick code check. And you need to move this definition of the ulimit flag to this if statement.

Luap99 · 2025-06-17T12:02:54Z

The commit needs a sign off, see https://github.com/containers/podman/blob/main/CONTRIBUTING.md#sign-your-prs

Also please add the Fixes: #xxx line to your commit message as well.

Luap99 · 2025-06-17T12:03:44Z

test/system/280-update.bats

 # bats test_tags=distro-integration
 @test "podman update - test all options" {
    local cgv=1
    if is_cgroupsv2; then
-        cgv=2;
+        cgv=2


please don't change the formatting of unrelated parts of this file.

@Luap99 does this apply to subsequent formatting changes too?

yes everything that is not related to this specific new test should not be touched as it will make git blame much harder to use.

(Now if anybody has opinions abut using a consistent formatter they can voice their arguments here #26340)

reverted the changes

baude · 2025-06-17T13:19:18Z

test/e2e/update_test.go

@@ -306,4 +306,65 @@ var _ = Describe("Podman update", func() {
 		Expect(env).ToNot(ContainSubstring("FOO"))
 		Expect(env).To(ContainSubstring("PATH="))
 	})
+
+	It("podman update sets ulimits", func() {
+		session := podmanTest.Podman([]string{"run", "-dt", ALPINE})


please consider podmanTest.PodmanExitCleanly here and throughout the new test where applicable

updated to reflect this

test/e2e/update_test.go

aaron-ang · 2025-06-18T06:46:52Z

What is the merge workflow for this project? Should I always squash my commits into a single commit?

Fixes containers#26381 Signed-off-by: Aaron Ang <[email protected]>

Luap99

Does this work correctly with --ulimit host on update? AFAICT it should unset all limts from the spec but the parser sets it to nil which means the update code considers it unset? I think the code likely needs to pass an explicit empty slice in that case?

test/system/280-update.bats

Luap99 · 2025-06-18T09:19:11Z

test/system/280-update.bats

+    # Test persists across container restart
+    run_podman restart $ctrname
+
+    # Verify the ulimits persist after restart
+    run_podman exec $ctrname sh -c "ulimit -n"
+    assert "$output" == "2048" "nofile ulimit persists after restart"
+
+    run_podman exec $ctrname sh -c "ulimit -u"
+    assert "$output" == "512" "nproc ulimit persists after restart"


Note you aren't actually verifying that the ulimit is set correctly for the main container process. exec spawns a new process so it has its own limits compared to the main pid.

You likely need to run something like `sh -c "ulimit -n; ulimit -u; sleep 100" as main process and then check with podman logs that the output has the right values.

Luap99 · 2025-06-18T09:20:23Z

test/system/280-update.bats

+    run_podman 125 update --ulimit nofile:1024:1024 $ctrname
+    assert "$output" =~ "error" "Invalid ulimit syntax should fail"
+
+    run_podman 125 update --ulimit nofile=2048:1024 $ctrname
+    assert "$output" =~ "error" "Invalid ulimit values should fail"


That is a match is bad, checking for just "error" can be anything. You should check for the proper error message so you know the actual right error is being returned to users.

docs/source/markdown/podman-update.1.md.in

Luap99 · 2025-06-18T09:27:33Z

What is the merge workflow for this project? Should I always squash my commits into a single commit?

For a small feature like this yes. In general commits must be logical units and we prefer code, test and docs to be part of the same commit. However if you do some slight refactoring or fix multiple things than they should be in separate commits.

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

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Jun 17, 2025

aaron-ang force-pushed the update-ulimit branch from 8d59487 to 096cd17 Compare June 17, 2025 04:43

aaron-ang force-pushed the update-ulimit branch from 096cd17 to 3c736e7 Compare June 17, 2025 04:51

Luap99 reviewed Jun 17, 2025

View reviewed changes

baude reviewed Jun 17, 2025

View reviewed changes

test/e2e/update_test.go Show resolved Hide resolved

aaron-ang force-pushed the update-ulimit branch 2 times, most recently from 22d4ec1 to 81fe400 Compare June 17, 2025 18:20

aaron-ang marked this pull request as ready for review June 17, 2025 18:23

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 Jun 17, 2025

aaron-ang force-pushed the update-ulimit branch from 81fe400 to 8fbe7b8 Compare June 17, 2025 18:25

Add ulimits to podman update

34c284d

Fixes containers#26381 Signed-off-by: Aaron Ang <[email protected]>

aaron-ang force-pushed the update-ulimit branch from 0969d43 to 34c284d Compare June 18, 2025 06:56

Luap99 reviewed Jun 18, 2025

View reviewed changes

Add ulimits to podman update #26445

Are you sure you want to change the base?

Add ulimits to podman update #26445

Uh oh!

Conversation

aaron-ang commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Does this PR introduce a user-facing change?

Uh oh!

openshift-ci bot commented Jun 17, 2025

Uh oh!

packit-as-a-service bot commented Jun 17, 2025

Uh oh!

packit-as-a-service bot commented Jun 17, 2025

Uh oh!

packit-as-a-service bot commented Jun 17, 2025

Uh oh!

Honny1 commented Jun 17, 2025

Uh oh!

Luap99 commented Jun 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aaron-ang commented Jun 18, 2025

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Luap99 commented Jun 18, 2025

Uh oh!

Uh oh!

Add ulimits to `podman update` #26445

Add ulimits to `podman update` #26445

aaron-ang commented Jun 17, 2025 •

edited

Loading