podman cgroup enhancement #14654

cdoern · 2022-06-17T19:30:20Z

currently, setting any sort of resource limit in a pod does nothing. With the newly refactored creation process in c/common, podman can now set resources at a pod level
meaning that resource related flags can now be exposed to podman pod create.

cgroupfs and systemd are both supported with varying completion. cgroupfs is a much simpler process and one that is virtually complete for all resource types, the flags now just need to be added. systemd on the other hand
has to be handeled via the dbus api meaning that the limits need to be passed as recognized properties to systemd. The properties added so far are the ones that podman pod create supports as well as cpuset-mems as this will
be the next flag I work on.

in this PR, runc is vendored in as a replacement rather than a requirement. This is because the changes this PR depends on within runc have not been added in a point or major release yet. Vendoring in the commit as a requirement causes regressions in other important repositories.

Signed-off-by: Charlie Doern [email protected]

Does this PR introduce a user-facing change?

users will be able to see a tangible change when settings resource limits on pods

cdoern · 2022-06-17T19:31:20Z

Just making a draft PR of this to see how CI likes it (probably won't) @giuseppe PTAL!

cc @edsantiago I could use some help on the system tests if you get a chance, not sure if what I did is legal.

edsantiago

GAH. Stupid github.

test/system/200-pod.bats

edsantiago · 2022-06-17T19:39:36Z

Review + force-push = collision, and I don't have that much time today (you caught me just before I moved on to other things).

Suggestion:

$ hack/bats 200:resource

This will let you test on your development environment. G'luck.

cdoern · 2022-06-17T19:46:19Z

thanks for the tips @edsantiago ! just wanted to get a head start on this for next week

cdoern · 2022-06-21T18:58:36Z

@giuseppe I think this might be ready for a formal review

also I need size approval for this PR @containers/podman-maintainers (250kb I think) This is about the sze we decided on in containers/common#936

giuseppe

Great work. I had a look at the PR and it seems to work well.

Should we also expose CLI flags like --memory? I do not see it for pod create

libpod/oci_conmon_linux.go

cdoern · 2022-06-22T15:50:14Z

Great work. I had a look at the PR and it seems to work well.

Should we also expose CLI flags like --memory? I do not see it for pod create

@giuseppe I did not expose that flag yet as I was thinking of doing it (and some other resource ones) in a separate PR to keep this one easier to understand.

giuseppe · 2022-06-23T09:17:14Z

@giuseppe I did not expose that flag yet as I was thinking of doing it (and some other resource ones) in a separate PR to keep this one easier to understand.

sure, it makes sense. We can expose the other knobs later.

giuseppe

LGTM

The bloat_approved label was already discussed in the containers/common PR that added the dependency

openshift-ci · 2022-06-24T17:27:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, giuseppe

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

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

cdoern · 2022-06-24T18:53:25Z

@giuseppe do you think this is ok (for now) with the two replace directives?

giuseppe · 2022-06-24T19:24:45Z

@giuseppe do you think this is ok (for now) with the two replace directives?

I am fine with them, although, couldn't we just put them in the require block? Isn't enough to update c/common? Isn't the updated runc a dependency for c/common?

cdoern · 2022-06-24T19:31:59Z

@giuseppe I think podman needed the runc replaced too since it calls it in stats.go. I am fine with either the replace or the require if it works.

EDIT: yeah, c/common works in the require but runc does not (c/image or something must be reverting it). I will leave that as a replace.

currently, setting any sort of resource limit in a pod does nothing. With the newly refactored creation process in c/common, podman ca now set resources at a pod level meaning that resource related flags can now be exposed to podman pod create. cgroupfs and systemd are both supported with varying completion. cgroupfs is a much simpler process and one that is virtually complete for all resource types, the flags now just need to be added. systemd on the other hand has to be handeled via the dbus api meaning that the limits need to be passed as recognized properties to systemd. The properties added so far are the ones that podman pod create supports as well as `cpuset-mems` as this will be the next flag I work on. Signed-off-by: Charlie Doern <[email protected]>

cdoern · 2022-06-27T11:52:56Z

@containers/podman-maintainers PTAL

vrothberg

/lgtm
/hold

Before merging. @cdoern can you elaborate on the go.mod change? Would a require do the trick as well?

vrothberg · 2022-06-27T12:31:39Z

go.mod

@@ -75,3 +75,5 @@ require (
 )

 require github.com/docker/libnetwork v0.8.0-dev.2.0.20190625141545-5a177b73e316 // indirect
+
+replace github.com/opencontainers/runc => github.com/opencontainers/runc v1.1.1-0.20220617142545-8b9452f75cbc


Why is the replace needed?

I have a feeling this is no longer needed. We should be able to use runc v1.1.3, @cdoern Could you repush and see if this works?

@rhatdan 1.1.3 did not include the two changes we need. Not sure why but runc chose not to pick those commits for the point release.

vrothberg · 2022-06-27T12:33:25Z

c/image or something must be reverting it

Just saw the comment now. I really want us to figure out what that is now. Otherwise, we are just deferring work that must be done at some point.

cdoern · 2022-06-27T13:00:14Z

@vrothberg let me try and see what is causing the vendoring weirdness. @rhatdan any ideas? seems like the same issue as we had in c/common where either c/image or another repo is forcing a particular runc version.

cdoern · 2022-06-27T14:38:24Z

in this PR, runc is vendored in as a replacement rather than a requirement. This is because the changes this PR depends on within runc have not been added in a point or major release yet. Vendoring in the commit as a requirement causes regressions in other important repositories.

rhatdan · 2022-06-27T14:50:14Z

/hold cancel.
We just need to remember to remove this replace when runc ships a new version.

cdoern · 2022-06-27T15:10:08Z

/hold cancel

I think this is blocking the CI bot, review was addressed a while back.

The replace was needed when working on github.com/containers/podman/pull/14654 but is not anymore. Drop the replace to consume the correct version of runc. Signed-off-by: Valentin Rothberg <[email protected]>

The replace was needed when working on github.com/containers/pull/14654 but is not anymore. Drop the replace to consume the correct version of runc. Signed-off-by: Valentin Rothberg <[email protected]>

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, 2022

cdoern force-pushed the cgroup branch from 16a4768 to 2dd95d2 Compare June 17, 2022 19:37

edsantiago previously requested changes Jun 17, 2022

View reviewed changes

test/system/200-pod.bats Show resolved Hide resolved

test/system/200-pod.bats Show resolved Hide resolved

test/system/200-pod.bats Outdated Show resolved Hide resolved

test/system/200-pod.bats Outdated Show resolved Hide resolved

test/system/200-pod.bats Outdated Show resolved Hide resolved

cdoern force-pushed the cgroup branch from 2dd95d2 to 86e3cdd Compare June 21, 2022 18:58

giuseppe reviewed Jun 22, 2022

View reviewed changes

libpod/oci_conmon_linux.go Show resolved Hide resolved

cdoern force-pushed the cgroup branch 4 times, most recently from 934023d to c93e632 Compare June 22, 2022 20:45

cdoern force-pushed the cgroup branch 2 times, most recently from 3f3359c to 65f4667 Compare June 24, 2022 13:34

cdoern marked this pull request as ready for review June 24, 2022 13:34

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 24, 2022

cdoern added the bloat_approved Approve a PR in which binary file size grows by over 50k label Jun 24, 2022

giuseppe approved these changes Jun 24, 2022

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2022

cdoern force-pushed the cgroup branch from 65f4667 to 2792e59 Compare June 24, 2022 19:39

vrothberg mentioned this pull request Jun 27, 2022

new warning message about mismatching expected and actual platform doesn't "canonicalize" the input before comparison (ie aarch64 vs arm64) #14669

Closed

vrothberg reviewed Jun 27, 2022

View reviewed changes

openshift-ci bot assigned vrothberg Jun 27, 2022

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jun 27, 2022

giuseppe mentioned this pull request Jun 27, 2022

volume: add two new options copy and nocopy #14734

Merged

cdoern added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 27, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2022

openshift-ci bot merged commit 088665d into containers:main Jun 27, 2022

vrothberg mentioned this pull request Oct 18, 2022

go.mod: drop runc replace containers/common#1196

Closed

vrothberg mentioned this pull request Oct 18, 2022

go.mod: drop runc replace #16212

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 Sep 21, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

podman cgroup enhancement #14654

podman cgroup enhancement #14654

cdoern commented Jun 17, 2022 •

edited

Loading

cdoern commented Jun 17, 2022

edsantiago left a comment

edsantiago commented Jun 17, 2022

cdoern commented Jun 17, 2022

cdoern commented Jun 21, 2022 •

edited

Loading

giuseppe left a comment

cdoern commented Jun 22, 2022

giuseppe commented Jun 23, 2022

giuseppe left a comment

openshift-ci bot commented Jun 24, 2022

cdoern commented Jun 24, 2022

giuseppe commented Jun 24, 2022

cdoern commented Jun 24, 2022 •

edited

Loading

cdoern commented Jun 27, 2022

vrothberg left a comment

vrothberg Jun 27, 2022

rhatdan Jun 27, 2022

cdoern Jun 27, 2022

vrothberg commented Jun 27, 2022

cdoern commented Jun 27, 2022 •

edited

Loading

cdoern commented Jun 27, 2022

rhatdan commented Jun 27, 2022

cdoern commented Jun 27, 2022

podman cgroup enhancement #14654

podman cgroup enhancement #14654

Conversation

cdoern commented Jun 17, 2022 • edited Loading

Does this PR introduce a user-facing change?

cdoern commented Jun 17, 2022

edsantiago left a comment

Choose a reason for hiding this comment

edsantiago commented Jun 17, 2022

cdoern commented Jun 17, 2022

cdoern commented Jun 21, 2022 • edited Loading

giuseppe left a comment

Choose a reason for hiding this comment

cdoern commented Jun 22, 2022

giuseppe commented Jun 23, 2022

giuseppe left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 24, 2022

cdoern commented Jun 24, 2022

giuseppe commented Jun 24, 2022

cdoern commented Jun 24, 2022 • edited Loading

cdoern commented Jun 27, 2022

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg Jun 27, 2022

Choose a reason for hiding this comment

rhatdan Jun 27, 2022

Choose a reason for hiding this comment

cdoern Jun 27, 2022

Choose a reason for hiding this comment

vrothberg commented Jun 27, 2022

cdoern commented Jun 27, 2022 • edited Loading

cdoern commented Jun 27, 2022

rhatdan commented Jun 27, 2022

cdoern commented Jun 27, 2022

cdoern commented Jun 17, 2022 •

edited

Loading

cdoern commented Jun 21, 2022 •

edited

Loading

cdoern commented Jun 24, 2022 •

edited

Loading

cdoern commented Jun 27, 2022 •

edited

Loading