1. docker
  2. Commit 5861a0db22fe43c684fac5dcd41c1e2ad8319fa1
Browse code

Fix container update resetting pidslimit on older API clients

Older API clients did not use a pointer for `PidsLimit`, so
API requests would always send `0`, resulting in any previous
value to be reset after an update:

Before this patch:

(using a 17.06 Docker CLI):

```bash
docker run -dit --name test --pids-limit=16 busybox
docker container inspect --format '{{json .HostConfig.PidsLimit}}' test
16

docker container update --memory=100M --memory-swap=200M test

docker container inspect --format '{{json .HostConfig.PidsLimit}}' test
0

docker container exec test cat /sys/fs/cgroup/pids/pids.max
max
```

With this patch applied:

(using a 17.06 Docker CLI):

```bash
docker run -dit --name test --pids-limit=16 busybox
docker container inspect --format '{{json .HostConfig.PidsLimit}}' test
16

docker container update --memory=100M --memory-swap=200M test

docker container inspect --format '{{json .HostConfig.PidsLimit}}' test
16

docker container exec test cat /sys/fs/cgroup/pids/pids.max
16
```

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Sebastiaan van Stijn authored on 2019/02/24 22:11:00
Showing 2 changed files
api/server/router/container/container_routes.go
History View file @ 5861a0d
... ... 
@@ -425,6 +425,9 @@ func (s *containerRouter) postContainerUpdate(ctx context.Context, w http.Respon
425 425 
 	if err := decoder.Decode(&updateConfig); err != nil {
426 426 
 		return err
427 427 
 	}
428 
+	if versions.LessThan(httputils.VersionFromContext(ctx), "1.40") {
429 
+		updateConfig.PidsLimit = nil
430 
+	}
428 431
429 432 
 	hostConfig := &container.HostConfig{
430 433 
 		Resources:     updateConfig.Resources,
integration/container/update_linux_test.go
History View file @ 5861a0d
... ... 
@@ -9,7 +9,9 @@ import (
9 9
10 10 
 	"github.com/docker/docker/api/types"
11 11 
 	containertypes "github.com/docker/docker/api/types/container"
12 
+	"github.com/docker/docker/client"
12 13 
 	"github.com/docker/docker/integration/internal/container"
14 
+	"github.com/docker/docker/internal/test/request"
13 15 
 	"gotest.tools/assert"
14 16 
 	is "gotest.tools/assert/cmp"
15 17 
 	"gotest.tools/poll"
... ... 
@@ -108,10 +110,11 @@ func TestUpdatePidsLimit(t *testing.T) {
108 108 
 	skip.If(t, !testEnv.DaemonInfo.PidsLimit)
109 109
110 110 
 	defer setupTest(t)()
111 
-	client := testEnv.APIClient()
111 
+	apiClient := testEnv.APIClient()
112 
+	oldAPIclient := request.NewAPIClient(t, client.WithVersion("1.24"))
112 113 
 	ctx := context.Background()
113 114
114 
-	cID := container.Run(t, ctx, client)
115 
+	cID := container.Run(t, ctx, apiClient)
115 116
116 117 
 	intPtr := func(i int64) *int64 {
117 118 
 		return &i
... ... 
@@ -119,6 +122,7 @@ func TestUpdatePidsLimit(t *testing.T) {
119 119
120 120 
 	for _, test := range []struct {
121 121 
 		desc     string
122 
+		oldAPI   bool
122 123 
 		update   *int64
123 124 
 		expect   int64
124 125 
 		expectCg string
... ... 
@@ -126,24 +130,30 @@ func TestUpdatePidsLimit(t *testing.T) {
126 126 
 		{desc: "update from none", update: intPtr(32), expect: 32, expectCg: "32"},
127 127 
 		{desc: "no change", update: nil, expectCg: "32"},
128 128 
 		{desc: "update lower", update: intPtr(16), expect: 16, expectCg: "16"},
129 
+		{desc: "update on old api ignores value", oldAPI: true, update: intPtr(10), expect: 16, expectCg: "16"},
129 130 
 		{desc: "unset limit", update: intPtr(0), expect: 0, expectCg: "max"},
130 131 
 	} {
132 
+		c := apiClient
133 
+		if test.oldAPI {
134 
+			c = oldAPIclient
135 
+		}
136 
+
131 137 
 		var before types.ContainerJSON
132 138 
 		if test.update == nil {
133 139 
 			var err error
134 
-			before, err = client.ContainerInspect(ctx, cID)
140 
+			before, err = c.ContainerInspect(ctx, cID)
135 141 
 			assert.NilError(t, err)
136 142 
 		}
137 143
138 144 
 		t.Run(test.desc, func(t *testing.T) {
139 
-			_, err := client.ContainerUpdate(ctx, cID, containertypes.UpdateConfig{
145 
+			_, err := c.ContainerUpdate(ctx, cID, containertypes.UpdateConfig{
140 146 
 				Resources: containertypes.Resources{
141 147 
 					PidsLimit: test.update,
142 148 
 				},
143 149 
 			})
144 150 
 			assert.NilError(t, err)
145 151
146 
-			inspect, err := client.ContainerInspect(ctx, cID)
152 
+			inspect, err := c.ContainerInspect(ctx, cID)
147 153 
 			assert.NilError(t, err)
148 154 
 			assert.Assert(t, inspect.HostConfig.Resources.PidsLimit != nil)
149 155
... ... 
@@ -157,7 +167,7 @@ func TestUpdatePidsLimit(t *testing.T) {
157 157 
 			ctx, cancel := context.WithTimeout(ctx, 60*time.Second)
158 158 
 			defer cancel()
159 159
160 
-			res, err := container.Exec(ctx, client, cID, []string{"cat", "/sys/fs/cgroup/pids/pids.max"})
160 
+			res, err := container.Exec(ctx, c, cID, []string{"cat", "/sys/fs/cgroup/pids/pids.max"})
161 161 
 			assert.NilError(t, err)
162 162 
 			assert.Assert(t, is.Len(res.Stderr(), 0))
163 163