Skip to content

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented Oct 11, 2024

Closes: #1576

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Copy link
Collaborator

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

PS you can see which props runc translates in https://github.com/opencontainers/runc/blob/main/docs/systemd.md#cgroup-v2

@giuseppe
Copy link
Member Author

Thanks for the review and the hint!

I've done some fixes to handle MemoryLow (so now memory->reservation is honored) and added a patch to handle cpu.idle.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe
Copy link
Member Author

giuseppe commented Oct 14, 2024

I've added basic support for devices management as well (barely tested though!). Pushed to trigger the CI

@giuseppe giuseppe force-pushed the more-systemd-props branch 4 times, most recently from 20a568b to 5e5527e Compare October 14, 2024 13:28
@kolyshkin
Copy link
Collaborator

One thing runc does is it checks systemd version to make sure we only set properties that this version of systemd can understand. For example, MemoryZSwapMax is only available since systemd v253 and it should not be set if we're running an older version.

This gets more complicated for distros which backports some systemd stuff, and I haven't found an easy solution to that. Theoretically, we can list all properties of whatever uint and see if the one we're about to set is among them, but as far as I remember I played with it only to realize this is costly (even when done once per container start).

@giuseppe
Copy link
Member Author

One thing runc does is it checks systemd version to make sure we only set properties that this version of systemd can understand. For example, MemoryZSwapMax is only available since systemd v253 and it should not be set if we're running an older version.

This gets more complicated for distros which backports some systemd stuff, and I haven't found an easy solution to that. Theoretically, we can list all properties of whatever uint and see if the one we're about to set is among them, but as far as I remember I played with it only to realize this is costly (even when done once per container start).

I've added a mechanism to detect unsupported properties and cache the result.

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe giuseppe force-pushed the more-systemd-props branch 2 times, most recently from 9464296 to 400a63c Compare October 16, 2024 14:15
introduce a mechanism to detect and register the properties systemd doesn't
support so that we don't attempt to set them next time.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@sohankunkerkar
Copy link
Member

LGTM from the CRI-O side (k8s e2e tests looks green).

@giuseppe
Copy link
Member Author

@rhatdan @flouthoc PTAL

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM

@flouthoc flouthoc merged commit e9c9294 into containers:main Oct 17, 2024
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

use systemd d-bus interface to set limits

4 participants