Restore the SetKey prestart hook. #47621

robmry · 2024-03-23T17:25:00Z

- What I did

Fix simple run command with --sysctl for network interface fails after upgrade #47619
Partially reverted 0046b16 - daemon: set libnetwork sandbox key w/o OCI hook; Detect IPv6 support in containers, generate '/etc/hosts' accordingly. #47062
Partially reverted ef5295c - Don't configure IPv6 addr/gw when IPv6 disabled; Don't configure IPv6 addr/gw when IPv6 disabled. #47521

Running SetKey to store the OCI Sandbox key after task creation, rather than from the OCI prestart hook, meant it happened after sysctl settings were applied by the runtime - which was the intention, we wanted to complete Sandbox configuration after IPv6 had been disabled by a sysctl if that was going to happen.

But, it meant '--sysctl' options for a specfic network interface caused container task creation to fail, because the interface is only moved into the network namespace during SetKey.

- How I did it

Restored the SetKey prestart hook.

Regenerate config files that depend on the container's support for IPv6 after the task has been created.

The changes in the second partially-reverted commit, to check IPv6 support before assigning an interface address/gateway would no longer work, but are no longer necessary. IPv6 addresses applied during the SetKey prestart hook will be removed when the sysctl disabling IPv6 in the container is applied.

- How to verify it

Added a regression test, to make sure it's possible to set an interface-specfic sysctl.

The tests for IPv6 addresses in '/etc/hosts' when IPv6 is disabled still work.

- Description for the changelog

Fix a regression that meant network interface specific `--sysctl` options prevented container startup.

thaJeztah · 2024-03-25T09:34:03Z

integration/networking/bridge_test.go

