run, create, connect: add support for gw-priority

[akerouanton]

Related to:

• api: add GwPriority field to EndpointSettings moby/moby#48936

- What I did

This adds support for the newly introduced GwPriority API field. It can be set on both docker container run and docker network connect.

For the run command, it's made available only through the extended syntax.

- How I did it

- How to verify it

CI is green.

- Description for the changelog

- Added a new `gw-priority` option to `docker run`, `docker container create`, and `docker network connect`. This option will be used by the Engine to determine which network provides the default gateway for a container. On `docker run`, this option is only available through the extended `--network` syntax.

[thaJeztah]

I'm probably a bit late realising, but should this have been a uint, or do we expect negative values to be ok? 🙈

[akerouanton]

It's an int on purpose as it allows to set the priority to a negative value when some existing endpoints have the default priority 0 set. @robmry wrote about that here: moby/moby#48936 (comment)

[thaJeztah]

But wasn't higher higher priority, so if I want a new network to become the default, I need to give it a higher priority?

[akerouanton]

Absolutely. But if one of the current network give you a default gateway, and you want to stick with it, you can connect new networks with a negative priority.

[thaJeztah]

(mostly considering if it's zero, then I did not consider it a priority (and alphabetic order is used), and only if I would explicitly want a network to take priority, I'd probably set a non-zero value; any value lower (including zero) would not take priority in that case)

[akerouanton]

You might consider priority of existing endpoints only when you connect a new network. In that case, having the ability to set a negative priority gives you a way to not touch the existing endpoint(s), but still get the behavior you want.

[thaJeztah]

Yeah, I'm good with this for now; perhaps we should brainstorm next week to see if there's still things to be done here.

• default network (say ddd) without gw_priority; this becomes the default gateway
• connect to other network (say ccc) without gw_priority; this sorts before ddd, so now takes over default gateway
• ☝️ this is current situation, and potentially problematic, as it's implicit / unexpected
• 👉 so, say, I don't want that to happen, I now set gw_priority=-999 to make sure it doesn't capture the gateway
• But now I connect to another network, aaa. I don't want that to take over the default gateway, so I set gw_priority=-999.
• But -999 was already set for ccc, so those are now "same priority", except for name.
• ⚠️ if I would to connect another network (say bbb), but do NOT set a gw_priority, it would takeover the default gateway from ddd (as those both have 0 priority)

So, yes, there's some more controls here, but it's also shifting the goal-posts, and we may now end up with something similar to z-index or oomscore-adjust; "let's pick a high enough value" or "low enough value" that probably no other network has picked. Or at least, these numbers will become rather complicated to use, without either

• "pick a low or high enough value" hoping no other network was given that value
• or knowing what values were given to other networks and pick that value "+1" or "-1"
• or keep track of values used, and continue incrementing or decrementing

I think at least we should consider setting a limit (which could be same as oom-score-adjust, e.g. -999 - 999); not sure if we need the whole range of +/- MAXINT32.

But perhaps we need to look at some other logic as well; I think in most cases, I'd expect the gateway to not change unless I want it to (i.e., once it's set, connecting another network should not change it, unless I want that new one to take over); almost considering if some of the enumerating could be handled daemon-side ("put me first, shift others one position down"), but that would mostly be for connecting to an existing container (for "create container with multiple networks", I could see a more declarative aproach being good).

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 59.53%. Comparing base (5afa739) to head (d4db289).
Report is 11 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5664   +/-   ##
=======================================
  Coverage   59.53%   59.53%           
=======================================
  Files         346      346           
  Lines       29356    29365    +9     
=======================================
+ Hits        17477    17483    +6     
- Misses      10909    10911    +2     
- Partials      970      971    +1     

[robmry]

It'd be nice to say it's the highest priority number that wins (it's not obvious, "priority 1" sounds important!). But, it's tricky without writing an essay. How about ...

Suggested change

	flags.IntVar(&options.gwPriority, "gw-priority", 0, "Used to determine which network provides the default gateway")
	flags.IntVar(&options.gwPriority, "gw-priority", 0, "Highest gw-priority provides the default gateway")

[robmry]

It'd be good to say it's the highest priority number that gets the default gw here too (and in the network_connect.md) ... but, with a bit more space to play with, these could also say the priority can be negative?

[robmry]

Suggested change

	return fmt.Errorf("invalid priority: %w", err)
	return fmt.Errorf("invalid gw-priority: %w", err)

[thaJeztah]

LGTM