Fix cgroups value types in the spec.

[vishh]

Follow up of this comment

[wking]

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]

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]

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]

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

LGTM

[wking]

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]

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]

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

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

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

[hqhq]

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

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]

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]

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]

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

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]

@wking No, I agree with the proposal of using pointers so that using nulland 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".

[crosbymichael]

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

[vishh]

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. @vishhhttps://github.com/vishh what do you think?

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

[cyphar]

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

[crosbymichael]

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]

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]

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

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

[crosbymichael]

LGTM

[mrunalp]

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]

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

[vishh]

Comments addressed

[mrunalp]

LGTM

[philips]

thanks for doing this @vishh