Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

additional cgroup testing, implementation of cpu-shares, and introduction of blkio limits. #1082

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

cdoern
Copy link
Contributor

@cdoern cdoern commented Jun 30, 2022

added some tests for more resource limits and put translation of cpu-weight in there. Podman and runc both only use cpu-shares but systemd has replaced CPUShares with CPUWeight in more recent versions, they are the same thing it seems.

Signed-off-by: Charlie Doern cdoern@redhat.com

@rhatdan
Copy link
Member

rhatdan commented Jun 30, 2022

LGTM
/approve

@cdoern cdoern force-pushed the cgroup branch 2 times, most recently from e303216 to 09c91eb Compare July 1, 2022 13:37
@cdoern
Copy link
Contributor Author

cdoern commented Jul 1, 2022

repushed with cpuset variables all equal to "0", testing just one node.

pkg/cgroups/systemd_linux.go Outdated Show resolved Hide resolved
@cdoern
Copy link
Contributor Author

cdoern commented Jul 5, 2022

/hold

working on an io implementation on this branch. I found an issue in godbus that i think needs to be patched first @giuseppe PTAL at godbus/dbus#328

@cdoern cdoern changed the title additional cgroup testing and implementation of cpu-shares additional cgroup testing, implementation of cpu-shares, and introduction of blkio limits. Jul 6, 2022
@cdoern
Copy link
Contributor Author

cdoern commented Jul 6, 2022

testing will fail until godbus/dbus#329 is merged.

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, code LGTM.

Could you split the commit first line and add the extra information as the body of the commit message?

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 6, 2022

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@cdoern
Copy link
Contributor Author

cdoern commented Jul 6, 2022

seems like the tests pass even when the value of the property is ["INVALID"] going to see if I can check it in some other way

@cdoern cdoern force-pushed the cgroup branch 6 times, most recently from 13a3dae to b73f940 Compare July 7, 2022 20:22
added support cpu-shares/weight and most Blkio throttle/weight devices.
podman currently support ThrottleReadBps devices so I added those, writeBps, and weight devices.
write/read iops seems to be lesser supported but will see if that is a possibility in the future.

this involved some rewiring in godbus and some questions in systemd. Seems like systemd is pretty rigorous with their
device paths, requiring a full path to either char or block as well as the major:minor. This was difficult to debug and will most likely lead to design limitations in the future.

Signed-off-by: Charlie Doern <cdoern@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Jul 8, 2022

/lgtm

@rhatdan
Copy link
Member

rhatdan commented Jul 8, 2022

/hold cancel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants