Allow rootless buildah to set resource limits on cgroup V2 #3594

rhatdan · 2021-10-21T21:36:15Z

First move podman/pkg/cgroups into Buildah.
Only set resources to nil on non cgroupsv2 systems in rootless mode.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

openshift-ci · 2021-10-21T21:36:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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~~ [rhatdan]

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

rhatdan · 2021-10-21T21:39:29Z

@nalind @giuseppe This seems to work, but Buildah is getting a different cgroupfs mounted then Podman.
Podman is getting

$ podman run -ti alpine ls -1 /sys/fs/cgroup/
cgroup.controllers
cgroup.events
cgroup.freeze
cgroup.kill
cgroup.max.depth
cgroup.max.descendants
cgroup.procs
cgroup.stat
cgroup.subtree_control
cgroup.threads
cgroup.type
cpu.max
cpu.max.burst
cpu.pressure
cpu.stat
cpu.weight
cpu.weight.nice
io.bfq.weight
io.latency
io.max
io.pressure
io.prio.class
io.stat
io.weight
memory.current
memory.events
memory.events.local
memory.high
memory.low
memory.max
memory.min
memory.numa_stat
memory.oom.group
memory.pressure
memory.stat
memory.swap.current
memory.swap.events
memory.swap.high
memory.swap.max
pids.current
pids.events
pids.max

But Buildah sees

STEP 3/3: RUN ls /sys/fs/cgroup/
cgroup.controllers
cgroup.max.depth
cgroup.max.descendants
cgroup.procs
cgroup.stat
cgroup.subtree_control
cgroup.threads
cpu.pressure
cpu.stat
cpuset.cpus.effective
cpuset.mems.effective
dev-hugepages.mount
dev-mqueue.mount
init.scope
io.cost.model
io.cost.qos
io.pressure
io.prio.class
io.stat
machine.slice
memory.numa_stat
memory.pressure
memory.stat
misc.capacity
sys-fs-fuse-connections.mount
sys-kernel-config.mount
sys-kernel-debug.mount
sys-kernel-tracing.mount
system.slice
user.slice

Any idea what I am doing wrong?
Tests are looking for /sys/fs/cgroup/cpu.max

giuseppe · 2021-11-03T16:39:23Z

Any idea what I am doing wrong?

are you adding a cgroupns to the container?

With cgroupv2 we do that by default, while on cgroupv1 by default it is --cgroupns=host

rhatdan · 2021-11-04T11:46:15Z

Ok it looks like buildah does not do anything with cgroupns. We need to add that support.
@giuseppe could you take this PR over and add cgroupns, or I will get to it when I have some time.

giuseppe · 2021-11-04T12:30:19Z

I've looked and we already use a cgroupns. I think we are hitting: containers/crun#765

rhatdan · 2021-11-04T13:00:42Z

Do we have a new crun release?

giuseppe · 2021-11-04T13:34:59Z

I'll work on a new one now, I need to fix the CI first :/

I've also found another issue in Buildah while debugging this PR: #3614

giuseppe · 2021-11-04T15:35:23Z

Do we have a new crun release?

tagging one: containers/crun#770

giuseppe · 2021-11-05T19:19:45Z

the new builds are on Bodhi

rhatdan · 2021-12-10T21:09:24Z

Verifying containers/common#854

rhatdan · 2021-12-13T13:34:34Z

@vrothberg Latest containers/common filters patch is passing here now.

rhatdan · 2021-12-14T15:31:08Z

@nalind @giuseppe @flouthoc @vrothberg @TomSweeneyRedHat @umohnani8 This is ready to review.

umohnani8 · 2021-12-14T15:36:07Z

LGTM

nalind · 2021-12-14T15:49:55Z

vendor/github.com/containers/common/pkg/seccomp/types.go

-	DefaultAction   Action `json:"defaultAction"`
+	DefaultAction Action `json:"defaultAction"`
+
+	// DefaultErrnoRet is obsolete, please use DefaultErrno


Do we need to update our seccomp handling in chroot to account for this?

@giuseppe PTAL

nalind · 2021-12-14T15:52:00Z

vendor/github.com/containers/common/libimage/import.go

