Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for sysctl options in services #37701

Merged
merged 1 commit into from Sep 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 22 additions & 4 deletions api/server/router/swarm/cluster_routes.go
Expand Up @@ -182,8 +182,17 @@ func (sr *swarmRouter) createService(ctx context.Context, w http.ResponseWriter,
encodedAuth := r.Header.Get("X-Registry-Auth")
cliVersion := r.Header.Get("version")
queryRegistry := false
if cliVersion != "" && versions.LessThan(cliVersion, "1.30") {
queryRegistry = true
if cliVersion != "" {
if versions.LessThan(cliVersion, "1.30") {
queryRegistry = true
}
if versions.LessThan(cliVersion, "1.39") {
if service.TaskTemplate.ContainerSpec != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to a function and use it here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i disagree, but mostly because i'm unsure what the best signature for that function would be, and considering it's only like 8 lines duplicated, i don't think there's much of a benefit to the refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

every duplicated line of code counts :)
But that's borderline bikeshedding

Copy link
Contributor

@wk8 wk8 Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would tend to agree with @anshulpundir here (though the question of the fun's signature is indeed interesting!)

Also, I'm a little worried that this file is gonna get littered with version checks really fast if we really want to ensure never adding new features to frozen versions; don't get me wrong, it makes sense, but I'm actually quite surprised there's only one check as of now. There's certainly going to be a lot more if we do all of #25303 . There might be a better way of doing this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for the sake of it, I tend to agree with @dperny : sometimes a little duplication is better than the wrong abstraction… once we find a correct abstraction for those, we'll refactor 😉

// Sysctls for docker swarm services weren't supported before
// API version 1.39
service.TaskTemplate.ContainerSpec.Sysctls = nil
}
}
}

resp, err := sr.backend.CreateService(service, encodedAuth, queryRegistry)
Expand Down Expand Up @@ -216,8 +225,17 @@ func (sr *swarmRouter) updateService(ctx context.Context, w http.ResponseWriter,
flags.Rollback = r.URL.Query().Get("rollback")
cliVersion := r.Header.Get("version")
queryRegistry := false
if cliVersion != "" && versions.LessThan(cliVersion, "1.30") {
queryRegistry = true
if cliVersion != "" {
if versions.LessThan(cliVersion, "1.30") {
queryRegistry = true
}
if versions.LessThan(cliVersion, "1.39") {
if service.TaskTemplate.ContainerSpec != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Good one; wondering now if creating a service without ContainerSpec is actually a valid request.

Not something to address in this PR, but just wondering that; and if we should validate/error in that situation 🤔

// Sysctls for docker swarm services weren't supported before
// API version 1.39
service.TaskTemplate.ContainerSpec.Sysctls = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this is needed?

Copy link
Contributor

@wk8 wk8 Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(to elaborate a bit more: shouldn't this be nil anyway if it's not present in the request?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
}
}

resp, err := sr.backend.UpdateService(vars["id"], version, service, flags, queryRegistry)
Expand Down
12 changes: 12 additions & 0 deletions api/swagger.yaml
Expand Up @@ -2750,6 +2750,18 @@ definitions:
description: "Run an init inside the container that forwards signals and reaps processes. This field is omitted if empty, and the default (as configured on the daemon) is used."
type: "boolean"
x-nullable: true
Sysctls:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the API version history, and add a bullet for this new option? https://github.com/moby/moby/blob/master/docs/api/version-history.md#v139-api-changes

(make sure to mention all of POST /services/create, POST /services/{id}/update, and GET /services/{id})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I always forget the API version history.

description: |
Set kernel namedspaced parameters (sysctls) in the container.
The Sysctls option on services accepts the same sysctls as the
are supported on containers. Note that while the same sysctls are
supported, no guarantees or checks are made about their
suitability for a clustered environment, and it's up to the user
to determine whether a given sysctl will work properly in a
Service.
type: "object"
additionalProperties:
type: "string"
NetworkAttachmentSpec:
description: |
Read-only spec type for non-swarm containers attached to swarm overlay
Expand Down
1 change: 1 addition & 0 deletions api/types/swarm/container.go
Expand Up @@ -71,4 +71,5 @@ type ContainerSpec struct {
Secrets []*SecretReference `json:",omitempty"`
Configs []*ConfigReference `json:",omitempty"`
Isolation container.Isolation `json:",omitempty"`
Sysctls map[string]string `json:",omitempty"`
vdemeester marked this conversation as resolved.
Show resolved Hide resolved
}
2 changes: 2 additions & 0 deletions daemon/cluster/convert/container.go
Expand Up @@ -36,6 +36,7 @@ func containerSpecFromGRPC(c *swarmapi.ContainerSpec) *types.ContainerSpec {
Configs: configReferencesFromGRPC(c.Configs),
Isolation: IsolationFromGRPC(c.Isolation),
Init: initFromGRPC(c.Init),
Sysctls: c.Sysctls,
}

if c.DNSConfig != nil {
Expand Down Expand Up @@ -251,6 +252,7 @@ func containerToGRPC(c *types.ContainerSpec) (*swarmapi.ContainerSpec, error) {
Configs: configReferencesToGRPC(c.Configs),
Isolation: isolationToGRPC(c.Isolation),
Init: initToGRPC(c.Init),
Sysctls: c.Sysctls,
}

if c.DNSConfig != nil {
Expand Down
1 change: 1 addition & 0 deletions daemon/cluster/executor/container/container.go
Expand Up @@ -364,6 +364,7 @@ func (c *containerConfig) hostConfig() *enginecontainer.HostConfig {
ReadonlyRootfs: c.spec().ReadOnly,
Isolation: c.isolation(),
Init: c.init(),
Sysctls: c.spec().Sysctls,
}

if c.spec().DNSConfig != nil {
Expand Down
6 changes: 6 additions & 0 deletions docs/api/version-history.md
Expand Up @@ -30,6 +30,12 @@ keywords: "API, Docker, rcli, REST, documentation"
on the node.label. The format of the label filter is `node.label=<key>`/`node.label=<key>=<value>`
to return those with the specified labels, or `node.label!=<key>`/`node.label!=<key>=<value>`
to return those without the specified labels.
* `GET /services` now returns `Sysctls` as part of the `ContainerSpec`.
* `GET /services/{id}` now returns `Sysctls` as part of the `ContainerSpec`.
* `POST /services/create` now accepts `Sysctls` as part of the `ContainerSpec`.
* `POST /services/{id}/update` now accepts `Sysctls` as part of the `ContainerSpec`.
* `GET /tasks` now returns `Sysctls` as part of the `ContainerSpec`.
* `GET /tasks/{id}` now returns `Sysctls` as part of the `ContainerSpec`.

## V1.38 API changes

Expand Down
8 changes: 8 additions & 0 deletions integration/internal/swarm/service.go
Expand Up @@ -159,6 +159,14 @@ func ServiceWithEndpoint(endpoint *swarmtypes.EndpointSpec) ServiceSpecOpt {
}
}

// ServiceWithSysctls sets the Sysctls option of the service's ContainerSpec.
func ServiceWithSysctls(sysctls map[string]string) ServiceSpecOpt {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this only used in the unit-test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes (well, it's technically an integration test), but a lot of these functions are only used in one test.

return func(spec *swarmtypes.ServiceSpec) {
ensureContainerSpec(spec)
spec.TaskTemplate.ContainerSpec.Sysctls = sysctls
}
}

// GetRunningTasks gets the list of running tasks for a service
func GetRunningTasks(t *testing.T, d *daemon.Daemon, serviceID string) []swarmtypes.Task {
t.Helper()
Expand Down
97 changes: 97 additions & 0 deletions integration/service/create_test.go
Expand Up @@ -10,13 +10,15 @@ import (
"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/filters"
swarmtypes "github.com/docker/docker/api/types/swarm"
"github.com/docker/docker/api/types/versions"
"github.com/docker/docker/client"
"github.com/docker/docker/integration/internal/network"
"github.com/docker/docker/integration/internal/swarm"
"github.com/docker/docker/internal/test/daemon"
"gotest.tools/assert"
is "gotest.tools/assert/cmp"
"gotest.tools/poll"
"gotest.tools/skip"
)

func TestServiceCreateInit(t *testing.T) {
Expand Down Expand Up @@ -309,6 +311,101 @@ func TestCreateServiceConfigFileMode(t *testing.T) {
assert.NilError(t, err)
}

// TestServiceCreateSysctls tests that a service created with sysctl options in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we have a similar test for updates too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there isn't one for the other things tested here.

// the ContainerSpec correctly applies those options.
//
// To test this, we're going to create a service with the sysctl option
//
// {"net.ipv4.ip_nonlocal_bind": "0"}
//
// We'll get the service's tasks to get the container ID, and then we'll
// inspect the container. If the output of the container inspect contains the
// sysctl option with the correct value, we can assume that the sysctl has been
// plumbed correctly.
//
// Next, we'll remove that service and create a new service with that option
// set to 1. This means that no matter what the default is, we can be confident
// that the sysctl option is applying as intended.
//
// Additionally, we'll do service and task inspects to verify that the inspect
// output includes the desired sysctl option.
//
// We're using net.ipv4.ip_nonlocal_bind because it's something that I'm fairly
// confident won't be modified by the container runtime, and won't blow
// anything up in the test environment
func TestCreateServiceSysctls(t *testing.T) {
skip.If(
t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.39"),
"setting service sysctls is unsupported before api v1.39",
)

defer setupTest(t)()
d := swarm.NewSwarm(t, testEnv)
defer d.Stop(t)
client := d.NewClientT(t)
defer client.Close()

ctx := context.Background()

// run thie block twice, so that no matter what the default value of
// net.ipv4.ip_nonlocal_bind is, we can verify that setting the sysctl
// options works
for _, expected := range []string{"0", "1"} {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if a subtest would make this clearer 🤔

for _, expected := range []string{"0", "1"} {
    t.Run(fmt.Sprintf("net.ipv4.ip_nonlocal_bind = %s", expected), func(t *testing.T) {
		// store the map we're going to be using everywhere.
 		sysctlOpts := map[string]string{"net.ipv4.ip_nonlocal_bind": expected}
 		...            
    })
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that a subtest is useful honestly, because it's just 1 test, we're just trying two values so we don't have to know what the default is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was looking if we should have a more generic service create test where we define a list of specs to create services with. Agreed that it's just a minor issue; advantage would be to more easily find which of the test-cases failed

// store the map we're going to be using everywhere.
expectedSysctls := map[string]string{"net.ipv4.ip_nonlocal_bind": expected}

// Create the service with the sysctl options
var instances uint64 = 1
serviceID := swarm.CreateService(t, d,
swarm.ServiceWithSysctls(expectedSysctls),
)

// wait for the service to converge to 1 running task as expected
poll.WaitOn(t, serviceRunningTasksCount(client, serviceID, instances))

// we're going to check 3 things:
//
// 1. Does the container, when inspected, have the sysctl option set?
// 2. Does the task have the sysctl in the spec?
// 3. Does the service have the sysctl in the spec?
//
// if all 3 of these things are true, we know that the sysctl has been
// plumbed correctly through the engine.
//
// We don't actually have to get inside the container and check its
// logs or anything. If we see the sysctl set on the container inspect,
// we know that the sysctl is plumbed correctly. everything below that
// level has been tested elsewhere. (thanks @thaJeztah, because an
// earlier version of this test had to get container logs and was much
// more complex)

// get all of the tasks of the service, so we can get the container
filter := filters.NewArgs()
filter.Add("service", serviceID)
tasks, err := client.TaskList(ctx, types.TaskListOptions{
Filters: filter,
})
assert.NilError(t, err)
assert.Check(t, is.Equal(len(tasks), 1))

// verify that the container has the sysctl option set
ctnr, err := client.ContainerInspect(ctx, tasks[0].Status.ContainerStatus.ContainerID)
assert.NilError(t, err)
assert.DeepEqual(t, ctnr.HostConfig.Sysctls, expectedSysctls)

// verify that the task has the sysctl option set in the task object
assert.DeepEqual(t, tasks[0].Spec.ContainerSpec.Sysctls, expectedSysctls)

// verify that the service also has the sysctl set in the spec.
service, _, err := client.ServiceInspectWithRaw(ctx, serviceID, types.ServiceInspectOptions{})
assert.NilError(t, err)
assert.DeepEqual(t,
service.Spec.TaskTemplate.ContainerSpec.Sysctls, expectedSysctls,
)
}
}

func serviceRunningTasksCount(client client.ServiceAPIClient, serviceID string, instances uint64) func(log poll.LogT) poll.Result {
return func(log poll.LogT) poll.Result {
filter := filters.NewArgs()
Expand Down