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

Do we need a separate test for service update ? (perhaps overkill)

Yes, we should make one, actually, to verify that service update goes through correctly when Sysctls are changed.

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}
 		...            
    })
}

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.

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

Services should already be removed when the test completes, so I think we can skip this, and just continue with the next iteration

We don't need a name for this service, so better remove it, and just use the serviceID (which is already used)

Remove the name

Should we use service logs instead?

As the person ostensibly responsible for service logs, no.

In that case, we must skip the test if it's running on a multi-node swarm.

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

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 run.

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.

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 👍

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"

Is there a way to reference or crosslink to the documentation for this option on the container HostConfig?

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

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

Ah I always forget the API version history.

missed a closing back tic after Sysctls

👍 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 🤔

Upstream was merged; can you un-fork the dependency?

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

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.

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

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?

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 😉

Is this only used in the unit-test?

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

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

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

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 😉