Merge pull request #38793 from thaJeztah/pids_limit_improvements

Some refactoring on PidsLimit

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,
@@ -484,6 +491,14 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo
 		}
 	}
 
+	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,

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

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,29 +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: "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,
@@ -156,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()

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) {