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

restore: apply resource limits #1399

Merged
merged 1 commit into from
Apr 13, 2017
Merged

Conversation

avagin
Copy link
Contributor

@avagin avagin commented Apr 6, 2017

When C/R was implemented, it was enough to call manager.Set to apply
limits and to move a task. Now .Set() and .Apply() have to be called
separately.

Fixes: 8a740d5 ("libcontainer: cgroups: don't Set in Apply")
Signed-off-by: Andrei Vagin avagin@virtuozzo.com

Cc: @cyphar

When C/R was implemented, it was enough to call manager.Set to apply
limits and to move a task. Now .Set() and .Apply() have to be called
separately.

Fixes: 8a740d5 ("libcontainer: cgroups: don't Set in Apply")
Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
@TomSweeneyRedHat
Copy link

IDK if you need it elsewhere or not for sure, but you're doing
return c.cgroupManager.Set(c.config)
Near line 187 in the Set function and I don't see a call to Apply elsewhere. Given your PR description, do you need to add a corresponding apply() call for that Set call too?

@avagin
Copy link
Contributor Author

avagin commented Apr 7, 2017

@TomSweeneyRedHat .Apply() adds processes into cgroups. container.Set() is called when a container is running, so all processes are in required cgroups already.

@hqhq
Copy link
Contributor

hqhq commented Apr 10, 2017

LGTM

Approved with PullApprove

1 similar comment
@mrunalp
Copy link
Contributor

mrunalp commented Apr 13, 2017

LGTM

Approved with PullApprove

@mrunalp mrunalp merged commit 7814a0d into opencontainers:master Apr 13, 2017
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.

4 participants