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.

Sign up for GitHub

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

Jump to bottom

Clarify cgroups path handling behavior #834

Closed
wants to merge 1 commit into from
Closed

Clarify cgroups path handling behavior #834

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions config-linux.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,13 +183,15 @@ If a `cgroupsPath` value is specified, the runtime MUST consistently attach to t
Implementations of the Spec can choose to name cgroups in any manner.
The Spec does not include naming schema for cgroups.
The Spec does not support per-controller paths for the reasons discussed in the [cgroupv2 documentation][cgroup-v2].
The cgroups will be created if they don't exist.

You can configure a container's cgroups via the `resources` field of the Linux configuration.
Do not specify `resources` unless limits have to be updated.
The runtime MUST create the cgroups specified by the `cgroupsPath` if they don't exist.
Copy link
Contributor

Choose a reason for hiding this comment

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

Which cgroups? Only those needed to fulfil resources, no? Or MUST they create a new devices cgroup even if there is nothing in linux.resources.devices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They must create all for the use case of orchestrator preparing the cgroups for you and just be asking the runtime to add the container process to the cgroups.

Copy link
Contributor

@wking wking May 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

They must create all for the use case of orchestrator preparing the cgroups for you and just be asking the runtime to add the container process to the cgroups.

How should the runtime distinguish between v1 and v2 cgroups (more on this here)? I think a safer approach for orchestrators would be to tell the runtime to leave cgroups alone and set them up completely in a hook (both creating/initializing any required cgroups and joining the runtime to them). On the other hand, with #237 rejected, there's no clear way to warn the runtime that you'll be messing with cgroups in the hook, so the runtime might be surprised with:

  1. No cgroupsPath or resources, so the runtime creates a new devices cgroup and adds the container process.
  2. Orchestration hook (or other post-create action) creates the cgroups that the orchestrator wants and moves the container process into them.
  3. The runtime looks in the devices cgroup it setup in step 1 and… where did that process go?

Copy link
Member

Choose a reason for hiding this comment

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

They must create all for the use case of orchestrator preparing the cgroups for you and just be asking the runtime to add the container process to the cgroups.

But should the runtime join any extra cgroups? The old language handled this case.

If `cgroupsPath` is empty, then the behavior is runtime implementation specific.
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 you mean “implementation-defined”.

And I liked the old language (where the runtime could pick a default cgroupsPath much more than the huge hole you poke with an unqualified “implementation-defined”. The runtime could drive all sorts of crazy things unrelated to cgroups through this big loophole.

Copy link
Member

Choose a reason for hiding this comment

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

i think this new sentence is fine and not any more of a "loophole" than the old language

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 new sentence is fine and not any more of a "loophole" than the old language

With the new language, config authors should not leave cgroupsPath empty unless they have read the implementation-specific docs for any runtime that might consume their config. With the old language… I'm not sure. It looks like this PR (as of 81c9f31) still has the “MAY define the default” line. So what is this new line about? Is it just underlining that the default may not be a fixed string (e.g. maybe the runtime defaults to the container ID), so if you leave this unset you might get new cgroup or might join an existing cgroup? If so, I'd rather clarify that that is what is being implementation-defined. The current language in 81c9f31 supports runtimes which error out on unset cgroupsPath (or ignore hooks, etc., etc.), and I don't think we want to give that much leeway here.

With the old language, config authors could safely leave cgroupsPath empty if they didn't care

Copy link
Member

Choose a reason for hiding this comment

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

I too prefer the old language. What is the reason for this change @mrunalp?


The runtime MUST ensure that the container process is attached to the cgroups specified by `cgroupsPath`.
Copy link
Member

Choose a reason for hiding this comment

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

In cgroupv1 (which we do support but not explicitly) it is not clear what cgroups are specified by a single path. Could you clarify what was wrong with the old language (which had similar semantics but was more explicit about how different controllers are handled).

Also with cgroupv2 it's not clear what cgroup.controllers should be set to in cgroupsPath. Should it be +everything or just +whatever_is_necessary? The former makes rootless containers no longer possible in an OCI runtime, while the latter should also allow for runtimes to join extra cgroups.

If any property is set under `resources` then the runtime MUST set it for the container.

For example, to run a new process in an existing container without updating limits, `resources` need not be specified.

Runtimes MAY attach the container process to additional cgroup controllers beyond those necessary to fulfill the `resources` settings.

### Example

Expand Down