Skip to content

runtime, envelope: add support for cgroup for child processes. #770

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

Closed
wants to merge 3 commits into from

Conversation

flouthoc
Copy link
Contributor

@flouthoc flouthoc commented Jun 3, 2024

  • Allow envelope to accept cgroup manager which gives users opportunity to
    constraint process created by weavelets, which is really helpful in case
    of multi and single deployment where user would want to limit cpu,
    memory usage or other resource usage.

  • multi: create and provide cgroup manager

@flouthoc
Copy link
Contributor Author

flouthoc commented Jun 3, 2024

@rgrandl @mwhittaker While working on one of my project I needed to spawn weavelets under fixed CPU and memory constraints so I added support for that in my private fork, I think this will be a handy feature for upstream. Do you have any opinions on this one ?

Ofcourse I need to harden this PR a bit more so cgroup specs can be accepted from config but I wanted to see if maintainers like this feature or not.

@rgrandl
Copy link
Contributor

rgrandl commented Jun 4, 2024

@flouthoc , very interesting feature. Will discuss this in our weekly meeting and will come back to you.

@flouthoc
Copy link
Contributor Author

@rgrandl just a gentle bump to see if you were able to get time to discuss this one :)

@rgrandl
Copy link
Contributor

rgrandl commented Jun 12, 2024

@flouthoc, we've discussed this.

We were thinking of an alternate solution: would it be possible to create these cgroups beforehand, and then when you start the child process, specify the name of the cgroup(s) under which the process should run?

@flouthoc
Copy link
Contributor Author

@rgrandl Yes sounds reasonable to me, so service-weaver should only contain the part of moving processes to the cgroup, is that correct and user should specify cgroup name via config ?

Also Just curious any specific reason about why cgroup created by the service-weaver is not in the vision ?

flouthoc added 3 commits June 12, 2024 13:55
Allow envelope to accept cgroup manager which gives users oppertunity to
constraint process created by weavelets, which is really helpful in case
of `multi` and `single` deployment where user would want to limit `cpu`,
`memory` usage or other resource usage.

Signed-off-by: flouthoc <[email protected]>
Allow loading cgroups from config and create envelope using `cgroup` if
they are configured.

Signed-off-by: flouthoc <[email protected]>
Run `./dev/build_and_test tidy generate build vet test testrace` on
source

Signed-off-by: flouthoc <[email protected]>
@flouthoc
Copy link
Contributor Author

@rgrandl Done, added change so now it can accept externally created cgroup via config. I have only wired multi deployer for now so its easier to review.

Let me know if config fields look fine then I can also add this feature to single deployer or maybe for single deployer I can open a new PR later.

@flouthoc
Copy link
Contributor Author

Let me know if config fields look fine then I can also add this feature to single deployer or maybe for single deployer I can open a new PR later.

@rgrandl Any thoughts on this ?

@flouthoc
Copy link
Contributor Author

flouthoc commented Jul 9, 2024

Let me know if config fields look fine then I can also add this feature to single deployer or maybe for single deployer I can open a new PR later.

@rgrandl Any thoughts on this ?

Gentle bump on the PR.

@flouthoc
Copy link
Contributor Author

Wondering if anything is pending on this PR from my side. I am just waiting for another review round on this one.

@rgrandl
Copy link
Contributor

rgrandl commented Jul 24, 2024

@flouthoc , sorry for being quiet on this. I've been distracted with some other things. I've been waiting to discuss your change with some other team members, because the design is a little bit intrusive/tricky, but haven't had a chance to do that yet.

Is this a blocker for you? I will come back with comments on this soon.

@flouthoc
Copy link
Contributor Author

@rgrandl No not a blocker at all. This is something which I had in my fork just thought it'd be a nice patch for upstream. Please take your time :)

@rgrandl
Copy link
Contributor

rgrandl commented Nov 5, 2024

This is something we've never managed to agree on as a team. Close it for now.

@rgrandl rgrandl closed this Nov 5, 2024
@flouthoc
Copy link
Contributor Author

flouthoc commented Nov 7, 2024

Sure no issues :)

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.

2 participants