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

Update cgroupsPath to cgroupName and clarify the semantics around that. #270

Closed
wants to merge 1 commit into from
+19 −9
Closed

Update cgroupsPath to cgroupName and clarify the semantics around that. #270

Show file tree
Hide file tree
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
20 changes: 15 additions & 5 deletions runtime-config-linux.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,19 +144,29 @@ Also known as cgroups, they are used to restrict resource usage for a container
cgroups provide controls to restrict cpu, memory, IO, pids and network for the container.
For more information, see the [kernel cgroups documentation](https://www.kernel.org/doc/Documentation/cgroups/cgroups.txt).

The path to the cgroups can be specified in the Spec via `cgroupsPath`.
`cgroupsPath` is expected to be relative to the cgroups mount point.
If `cgroupsPath` is not specified, implementations can define the default cgroup path.
The name of the cgroups can be specified in the Spec via `cgroupsName`.
Cgroup names mimic filesystem paths closely since they express a hierarchy of cgroups.
`cgroupsName` is expected to be absolute.
An absolute name is one that is defined from the root cgroup `/`.
For example, `/foo/bar` is acceptable. `foo/bar` is not acceptable.
Allowable characters for cgroup names are,
Alpha numeric ([a-zA-Z0-9]+)
Underscores (_)
Dashes (-)
Periods (.)
In general and unless otherwise specified, regular filesystem path rules apply.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the explicit allowed-character restrictions if we're punting to “regular filesystem rules” (which are a bit more relaxed than the character restrictions you list here)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, a link would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In terms of characters, only the ones mentioned above are supported. But path construction follow filesystem path rules.
@hqhq: What link do you need?

Copy link
Contributor

Choose a reason for hiding this comment

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

On Mon, Dec 28, 2015 at 11:56:49AM -0800, Vish Kannan wrote:

+In general and unless otherwise specified, regular filesystem path rules apply.

In terms of characters, only the ones mentioned above are
supported. But path construction follow filesystem path rules.
@hqhq: What link do you need?

I can't find a reference to this in the Linux kernel docs, but POSIX
says pretty clearly that everything except the null and slash bytes
are fair game as long as the full byte name isn't ‘.’ or ‘..’ 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vishh I was thinking of a link about the definition of "what is the allowable characters of a regular filesystem path", if that does exist. Or I'm also OK if we can't find a proper link because I think that's common enough to be accepted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this is false for systemd, / within the parent path are rejected by the dbus service.


If `cgroupsName` is not specified, implementations can choose to use (or not use) any cgroups.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of this. What if you want to disable the runtime's cgroup handling so you can setup your own cgroups via hooks? See #237 and the associated sub-thread.

Copy link
Contributor

Choose a reason for hiding this comment

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

Clarification: my previous suggestions about disabling runtime cgroup handling involved the resources setting. If the config includes a resources setting but doesn't include cgroupsName, I'm not sure how this proposed line would work. Are some runtimes going to pick a path automatically and apply the configured limitations while others error out saying that cgroupsName wasn't set?

Copy link
Contributor

Choose a reason for hiding this comment

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

@wking IMO, it should mean pick their own default. We need a way to say don't do anything and for that resources == nil and cgroupsName == "" should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Wed, Dec 16, 2015 at 12:57:42PM -0800, Mrunal Patel wrote:

+If cgroupsName is not specified, implementations can choose to use (or not use) any cgroups.

@wking IMO, it should mean pick their own default. We need a way to
say don't do anything and for that resources == nil and cgroupsName
== "" should work.

Or cgroupsName = nil to avoid needing to qualify the slash-start
requirement (see also *string discussion in #233), but yeah.

For the requirements != nil && cgroupsName = nil case, are you also
recommending we keep the current:

If cgroupsName is not specified, implementations can define the
default cgroup path.

instead of the proposed “optional cgroups [and error if the runtime
choses not to use cgroups]” (keeping the current logic makes more
sense to me 1)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@wking Yes, if the resources != nil and no cgrouppath is specified then the runtime should pick a cgroup path on its own to apply those settings. It should be clear that this happens and runtime shouldn't opt out of applying the settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If resources are mentioned, runtimes will have to somehow satisfy them. How they setup and manage cgroups to achieve that can remain outside of the scope of the Spec.

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 [split hierarchy](https://www.kernel.org/doc/Documentation/cgroups/unified-hierarchy.txt).
The cgroups will be created if they don't exist.

```json
"cgroupsPath": "/myRuntime/myContainer"
"cgroupsName": "/foo-runtime/bar.container"
```

`cgroupsPath` can be used to either control the cgroups hierarchy for containers or to run a new process in an existing container.
`cgroupsName` can be used to either control the cgroups for new containers or to run a new process in an existing container.

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.
Expand Down
8 changes: 4 additions & 4 deletions runtime_config_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ type LinuxRuntime struct {
// Resources contain cgroup information for handling resource constraints
// for the container
Resources *Resources `json:"resources,omitempty"`
// CgroupsPath specifies the path to cgroups that are created and/or joined by the container.
// The path is expected to be relative to the cgroups mountpoint.
// If resources are specified, the cgroups at CgroupsPath will be updated based on resources.
CgroupsPath *string `json:"cgroupsPath,omitempty"`
// CgroupsName specifies the name of to cgroups that are created and/or joined by the container.
// The name is expected to be absolute.
// If resources are specified, the CgroupsName cgroups will be updated based on resources.
CgroupsName string `json:"cgroupsName,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This should keep the *string from #233.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? There are only two options here, an empty string "" or a valid name.
There isn't a third system default.

On Mon, Dec 28, 2015 at 12:11 PM, W. Trevor King notifications@github.com
wrote:

In runtime_config_linux.go
#270 (comment):

@@ -25,10 +25,10 @@ type LinuxRuntime struct {
// Resources contain cgroup information for handling resource constraints
// for the container
Resources *Resources json:"resources,omitempty"

  • // CgroupsPath specifies the path to cgroups that are created and/or joined by the container.
  • // The path is expected to be relative to the cgroups mountpoint.
  • // If resources are specified, the cgroups at CgroupsPath will be updated based on resources.
  • CgroupsPath *string json:"cgroupsPath,omitempty"
  • // CgroupsName specifies the name of to cgroups that are created and/or joined by the container.
  • // The name is expected to be absolute.
  • // If resources are specified, the CgroupsName cgroups will be updated based on resources.
  • CgroupsName string json:"cgroupsName,omitempty"

This should keep the *string from #233
#233.

—
Reply to this email directly or view it on GitHub
/~https://github.com/opencontainers/specs/pull/270/files#r48502724.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Mon, Dec 28, 2015 at 12:14:05PM -0800, Vish Kannan wrote:

  • // CgroupsName specifies the name of to cgroups that are created and/or joined by the container.
  • // The name is expected to be absolute.
  • // If resources are specified, the CgroupsName cgroups will be updated based on resources.
  • CgroupsName string json:"cgroupsName,omitempty"

Why? There are only two options here, an empty string "" or a valid name.
There isn't a third system default.

This:

"cgroupsName": "",

seems illegal to me (it's not absolute). If we use missing/null
cgroupsName, we don't have to put in an absolute-path loophole for the
empty string. For more background on nil over empty strings for
“unset”, see 1 and surrounding comments, as well as 2.

// Namespaces contains the namespaces that are created and/or joined by the container
Namespaces []Namespace `json:"namespaces"`
// Devices are a list of device nodes that are created and enabled for the container
Expand Down