config-linux: RFC 2119 wording for disableOOMKiller

[wking]

The previous wording tiptoes between background docs about kernel behavior and runtime requirements, so it's not clear what the runtime is required to do or how you'd compliance-test runtimes. The new wording uses MUST (NOT) wording for the various cases, so the runtime responsibilities are clear.

The implementation-defined case avoids specifying when:

• The parent memory cgroup may have different values of oom_kill_disable, and
• The runtime may or may not create a new memory cgroup for the controller (because of the “MAY attach the container process to additional cgroup controllers” language).

[hqhq]

LGTM

[vbatts]

while this is more direct, I feel the reason the tip-toeing was accomplishing the ambiguity of whether that means failing the runtime if the feature isn't present.

[wking]

On Wed, Apr 05, 2017 at 07:08:33AM -0700, Vincent Batts wrote: while this is more direct, I feel the reason the tip-toeing was accomplishing the ambiguity of whether that means failing the runtime if the feature isn't present.

My reading of the current compliance wording [1] is that there are three cases: * compliant: satisfies all the MUST, REQUIRED, and SHALL requirements for the platforms it implements. * non compliant: fails to satisfy one or more of the MUST, REQUIRED, or SHALL requirements for the platforms it implements. * neither (we don't have a compact name for this): does not fail a MUST-level requirement, but takes advantage of a legal out, e.g. erroring out of ‘create’ with “I can't do that” [2] which makes it impossible to test whether a deeper requirement (e.g. a disableOOMKiller support MUST) is satisfied or not. If that is also how the compliance folks read our compliance language, then with this PR would mean that runtimes which did not support disableOOMKiller would have to error out at ‘create’ with “I don't support disableOOMKiller” and that such a runtime would fall into the ‘neither’ compliance case (assuming it didn't fail any other MUST-level requirements). I think that's appropriate (e.g. see similar discussion in #472 and #476, where we decided against “silently don't support”). If, on the other hand, we want to allow completely compliant runtimes which don't support disableOOMKiller, I think the proper approach would be to pull that component out into a separate protocol/platform [3]. So a runtime would get badged as: Compliant with runtime-spec 1.0 protocols ‘linux’ and ‘disableOOMkiller’ on amd64. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc5/spec.md#notational-conventions [2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/runtime.md#L105> [3]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc5/spec.md#platforms

[tianon]

The part of this I find irritating while reading it is that it re-references cgroupsPath over and over again. IMO it's an OK change if we drop the last two lines.

I think if a config.json includes "disableOOMKiller": true and the kernel doesn't support it, that runtime should fail, which is exactly what If disableOOMKilleris set, the runtime MUST setoom_kill_disable to the appropriate value. will ensure (since the kernel will error out, causing the runtime to be unable to fulfill the requirement specified by the config).

I also don't think this is worth holding up 1.0 for -- this seems like a fine clarification to make in 1.1.0.

[wking]

On Wed, May 10, 2017 at 09:33:17PM -0700, Tianon Gravi wrote: The part of this I find irritating while reading it is that it re-references `cgroupsPath` over and over again.

I think this is the wording that @crosbymichael and @mrunalp were considering pulling out into some central location. I'm happy to rebase this PR after they land the generic wording for that.

I also don't think this is worth holding up 1.0 for -- this seems like a fine clarification to make in 1.1.0.

Without something like this PR, there is no compliance testing for disableOOMKiller. So if we don't land it by 1.0, runtimes which silently ignore disableOOMKiller could still be certified compliant. I mentioned this (but maybe not for this PR?) in the meeting, and remember @mrunalp saying something like “I think the certifiers will take a closer look in cases like that” (although I didn't even get a paraphrased form in the log [1] and may be misremembering who said what). However, we're currently very clear about tying “compliance” to the RFC 2119 language [2], so I'm not sure what the certifiers would do (badge the runtime compliant with a “seems to ignore disableOOMKiller” note)? Or is the idea that the functionality of this property so obvious that it doesn't need compliance testing (and runtimes will take care of testing this property on their own)? [1]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2017/opencontainers.2017-05-10-21.03.log.html [2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/spec.md#L40-L41

[wking]

On Wed, May 10, 2017 at 11:50:35PM -0700, W. Trevor King wrote: Wed, May 10, 2017 at 09:33:17PM -0700, Tianon Gravi: > The part of this I find irritating while reading it is that it > re-references `cgroupsPath` over and over again. I think this is the wording that @crosbymichael and @mrunalp were considering pulling out into some central location. I'm happy to rebase this PR after they land the generic wording for that.

While digging through issues I'd marked unresolved, I came across #576. It's pretty old, but still rebases cleanly onto master and it looks like what folks want. At least, I think it covers the “If `disableOOMKiller` is not set and the `cgroupsPath` value would not result in a new cgroup being created” case generically. It does not have wording for the “If `disableOOMKiller` is not set and the `cgroupsPath` value would result in a new cgroup being created” case, so that still needs to be worked out. If #576 is close, but not quite what you're looking for, then maybe it will help the maintainers while they draft their own language.

[vbatts]

i'm closing this. we'll follow the discussion of #834

[wking]

On Wed, May 24, 2017 at 08:51:58AM -0700, v1.0.0.batts wrote: i'm closing this. we'll follow the discussion of #834

Is #834 going to say anything about [1]? I thought it was just covering lines 255 and 256 from this PR. [1]: https://github.com/opencontainers/runtime-spec/pull/745/files#diff-428eec4013a655816cdefafd5d3505f1R254