Fix cgroups value types in the spec. #233

vishh · 2015-10-27T17:59:47Z

wking · 2015-10-27T18:16:54Z

On Tue, Oct 27, 2015 at 10:59:48AM -0700, Vish Kannan wrote:

Fix cgroups value types in the spec.

This unwinds a lot of type changes from #199. Are we sure we want to
use “-1 means skip this” instead of “just don't set this field in the
JSON to skip it”? And then use pointers or protobuf with optional
fields for the Go?

vishh · 2015-10-27T18:20:59Z

Unless these field become pointers, I do not see how I can skip these
fields in json.

On Tue, Oct 27, 2015 at 11:17 AM, W. Trevor King notifications@github.com
wrote:

On Tue, Oct 27, 2015 at 10:59:48AM -0700, Vish Kannan wrote:

Fix cgroups value types in the spec.

This unwinds a lot of type changes from #199. Are we sure we want to
use “-1 means skip this” instead of “just don't set this field in the
JSON to skip it”? And then use pointers or protobuf with optional
fields for the Go?

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

wking · 2015-10-27T18:35:06Z

On Tue, Oct 27, 2015 at 11:21:02AM -0700, Vish Kannan wrote:

Unless these field become pointers, I do not see how I can skip
these fields in json.

I'm fine with pointers (and I expect Go generated from a protobuf
schema will use something like that too).

vbatts · 2015-10-27T20:36:49Z

ah no. int64 makes more sense here. Pointers is a bit too much complication here.

LGTM

wking · 2015-10-27T20:48:45Z

On Tue, Oct 27, 2015 at 01:36:50PM -0700, Vincent Batts wrote:

ah no. int64 makes more sense here. Pointers is a bit too much
complication here.

I'm fine punting this to the protobuf transition (at which point these
fields will become optional as far as the parser is concerned), but I
think a bit of extra complication on the bindings side is a fair price
to pay for cleaner semantics on the JSON/spec side.

hqhq · 2015-10-28T03:46:26Z

I don't understand why we take -1 as unlimited? What's the intention here?
With this change, we need to change some codes in runc as well, I just didn't see the necessity.
@vishh Could you elaborate?

vishh · 2015-10-28T19:43:10Z

@hqhq: Sometimes, we want to use the system defaults for certain cgroups values. The default value of 0 in go cannot be used to express that intent, because 0 can be an explicit value that the user has set. Using -1 to use the system defaults is just one possible way to address this. Other options are, using a complex type that can understand the notion of unspecified, or convert cgroup fields into key value pairs in a map, etc.

hqhq · 2015-10-29T06:10:18Z

@vishh I understand and agree with this intention, but so far there is only swappiness would take 0 as an explicit value that user has set, and I think this would be the only one with the latest kernel support. So can we just fix Swappiness type to int64 and leave the others? Otherwise there would be backward compatibility problems for runc users like Docker.

vishh · 2015-10-29T17:42:12Z

Without this change, how do you propose that a user will express their intent of using the system defaults?

hqhq · 2015-10-30T01:06:39Z

@vishh Except exception like Swappiness, all other system defaults are invalid, using them will be taken as equally as not using them, they are expressed in docs.

cyphar · 2015-10-30T14:07:09Z

Requiring to explicitly set -1 to mean "I want the default" breaks the backwards-compatibility of the spec. If a user wishes to update the spec version they have to merge their old version with the new version with -1 set. Surely it makes more sense to set the JSON defaults value (0) to be the system default? I understand swappiness is an exception, but why change the rest? Also, I'm fairly sure that some of those settings actually accept uint64 values, so what are we going to do in that case?

If we had to do this, surely a better way would be to set them to null (so we use pointers -- AND we get the choice of having the JSON default producing the system default value) and then in consumers of the spec, they could then translate the spec to their own struct which uses uint64 with the defaults set for values which were null.

vishh · 2015-10-30T16:42:48Z

