From ffa1728d4bc7c9c662c6090e48d438bc728df5dc Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 24 Feb 2019 15:36:45 +0100 Subject: [PATCH 1/2] Normalize values for pids-limit - Don't set `PidsLimit` when creating a container and no limit was set (or the limit was set to "unlimited") - Don't set `PidsLimit` if the host does not have pids-limit support (previously "unlimited" was set). - Do not generate a warning if the host does not have pids-limit support, but pids-limit was set to unlimited (having no limit set, or the limit set to "unlimited" is equivalent, so no warning is nescessary in that case). - When updating a container, convert `0`, and `-1` to "unlimited" (`0`). Signed-off-by: Sebastiaan van Stijn --- .../router/container/container_routes.go | 15 +++++++++++ daemon/daemon_unix.go | 27 ++++++++++--------- integration/container/update_linux_test.go | 4 +++ 3 files changed, 33 insertions(+), 13 deletions(-) diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index 5b76d6208d..549d1c1c3a 100644 --- a/api/server/router/container/container_routes.go +++ b/api/server/router/container/container_routes.go @@ -428,6 +428,13 @@ func (s *containerRouter) postContainerUpdate(ctx context.Context, w http.Respon if versions.LessThan(httputils.VersionFromContext(ctx), "1.40") { updateConfig.PidsLimit = nil } + if updateConfig.PidsLimit != nil && *updateConfig.PidsLimit <= 0 { + // Both `0` and `-1` are accepted to set "unlimited" when updating. + // Historically, any negative value was accepted, so treat them as + // "unlimited" as well. + var unlimited int64 + updateConfig.PidsLimit = &unlimited + } hostConfig := &container.HostConfig{ Resources: updateConfig.Resources, @@ -481,6 +488,14 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo hostConfig.Capabilities = nil } + if hostConfig != nil && hostConfig.PidsLimit != nil && *hostConfig.PidsLimit <= 0 { + // Don't set a limit if either no limit was specified, or "unlimited" was + // explicitly set. + // Both `0` and `-1` are accepted as "unlimited", and historically any + // negative value was accepted, so treat those as "unlimited" as well. + hostConfig.PidsLimit = nil + } + ccr, err := s.backend.ContainerCreate(types.ContainerCreateConfig{ Name: name, Config: config, diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 810f451f76..ec53ab06d4 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -119,16 +119,16 @@ func getMemoryResources(config containertypes.Resources) *specs.LinuxMemory { } func getPidsLimit(config containertypes.Resources) *specs.LinuxPids { - limit := &specs.LinuxPids{} - if config.PidsLimit != nil { - limit.Limit = *config.PidsLimit - if limit.Limit == 0 { - // docker API allows 0 to unset this to be consistent with default values. - // when updating values, runc requires -1 - limit.Limit = -1 - } + if config.PidsLimit == nil { + return nil } - return limit + if *config.PidsLimit <= 0 { + // docker API allows 0 and negative values to unset this to be consistent + // with default values. When updating values, runc requires -1 to unset + // the previous limit. + return &specs.LinuxPids{Limit: -1} + } + return &specs.LinuxPids{Limit: *config.PidsLimit} } func getCPUResources(config containertypes.Resources) (*specs.LinuxCPU, error) { @@ -466,10 +466,11 @@ func verifyPlatformContainerResources(resources *containertypes.Resources, sysIn if resources.OomKillDisable != nil && *resources.OomKillDisable && resources.Memory == 0 { warnings = append(warnings, "OOM killer is disabled for the container, but no memory limit is set, this can result in the system running out of resources.") } - if resources.PidsLimit != nil && *resources.PidsLimit != 0 && !sysInfo.PidsLimit { - warnings = append(warnings, "Your kernel does not support pids limit capabilities or the cgroup is not mounted. PIDs limit discarded.") - var limit int64 - resources.PidsLimit = &limit + if resources.PidsLimit != nil && !sysInfo.PidsLimit { + if *resources.PidsLimit > 0 { + warnings = append(warnings, "Your kernel does not support PIDs limit capabilities or the cgroup is not mounted. PIDs limit discarded.") + } + resources.PidsLimit = nil } // cpu subsystem checks and adjustments diff --git a/integration/container/update_linux_test.go b/integration/container/update_linux_test.go index 1b753e28f9..5efda6d411 100644 --- a/integration/container/update_linux_test.go +++ b/integration/container/update_linux_test.go @@ -132,6 +132,10 @@ func TestUpdatePidsLimit(t *testing.T) { {desc: "update lower", update: intPtr(16), expect: 16, expectCg: "16"}, {desc: "update on old api ignores value", oldAPI: true, update: intPtr(10), expect: 16, expectCg: "16"}, {desc: "unset limit", update: intPtr(0), expect: 0, expectCg: "max"}, + {desc: "reset", update: intPtr(32), expect: 32, expectCg: "32"}, + {desc: "unset limit with minus one", update: intPtr(-1), expect: 0, expectCg: "max"}, + {desc: "reset", update: intPtr(32), expect: 32, expectCg: "32"}, + {desc: "unset limit with minus two", update: intPtr(-2), expect: 0, expectCg: "max"}, } { c := apiClient if test.oldAPI { From 1101568fa11cb00d8f8de3ff8953c581c351928f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 24 Feb 2019 16:26:37 +0100 Subject: [PATCH 2/2] Update TestUpdatePidsLimit to be more atomic Create a new container for each subtest, so that individual subtests are self-contained, and there's no need to execute them in the exact order, or resetting the container in between. This makes the test slower (6.54s vs 3.43s), but reduced the difference by using `network=host`, which made a substantial difference (without `network=host`, the test took more than twice as long: 13.96s). Signed-off-by: Sebastiaan van Stijn --- integration/container/update_linux_test.go | 36 +++++++--------------- integration/internal/container/ops.go | 10 ++++++ 2 files changed, 21 insertions(+), 25 deletions(-) diff --git a/integration/container/update_linux_test.go b/integration/container/update_linux_test.go index 5efda6d411..e4f4e2c42d 100644 --- a/integration/container/update_linux_test.go +++ b/integration/container/update_linux_test.go @@ -7,7 +7,6 @@ import ( "testing" "time" - "github.com/docker/docker/api/types" containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/client" "github.com/docker/docker/integration/internal/container" @@ -114,8 +113,6 @@ func TestUpdatePidsLimit(t *testing.T) { oldAPIclient := request.NewAPIClient(t, client.WithVersion("1.24")) ctx := context.Background() - cID := container.Run(t, ctx, apiClient) - intPtr := func(i int64) *int64 { return &i } @@ -123,33 +120,28 @@ func TestUpdatePidsLimit(t *testing.T) { for _, test := range []struct { desc string oldAPI bool + initial *int64 update *int64 expect int64 expectCg string }{ {desc: "update from none", update: intPtr(32), expect: 32, expectCg: "32"}, - {desc: "no change", update: nil, expectCg: "32"}, - {desc: "update lower", update: intPtr(16), expect: 16, expectCg: "16"}, - {desc: "update on old api ignores value", oldAPI: true, update: intPtr(10), expect: 16, expectCg: "16"}, - {desc: "unset limit", update: intPtr(0), expect: 0, expectCg: "max"}, - {desc: "reset", update: intPtr(32), expect: 32, expectCg: "32"}, - {desc: "unset limit with minus one", update: intPtr(-1), expect: 0, expectCg: "max"}, - {desc: "reset", update: intPtr(32), expect: 32, expectCg: "32"}, - {desc: "unset limit with minus two", update: intPtr(-2), expect: 0, expectCg: "max"}, + {desc: "no change", initial: intPtr(32), expect: 32, expectCg: "32"}, + {desc: "update lower", initial: intPtr(32), update: intPtr(16), expect: 16, expectCg: "16"}, + {desc: "update on old api ignores value", oldAPI: true, initial: intPtr(32), update: intPtr(16), expect: 32, expectCg: "32"}, + {desc: "unset limit with zero", initial: intPtr(32), update: intPtr(0), expect: 0, expectCg: "max"}, + {desc: "unset limit with minus one", initial: intPtr(32), update: intPtr(-1), expect: 0, expectCg: "max"}, + {desc: "unset limit with minus two", initial: intPtr(32), update: intPtr(-2), expect: 0, expectCg: "max"}, } { c := apiClient if test.oldAPI { c = oldAPIclient } - var before types.ContainerJSON - if test.update == nil { - var err error - before, err = c.ContainerInspect(ctx, cID) - assert.NilError(t, err) - } - t.Run(test.desc, func(t *testing.T) { + // Using "network=host" to speed up creation (13.96s vs 6.54s) + cID := container.Run(t, ctx, apiClient, container.WithPidsLimit(test.initial), container.WithNetworkMode("host")) + _, err := c.ContainerUpdate(ctx, cID, containertypes.UpdateConfig{ Resources: containertypes.Resources{ PidsLimit: test.update, @@ -160,13 +152,7 @@ func TestUpdatePidsLimit(t *testing.T) { inspect, err := c.ContainerInspect(ctx, cID) assert.NilError(t, err) assert.Assert(t, inspect.HostConfig.Resources.PidsLimit != nil) - - if test.update == nil { - assert.Assert(t, before.HostConfig.Resources.PidsLimit != nil) - assert.Equal(t, *before.HostConfig.Resources.PidsLimit, *inspect.HostConfig.Resources.PidsLimit) - } else { - assert.Equal(t, *inspect.HostConfig.Resources.PidsLimit, test.expect) - } + assert.Equal(t, *inspect.HostConfig.Resources.PidsLimit, test.expect) ctx, cancel := context.WithTimeout(ctx, 60*time.Second) defer cancel() diff --git a/integration/internal/container/ops.go b/integration/internal/container/ops.go index 7bbf6cf0a2..7ce9398265 100644 --- a/integration/internal/container/ops.go +++ b/integration/internal/container/ops.go @@ -137,6 +137,16 @@ func WithAutoRemove(c *TestContainerConfig) { c.HostConfig.AutoRemove = true } +// WithPidsLimit sets the container's "pids-limit +func WithPidsLimit(limit *int64) func(*TestContainerConfig) { + return func(c *TestContainerConfig) { + if c.HostConfig == nil { + c.HostConfig = &containertypes.HostConfig{} + } + c.HostConfig.PidsLimit = limit + } +} + // WithRestartPolicy sets container's restart policy func WithRestartPolicy(policy string) func(c *TestContainerConfig) { return func(c *TestContainerConfig) {