Move "OOM Kill disable" warning to the daemon

[thaJeztah]

Relates to docker/cli#1571

Disabling the oom-killer for a container without setting a memory limit
is dangerous, as it can result in the container consuming unlimited memory,
without the kernel being able to kill it. A check for this situation is curently
done in the CLI, but other consumers of the API won't receive this warning.

This patch adds a check for this situation to the daemon, so that all consumers
of the API will receive this warning.

This patch will have one side-effect; docker cli's that also perform this check
client-side will print the warning twice; this can be addressed by disabling
the cli-side check for newer API versions, but will generate a bit of extra
noise when using an older CLI.

With this patch applied (and a cli that does not take the new warning into account);

docker create --oom-kill-disable busybox
WARNING: OOM killer is disabled for the container, but no memory limit is set, this can result in the system running out of resources.
669933b9b237fa27da699483b5cf15355a9027050825146587a0e5be0d848adf

docker run --rm --oom-kill-disable busybox
WARNING: Disabling the OOM killer on containers without setting a '-m/--memory' limit may be dangerous.
WARNING: OOM killer is disabled for the container, but no memory limit is set, this can result in the system running out of resources.

- How to verify it

make BIND_DIR=. TESTDIRS='github.com/docker/docker/daemon/' TESTFLAGS='-test.run ^TestVerifyContainerResources$' test-unit

- Description for the changelog

+ Return warnings when creating a container with OOM-kill-disabled, but no memory-constraints set

[kolyshkin]

So I see that the code prints every warning it adds. Maybe it makes sense to drop all the logrus.Warn lines and add something like this instead?

--- a/daemon/daemon_unix.go
+++ b/daemon/daemon_unix.go
@@ -358,6 +358,14 @@ func verifyContainerResources(resources *containertypes.Resources, sysInfo *sysi
        warnings := []string{}
        fixMemorySwappiness(resources)
 
+       defer func() {
+               if len(warnings) > 0 {
+                       for _, w := range warnings {
+                               logrus.Warn(w)
+                       }
+               }
+       }
+

[thaJeztah]

Hm, good one; perhaps instead of a defer, we can do this in the calling function; in case of an error, I think it's ok as well to skip logging the warnings, and only log the error (although we could do both of course), i.e.;

warnings, err := verifyContainerResources(....)
if err != nil {
    return err
}
for _, w := range warnings {
    logrus.Warn(w)
}

[kolyshkin]

we can do this in the calling function

Yes better, put it to verifyContainerSettings() in daemon/container.go as it calls a few functions that prints some warnings. Someplace here:

moby/daemon/container.go

Line 354 in 419972a

if hostConfig.NetworkMode.IsHost() && len(hostConfig.PortBindings) > 0 {

That way we can remove a lot of logrus.Warn() calls in both daemon_unix.go and daemon_windows.go.

Also maybe add container id field:

logrus.WithField("container", container.ID).Warn(w)

[thaJeztah]

Oops, missed your last comment; didn't make this change yet:

logrus.WithField("container", container.ID).Warn(w)

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@419972a). Click here to learn what that means.
The diff coverage is 80%.

@@            Coverage Diff            @@
##             master   #38382   +/-   ##
=========================================
  Coverage          ?   36.55%           
=========================================
  Files             ?      608           
  Lines             ?    44958           
  Branches          ?        0           
=========================================
  Hits              ?    16433           
  Misses            ?    26247           
  Partials          ?     2278

[thaJeztah]

@kolyshkin updated, PTAL. Also did a follow-up to do some additional refactoring/cleaning up; #38393

[kolyshkin]

AFAIK this comment should not be removed. What it says is "even if err != nil, we still want those warnings". Without such comment, just by looking at the code I want to move the append line to after the err check.

[thaJeztah]

Ah, yes, I thought it would be apparent from the function as a whole, but you're right; better to add it back

[kolyshkin]

LGTM except for the removed comment. Ideally it would be awesome to change the patch order, switching patches 1 and 3 (cleanups first, new functionality next) as otherwise you are removing the line you just added. Not super important though...

[thaJeztah]

Ideally it would be awesome to change the patch order, switching patches 1 and 3 (cleanups first, new functionality next)

I'm always on the fence on that one, as there's benefits to both; refactor first makes the implementation slightly cleaner, however, I've been in situations where a change had to be backported and could not, because there was a big refactor before it, so I guess that's why I did it the other way round.

[kolyshkin]

a change had to be backported and could not, because there was a big refactor before it, so I guess that's why I did it the other way round.

Makes total sense to me, thanks for the explanation