Hmm. Considering that the Spec is still not v1.0, do we really care about
backwards compat? There are extensive changes being proposed which will
definitely break backwards compat.
I feel using different values as defaults will add cognitive burden to
users of the Spec.
That said, if all other maintainers and users feel that the cognitive
burden isn't a big deal, I can rework this PR.

The general issue here is the fact that the Spec is wrapping the linux APIs
without adding much value.

On Fri, Oct 30, 2015 at 7:07 AM, Aleksa Sarai notifications@github.com
wrote:

Requiring to explicitly set -1 to mean "I want the default" breaks the
backwards-compatibility of the spec. If a user wishes to update the spec
version they have to merge their old version with the new version with -1
set. Surely it makes more sense to set the JSON defaults value (0) to be
the system default? I understand swappiness is an exception, but why
change the rest? Also, I'm fairly sure that some of those settings actually
accept uint64 values, so what are we going to do in that case?

If we had to do this, surely a better way would be to set them to null
(so we use pointers -- AND we get the choice of having the JSON default
producing the system default value) and then in consumers of the spec, they
could then translate the spec to their own struct which uses uint64 with
the defaults set for values which were null.

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

wking · 2015-10-30T18:14:13Z

On Fri, Oct 30, 2015 at 09:42:51AM -0700, Vish Kannan wrote:

I feel using different values as defaults will add cognitive burden
to users of the Spec.

Using pointers in the Go types so ‘null’ and missing are valid JSON
for “don't touch this” is consistent and has a low cognitive burden.
It just has a slightly higher implementation burden, but I don't
think the increase there is too big an ask.

cyphar · 2015-10-31T00:44:32Z

@vishh

Hmm. Considering that the Spec is still not v1.0, do we really care about
backwards compat? There are extensive changes being proposed which will
definitely break backwards compat.

Ah, fair enough. But there are other reasons why I don't agree with using -1 to mean "use the default".

I feel using different values as defaults will add cognitive burden to
users of the Spec.

TBH, I feel like -1 is more of a cognitive burden, because omitting an option in the spec would mean "use the actual value 0, not the default". Explicitly setting "I want the default" IMHO makes more intuitive sense if it's a clearly non-integer value like null.

wking · 2015-10-31T03:46:07Z

On Fri, Oct 30, 2015 at 05:44:34PM -0700, Aleksa Sarai wrote:

TBH, I feel like -1 is more of a cognitive burden, because
omitting an option in the spec would mean "use the actual value 0,
not the default". Explicitly setting "I want the default" IMHO makes
more intuitive sense if it's a clearly non-integer value like
null.

I may be misunderstanding, but it sounds like you're saying:

{
"linux": {
"resources": {
"memory": {
"limit": null,
…
}
}
}
}

would mean “don't touch memory.limit_in_bytes”. That makes sense to
me. It also sounds like you're saying:

{
"linux": {
"resources": {
"memory": {
}
}
}
}

through an empty/missing runtime.json would mean “set
memory.limit_in_bytes to zero”. I think all the cases that don't set
a .linux.resources.memory.limit key should also be “don't touch
memory.limit_in_bytes”.

For example, say the 1.0 spec defines cgroups that are pretty much
like we have now, but the 1.1 minor bump adds support for
cpuset.cpu_exclusive. With “unset means use Go's default value for
that type”, every bundle using a 1.0 config will then start disabling
cpuset.cpu_exclusive. With “unset means don't touch it”, bundles with
1.0 configs would keep not touching file, just like they had with 1.0
runtimes.

cyphar · 2015-10-31T04:45:28Z

@wking No, I agree with the proposal of using pointers so that using null and not setting it will both result in the semantic of "use the default". When I said

TBH, I feel like -1 is more of a cognitive burden, because omitting an option in the spec would mean "use the actual value 0, not the default".

