Remove the deprecated '--kernel-memory' option #42854

aiordache · 2021-09-14T15:57:18Z

Ignore the kernel memory option if set in HostConfig on container create.
We keep the option in the API ( for the containers/create and /info endpoints) until the next API version release but ignore any value it is set to.

root@80b7d3e03521:~# curl --unix-socket /var/run/docker.sock -H "Content-Type: application/json" -d '{"Image": "alpine", "Cmd": ["echo", "hello world"], "HostConfig": {"KernelMemory": 299900}}' -X POST http://localhost/v1.41/containers/create
{"Id":"4d8537d9cee548938492d489b7d8394b46ae75992d89875fbf2b33971762a552","Warnings":["Specifying a kernel memory limit is not supported anymore."]}

root@80b7d3e03521:~# docker inspect 4d8537d9cee5
[
    {
        "Id": "4d8537d9cee548938492d489b7d8394b46ae75992d89875fbf2b33971762a552",
....
            "KernelMemory": 0,
            "KernelMemoryTCP": 0,
            "MemoryReservation": 0,
....

root@80b7d3e03521:~# curl -s --unix-socket /var/run/docker.sock http://localhost/info | jq .| grep KernelMemory
  "KernelMemory": false,
  "KernelMemoryTCP": false,

tianon · 2021-09-20T20:51:24Z

daemon/daemon_unix.go

@@ -102,7 +102,7 @@ func getMemoryResources(config containertypes.Resources) *specs.LinuxMemory {
 	}

 	if config.KernelMemory != 0 {
-		memory.Kernel = &config.KernelMemory
+		logrus.Warn("Your kernel does not support kernel memory limit capabilities or the cgroup is not mounted. Limitation discarded.")


This message seems inaccurate here -- to give this warning, shouldn't it check sysInfo.KernelMemory first like it did previously? Alternatively, perhaps we don't warn about kernel support at all, given we're going to ignore the container setting entirely?

tianon · 2021-09-20T20:52:10Z

Just to thread the needle a little bit, this is related to opencontainers/runtime-spec#1093 and opencontainers/runc#2840.

thaJeztah · 2021-09-21T07:48:26Z

Just to thread the needle a little bit, this is related to opencontainers/runtime-spec#1093 and opencontainers/runc#2840.

Yes; also related to opencontainers/runc#3174 (which makes things more complicated)

FWIW; I discussed with @aiordache to change this implementation to

ignore the option in the API for API 1.42 and up (instead of removing it everywhere)
remove the"your host does not support ..." warnings (which this PR also does)
update the API documentation (swagger) for 1.42 to remove mention of the field, and to warn users that the "kernel-memory-tcp" is supported by the API but won't take effect (depending on the version of runc and/or OCI runtime being used, see below).

I had a draft branch where I started working on this, (master...thaJeztah:remove_kmem_options) which is when I found out that kmem.tcp was not deprecated in the kernel, so runc (and the spec to an extend) had been a bit too optimistic deprecating the options.

So now we're in a situation where;

we deprecated the --kernel-memory option, but did NOT (yet?) deprecate --kernel-memory-tcp
runc v1.0.0 removed both (so even if we would set them, they would take no effect)
other runtimes MAY still support them (as well as older versions of runc)
the runtime spec "discourages" both

So what's the best approach here?

Fully remove the --kernel-memory option (current implementation in this PR); technically "incorrect" as we'd be modifying older API versions (which should be considered "frozen")
Ignore the --kernel-memory option for API 1.42 and up (proposed change), knowing that current runc versions will ignore it (but other runtimes may still support it, so keeping that use-case)
same as 1. or 2. but for both --kernel-memory and --kernel-memory-tcp (problem: we didn't announce the latter one to be deprecated)
do 1. or 2., but un-deprecate the --kernel-memory-tcp in runc (TBD: Should memory.kmem.tcp.limit_in_bytes still be supported? opencontainers/runc#3174 - it's not 100% clear to me if re-introducing that option also brings back the problems with the broken 3.10 RHEL kernels)

thaJeztah

Arf; of course it helps if I actually submit my review 😂

Looks good; some minor nits:

the first commit message has a typo (s/KernelMemeory/KernelMemory/)
perhaps you can squash at least the first two commits (I think those should go together)
could you add a bullet to https://github.com/moby/moby/blob/master/docs/api/version-history.md to mention the field has been removed from the API and will be ignored for API v1.42 and above?

docs/api/version-history.md

api/swagger.yaml

- remove KernelMemory option from `v1.42` api docs - remove KernelMemory warning on `/info` - update changes for `v1.42` Signed-off-by: aiordache <anca.iordache@docker.com>

Signed-off-by: aiordache <anca.iordache@docker.com>

thaJeztah

LGTM

thaJeztah · 2021-09-24T08:35:07Z

@tianon ptal

tianon

Given runc is now ignoring this value, I'm not sure I understand why we're working so hard to pretend it really is still supported on the older API levels? Wouldn't it be more useful for users to bubble up that it's not going to do anything, even retroactively?

tianon · 2021-09-28T22:09:26Z

daemon/daemon_unix.go

-	}
-	if resources.KernelMemory > 0 && !kernel.CheckKernelVersion(4, 0, 0) {
-		warnings = append(warnings, "You specified a kernel memory limit on a kernel older than 4.0. Kernel memory limits are experimental on older kernels, it won't work as expected and can cause your system to be unstable.")
+		if !sysInfo.KernelMemory {


Isn't the change over in pkg/sysinfo/sysinfo_linux.go deleting the code that sets this value? (So this will never be invoked?)

Ahm, yes, you're right; I mistook that one for the /info endpoint (which uses that information)

I guess instead, the systemInfo() endpoint would have to be updated to reset the field in the response for API >= 1.42 (and the field to have omitempty)

moby/api/server/router/system/system_routes.go

Lines 46 to 47 in 0fdd23b

func (s *systemRouter) getInfo(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {

info := s.backend.SystemInfo()

thaJeztah · 2021-09-29T09:27:08Z

Given runc is now ignoring this value, I'm not sure I understand why we're working so hard to pretend it really is still supported on the older API levels? Wouldn't it be more useful for users to bubble up that it's not going to do anything, even retroactively?

The main train of thought here was that;

we (at least so far tried to) don't change existing APIs
runtimes other than runc may still support it (although admittedly, they'd be living on borrowed time as the kernel removed support for this)

tianon

Discussed in maintainers meeting -- approach seems fair enough to me. 👍

thaJeztah · 2021-10-21T18:23:18Z

Need to double check #42854 (comment) - let me do so tomorrow and let you know 😅

thaJeztah · 2022-02-07T13:59:39Z

daemon/daemon_unix.go

+			warnings = append(warnings, "Your kernel does not support kernel memory limit capabilities or the cgroup is not mounted. Limitation discarded.")
+			resources.KernelMemory = 0
+		}
+		if resources.KernelMemory < linuxMinMemory {


Looks like this should check if resources.KernelMemory > 0, otherwise this will produce an error if it's reset above (line 468)

thaJeztah · 2022-03-17T12:31:45Z

For those arriving here; this was carried, and merged through #43214

thaJeztah added area/api impact/api impact/changelog impact/deprecation status/2-code-review labels Sep 16, 2021

thaJeztah added this to the 21.xx milestone Sep 16, 2021

tianon reviewed Sep 20, 2021

View reviewed changes

aiordache force-pushed the rm_options branch 2 times, most recently from e49e3f2 to e8b0b4a Compare September 21, 2021 08:24

thaJeztah requested changes Sep 23, 2021

View reviewed changes

aiordache force-pushed the rm_options branch from e85f4cd to d3af5ff Compare September 23, 2021 13:49

thaJeztah reviewed Sep 23, 2021

View reviewed changes

docs/api/version-history.md Outdated Show resolved Hide resolved

aiordache force-pushed the rm_options branch from d3af5ff to 09128b1 Compare September 23, 2021 15:52

thaJeztah reviewed Sep 23, 2021

View reviewed changes

api/swagger.yaml Show resolved Hide resolved

aiordache added 2 commits September 23, 2021 19:30

Remove KernelMemory option from /containers/create endpoint

d066e54

- remove KernelMemory option from `v1.42` api docs - remove KernelMemory warning on `/info` - update changes for `v1.42` Signed-off-by: aiordache <anca.iordache@docker.com>

Remove KernelMemory field from endpoints docs

3acf0b9

Signed-off-by: aiordache <anca.iordache@docker.com>

aiordache force-pushed the rm_options branch from 09128b1 to 3acf0b9 Compare September 23, 2021 17:37

thaJeztah approved these changes Sep 23, 2021

View reviewed changes

AkihiroSuda approved these changes Sep 28, 2021

View reviewed changes

tianon reviewed Sep 28, 2021

View reviewed changes

tianon approved these changes Oct 21, 2021

View reviewed changes

thaJeztah reviewed Feb 7, 2022

View reviewed changes

This was referenced Feb 7, 2022

Review options no longer supported by cgroups v2 #43213

Open

Remove the deprecated '--kernel-memory' option on API v1.42 and up #43214

Merged

AkihiroSuda closed this in #43214 Mar 17, 2022

thaJeztah mentioned this pull request Apr 9, 2022

update status of deprecated features for 22.04 (docs, and some code) docker/cli#3542

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the deprecated '--kernel-memory' option #42854

Remove the deprecated '--kernel-memory' option #42854

aiordache commented Sep 14, 2021

tianon Sep 20, 2021

tianon commented Sep 20, 2021

thaJeztah commented Sep 21, 2021

thaJeztah left a comment

thaJeztah left a comment

thaJeztah commented Sep 24, 2021

tianon left a comment

tianon Sep 28, 2021

thaJeztah Sep 29, 2021

thaJeztah commented Sep 29, 2021

tianon left a comment

thaJeztah commented Oct 21, 2021

thaJeztah Feb 7, 2022

thaJeztah commented Mar 17, 2022

	func (s systemRouter) getInfo(ctx context.Context, w http.ResponseWriter, r http.Request, vars map[string]string) error {
	info := s.backend.SystemInfo()

Remove the deprecated '--kernel-memory' option #42854

Remove the deprecated '--kernel-memory' option #42854

Conversation

aiordache commented Sep 14, 2021

tianon Sep 20, 2021

Choose a reason for hiding this comment

tianon commented Sep 20, 2021

thaJeztah commented Sep 21, 2021

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Sep 24, 2021

tianon left a comment

Choose a reason for hiding this comment

tianon Sep 28, 2021

Choose a reason for hiding this comment

thaJeztah Sep 29, 2021

Choose a reason for hiding this comment

thaJeztah commented Sep 29, 2021

tianon left a comment

Choose a reason for hiding this comment

thaJeztah commented Oct 21, 2021

thaJeztah Feb 7, 2022

Choose a reason for hiding this comment

thaJeztah commented Mar 17, 2022