+// Test that it's possible to set a sysctl on an interface in the container.
+// Regression test for https://github.com/moby/moby/issues/47619
+func TestSetInterfaceSysctl(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows", "no sysctl on Windows")


For a follow-up; all tests in this file (bridge_test.go) are Linux-only; if this file is specific for the bridge driver (which is not supported on Windows), perhaps we should consider renaming the file to bridge_linux_test.go, or adding a //go:build linux build-tag to it.

It's got TestBridgeICCWindows - which uses "nat" instead of "bridge", I think I put it there because it covers some of the same ground as TestBridgeICC ... but yes, we could split the file in to windows/not-windows.

thaJeztah · 2024-03-25T09:36:38Z

daemon/start_linux.go

-// prepare a freshly-created task to be started.
-func (daemon *Daemon) initializeCreatedTask(ctx context.Context, tsk types.Task, container *container.Container, spec *specs.Spec) error {
+// prepare a freshly-created container to be started.
+func (daemon *Daemon) initializeCreatedContainer(container *container.Container, spec *specs.Spec) error {


Wondering if we should keep the context.Context (assuming we may need it again at some point), although I guess we could add it back when that moment arrives.

I'm happy to put it back ... but yes, my thinking was that as it's a non-exported function it can always be added back when needed.

I'd like to keep the function name and signature unchanged as it makes for less code to review in both this PR and in the future PR which carries the next version of the network config code. (It's just noise to have to touch start_notlinux.go)

@corhere you mean keeping the signature as it was before this PR? (so adjust the patch to not change the signature?)

akerouanton · 2024-03-25T15:44:33Z

I don't think #47619 alone fully outweighs the benefits of 0046b16. We should probably find a way to filter out interface-specific sysctls from the OCI spec and apply them after all the endpoints are created / joined. But I think that'd be an important change for a patch release, so it's probably better to do that for the next major release.

corhere

Blocking until I have confirmed to my satisfaction that there is no alternative whatsoever to reverting to the use of OCI hooks.

corhere · 2024-03-25T17:04:24Z

~~Only a8f7c5e would need to be reverted. The OCI hook is a red herring; the initial network configuration still happens at the same point in the container lifecycle with or without 0046b16.~~

Nope, I'm wrong.

corhere · 2024-03-25T17:29:30Z

runC executes prestart hooks before it applies sysctls, and both are performed as part of the create command. There are two possible paths forward: revert, or roll forward with a solution that fixes the regression while also retaining support for handling containers with IPv6 disabled by --sysctl. I'm not exactly sure how we can do that (or if we need to implement some other affordance to support users specifying sysctls per iface) so reverting for now and trying again for v27 is looking to be the best solution.

I'm convinced; we have to revert to using OCI hooks

corhere · 2024-03-25T17:34:52Z

daemon/start_linux.go

-// prepare a freshly-created task to be started.
-func (daemon *Daemon) initializeCreatedTask(ctx context.Context, tsk types.Task, container *container.Container, spec *specs.Spec) error {
+// prepare a freshly-created container to be started.
+func (daemon *Daemon) initializeCreatedContainer(container *container.Container, spec *specs.Spec) error {


I'd like to keep the function name and signature unchanged as it makes for less code to review in both this PR and in the future PR which carries the next version of the network config code. (It's just noise to have to touch start_notlinux.go)

akerouanton

LGTM

robmry · 2024-03-25T19:22:54Z

I'd like to keep the function name and signature unchanged as it makes for less code to review in both this PR and in the future PR which carries the next version of the network config code. (It's just noise to have to touch start_notlinux.go)

So - add back code that isn't needed, but might be in future if a change that isn't planned ends up looking like we think it might, to make the already-reviewed code easier to review, but needing another review. Sure, why not!

Partially reverts 0046b16 "daemon: set libnetwork sandbox key w/o OCI hook" Running SetKey to store the OCI Sandbox key after task creation, rather than from the OCI prestart hook, meant it happened after sysctl settings were applied by the runtime - which was the intention, we wanted to complete Sandbox configuration after IPv6 had been disabled by a sysctl if that was going to happen. But, it meant '--sysctl' options for a specfic network interface caused container task creation to fail, because the interface is only moved into the network namespace during SetKey. This change restores the SetKey prestart hook, and regenerates config files that depend on the container's support for IPv6 after the task has been created. It also adds a regression test that makes sure it's possible to set an interface-specfic sysctl. Signed-off-by: Rob Murray <rob.murray@docker.com>

robmry force-pushed the 47619_restore_prestart_hook branch from 6fedbcf to 78b903b Compare March 23, 2024 18:35

robmry marked this pull request as ready for review March 23, 2024 19:44

robmry self-assigned this Mar 23, 2024

robmry added area/networking kind/bugfix PR's that fix bugs labels Mar 23, 2024

robmry added this to the 26.0.1 milestone Mar 23, 2024

robmry requested review from corhere and akerouanton March 23, 2024 19:46

robmry force-pushed the 47619_restore_prestart_hook branch from 78b903b to 1cd08ae Compare March 25, 2024 09:01

thaJeztah added status/2-code-review process/cherry-pick/26.0 labels Mar 25, 2024

thaJeztah modified the milestones: 26.0.1, 27.0.0 Mar 25, 2024

thaJeztah reviewed Mar 25, 2024

View reviewed changes

robmry mentioned this pull request Mar 25, 2024

Docker 26 breaks kata networking #47626

Closed

corhere previously requested changes Mar 25, 2024

View reviewed changes

corhere approved these changes Mar 25, 2024

View reviewed changes

akerouanton approved these changes Mar 25, 2024

View reviewed changes

robmry force-pushed the 47619_restore_prestart_hook branch from 1cd08ae to c51730a Compare March 25, 2024 19:34

robmry force-pushed the 47619_restore_prestart_hook branch from c51730a to fde80fe Compare March 25, 2024 19:36

akerouanton merged commit d57b899 into moby:master Mar 27, 2024
126 checks passed

This was referenced Mar 27, 2024

[failed 26.0 backport - wrong branch] Restore the SetKey prestart hook. #47635

Merged

Per-interface sysctls #47639

Open

robmry deleted the 47619_restore_prestart_hook branch March 27, 2024 16:31

vvoland mentioned this pull request Mar 28, 2024

[26.0 backport] Restore the SetKey prestart hook. #47646

Merged

akerouanton mentioned this pull request Apr 8, 2024

sysctl issues after upgrade from 25.0.4 to 26.0.0 docker/cli#4990

Closed

thaJeztah added process/cherry-picked and removed process/cherry-pick/26.0 labels Apr 8, 2024

robmry mentioned this pull request May 9, 2024

Allocate IPv6 addresses after detecting IPv6 support #47406

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore the SetKey prestart hook. #47621

Restore the SetKey prestart hook. #47621

robmry commented Mar 23, 2024 •

edited by thaJeztah

thaJeztah Mar 25, 2024

robmry Mar 25, 2024

thaJeztah Mar 25, 2024

robmry Mar 25, 2024

corhere Mar 25, 2024

thaJeztah Mar 25, 2024

akerouanton commented Mar 25, 2024

corhere left a comment

corhere commented Mar 25, 2024 •

edited

corhere commented Mar 25, 2024

corhere Mar 25, 2024

akerouanton left a comment

robmry commented Mar 25, 2024

Restore the SetKey prestart hook. #47621

Restore the SetKey prestart hook. #47621

Conversation

robmry commented Mar 23, 2024 • edited by thaJeztah

thaJeztah Mar 25, 2024

Choose a reason for hiding this comment

robmry Mar 25, 2024

Choose a reason for hiding this comment

thaJeztah Mar 25, 2024

Choose a reason for hiding this comment

robmry Mar 25, 2024

Choose a reason for hiding this comment

corhere Mar 25, 2024

Choose a reason for hiding this comment

thaJeztah Mar 25, 2024

Choose a reason for hiding this comment

akerouanton commented Mar 25, 2024

corhere left a comment

Choose a reason for hiding this comment

corhere commented Mar 25, 2024 • edited

corhere commented Mar 25, 2024

corhere Mar 25, 2024

Choose a reason for hiding this comment

akerouanton left a comment

Choose a reason for hiding this comment

robmry commented Mar 25, 2024

robmry commented Mar 23, 2024 •

edited by thaJeztah

corhere commented Mar 25, 2024 •

edited