i was referring specifically to the current state of this PR (using -1 as the "please use the default" value). Because if you omit an option, Go's default JSON semantic is such that it would mean "use the value 0".

I don't agree this is how it should work, and I'm all for making both setting a value to null and omitting it from the config meaning "use the default".

- add information to cgroup resources controllers with examples - add pids cgroup information and example - reflect kernel types Signed-off-by: Antonio Murdaca <runcom@linux.com>

crosbymichael · 2015-11-09T22:29:49Z

I'm ok with nil for values where the zero value means something for our settings. @vishh what do you think?

vishh · 2015-11-09T22:58:13Z

SGTM.

On Mon, Nov 9, 2015 at 2:29 PM, Michael Crosby notifications@github.com
wrote:

I'm ok with nil for values where the zero value means something for our
settings. @vishh https://github.com/vishh what do you think?

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

cyphar · 2015-11-10T01:05:35Z

@crosbymichael Weren't we talking about making all settings have nil mean "use the default".

crosbymichael · 2015-11-10T01:07:34Z

All or only the ones that make sense. Its so we don't end up with things like memory swap where you have to set it to -1 if your system does not support it or your containers will fail

wking · 2015-11-10T03:49:07Z

On Mon, Nov 09, 2015 at 05:07:36PM -0800, Michael Crosby wrote:

All or only the ones that make sense.

I think “don't set this value” or “don't create this cgroup” are
things that make sense for all of our settings under runtime
linux.resources. Is there anything under there you feel should not be
nullable? It may be easier to have this conversation on a per-setting
basis, so I'm happy to do that (on IRC? On the mailing list?) if it's
easier than trying to decide on large blocks of settings en masse.

cyphar · 2015-11-10T07:18:09Z

@crosbymichael setting memory swap as null means "use the default" which should be read as "use the system default". If it's not implemented, the system default is to not use the limit.

mrunalp · 2015-11-13T19:00:45Z

@cyphar Yes, that's what we agreed to. @vishh Could you update the PR?

crosbymichael · 2015-12-18T18:27:10Z

LGTM

mrunalp · 2015-12-18T18:45:37Z

runtime_config_linux.go

-	// MEM to use within the cpuset
-	Mems string `json:"mems"`
+	// CPU shares (relative weight (ratio) vs. other cgroups with cpu shares).
+	Shares *int64 `json:"shares,omitempty"`


Since these are pointers now, shouldn't we match the kernel data types and have these be unsigned?
If it is nil, it means that we don't touch the value. There is no need for setting these to negative values.

cyphar · 2015-12-18T21:51:16Z

@crosbymichael I'm not a maintainer, but NACK. There are still some issues with types that should be unsigned.

