(Re)create endpoints in a stable order by akerouanton · Pull Request #46036 · moby/moby

akerouanton · 2023-07-20T11:37:19Z

Related to:

Fixes:

Make a container's gateway network configurable #48868

- What I did

1st commit: Add a new Priority field to EndpointSettings ;
2nd commit:
- Use this new field to make sure the calls to daemon.connectToNetwork are done in descending order ;
- Also, use it to make sure the endpoints join the sandbox in the same order (that's what guarantees interface names are stable over restarts) ;
3rd commit: On NetworkConnect, the Priority value is automatically computed based on the lowest Priority minus one ;

The choice to use descending over ascending order has been made to match what the Compose spec defines.

This change brings interesting advantages for end users:

Since the interface name/order is now stable, users can bind a specific interface without needing an external script to resolve what interface should be used ;
The DNS resolver will always iterate on a container's networks in the exact same order, hence DNS resolution for a given name present on multiple networks a container is connected to will always resolve to the same thing ;

- How to verify it

CI should be green.

- Description for the changelog

Interfaces are always created in the same order and their name is stable over restarts.

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Albin Kerouanton <[email protected]>

akerouanton · 2023-07-20T11:54:21Z

api/server/router/network/network_routes.go

 	// may not be available at the time. At the same time, inside daemon `ConnectContainerToNetwork`
 	// does the ambiguity check anyway. Therefore, passing the name to daemon would be enough.
-	return n.backend.ConnectContainerToNetwork(connect.Container, vars["id"], connect.EndpointConfig)
+	return n.backend.ConnectContainerToNetwork(connect.Container, vars["id"], connect.EndpointConfig, true)


I'm not sure whether this should be set to false for API < 1.44 🤔

If the current order is randomised by go (thus "undefined"), then applying things in the correct order would make it equally "undefined" (just 100% more predictable), so I think it's fine to make it independent of the API version.

The Priority FIELD should not be looked at for older API versions (when creating).

The Priority FIELD should not be looked at for older API versions (when creating).

Mh, I'm assuming Priority is 0 for older API versions but that might be only for our Go client 🤔 Maybe I should force it to 0 for older versions.

thaJeztah · 2023-07-20T12:54:28Z

daemon/container_operations.go


 // ConnectToNetwork connects a container to a network
-func (daemon *Daemon) ConnectToNetwork(container *container.Container, idOrName string, endpointConfig *networktypes.EndpointSettings) error {
+func (daemon *Daemon) ConnectToNetwork(container *container.Container, idOrName string, endpointConfig *networktypes.EndpointSettings, autoPriority bool) error {


Looks like autoPriority is unused?

Oh, good catch!

thaJeztah · 2023-07-20T12:58:47Z

daemon/network.go

 		}
 	}

+	joinOptions = append(joinOptions, libnetwork.JoinOptionPriority(priority))


We're passing priority here, but wouldn't networkSettings.Networks[n.Name()] (which is an EndpointSettings) already have a Priority field?

(haven't checked how/where that's set though; so many levels of indirection 😂)

Nope, the networkSettings here is github.com/docker/docker/daemon/network.Settings, whereas I added Priority to github.com/docker/docker/api/types/network.EndpointSettings.

It's a bit confusing in my opinion. I have some comments laying somewhere in a stash, I need to open up a PR to add them, at least it'd help to easily know who's who.

The newly introduced Priority API field is now used to create the container endpoints in descending Priority order. Also the `JoinOptionPriority()` is now used to make sure the interface names are stable over restarts. Signed-off-by: Albin Kerouanton <[email protected]>

Without this change, clients would have to call ContainerInspect before NetworkConnect to set the right Priority on the endpoint. And this wouldn't be without potential timing issues. As such, this change totally ignores the client-provided Priority value and erase it with a computed value. Signed-off-by: Albin Kerouanton <[email protected]>

akerouanton · 2023-07-20T18:12:47Z

So, one of the failing test is checking if the default gateway and "anonymous" port mappings change whenever the container connects to a network which comes before other networks the container is connected to in the lexicographic order:

$ docker network create testnet1
$ docker network create aaaa
$ docker run -d --name test -p 9000:90 -p 70 busybox top

$ docker port test
70/tcp -> 0.0.0.0:32774
70/tcp -> [::]:32774
90/tcp -> 0.0.0.0:9000
90/tcp -> [::]:9000
$ docker exec test ip -o route show      
default via 172.18.0.1 dev eth0 
172.18.0.0/16 dev eth0 scope link  src 172.18.0.2 

$ docker network connect testnet1 test
$ docker port test                       
70/tcp -> 0.0.0.0:32774
70/tcp -> [::]:32774
90/tcp -> 0.0.0.0:9000
90/tcp -> [::]:9000
$ docker exec test ip -o route show      
default via 172.18.0.1 dev eth0 
172.18.0.0/16 dev eth0 scope link  src 172.18.0.2 
172.19.0.0/16 dev eth3 scope link  src 172.19.0.2 

# Here we go:
$ docker network connect aaaa test 
$ docker port test                 
70/tcp -> 0.0.0.0:32775
70/tcp -> [::]:32775
90/tcp -> 0.0.0.0:9000
90/tcp -> [::]:9000
$ docker exec test ip -o route show
default via 172.21.0.1 dev eth4 
172.18.0.0/16 dev eth0 scope link  src 172.18.0.2 
172.19.0.0/16 dev eth3 scope link  src 172.19.0.2 
172.21.0.0/16 dev eth4 scope link  src 172.21.0.2

And I'm pretty sure some people rely on this to set their default gateway. I need to check if that's still documented (it was at some point) and what to do next.

mbrodala · 2024-03-04T14:44:23Z

Is there still work in progress here? Or something missing?

thaJeztah · 2024-11-29T00:47:06Z

superseded by api: add GwPriority field to EndpointSettings #48936

api: Add Priority field to EndpointSettings

6e6334f

Signed-off-by: Albin Kerouanton <[email protected]>

akerouanton added area/api API status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/networking Networking impact/api impact/changelog area/daemon Core Engine labels Jul 20, 2023

akerouanton requested review from corhere, neersighted and thaJeztah July 20, 2023 11:37

akerouanton force-pushed the endpoint-settings-priority branch from 55da585 to b10607d Compare July 20, 2023 11:51

akerouanton commented Jul 20, 2023

View reviewed changes

akerouanton mentioned this pull request Jul 20, 2023

[do not merge] Set endpoint Priority docker/cli#4454

Closed

thaJeztah reviewed Jul 20, 2023

View reviewed changes

akerouanton force-pushed the endpoint-settings-priority branch from b10607d to 2d3308e Compare July 20, 2023 14:24

akerouanton added 2 commits July 20, 2023 17:08

akerouanton force-pushed the endpoint-settings-priority branch from 2d3308e to 72789d7 Compare July 20, 2023 15:08

akerouanton mentioned this pull request Jul 27, 2023

25.0 Release Tracking #45926

Closed

26 tasks

akerouanton mentioned this pull request Nov 23, 2024

api: add GwPriority field to EndpointSettings #48936

Merged

thaJeztah closed this Nov 29, 2024

akerouanton deleted the endpoint-settings-priority branch November 29, 2024 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

(Re)create endpoints in a stable order#46036

(Re)create endpoints in a stable order#46036
akerouanton wants to merge 3 commits intomoby:masterfrom
akerouanton:endpoint-settings-priority

akerouanton commented Jul 20, 2023 •

edited

Loading

Uh oh!

akerouanton Jul 20, 2023

Uh oh!

thaJeztah Jul 20, 2023

Uh oh!

akerouanton Jul 20, 2023 •

edited

Loading

Uh oh!

thaJeztah Jul 20, 2023

Uh oh!

akerouanton Jul 20, 2023

Uh oh!

thaJeztah Jul 20, 2023

Uh oh!

akerouanton Jul 20, 2023

Uh oh!

akerouanton commented Jul 20, 2023

Uh oh!

mbrodala commented Mar 4, 2024

Uh oh!

thaJeztah commented Nov 29, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

akerouanton commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akerouanton Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

akerouanton Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

akerouanton Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

akerouanton Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

akerouanton commented Jul 20, 2023

Uh oh!

mbrodala commented Mar 4, 2024

Uh oh!

thaJeztah commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

akerouanton commented Jul 20, 2023 •

edited

Loading

akerouanton Jul 20, 2023 •

edited

Loading

thaJeztah commented Nov 29, 2024 •

edited

Loading