From 58a35692d698a9a4c72b464c875c14e6147e52aa Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 8 Mar 2025 15:36:45 +0100 Subject: [PATCH 1/2] remove duplicate `--oom-kill-disable` warnings on docker run / docker create This warning was originally added in [moby@3aa70c1], and moved to be printed on both `run` and `create` in commit 7c514a31c97cc0a4e74b53f5cff73da72e680a01. However, [moby@57f1305] (docker 19.03, API 1.40) moved such warnings to the daemon side. The patch mentioned this issue: > 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. The CLI does not take this into account currently, and still prints warnings twice; even in cases where the option is not supported by the daemon, and discarded: On a host without OomKillDisable support: docker create --oom-kill-disable alpine WARNING: Disabling the OOM killer on containers without setting a '-m/--memory' limit may be dangerous. WARNING: Your kernel does not support OomKillDisable. OomKillDisable discarded. On a host that supports it: docker create --oom-kill-disable alpine 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. This patch removes the client-side warning, leaving it to the daemon to report if any warnings should produced (and the client to print them). With this patch applied: On a host without OomKillDisable support: docker create --oom-kill-disable alpine WARNING: Your kernel does not support OomKillDisable. OomKillDisable discarded. On a host that supports it: docker create --oom-kill-disable alpine WARNING: OOM killer is disabled for the container, but no memory limit is set, this can result in the system running out of resources. [moby@3aa70c1]: https://github.com/moby/moby/commit/3aa70c1948e44ab523db0be37a602b63e7eac882 [moby@57f1305]: https://github.com/moby/moby/commit/57f1305e749cbf909238b407b3437d5859a747e2 Signed-off-by: Sebastiaan van Stijn --- cli/command/container/create.go | 7 ---- cli/command/container/create_test.go | 33 ++++++++----------- ...ner-create-daemon-multiple-warnings.golden | 2 ++ ...tainer-create-daemon-single-warning.golden | 1 + ...-oom-kill-true-without-memory-limit.golden | 1 - ...reate-oom-kill-without-memory-limit.golden | 1 - 6 files changed, 16 insertions(+), 29 deletions(-) create mode 100644 cli/command/container/testdata/container-create-daemon-multiple-warnings.golden create mode 100644 cli/command/container/testdata/container-create-daemon-single-warning.golden delete mode 100644 cli/command/container/testdata/container-create-oom-kill-true-without-memory-limit.golden delete mode 100644 cli/command/container/testdata/container-create-oom-kill-without-memory-limit.golden diff --git a/cli/command/container/create.go b/cli/command/container/create.go index ac8e6083fe..9930e5f8e7 100644 --- a/cli/command/container/create.go +++ b/cli/command/container/create.go @@ -207,7 +207,6 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *c hostConfig := containerCfg.HostConfig networkingConfig := containerCfg.NetworkingConfig - warnOnOomKillDisable(*hostConfig, dockerCli.Err()) warnOnLocalhostDNS(*hostConfig, dockerCli.Err()) var ( @@ -299,12 +298,6 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *c return response.ID, err } -func warnOnOomKillDisable(hostConfig container.HostConfig, stderr io.Writer) { - if hostConfig.OomKillDisable != nil && *hostConfig.OomKillDisable && hostConfig.Memory == 0 { - _, _ = fmt.Fprintln(stderr, "WARNING: Disabling the OOM killer on containers without setting a '-m/--memory' limit may be dangerous.") - } -} - // check the DNS settings passed via --dns against localhost regexp to warn if // they are trying to set a DNS to a localhost address func warnOnLocalhostDNS(hostConfig container.HostConfig, stderr io.Writer) { diff --git a/cli/command/container/create_test.go b/cli/command/container/create_test.go index a5a09ce54a..0b39542c56 100644 --- a/cli/command/container/create_test.go +++ b/cli/command/container/create_test.go @@ -270,31 +270,24 @@ func TestNewCreateCommandWithContentTrustErrors(t *testing.T) { func TestNewCreateCommandWithWarnings(t *testing.T) { testCases := []struct { - name string - args []string - warning bool + name string + args []string + warnings []string + warning bool }{ { - name: "container-create-without-oom-kill-disable", + name: "container-create-no-warnings", args: []string{"image:tag"}, }, { - name: "container-create-oom-kill-disable-false", - args: []string{"--oom-kill-disable=false", "image:tag"}, + name: "container-create-daemon-single-warning", + args: []string{"image:tag"}, + warnings: []string{"warning from daemon"}, }, { - name: "container-create-oom-kill-without-memory-limit", - args: []string{"--oom-kill-disable", "image:tag"}, - warning: true, - }, - { - name: "container-create-oom-kill-true-without-memory-limit", - args: []string{"--oom-kill-disable=true", "image:tag"}, - warning: true, - }, - { - name: "container-create-oom-kill-true-with-memory-limit", - args: []string{"--oom-kill-disable=true", "--memory=100M", "image:tag"}, + name: "container-create-daemon-multiple-warnings", + args: []string{"image:tag"}, + warnings: []string{"warning from daemon", "another warning from daemon"}, }, { name: "container-create-localhost-dns", @@ -316,7 +309,7 @@ func TestNewCreateCommandWithWarnings(t *testing.T) { platform *specs.Platform, containerName string, ) (container.CreateResponse, error) { - return container.CreateResponse{}, nil + return container.CreateResponse{Warnings: tc.warnings}, nil }, }) cmd := NewCreateCommand(fakeCLI) @@ -324,7 +317,7 @@ func TestNewCreateCommandWithWarnings(t *testing.T) { cmd.SetArgs(tc.args) err := cmd.Execute() assert.NilError(t, err) - if tc.warning { + if tc.warning || len(tc.warnings) > 0 { golden.Assert(t, fakeCLI.ErrBuffer().String(), tc.name+".golden") } else { assert.Equal(t, fakeCLI.ErrBuffer().String(), "") diff --git a/cli/command/container/testdata/container-create-daemon-multiple-warnings.golden b/cli/command/container/testdata/container-create-daemon-multiple-warnings.golden new file mode 100644 index 0000000000..8198798ce7 --- /dev/null +++ b/cli/command/container/testdata/container-create-daemon-multiple-warnings.golden @@ -0,0 +1,2 @@ +WARNING: warning from daemon +WARNING: another warning from daemon diff --git a/cli/command/container/testdata/container-create-daemon-single-warning.golden b/cli/command/container/testdata/container-create-daemon-single-warning.golden new file mode 100644 index 0000000000..abdd32ebad --- /dev/null +++ b/cli/command/container/testdata/container-create-daemon-single-warning.golden @@ -0,0 +1 @@ +WARNING: warning from daemon diff --git a/cli/command/container/testdata/container-create-oom-kill-true-without-memory-limit.golden b/cli/command/container/testdata/container-create-oom-kill-true-without-memory-limit.golden deleted file mode 100644 index 5fb6aeb429..0000000000 --- a/cli/command/container/testdata/container-create-oom-kill-true-without-memory-limit.golden +++ /dev/null @@ -1 +0,0 @@ -WARNING: Disabling the OOM killer on containers without setting a '-m/--memory' limit may be dangerous. diff --git a/cli/command/container/testdata/container-create-oom-kill-without-memory-limit.golden b/cli/command/container/testdata/container-create-oom-kill-without-memory-limit.golden deleted file mode 100644 index 5fb6aeb429..0000000000 --- a/cli/command/container/testdata/container-create-oom-kill-without-memory-limit.golden +++ /dev/null @@ -1 +0,0 @@ -WARNING: Disabling the OOM killer on containers without setting a '-m/--memory' limit may be dangerous. From bc90bb68558dcc3c860a20c8699a18a2e1e40df5 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 8 Mar 2025 16:03:45 +0100 Subject: [PATCH 2/2] container create: combine client-side warning with daemon-side Use a consistent approach for producing warnings, but add a TODO for moving this warning to the daemon, which can make a better call if it will work or not (depending on networking mode). This warning was originally added in [moby@afa92a9], before integration with libnetwork, and this warning may be incorrect in many scenarios. While updating, also removing the custom regular expression used to detect if the IP is a loopback address, and using go's netip package instead. [moby@afa92a9]: https://github.com/moby/moby/commit/afa92a9af0f1a77ef25aab73b11aa855a1823666 Signed-off-by: Sebastiaan van Stijn --- cli/command/container/create.go | 31 +++++++------------ ...container-create-localhost-dns-ipv6.golden | 2 +- .../container-create-localhost-dns.golden | 2 +- 3 files changed, 13 insertions(+), 22 deletions(-) diff --git a/cli/command/container/create.go b/cli/command/container/create.go index 9930e5f8e7..486d2852f9 100644 --- a/cli/command/container/create.go +++ b/cli/command/container/create.go @@ -4,8 +4,8 @@ import ( "context" "fmt" "io" + "net/netip" "os" - "regexp" "github.com/containerd/platforms" "github.com/distribution/reference" @@ -207,8 +207,6 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *c hostConfig := containerCfg.HostConfig networkingConfig := containerCfg.NetworkingConfig - warnOnLocalhostDNS(*hostConfig, dockerCli.Err()) - var ( trustedRef reference.Canonical namedRef reference.Named @@ -291,6 +289,9 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *c } } + if warn := localhostDNSWarning(*hostConfig); warn != "" { + response.Warnings = append(response.Warnings, warn) + } for _, w := range response.Warnings { _, _ = fmt.Fprintln(dockerCli.Err(), "WARNING:", w) } @@ -299,26 +300,16 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *c } // check the DNS settings passed via --dns against localhost regexp to warn if -// they are trying to set a DNS to a localhost address -func warnOnLocalhostDNS(hostConfig container.HostConfig, stderr io.Writer) { +// they are trying to set a DNS to a localhost address. +// +// TODO(thaJeztah): move this to the daemon, which can make a better call if it will work or not (depending on networking mode). +func localhostDNSWarning(hostConfig container.HostConfig) string { for _, dnsIP := range hostConfig.DNS { - if isLocalhost(dnsIP) { - _, _ = fmt.Fprintf(stderr, "WARNING: Localhost DNS setting (--dns=%s) may fail in containers.\n", dnsIP) - return + if addr, err := netip.ParseAddr(dnsIP); err == nil && addr.IsLoopback() { + return fmt.Sprintf("Localhost DNS (%s) may fail in containers.", addr) } } -} - -// IPLocalhost is a regex pattern for IPv4 or IPv6 loopback range. -const ipLocalhost = `((127\.([0-9]{1,3}\.){2}[0-9]{1,3})|(::1)$)` - -var localhostIPRegexp = regexp.MustCompile(ipLocalhost) - -// IsLocalhost returns true if ip matches the localhost IP regular expression. -// Used for determining if nameserver settings are being passed which are -// localhost addresses -func isLocalhost(ip string) bool { - return localhostIPRegexp.MatchString(ip) + return "" } func validatePullOpt(val string) error { diff --git a/cli/command/container/testdata/container-create-localhost-dns-ipv6.golden b/cli/command/container/testdata/container-create-localhost-dns-ipv6.golden index 5c98b97716..bb07a137dc 100644 --- a/cli/command/container/testdata/container-create-localhost-dns-ipv6.golden +++ b/cli/command/container/testdata/container-create-localhost-dns-ipv6.golden @@ -1 +1 @@ -WARNING: Localhost DNS setting (--dns=::1) may fail in containers. +WARNING: Localhost DNS (::1) may fail in containers. diff --git a/cli/command/container/testdata/container-create-localhost-dns.golden b/cli/command/container/testdata/container-create-localhost-dns.golden index 1c8b0e1f7f..409082ad6a 100644 --- a/cli/command/container/testdata/container-create-localhost-dns.golden +++ b/cli/command/container/testdata/container-create-localhost-dns.golden @@ -1 +1 @@ -WARNING: Localhost DNS setting (--dns=127.0.0.11) may fail in containers. +WARNING: Localhost DNS (127.0.0.11) may fail in containers.