…mpty` json tag. Signed-off-by: Vishnu kannan <vishnuk@google.com>

vishh · 2015-12-19T01:44:01Z

Comments addressed

mrunalp · 2015-12-19T01:50:33Z

LGTM

Fix cgroups value types in the spec.

philips · 2015-12-23T06:25:27Z

thanks for doing this @vishh

The general rule seems to be: If Go's default value has the same semantics we'd use for an unset value, don't bother with a pointer. I'm not sure how well that squares with [1]: We want a consistent way to identify unset settings. But if the falsy values count as "unset", maybe the "null is a consistent identifier for unset" approach was never really viable. I'm also not sure if the new style extends to integers where zero has the same semantics as unset values. It sounds like Michael was ok with no pointers for those values [2], but OOMScoreAdj (where zero clearly means "do nothing") got a pointer in opencontainers#233 [3]. More clarity on the threshold would be nice. [1]: opencontainers#233 (comment) [2]: opencontainers#233 (comment) [3]: https://github.com/opencontainers/specs/pull/233/files#diff-34c30be66233f08b447fb608ea0e66bbR206

The general rule seems to be: If Go's default value has the same semantics we'd use for an unset value, don't bother with a pointer. I'm not sure how well that squares with [1]: We want a consistent way to identify unset settings. But if the falsy values count as "unset", maybe the "null is a consistent identifier for unset" approach was never really viable. I'm also not sure if the new style extends to integers where zero has the same semantics as unset values. It sounds like Michael was ok with no pointers for those values [2], but OOMScoreAdj (where zero clearly means "do nothing") got a pointer in opencontainers#233 [3]. More clarity on the threshold would be nice. [1]: opencontainers#233 (comment) [2]: opencontainers#233 (comment) [3]: https://github.com/opencontainers/specs/pull/233/files#diff-34c30be66233f08b447fb608ea0e66bbR206 Signed-off-by: W. Trevor King <wking@tremily.us>

The general rule seems to be: If Go's default value has the same semantics we'd use for an unset value, don't bother with a pointer. I'm not sure how well that squares with [1]: We want a consistent way to identify unset settings. But if the falsy values count as "unset", maybe the "null is a consistent identifier for unset" approach was never really viable. Qiang points out that pointers are required to opt-out of boolean settings where both true and false would require action [2], so I've worded the exception to only apply when the Go default for the type is expicitly a no-op in the spec. I'm also not sure if the new style extends to integers where zero has the same semantics as unset values. It sounds like Michael was ok with no pointers for those values [3], but OOMScoreAdj (where zero clearly means "do nothing") got a pointer in opencontainers#233 [4]. More clarity on the threshold would be nice; in this commit I've laid out the logic and not explicitly listed the types it applies to. [1]: opencontainers#233 (comment) [2]: https://github.com/opencontainers/specs/pull/317/files#r50932706 [3]: opencontainers#233 (comment) [4]: https://github.com/opencontainers/specs/pull/233/files#diff-34c30be66233f08b447fb608ea0e66bbR206 Signed-off-by: W. Trevor King <wking@tremily.us>

wking mentioned this pull request Oct 28, 2015

config: linux: update description of PidsLimit #234

Merged

yangdongsheng mentioned this pull request Oct 29, 2015

cgroups: add pids controller support opencontainers/runc#58

Merged

cyphar mentioned this pull request Nov 2, 2015

Wrap Linux 4.3 pids cgroup opencontainers/runc#382

Closed

runcom referenced this pull request Nov 4, 2015

runtime: config: linux: add cgroups information

2ce2c86

- add information to cgroup resources controllers with examples - add pids cgroup information and example - reflect kernel types Signed-off-by: Antonio Murdaca <runcom@linux.com>

hqhq mentioned this pull request Nov 12, 2015

Modifying cpu attribute data type as per specs opencontainers/runc#395

Closed

hqhq mentioned this pull request Nov 17, 2015

runtime-config-linux.md: fix the type of cpus and mems #252

Merged

wking mentioned this pull request Nov 17, 2015

change spec test from exact match to greater than opencontainers/runc#406

Closed

cyphar mentioned this pull request Dec 17, 2015

Add PIDs cgroup support to Docker moby/moby#18697

Merged

runcom mentioned this pull request Dec 18, 2015

cgroups resources need update containerd/containerd#45

Closed

mrunalp reviewed Dec 18, 2015
View reviewed changes

Make optional Cgroup related config params pointers along with `omite…

488f174

