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

[WIP] Initial implementation of resource-control via systemd and dbus #120

Closed
wants to merge 1 commit into from

Conversation

nimrodshn
Copy link
Contributor

@nimrodshn nimrodshn commented Jul 2, 2021

What this does?

Adds easy API for interacting with systemd via dbus (namely creating units), using this we can apply resource control via systemd.

Please note: I try to align with runc's approach in this line of work to walk in the steps of giants.

TODO:

  • Implement the "controllers" abstraction for each resource supported by systemd properties. (Traits and respective implementation).
  • Finish implementing start_transient_unit_for_container which eventually applies the properties to the newly created unit associated with the container (the last for loop doesn't compile in the current vesion 😢 ). The bindings generated to the dbus interfaces are used using the following command: > dbus-codegen-rust -s -g -m None -d org.freedesktop.systemd1 -p /org/freedesktop/systemd1 (See https://github.com/diwic/dbus-rs)

Related issue: #24

cc: @utam0k @Furisto

@nimrodshn nimrodshn changed the title [WIP] Initial implementation of systemd resource-control via dbus [WIP] Initial implementation of container resource-control via systemd and dbus Jul 2, 2021
@nimrodshn nimrodshn changed the title [WIP] Initial implementation of container resource-control via systemd and dbus [WIP] Initial implementation of resource-control via systemd and dbus Jul 2, 2021
src/dbus/client.rs Outdated Show resolved Hide resolved
@utam0k utam0k requested review from Furisto and utam0k July 3, 2021 01:13
@nimrodshn nimrodshn force-pushed the implement_systemd_dbus branch 3 times, most recently from a7c60f6 to 8810669 Compare July 3, 2021 18:01
@utam0k
Copy link
Member

utam0k commented Jul 17, 2021

@nimrodshn Is there anything I can do to help?

@nimrodshn
Copy link
Contributor Author

@utam0k Sorry I was busy with other projects 😢 I will try and get some work on this soon..

@utam0k
Copy link
Member

utam0k commented Jul 18, 2021

@nimrodshn I understand. You can do it at your own pace. I was a little worried because you've been contributing to youki quite a bit.

@nimrodshn
Copy link
Contributor Author

nimrodshn commented Jul 18, 2021

@utam0k Sorry I was busy with life 😅 . I will try to advance this asap. Dont worry - I'm here 😸

I was a little worried because you've been contributing to youki quite a bit.

@Furisto suggested I make the resource control part modular (similar to how we provide Controller impls for each controller type, e.g. memory, io etc.) in the v1/v2 cgroups managers. How do you envision that API work here? Should we pass a list of controllers to start_transient_unit_for_container in addition to LinuxResources? As These are provided statically in each cgroups manager (For example here.) - I think that might be a simple solution that way this dbus client could be used both for v1 and v2 systemd cgroup managers.

@Furisto
Copy link
Collaborator

Furisto commented Jul 18, 2021

@nimrodshn My idea was that the dbus client should not know anything about resources or controllers. The responsibility of the dbus client is to communicate with systemd and nothing else. The responsibility of the controllers is to translate the resource restrictions described in the spec to systemd properties.
That means they get passed in a Resource and return back properties. We collect these properties from the controllers (and maybe additional ones that are created by the cgroup manager directly like for pids) and hand them over do the dbus client. Should systemd support further resources in the future we can just add another controller without having to touch the surrounding code. It also means that v1 and v2 controllers can generate different properties, but the dbus client does not need to have any special handling for this as it just receives a list of properties.

You do not need to implement it exactly like this though, if you think you have a better idea, go for it.

@nimrodshn nimrodshn force-pushed the implement_systemd_dbus branch 2 times, most recently from 37fabbe to 5154b60 Compare July 25, 2021 05:53
@utam0k utam0k added the enhancement New feature or request label Aug 31, 2021
@Furisto
Copy link
Collaborator

Furisto commented Sep 5, 2021

@nimrodshn Can I support you in any way?

@nimrodshn
Copy link
Contributor Author

@Furisto No I'm good thanks! I just came back to this 😄

@utam0k
Copy link
Member

utam0k commented Sep 24, 2021

@nimrodshn Hi! Can I ask you to write out the remaining work?

@nimrodshn
Copy link
Contributor Author

@utam0k Wrote the work to be done in the PRs description ⬆️

@utam0k
Copy link
Member

utam0k commented Sep 29, 2021

@nimrodshn Thanks! Let me know if you are busy or need any help. This isn't going to be included in the first release of youki, so taking it slow is fine. Please don't feel pressured. We need your help, I'll be waiting for you to come back ;)

@nimrodshn
Copy link
Contributor Author

Thanks @utam0k currently finding it hard to work on open source 😿 as I'm in between jobs so taking it one step at a time 😸

@utam0k
Copy link
Member

utam0k commented Sep 29, 2021

Thanks @utam0k currently finding it hard to work on open source as I'm in between jobs so taking it one step at a time

Of course, there are times like that. No problem. I'm waiting for this PR to be open, so let me know when you have more time. If you'll have plans to resume trying this issue, please let me know.

@utam0k utam0k marked this pull request as draft September 29, 2021 12:28
@utam0k
Copy link
Member

utam0k commented Oct 24, 2021

@Furisto @yihuaf
This PR has been pretty much left out of the main codes, so leaving it any longer would be a huge burden on @nimrodshn. I hope he will take on another challenge when he has more time to contribute to youki.
Would you be interested in taking over this PR? I'd like to remain his commits and take over.

@yihuaf
Copy link
Collaborator

yihuaf commented Oct 24, 2021

@Furisto seems to have more context on this than I do. If @Furisto is busy, I can certainly help to finish this. Just let me know :)

@Furisto
Copy link
Collaborator

Furisto commented Oct 24, 2021

@utam0k @yihuaf Sure, I can take over.

@utam0k
Copy link
Member

utam0k commented Oct 24, 2021

@Furisto Thank you for accepting it. I have assigned you this issue. Can I leave it to you to decide how to proceed?

@Furisto
Copy link
Collaborator

Furisto commented Oct 24, 2021

@utam0k Sure

@utam0k
Copy link
Member

utam0k commented Nov 6, 2021

We'll continue here. @nimrodshn Thanks a lot.
#451

@utam0k utam0k closed this Nov 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants