From 687bdc7c71a4722caa598f809b05deb2572191d5 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 8 May 2020 21:11:51 +0200 Subject: [PATCH] API: swarm: move PidsLimit to TaskTemplate.Resources The initial implementation followed the Swarm API, where PidsLimit is located in ContainerSpec. This is not the desired place for this property, so moving the field to TaskTemplate.Resources in our API. A similar change should be made in the SwarmKit API (likely keeping the old field for backward compatibility, because it was merged some releases back) Signed-off-by: Sebastiaan van Stijn --- api/server/router/swarm/helpers.go | 7 +++-- api/server/router/swarm/helpers_test.go | 14 ++++++---- api/swagger.yaml | 14 +++++----- api/types/swarm/container.go | 1 - api/types/swarm/task.go | 1 + daemon/cluster/convert/container.go | 2 -- daemon/cluster/convert/service.go | 35 ++++++++++++++++++++----- docs/api/version-history.md | 13 ++++----- integration/internal/swarm/service.go | 18 ++++++++++--- integration/service/update_test.go | 10 +++++-- 10 files changed, 80 insertions(+), 35 deletions(-) diff --git a/api/server/router/swarm/helpers.go b/api/server/router/swarm/helpers.go index 96e6f02f3a..e93ce2af94 100644 --- a/api/server/router/swarm/helpers.go +++ b/api/server/router/swarm/helpers.go @@ -97,10 +97,13 @@ func adjustForAPIVersion(cliVersion string, service *swarm.ServiceSpec) { } if versions.LessThan(cliVersion, "1.41") { if service.TaskTemplate.ContainerSpec != nil { - // Capabilities and PidsLimit for docker swarm services weren't + // Capabilities for docker swarm services weren't // supported before API version 1.41 service.TaskTemplate.ContainerSpec.Capabilities = nil - service.TaskTemplate.ContainerSpec.PidsLimit = 0 + } + if service.TaskTemplate.Resources != nil && service.TaskTemplate.Resources.Limits != nil { + // Limits.Pids not supported before API version 1.41 + service.TaskTemplate.Resources.Limits.Pids = 0 } // jobs were only introduced in API version 1.41. Nil out both Job diff --git a/api/server/router/swarm/helpers_test.go b/api/server/router/swarm/helpers_test.go index 881a6d8334..cfbb6ff40d 100644 --- a/api/server/router/swarm/helpers_test.go +++ b/api/server/router/swarm/helpers_test.go @@ -17,8 +17,7 @@ func TestAdjustForAPIVersion(t *testing.T) { spec := &swarm.ServiceSpec{ TaskTemplate: swarm.TaskSpec{ ContainerSpec: &swarm.ContainerSpec{ - Sysctls: expectedSysctls, - PidsLimit: 300, + Sysctls: expectedSysctls, Privileges: &swarm.Privileges{ CredentialSpec: &swarm.CredentialSpec{ Config: "someconfig", @@ -44,6 +43,11 @@ func TestAdjustForAPIVersion(t *testing.T) { Placement: &swarm.Placement{ MaxReplicas: 222, }, + Resources: &swarm.ResourceRequirements{ + Limits: &swarm.Limit{ + Pids: 300, + }, + }, }, } @@ -55,10 +59,10 @@ func TestAdjustForAPIVersion(t *testing.T) { t.Error("Sysctls was stripped from spec") } - if spec.TaskTemplate.ContainerSpec.PidsLimit == 0 { + if spec.TaskTemplate.Resources.Limits.Pids == 0 { t.Error("PidsLimit was stripped from spec") } - if spec.TaskTemplate.ContainerSpec.PidsLimit != 300 { + if spec.TaskTemplate.Resources.Limits.Pids != 300 { t.Error("PidsLimit did not preserve the value from spec") } @@ -80,7 +84,7 @@ func TestAdjustForAPIVersion(t *testing.T) { t.Error("Sysctls was not stripped from spec") } - if spec.TaskTemplate.ContainerSpec.PidsLimit != 0 { + if spec.TaskTemplate.Resources.Limits.Pids != 0 { t.Error("PidsLimit was not stripped from spec") } diff --git a/api/swagger.yaml b/api/swagger.yaml index dbfcf652f0..c877f2dcf6 100644 --- a/api/swagger.yaml +++ b/api/swagger.yaml @@ -638,6 +638,13 @@ definitions: type: "integer" format: "int64" example: 8272408576 + Pids: + description: | + Limits the maximum number of PIDs in the container. Set `0` for unlimited. + type: "integer" + format: "int64" + default: 0 + example: 100 ResourceObject: description: | @@ -3220,13 +3227,6 @@ definitions: configured on the daemon) is used. type: "boolean" x-nullable: true - PidsLimit: - description: | - Tune a container's PIDs limit. Set `0` for unlimited. - type: "integer" - format: "int64" - default: 0 - example: 100 Sysctls: description: | Set kernel namedspaced parameters (sysctls) in the container. diff --git a/api/types/swarm/container.go b/api/types/swarm/container.go index 2eeee9f7f5..5bbedfcf68 100644 --- a/api/types/swarm/container.go +++ b/api/types/swarm/container.go @@ -72,7 +72,6 @@ type ContainerSpec struct { Secrets []*SecretReference `json:",omitempty"` Configs []*ConfigReference `json:",omitempty"` Isolation container.Isolation `json:",omitempty"` - PidsLimit int64 `json:",omitempty"` Sysctls map[string]string `json:",omitempty"` Capabilities []string `json:",omitempty"` } diff --git a/api/types/swarm/task.go b/api/types/swarm/task.go index baeef685e0..a6f7ab7b5c 100644 --- a/api/types/swarm/task.go +++ b/api/types/swarm/task.go @@ -103,6 +103,7 @@ type Resources struct { type Limit struct { NanoCPUs int64 `json:",omitempty"` MemoryBytes int64 `json:",omitempty"` + Pids int64 `json:",omitempty"` } // GenericResource represents a "user defined" resource which can diff --git a/daemon/cluster/convert/container.go b/daemon/cluster/convert/container.go index c7742c4e01..7f1e0af22c 100644 --- a/daemon/cluster/convert/container.go +++ b/daemon/cluster/convert/container.go @@ -36,7 +36,6 @@ func containerSpecFromGRPC(c *swarmapi.ContainerSpec) *types.ContainerSpec { Configs: configReferencesFromGRPC(c.Configs), Isolation: IsolationFromGRPC(c.Isolation), Init: initFromGRPC(c.Init), - PidsLimit: c.PidsLimit, Sysctls: c.Sysctls, Capabilities: c.Capabilities, } @@ -264,7 +263,6 @@ func containerToGRPC(c *types.ContainerSpec) (*swarmapi.ContainerSpec, error) { Secrets: secretReferencesToGRPC(c.Secrets), Isolation: isolationToGRPC(c.Isolation), Init: initToGRPC(c.Init), - PidsLimit: c.PidsLimit, Sysctls: c.Sysctls, Capabilities: c.Capabilities, } diff --git a/daemon/cluster/convert/service.go b/daemon/cluster/convert/service.go index 1daabd0156..4161f89479 100644 --- a/daemon/cluster/convert/service.go +++ b/daemon/cluster/convert/service.go @@ -193,6 +193,10 @@ func ServiceSpecToGRPC(s types.ServiceSpec) (swarmapi.ServiceSpec, error) { if err != nil { return swarmapi.ServiceSpec{}, err } + if s.TaskTemplate.Resources != nil && s.TaskTemplate.Resources.Limits != nil { + // TODO remove this (or keep for backward compat) once SwarmKit API moved PidsLimit into Resources + containerSpec.PidsLimit = s.TaskTemplate.Resources.Limits.Pids + } spec.Task.Runtime = &swarmapi.TaskSpec_Container{Container: containerSpec} } else { // If the ContainerSpec is nil, we can't set the task runtime @@ -396,15 +400,31 @@ func GenericResourcesFromGRPC(genericRes []*swarmapi.GenericResource) []types.Ge return generic } -func resourcesFromGRPC(res *swarmapi.ResourceRequirements) *types.ResourceRequirements { +// resourcesFromGRPC creates a ResourceRequirements from the GRPC TaskSpec. +// We currently require the whole TaskSpec to be passed, because PidsLimit +// is returned as part of the container spec, instead of Resources +// TODO move PidsLimit to Resources in the Swarm API +func resourcesFromGRPC(ts *swarmapi.TaskSpec) *types.ResourceRequirements { var resources *types.ResourceRequirements - if res != nil { - resources = &types.ResourceRequirements{} + + if cs := ts.GetContainer(); cs != nil && cs.PidsLimit != 0 { + resources = &types.ResourceRequirements{ + Limits: &types.Limit{ + Pids: cs.PidsLimit, + }, + } + } + if ts.Resources != nil { + if resources == nil { + resources = &types.ResourceRequirements{} + } + res := ts.Resources if res.Limits != nil { - resources.Limits = &types.Limit{ - NanoCPUs: res.Limits.NanoCPUs, - MemoryBytes: res.Limits.MemoryBytes, + if resources.Limits == nil { + resources.Limits = &types.Limit{} } + resources.Limits.NanoCPUs = res.Limits.NanoCPUs + resources.Limits.MemoryBytes = res.Limits.MemoryBytes } if res.Reservations != nil { resources.Reservations = &types.Resources{ @@ -441,6 +461,7 @@ func resourcesToGRPC(res *types.ResourceRequirements) *swarmapi.ResourceRequirem if res != nil { reqs = &swarmapi.ResourceRequirements{} if res.Limits != nil { + // TODO add PidsLimit once Swarm API has been updated to move it into Limits reqs.Limits = &swarmapi.Resources{ NanoCPUs: res.Limits.NanoCPUs, MemoryBytes: res.Limits.MemoryBytes, @@ -657,7 +678,7 @@ func taskSpecFromGRPC(taskSpec swarmapi.TaskSpec) (types.TaskSpec, error) { } t := types.TaskSpec{ - Resources: resourcesFromGRPC(taskSpec.Resources), + Resources: resourcesFromGRPC(&taskSpec), RestartPolicy: restartPolicyFromGRPC(taskSpec.Restart), Placement: placementFromGRPC(taskSpec.Placement), LogDriver: driverFromGRPC(taskSpec.LogDriver), diff --git a/docs/api/version-history.md b/docs/api/version-history.md index a4f8cddff1..e4ac6efd38 100644 --- a/docs/api/version-history.md +++ b/docs/api/version-history.md @@ -31,12 +31,13 @@ keywords: "API, Docker, rcli, REST, documentation" * `POST /services/{id}/update` now accepts `Capabilities` as part of the `ContainerSpec`. * `GET /tasks` now returns `Capabilities` as part of the `ContainerSpec`. * `GET /tasks/{id}` now returns `Capabilities` as part of the `ContainerSpec`. -* `GET /services` now returns `PidsLimit` as part of the `ContainerSpec`. -* `GET /services/{id}` now returns `PidsLimit` as part of the `ContainerSpec`. -* `POST /services/create` now accepts `PidsLimit` as part of the `ContainerSpec`. -* `POST /services/{id}/update` now accepts `PidsLimit` as part of the `ContainerSpec`. -* `GET /tasks` now returns `PidsLimit` as part of the `ContainerSpec`. -* `GET /tasks/{id}` now returns `PidsLimit` as part of the `ContainerSpec`. +* `GET /services` now returns `Pids` in `TaskTemplate.Resources.Limits`. +* `GET /services/{id}` now returns `Pids` in `TaskTemplate.Resources.Limits`. +* `POST /services/create` now accepts `Pids` in `TaskTemplate.Resources.Limits`. +* `POST /services/{id}/update` now accepts `Pids` in `TaskTemplate.Resources.Limits` + to limit the maximum number of PIDs. +* `GET /tasks` now returns `Pids` in `TaskTemplate.Resources.Limits`. +* `GET /tasks/{id}` now returns `Pids` in `TaskTemplate.Resources.Limits`. * `POST /containers/create` on Linux now accepts the `HostConfig.CgroupnsMode` property. Set the property to `host` to create the container in the daemon's cgroup namespace, or `private` to create the container in its own private cgroup namespace. The per-daemon diff --git a/integration/internal/swarm/service.go b/integration/internal/swarm/service.go index 1ee5004860..5e9bb416ef 100644 --- a/integration/internal/swarm/service.go +++ b/integration/internal/swarm/service.go @@ -196,11 +196,11 @@ func ServiceWithCapabilities(Capabilities []string) ServiceSpecOpt { } } -// ServiceWithPidsLimit sets the PidsLimit option of the service's ContainerSpec. +// ServiceWithPidsLimit sets the PidsLimit option of the service's Resources.Limits. func ServiceWithPidsLimit(limit int64) ServiceSpecOpt { return func(spec *swarmtypes.ServiceSpec) { - ensureContainerSpec(spec) - spec.TaskTemplate.ContainerSpec.PidsLimit = limit + ensureResources(spec) + spec.TaskTemplate.Resources.Limits.Pids = limit } } @@ -235,6 +235,18 @@ func ExecTask(t *testing.T, d *daemon.Daemon, task swarmtypes.Task, config types return attach } +func ensureResources(spec *swarmtypes.ServiceSpec) { + if spec.TaskTemplate.Resources == nil { + spec.TaskTemplate.Resources = &swarmtypes.ResourceRequirements{} + } + if spec.TaskTemplate.Resources.Limits == nil { + spec.TaskTemplate.Resources.Limits = &swarmtypes.Limit{} + } + if spec.TaskTemplate.Resources.Reservations == nil { + spec.TaskTemplate.Resources.Reservations = &swarmtypes.Resources{} + } +} + func ensureContainerSpec(spec *swarmtypes.ServiceSpec) { if spec.TaskTemplate.ContainerSpec == nil { spec.TaskTemplate.ContainerSpec = &swarmtypes.ContainerSpec{} diff --git a/integration/service/update_test.go b/integration/service/update_test.go index 66f81689e1..db12ec210d 100644 --- a/integration/service/update_test.go +++ b/integration/service/update_test.go @@ -295,7 +295,13 @@ func TestServiceUpdatePidsLimit(t *testing.T) { serviceID = swarm.CreateService(t, d, swarm.ServiceWithPidsLimit(tc.pidsLimit)) } else { service = getService(t, cli, serviceID) - service.Spec.TaskTemplate.ContainerSpec.PidsLimit = tc.pidsLimit + if service.Spec.TaskTemplate.Resources == nil { + service.Spec.TaskTemplate.Resources = &swarmtypes.ResourceRequirements{} + } + if service.Spec.TaskTemplate.Resources.Limits == nil { + service.Spec.TaskTemplate.Resources.Limits = &swarmtypes.Limit{} + } + service.Spec.TaskTemplate.Resources.Limits.Pids = tc.pidsLimit _, err := cli.ServiceUpdate(ctx, serviceID, service.Version, service.Spec, types.ServiceUpdateOptions{}) assert.NilError(t, err) poll.WaitOn(t, serviceIsUpdated(cli, serviceID), swarm.ServicePoll) @@ -304,7 +310,7 @@ func TestServiceUpdatePidsLimit(t *testing.T) { poll.WaitOn(t, swarm.RunningTasksCount(cli, serviceID, 1), swarm.ServicePoll) service = getService(t, cli, serviceID) container := getServiceTaskContainer(ctx, t, cli, serviceID) - assert.Equal(t, service.Spec.TaskTemplate.ContainerSpec.PidsLimit, tc.expected) + assert.Equal(t, service.Spec.TaskTemplate.Resources.Limits.Pids, tc.expected) if tc.expected == 0 { if container.HostConfig.Resources.PidsLimit != nil { t.Fatalf("Expected container.HostConfig.Resources.PidsLimit to be nil")