Add support for sysctl options in services #37701
Conversation
Thanks! Left comments/thoughts inline. We should also look at the docker/cli side for this (and add this option to the docker-compose file format) |
// confident won't be modified by the container runtime, and won't blow | ||
// anything up in the test environment | ||
func TestCreateServiceSysctls(t *testing.T) { | ||
defer setupTest(t)() |
thaJeztah
Aug 23, 2018
Member
Can you add a skip for older daemon versions? (assuming this is planned for inclusion in API 1.39)
skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.39"), "feature was added in API version 1.39")
Can you add a skip for older daemon versions? (assuming this is planned for inclusion in API 1.39)
skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.39"), "feature was added in API version 1.39")
thaJeztah
Aug 23, 2018
Member
Do we need a separate test for service update
? (perhaps overkill)
Do we need a separate test for service update
? (perhaps overkill)
dperny
Aug 23, 2018
Author
Contributor
Yes, we should make one, actually, to verify that service update
goes through correctly when Sysctls are changed.
Yes, we should make one, actually, to verify that service update
goes through correctly when Sysctls are changed.
// net.ipv4.ip_nonlocal_bind is, we can verify that setting the sysctl | ||
// options works | ||
for _, expected := range []string{"0", "1"} { | ||
|
thaJeztah
Aug 23, 2018
Member
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}
...
})
}
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}
...
})
}
dperny
Aug 23, 2018
Author
Contributor
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.
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.
thaJeztah
Aug 23, 2018
Member
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
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
) | ||
|
||
// remove the service | ||
err = client.ServiceRemove(ctx, serviceID) |
thaJeztah
Aug 23, 2018
Member
Services should already be removed when the test completes, so I think we can skip this, and just continue with the next iteration
Services should already be removed when the test completes, so I think we can skip this, and just continue with the next iteration
|
||
// Create the service with the sysctl options | ||
var instances uint64 = 1 | ||
serviceName := "TestService_" + t.Name() + "_" + expected |
thaJeztah
Aug 23, 2018
Member
We don't need a name for this service, so better remove it, and just use the serviceID
(which is already used)
We don't need a name for this service, so better remove it, and just use the serviceID
(which is already used)
var instances uint64 = 1 | ||
serviceName := "TestService_" + t.Name() + "_" + expected | ||
serviceID := swarm.CreateService(t, d, | ||
swarm.ServiceWithName(serviceName), |
thaJeztah
Aug 23, 2018
Member
Remove the name
Remove the name
// avoid that case, we're using poll.WaitOn with a closure to wait | ||
// until logs actually respond | ||
poll.WaitOn(t, func(t poll.LogT) poll.Result { | ||
body, err := client.ContainerLogs( |
thaJeztah
Aug 23, 2018
Member
Should we use service logs instead?
Should we use service logs instead?
dperny
Aug 23, 2018
Author
Contributor
As the person ostensibly responsible for service logs, no.
As the person ostensibly responsible for service logs, no.
thaJeztah
Aug 23, 2018
Member
In that case, we must skip the test if it's running on a multi-node swarm.
In that case, we must skip the test if it's running on a multi-node swarm.
// if the value is empty, we may not have gotten the output of the | ||
// container logs yet, and we should try again. | ||
if val == "" { | ||
return poll.Continue("output of container logs is empty") |
thaJeztah
Aug 23, 2018
Member
Let me think about this;
Instead of using the container's / service's logs to check if the value was set, could we abuse the container's command for this (something like if cat /proc/sys/net/ipv4/ip_nonlocal_bind != expected value; exit 1
)?
Thinking a bit further about this; I'm wondering if we need to check this at all. Creating a container with custom sysctl
settings is an existing feature. The only thing being added in this PR is that that container is now created through SwarmKit instead of docker run
.
Because of that, I think all we need to verify is if the container-spec is correct (the expected sysctl options are set).
Let me think about this;
Instead of using the container's / service's logs to check if the value was set, could we abuse the container's command for this (something like if cat /proc/sys/net/ipv4/ip_nonlocal_bind != expected value; exit 1
)?
Thinking a bit further about this; I'm wondering if we need to check this at all. Creating a container with custom sysctl
settings is an existing feature. The only thing being added in this PR is that that container is now created through SwarmKit instead of docker run
.
Because of that, I think all we need to verify is if the container-spec is correct (the expected sysctl options are set).
dperny
Aug 23, 2018
Author
Contributor
Instead of using the container's / service's logs to check if the value was set, could we abuse the container's command for this (something like if cat /proc/sys/net/ipv4/ip_nonlocal_bind != expected value; exit 1)?
What do you think is better about doing it this way? Having containers exit in a swarm service test leaves us dealing with swarmkit rescheduling those containers.
Thinking a bit further about this; I'm wondering if we need to check this at all. Creating a container with custom sysctl
settings is an existing feature. The only thing being added in this PR is that that container is now created through SwarmKit instead of docker ru
n.
We do, actually, to make sure that the value of the Sysctls is plumbed through correctly. When writing this test, I actually missed a location where the value was plumbed through and so the value was making its way into the container spec, but not into the actual container.
Instead of using the container's / service's logs to check if the value was set, could we abuse the container's command for this (something like if cat /proc/sys/net/ipv4/ip_nonlocal_bind != expected value; exit 1)?
What do you think is better about doing it this way? Having containers exit in a swarm service test leaves us dealing with swarmkit rescheduling those containers.
Thinking a bit further about this; I'm wondering if we need to check this at all. Creating a container with custom
sysctl
settings is an existing feature. The only thing being added in this PR is that that container is now created through SwarmKit instead ofdocker ru
n.
We do, actually, to make sure that the value of the Sysctls is plumbed through correctly. When writing this test, I actually missed a location where the value was plumbed through and so the value was making its way into the container spec, but not into the actual container.
thaJeztah
Aug 23, 2018
Member
We do, actually, to make sure that the value of the Sysctls is plumbed through correctly.
Discussed this with @dperny on Slack; inspecting the container should contain enough information that the correct container-spec was created. Creating containers from a spec/config is already covered by other tests, so for this feature we don't have to test that part 👍
We do, actually, to make sure that the value of the Sysctls is plumbed through correctly.
Discussed this with @dperny on Slack; inspecting the container should contain enough information that the correct container-spec was created. Creating containers from a spec/config is already covered by other tests, so for this feature we don't have to test that part
description: "Set kernel namedspaced parameters (sysctls) in the container." | ||
type: "object" | ||
additionalProperties: | ||
type: "string" |
thaJeztah
Aug 23, 2018
Member
Can you add an example here?
Sysctls:
description: "Set kernel namedspaced parameters (sysctls) in the container."
type: "object"
additionalProperties:
type: "string"
example:
net.ipv4.tcp_keepalive_time: "600"
net.ipv4.tcp_keepalive_intvl: "60"
net.ipv4.tcp_keepalive_probes: "3"
net.ipv4.tcp_timestamps: "0"
Can you add an example here?
Sysctls:
description: "Set kernel namedspaced parameters (sysctls) in the container."
type: "object"
additionalProperties:
type: "string"
example:
net.ipv4.tcp_keepalive_time: "600"
net.ipv4.tcp_keepalive_intvl: "60"
net.ipv4.tcp_keepalive_probes: "3"
net.ipv4.tcp_timestamps: "0"
dperny
Aug 23, 2018
Author
Contributor
Is there a way to reference or crosslink to the documentation for this option on the container HostConfig
?
Is there a way to reference or crosslink to the documentation for this option on the container HostConfig
?
thaJeztah
Aug 23, 2018
Member
Hm; good one, perhaps just refer to it in the description 🤔 ("this option accepts the same sysctls as can be specified for containers", or something along those lines 😂 )
Hm; good one, perhaps just refer to it in the description
@@ -2725,6 +2725,11 @@ 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: |
thaJeztah
Aug 23, 2018
Member
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}
)
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}
)
dperny
Aug 23, 2018
Author
Contributor
Ah I always forget the API version history.
Ah I always forget the API version history.
3ff99e4
to
a8e023b
@@ -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`. |
thaJeztah
Aug 23, 2018
Member
missed a closing back tic after Sysctls
missed a closing back tic after Sysctls
queryRegistry = true | ||
} | ||
if versions.LessThan(cliVersion, "1.39") { | ||
if service.TaskTemplate.ContainerSpec != nil { |
thaJeztah
Aug 23, 2018
Member
👍 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 🤔
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
a8e023b
to
6a24f7b
Codecov Report
@@ Coverage Diff @@
## master #37701 +/- ##
==========================================
+ Coverage 36.12% 36.14% +0.01%
==========================================
Files 610 610
Lines 45083 45086 +3
==========================================
+ Hits 16288 16297 +9
+ Misses 26555 26551 -4
+ Partials 2240 2238 -2 |
LGTM after the of course, depends on the upstream swarmkit PR to be merged |
I forgot to add the t.Skip(), so I'll go back and do so when I revendor swarmkit |
6a24f7b
to
1f8f6c3
@thaJeztah updated the swagger.yml description of the |
@@ -125,7 +125,7 @@ github.com/containerd/ttrpc 94dde388801693c54f88a6596f713b51a8b30b2d | |||
github.com/gogo/googleapis 08a7655d27152912db7aaf4f983275eaf8d128ef | |||
|
|||
# cluster | |||
github.com/docker/swarmkit cfa742c8abe6f8e922f6e4e920153c408e7d9c3b | |||
github.com/docker/swarmkit 0e685ac60386c15cfefbf6626421bf0468b9444e git://github.com/dperny/swarmkit-1 |
thaJeztah
Aug 27, 2018
Member
Upstream was merged; can you un-fork the dependency?
Upstream was merged; can you un-fork the dependency?
OH, can you also add the example to the swagger YAML (see my comment here; #37701 (comment)) |
1f8f6c3
to
dd7434e
queryRegistry = true | ||
} | ||
if versions.LessThan(cliVersion, "1.39") { | ||
if service.TaskTemplate.ContainerSpec != nil { |
anshulpundir
Aug 28, 2018
Contributor
Move this to a function and use it here and below.
Move this to a function and use it here and below.
dperny
Sep 10, 2018
Author
Contributor
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.
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.
anshulpundir
Sep 12, 2018
Contributor
every duplicated line of code counts :)
But that's borderline bikeshedding
every duplicated line of code counts :)
But that's borderline bikeshedding
wk8
Sep 12, 2018
•
Contributor
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?
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?
vdemeester
Sep 26, 2018
Member
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 😉
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
@@ -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 { |
anshulpundir
Aug 28, 2018
Contributor
Is this only used in the unit-test?
Is this only used in the unit-test?
dperny
Sep 10, 2018
Author
Contributor
Yes (well, it's technically an integration test), but a lot of these functions are only used in one test.
Yes (well, it's technically an integration test), but a lot of these functions are only used in one test.
if service.TaskTemplate.ContainerSpec != nil { | ||
// Sysctls for docker swarm services weren't supported before | ||
// API version 1.39 | ||
service.TaskTemplate.ContainerSpec.Sysctls = nil |
wk8
Sep 12, 2018
Contributor
I don't understand why this is needed?
I don't understand why this is needed?
wk8
Sep 12, 2018
•
Contributor
(to elaborate a bit more: shouldn't this be nil
anyway if it's not present in the request?)
(to elaborate a bit more: shouldn't this be nil
anyway if it's not present in the request?)
@@ -309,6 +311,101 @@ func TestCreateServiceConfigFileMode(t *testing.T) { | |||
assert.NilError(t, err) | |||
} | |||
|
|||
// TestServiceCreateSysctls tests that a service created with sysctl options in |
wk8
Sep 12, 2018
Contributor
Shouldn't we have a similar test for updates too?
Shouldn't we have a similar test for updates too?
dperny
Sep 24, 2018
Author
Contributor
there isn't one for the other things tested here.
there isn't one for the other things tested here.
The pre-req, docker/swarmkit#2729, was already merged into master 22 days ago. How close are we to seeing this in |
@vdemeester Just checking if it's possible to get this in |
8e0ce06
to
5785f17
I had an error from not running swagger generation. However, when I ran it, |
During a load test against one of our containers, we found that hitting the container directly, outside of swarm mode, resulted in 1.8x more throughput than when that container was running as one service instance in a one node Swarm. Like @dperny and @cballou, we are really looking forward to having performance fixes / sysctl options expedited |
Adds support for sysctl options in docker services. * Adds API plumbing for creating services with sysctl options set. * Adds swagger.yaml documentation for new API field. * Updates the API version history document. * Changes executor package to make use of the Sysctls field on objects * Includes integration test to verify that new behavior works. Essentially, everything needed to support the equivalent of docker run's `--sysctl` option except the CLI. Includes a vendoring of swarmkit for proto changes to support the new behavior. Signed-off-by: Drew Erny <drew.erny@docker.com>
5785f17
to
14da20f
LGTM ping @vdemeester @anshulpundir PTAL |
LGTM |
queryRegistry = true | ||
} | ||
if versions.LessThan(cliVersion, "1.39") { | ||
if service.TaskTemplate.ContainerSpec != nil { |
vdemeester
Sep 26, 2018
Member
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 😉
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
9f296d1
into
moby:master
So are we finally able to set "sysctl" options for Docker services or is this just the underlying updates required to eventually expose this to the CLI ? |
So I see that the API version was changed from |
@cballou API v1.40 is not finalised yet (as in; changes will still arrive). That version will be included in the next Docker release (after Docker 18.09, which has API v1.39) |
@thaJeztah hey, did the feature make it into the release? I am trying set the sysctl for a stack via Golang code and it doesn't work for me (but works for the regular containers). I set my API version in Golang client code to 1.39 to no avail. Also trying to deploy from command line give me this:
|
No, this didn't make it for 18.09, so will be in 19.03 |
Hi, updated to docker-ce nightly and tried to pass This is the resulting output:
Anything required to make this work? @thaJeztah |
Opened a PR for the cli and compose-file changes (for |
@thaJeztah I have tried with the latest 19.0.3 beta to no avail:
Still returns 128. I have copied your test to reproduce the issue programmatically, and it also fails: https://github.com/orbs-network/boyarin/pull/80/files#diff-e5db09d7878227a83cc084a0fd016450 I have tried both API versions
|
- What I did
Adds support for sysctl options in docker services.
Essentially, everything needed to support the equivalent of docker run's
--sysctl
option except the CLI.Depends on docker/swarmkit#2729, which is not merged yet, and so has my fork branch of swarmkit vendored in to demonstrate passing integration test.
- How I did it
Altered all of the API objects and plumbing to accommodate the new field, which swarmkit blindly passes into the executor.
- How to verify it
Includes an integration test.
Related issues:
- Description for the changelog
Add support for sysctl options on docker services.