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

cgroup2: cpuset_v2: skip Apply when no limit is specified #2148

Merged

Conversation

AkihiroSuda
Copy link
Member

Signed-off-by: Akihiro Suda akihiro.suda.cz@hco.ntt.co.jp

Fix #2143

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@@ -22,11 +22,14 @@ func (s *CpusetGroupV2) Name() string {
}

func (s *CpusetGroupV2) Apply(d *cgroupData) error {
if d.config.Resources.CpusetCpus == "" && d.config.Resources.CpusetMems == "" {
return nil
}
dir, err := d.path("cpuset")
Copy link
Member Author

Choose a reason for hiding this comment

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

not directly related to this PR, but I'm not convinced of d.path() logic in v2 Apply() functions.

We should check controller availability and maybe write cgroup.subtree_control as crun does?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

yes, I think we should try to enable controllers that are needed

@giuseppe
Copy link
Member

LGTM

@rhatdan
Copy link
Contributor

rhatdan commented Oct 21, 2019

@cyphar @mrunalp @crosbymichael PTAL

@crosbymichael
Copy link
Member

But should we still join the controllers like in v1, even if no limits are set? Do we still have to join everything to collect stats on the processes?

@AkihiroSuda
Copy link
Member Author

For unified hierarchy, I think joining (writing cgroup.procs) should be managed by Manager rather than each of subsystem, because it is impossible to join into a specific subsystem.

@crosbymichael
Copy link
Member

crosbymichael commented Oct 24, 2019

LGTM

Approved with PullApprove

@AkihiroSuda AkihiroSuda changed the title cpuset_v2: skip Apply when no limit is specified cgroup2: cpuset_v2: skip Apply when no limit is specified Oct 26, 2019
@AkihiroSuda
Copy link
Member Author

I suggest removing Apply() functions from v2 controllers (#2157), but I think this PR can be merged now, as a workaround until we make decision on #2157.

@AkihiroSuda
Copy link
Member Author

@mrunalp @hqhq @dqminh @cyphar PTAL?

@hqhq
Copy link
Contributor

hqhq commented Oct 29, 2019

LGTM

Approved with PullApprove

@hqhq hqhq merged commit d239ca8 into opencontainers:master Oct 29, 2019
This pull request was closed.
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.

cgroup2: v1.0.0-rc.9 doesn't start up without adding +cpuset to /sys/fs/cgroup/cgroup.subtree_control
5 participants