@@ -61,7 +63,8 @@ func (r *Runtime) Import(ctx context.Context, path string, options *ImportOption
 	u, err := url.ParseRequestURI(path)
 	if err == nil && u.Scheme != "" {
 		// If source is a URL, download the file.
-		file, err := r.downloadFromURL(path)
+		fmt.Printf("Downloading from %q\n", path)


Is this a bug?

@vrothberg PTAL

It's preserving previous behavior, see containers/common@2f3c4bcdfdcf. But I am for changing the behavior. Libraries should not print on stdout.

Well that should happen in a separate PR.

Agreed, that's what I also wrote in the PR of containers/common@2f3c4bcdfdcf. It may require changes to Podman to preserve behavior but I did not investigate.

First move podman/pkg/cgroups into Buildah. Only set resources to nil on non cgroupsv2 systems in rootless mode. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

TomSweeneyRedHat · 2021-12-14T20:25:13Z

The changes in here LGTM. Are we holding this for an update to common to fix the printf and/or updates to Podman?

openshift-ci bot added the approved label Oct 21, 2021

rhatdan force-pushed the group branch from 9ed814f to d3bab41 Compare October 21, 2021 21:42

rhatdan force-pushed the group branch from d3bab41 to 1aeaeef Compare November 1, 2021 17:44

rhatdan force-pushed the group branch from 1aeaeef to 1ef8f39 Compare November 11, 2021 14:52

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2021

rhatdan force-pushed the group branch from 1ef8f39 to 6792979 Compare November 23, 2021 12:50

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2021

rhatdan force-pushed the group branch 3 times, most recently from 4753038 to b899f75 Compare November 23, 2021 19:30

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2021

rhatdan force-pushed the group branch from b899f75 to 560a81e Compare December 1, 2021 19:09

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2021

rhatdan force-pushed the group branch from 560a81e to 29b9812 Compare December 3, 2021 18:08

rhatdan changed the title ~~Allow rootless buildah to set resource limits on cgroup V2~~ [wip] Allow rootless buildah to set resource limits on cgroup V2 Dec 3, 2021

openshift-ci bot added the do-not-merge/work-in-progress label Dec 3, 2021

rhatdan force-pushed the group branch 2 times, most recently from d2bf90d to ed556f0 Compare December 3, 2021 18:54

rhatdan changed the title ~~[wip] Allow rootless buildah to set resource limits on cgroup V2~~ Allow rootless buildah to set resource limits on cgroup V2 Dec 3, 2021

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 3, 2021

rhatdan force-pushed the group branch from 4b28194 to d9b6f7d Compare December 6, 2021 11:44

rhatdan force-pushed the group branch 2 times, most recently from ac7c9f7 to 818a01e Compare December 10, 2021 21:08

rhatdan changed the title ~~Allow rootless buildah to set resource limits on cgroup V2~~ [wip] Allow rootless buildah to set resource limits on cgroup V2 Dec 10, 2021

openshift-ci bot added the do-not-merge/work-in-progress label Dec 10, 2021

rhatdan force-pushed the group branch 4 times, most recently from b42bb16 to 55596e1 Compare December 13, 2021 10:59

rhatdan force-pushed the group branch from 55596e1 to 28caf53 Compare December 14, 2021 15:29

rhatdan changed the title ~~[wip] Allow rootless buildah to set resource limits on cgroup V2~~ Allow rootless buildah to set resource limits on cgroup V2 Dec 14, 2021

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 14, 2021

nalind reviewed Dec 14, 2021

View reviewed changes

rhatdan force-pushed the group branch from 28caf53 to 925329f Compare December 14, 2021 17:33

Allow rootless buildah to set resource limits on cgroup V2

300d9d1

First move podman/pkg/cgroups into Buildah. Only set resources to nil on non cgroupsv2 systems in rootless mode. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

rhatdan force-pushed the group branch from 925329f to 300d9d1 Compare December 14, 2021 17:36

rhatdan added the lgtm label Dec 14, 2021

openshift-merge-robot merged commit 857ba5a into containers:main Dec 14, 2021

github-actions bot added the locked - please file new issue/PR label Sep 14, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow rootless buildah to set resource limits on cgroup V2 #3594

Allow rootless buildah to set resource limits on cgroup V2 #3594

rhatdan commented Oct 21, 2021

openshift-ci bot commented Oct 21, 2021

rhatdan commented Oct 21, 2021 •

edited

giuseppe commented Nov 3, 2021

rhatdan commented Nov 4, 2021

giuseppe commented Nov 4, 2021

rhatdan commented Nov 4, 2021

giuseppe commented Nov 4, 2021

giuseppe commented Nov 4, 2021

giuseppe commented Nov 5, 2021

rhatdan commented Dec 10, 2021

rhatdan commented Dec 13, 2021

rhatdan commented Dec 14, 2021

umohnani8 commented Dec 14, 2021

nalind Dec 14, 2021

rhatdan Dec 14, 2021

nalind Dec 14, 2021

rhatdan Dec 14, 2021

vrothberg Dec 14, 2021

rhatdan Dec 14, 2021

vrothberg Dec 14, 2021

TomSweeneyRedHat commented Dec 14, 2021

Allow rootless buildah to set resource limits on cgroup V2 #3594

Allow rootless buildah to set resource limits on cgroup V2 #3594

Conversation

rhatdan commented Oct 21, 2021

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

openshift-ci bot commented Oct 21, 2021

rhatdan commented Oct 21, 2021 • edited

giuseppe commented Nov 3, 2021

rhatdan commented Nov 4, 2021

giuseppe commented Nov 4, 2021

rhatdan commented Nov 4, 2021

giuseppe commented Nov 4, 2021

giuseppe commented Nov 4, 2021

giuseppe commented Nov 5, 2021

rhatdan commented Dec 10, 2021

rhatdan commented Dec 13, 2021

rhatdan commented Dec 14, 2021

umohnani8 commented Dec 14, 2021

nalind Dec 14, 2021

Choose a reason for hiding this comment

rhatdan Dec 14, 2021

Choose a reason for hiding this comment

nalind Dec 14, 2021

Choose a reason for hiding this comment

rhatdan Dec 14, 2021

Choose a reason for hiding this comment

vrothberg Dec 14, 2021

Choose a reason for hiding this comment

rhatdan Dec 14, 2021

Choose a reason for hiding this comment

vrothberg Dec 14, 2021

Choose a reason for hiding this comment

TomSweeneyRedHat commented Dec 14, 2021

rhatdan commented Oct 21, 2021 •

edited