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

Add cgroup limiting to Linux Executor #52

Closed
wants to merge 49 commits into from
Closed

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Sep 15, 2015

Spike at adding some basic cgroup limits based off of the Tasks Resources

/cc @cbednarski

Added executor interface, includes drop privileges for linux
@catsby
Copy link
Contributor Author

catsby commented Sep 16, 2015

I've updated this PR to move Limit() after Start()in the Exec Driver, and some other cleanups for limiting with cgroups.

I have not looked into any kind of hooks yet.
/cc @dadgar @cbednarski

@dadgar
Copy link
Contributor

dadgar commented Sep 16, 2015

Yeah it doesn't seem like there is a good way to do it with the Manager. The Apply method is actually creating the cgroups. You could just keep Limit the same but move the Apply call on the manager to the Start method.

@cbednarski
Copy link
Contributor

I'm curious what corollary, if any, exists in Windows or FreeBSD. We may have to change this around later anyway, so for now if it's simpler / faster / easier to swap Start and Limit let's do that to save time. We can switch it back later if we need to.

// then placed in the corresponding files.
// Ex: restricting a process to 2048Mhz CPU and 2MB of memory:
// $ cat /sys/fs/cgroup/cpu/user/1000.user/4.session/<uuid>/cpu.shares
// 2028
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be 2048

@dadgar
Copy link
Contributor

dadgar commented Sep 16, 2015

Well there is actually a slightly bigger problem with the way it is done now:
Call Start() -> spawns PID 10
10 can fork/exec() -> creating PID 11
Then we Limit(10) to a cgroup but 11 has escaped it.

That is why you want to build the cgroup first and then launch the pid into the group rather than joining it.

@dadgar
Copy link
Contributor

dadgar commented Sep 16, 2015

Update from slack discussion: Will go with the double fork approach where the intermediate process joins the cgroup, launches the desired process, sending over the child pid to the original process and then killing itself.

I will be implementing this

@catsby
Copy link
Contributor Author

catsby commented Sep 17, 2015

Will you be building off this and sending a PR here, or should I close this?

@dadgar
Copy link
Contributor

dadgar commented Sep 17, 2015

Yeah I am building on that branch! Thanks for asking!
On Wed, Sep 16, 2015 at 6:29 PM Clint [email protected] wrote:

Will you be building off this and sending a PR here, or should I close
this?


Reply to this email directly or view it on GitHub
#52 (comment).

@dadgar
Copy link
Contributor

dadgar commented Sep 20, 2015

Closed in favor of #68

@dadgar dadgar closed this Sep 20, 2015
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants