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

vishh · 2015-12-16T19:40:03Z

wking · 2015-12-16T19:50:42Z

runtime-config-linux.md

+Underscores (_)
+Dashes (-)
+Periods (.)
+In general and unless otherwise specified, regular filesystem path rules apply.


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)?

Yeah, a link would be better.

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

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.

@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.

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

wking · 2015-12-16T19:56:33Z

I left a few nits inline, but except for 1 this PR is a wash for me.

mrunalp · 2015-12-16T20:33:22Z

runtime-config-linux.md

+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 acceptible. `foo/bar` is not acceptible.


s/acceptible/acceptable/

hqhq · 2015-12-17T13:24:01Z

Any thoughts about how systemd implementation should perform on this? (not a question for this PR, but for the general cgroupsPath/Name definition)

mrunalp · 2015-12-17T17:42:52Z

@hqhq systemd api work with a slice and a scope name, so would have to split the name into those parts and make the call to create a scope.

hqhq · 2015-12-22T07:42:07Z

@mrunalp So we want users on systemd cgroup specify the exactly right cgroupName such as /system.slice/my.slice/app.scope and we need to validate it before apply?
Because for now, we are handling name and parent to assign slice and unitName. I'm facing the problem when working on opencontainers/runc#397 as trying to remove name and parent in runc.

Signed-off-by: Vishnu kannan <vishnuk@google.com>

vishh · 2015-12-28T20:01:49Z

@hqhq @mrunalp: Comment addressed. PTAL.

wking · 2015-12-28T20:11:48Z

runtime_config_linux.go

+	// 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.

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.

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.

vishh · 2016-01-26T18:41:24Z

@mrunalp @crosbymichael: Can you review this patch if you get a chance?

hqhq · 2016-01-27T04:18:14Z

The major problem is it's not working on systemd cgroup. Should that be concerned in spec?

mlaventure · 2016-01-27T16:01:35Z

Couldn't we just state that the format of the path depends on the backend and redirect the user to those for more information?

vishh · 2016-01-27T18:05:49Z

@hqhq: What part of this PR is not working with systemd?

vishh · 2016-01-27T21:33:17Z

In the case of systemd, would it be possible to translate the path into a
systemd compatible name?
The Spec currently aligns closely with Linux APIs. I'd like to stick to
that here as well.

On Wed, Jan 27, 2016 at 8:01 AM Kenfe-Mickaël Laventure <
notifications@github.com> wrote:

Couldn't we just state that the format of the path depends on the backend
and redirect the user to those for more information?

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

mlaventure · 2016-01-27T21:57:28Z

That's certainly doable, although in the unlikely event that systemd changes its syntax or that a new manager chooses something different we would have to update our implementation.

wking · 2016-01-27T22:07:49Z

On Wed, Jan 27, 2016 at 01:57:31PM -0800, Kenfe-Mickaël Laventure wrote:

That's certainly doable, although in the unlikely event that systemd
changes its syntax…

I don't know how much stock I'd put in systemd's commitment to
backwards compatibility ;). Decoupling cgroup managers from the
runtime, so you can use a systemd-driving manager or a
filesystem-driving-manager as you see fit still sounds good to me 1.
As far as I know, progress in that direction is still blocking on a
way to opt-out from runtime cgroup manipulation 2.

 Subject: removal of cgroups from the OCI Linux spec
 Date: Wed, 28 Oct 2015 17:01:59 +0000
 Message-ID: <CAD2oYtO1RMCcUp52w-xXemzDTs+J6t4hS5Mm4mX+uBnVONGDfA@mail.gmail.com>

 Subject: Make runtime cgroups optional (was: removal of cgroups from the OCI Linux spec)
 Date: Thu, 29 Oct 2015 12:44:27 -0700
 Message-ID: <20151029194427.GA30073@odin.tremily.us>

hqhq · 2016-01-28T11:53:46Z

The intention of cgroupsName is to specify which cgroup will the container be in, for this purpose, cgroupfs API and systemd API need different information, a path can be used by cgroupfs API, but all systemd API needs is a slice name and a unit name (a scope according to the rule of systemd).

I'm not sure what to be defined in specs is the best to cover these. Maybe add a new entry like slice?

mrunalp · 2016-01-28T15:13:13Z

We should probably leave this out of the spec and handle this in the runtime.

Sent from my iPhone

On Jan 28, 2016, at 3:53 AM, Qiang Huang notifications@github.com wrote:

The intention of cgroupName is to specify which cgroup will the container be in, for this purpose, cgroupfs API and systemd API need different information, a path can be used by cgroupfs API, but all systemd API needs is a slice name and a unit name (a scope according to the rule of systemd).

I'm not sure what to be defined in specs is the best to cover these. Maybe add a new entry like slice?

—
Reply to this email directly or view it on GitHub.

vishh · 2016-01-28T17:16:17Z

Leave what out of the Spec? We have to include cgroup names right?

On Thu, Jan 28, 2016 at 7:13 AM, Mrunal Patel notifications@github.com
wrote:

We should probably leave this out of the spec and handle this in the
runtime.

Sent from my iPhone

On Jan 28, 2016, at 3:53 AM, Qiang Huang notifications@github.com
wrote:

The intention of cgroupName is to specify which cgroup will the
container be in, for this purpose, cgroupfs API and systemd API need
different information, a path can be used by cgroupfs API, but all systemd
API needs is a slice name and a unit name (a scope according to the rule of
systemd).

I'm not sure what to be defined in specs is the best to cover these.
Maybe add a new entry like slice?

—
Reply to this email directly or view it on GitHub.

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

mlaventure · 2016-01-28T17:27:08Z

@vishh: I think @mrunalp meant to leave the actual format taken by cgroupsName out of the spec, not the field itself. I tend to agree with this.

vishh · 2016-03-04T19:36:34Z

@mrunalp @mlaventure @crosbymichael: How do we make progress here? The spec needs to define a standard way to specify cgroups. Systemd is an implementation detail. We need to standardize on the naming format. Otherwise the Spec and the corresponding bundle will not be portable.
I'd really like for us to not tie the Spec to an specific implementation. We can always construct implementation specific names from a standardized naming format.

mlaventure · 2016-03-04T19:53:50Z

The issue is that atm both implementations are hugely different.

systemd requires a name and a scope (which can't not include any / character) whereas the default linux usage is based on filepath.

So far the only way I can see us reconciliating both of them is by requiring a cgroup name of the form x-y-z then systemd would take the last element as the scope (i.e. z in that example) and everything else as the slice name (i.e. x-y) whereas for fs we would just replace the - with /.

But I do not find this particularly elegant.

vishh · 2016-03-04T21:13:50Z

What about converting the Linux format to systemd compatible one when
required within the runtime?

On Fri, Mar 4, 2016, 11:54 AM Kenfe-Mickaël Laventure <
notifications@github.com> wrote:

The issue is that atm both implementations are hugely different.

systemd requires a name and a scope (which can't not include any /
character) whereas the default linux usage is based on filepath.

So far the only way I can see us reconciliating both of them is by
requiring a cgroup name of the form x-y-z then systemd would take the
last element as the scope (i.e. z in that example) and everything else as
the slice name (i.e. x-y) whereas for fs we would just replace the - with
/.

But I do not find this particularly elegant.

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

hqhq · 2016-07-25T11:48:06Z

Close in favor of #493

wking reviewed Dec 16, 2015
View reviewed changes

mrunalp reviewed Dec 16, 2015
View reviewed changes

hqhq mentioned this pull request Dec 22, 2015

Clarify cgroupsPath to be absolute #266

Closed

vishh force-pushed the cgroupsname branch from c0e01b1 to a9a943b Compare December 28, 2015 20:00

Update cgroupsPath to cgroupName and clarify the semantics around that.

9d9a88b

Signed-off-by: Vishnu kannan <vishnuk@google.com>

vishh force-pushed the cgroupsname branch from a9a943b to 9d9a88b Compare December 28, 2015 20:01

wking reviewed Dec 28, 2015
View reviewed changes

hqhq mentioned this pull request Jan 22, 2016

Replace Cgroup Parent and Name fields by CgroupsPath opencontainers/runc#497

Merged

wking mentioned this pull request Mar 18, 2016

Fixing valid-id in regex opencontainers/runc#647

Merged

cyphar mentioned this pull request Jun 8, 2016

config-linux: clarify cgroup requirements #493

Merged

hqhq closed this Jul 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vishh commented Dec 16, 2015

wking Dec 16, 2015

hqhq Dec 17, 2015

vishh Dec 28, 2015

wking Dec 28, 2015

hqhq Dec 29, 2015

mlaventure Jan 26, 2016

wking commented Dec 16, 2015

mrunalp Dec 16, 2015

vishh Dec 28, 2015

hqhq commented Dec 17, 2015

mrunalp commented Dec 17, 2015

hqhq commented Dec 22, 2015

vishh commented Dec 28, 2015

wking Dec 28, 2015

vishh Dec 28, 2015

wking Dec 28, 2015

vishh commented Jan 26, 2016

hqhq commented Jan 27, 2016

mlaventure commented Jan 27, 2016

vishh commented Jan 27, 2016

vishh commented Jan 27, 2016

mlaventure commented Jan 27, 2016

wking commented Jan 27, 2016

hqhq commented Jan 28, 2016

mrunalp commented Jan 28, 2016

vishh commented Jan 28, 2016

mlaventure commented Jan 28, 2016

vishh commented Mar 4, 2016

mlaventure commented Mar 4, 2016

vishh commented Mar 4, 2016

hqhq commented Jul 25, 2016

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

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

Conversation

vishh commented Dec 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Dec 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hqhq commented Dec 17, 2015

mrunalp commented Dec 17, 2015

hqhq commented Dec 22, 2015

vishh commented Dec 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishh commented Jan 26, 2016

hqhq commented Jan 27, 2016

mlaventure commented Jan 27, 2016

vishh commented Jan 27, 2016

vishh commented Jan 27, 2016

mlaventure commented Jan 27, 2016

wking commented Jan 27, 2016

hqhq commented Jan 28, 2016

mrunalp commented Jan 28, 2016

vishh commented Jan 28, 2016

mlaventure commented Jan 28, 2016

vishh commented Mar 4, 2016

mlaventure commented Mar 4, 2016

vishh commented Mar 4, 2016

hqhq commented Jul 25, 2016