…mpty` json tag. Signed-off-by: Vishnu kannan <vishnuk@google.com>

vishh force-pushed the uint64 branch from 53087ab to 488f174 Compare December 19, 2015 01:43

mrunalp pushed a commit that referenced this pull request Dec 19, 2015

Merge pull request #233 from vishh/uint64

e298027

Fix cgroups value types in the spec.

mrunalp merged commit e298027 into opencontainers:master Dec 19, 2015

wking mentioned this pull request Dec 21, 2015

source and schema: differentiate with examples #276

Merged

hqhq mentioned this pull request Dec 22, 2015

specs: linux: add intel_rdt cgroup support in specs #267

Closed

wking mentioned this pull request Dec 22, 2015

Fix value of swappiness #279

Merged

Mashimiao mentioned this pull request Dec 30, 2015

fix bugs to parse oci specs zenlint/ocitools-bak#23

Open

wking mentioned this pull request Jan 4, 2016

Add NoNewPrivileges setting for linux #290

Merged

wking mentioned this pull request Jan 26, 2016

style: Document recent Go-pointer exceptions #317

Merged

caiolima mentioned this pull request Feb 9, 2016

It is not possible set memory.swappiness -1 on old kernel opencontainers/runc#543

Closed

cyphar mentioned this pull request Mar 15, 2016

libcontainer: cgroups: deal with unlimited case for pids.max opencontainers/runc#644

Merged

vbatts mentioned this pull request Apr 8, 2016

Anchor examples #348

Merged

wking mentioned this pull request Jun 16, 2016

The default value of cfs_quota_us should be -1, which is negative #499

Closed

cyphar mentioned this pull request Aug 18, 2016

schema/config-linux: add min/max limit for resource memory and cpu #533

Closed

wking mentioned this pull request Jan 12, 2017

Remove string pointers #653

Merged

wking mentioned this pull request Apr 15, 2017

specs-go: Consistent pointer policy for invalid-value detection? #772

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cgroups value types in the spec. #233

Fix cgroups value types in the spec. #233

vishh commented Oct 27, 2015

wking commented Oct 27, 2015

vishh commented Oct 27, 2015

wking commented Oct 27, 2015

vbatts commented Oct 27, 2015

wking commented Oct 27, 2015

hqhq commented Oct 28, 2015

vishh commented Oct 28, 2015

hqhq commented Oct 29, 2015

vishh commented Oct 29, 2015

hqhq commented Oct 30, 2015

cyphar commented Oct 30, 2015

vishh commented Oct 30, 2015

wking commented Oct 30, 2015

cyphar commented Oct 31, 2015

wking commented Oct 31, 2015

cyphar commented Oct 31, 2015

crosbymichael commented Nov 9, 2015

vishh commented Nov 9, 2015

cyphar commented Nov 10, 2015

crosbymichael commented Nov 10, 2015

wking commented Nov 10, 2015

cyphar commented Nov 10, 2015

mrunalp commented Nov 13, 2015

crosbymichael commented Dec 18, 2015

mrunalp Dec 18, 2015

cyphar commented Dec 18, 2015

vishh commented Dec 19, 2015

mrunalp commented Dec 19, 2015

philips commented Dec 23, 2015

Fix cgroups value types in the spec. #233

Fix cgroups value types in the spec. #233

Conversation

vishh commented Oct 27, 2015

wking commented Oct 27, 2015

vishh commented Oct 27, 2015

wking commented Oct 27, 2015

vbatts commented Oct 27, 2015

wking commented Oct 27, 2015

hqhq commented Oct 28, 2015

vishh commented Oct 28, 2015

hqhq commented Oct 29, 2015

vishh commented Oct 29, 2015

hqhq commented Oct 30, 2015

cyphar commented Oct 30, 2015

vishh commented Oct 30, 2015

wking commented Oct 30, 2015

cyphar commented Oct 31, 2015

wking commented Oct 31, 2015

cyphar commented Oct 31, 2015

crosbymichael commented Nov 9, 2015

vishh commented Nov 9, 2015

cyphar commented Nov 10, 2015

crosbymichael commented Nov 10, 2015

wking commented Nov 10, 2015

cyphar commented Nov 10, 2015

mrunalp commented Nov 13, 2015

crosbymichael commented Dec 18, 2015

mrunalp Dec 18, 2015

Choose a reason for hiding this comment

cyphar commented Dec 18, 2015

vishh commented Dec 19, 2015

mrunalp commented Dec 19, 2015

philips commented Dec 23, 2015