Global Default AddressPool - Update #37736
Conversation
There was a problem hiding this comment.
Pity that we still need the conversion from uint32 to int here; I assume changing ipamutils.NetworkToSplit will be problematic?
There was a problem hiding this comment.
This old feature code spread across moby and libnetwork. I will have to go back and change everywhere.
There was a problem hiding this comment.
Can we define this type somewhere else? I see we now define it in two separate locations; once here, and once in manager/allocator/cnmallocator/networkallocator.go
There was a problem hiding this comment.
Initially Anshul suggested to keep it in manager/controlapi/network.go. I had an issue with "cyclic import" and discussed with him again to make sure I don't break if there is any existing swarmkit design rules. We decided to implement in allocator code. But again we had cnmallocator package where we got into "cyclic import" issue. Hence I decided to add one more place to avoid this issue. I will reach out to Anshul again for his comments and update accordingly. Is that ok ?
Codecov Report
@@ Coverage Diff @@
## master #37736 +/- ##
==========================================
- Coverage 36.15% 36.11% -0.04%
==========================================
Files 610 610
Lines 45054 45072 +18
==========================================
- Hits 16288 16279 -9
- Misses 26527 26554 +27
Partials 2239 2239 |
|
@thaJeztah , PTAL. swarmkit refactor code got merged. |
|
Experimental failure is not related to this PR. 21:24:16 FAIL: docker_api_swarm_test.go:296: DockerSwarmSuite.TestAPISwarmLeaderElection |
Addressing few review comments as part of code refactoring. Also moved validation logic from CLI to Moby. Signed-off-by: selansen <[email protected]>
| defaultAddrPool[i] = strings.TrimSpace(defaultAddrPool[i]) | ||
| _, b, err := net.ParseCIDR(defaultAddrPool[i]) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid base pool %s: %v", defaultAddrPool[i], err) |
There was a problem hiding this comment.
nit: this could've used errors.Wrapf(err, "invalid base pool %s", defaultAddrPool[i]) to preserve the original err (not a blocker)
| "github.com/docker/docker/daemon/cluster/executor/container" | ||
| lncluster "github.com/docker/libnetwork/cluster" | ||
| swarmapi "github.com/docker/swarmkit/api" | ||
| swarmallocator "github.com/docker/swarmkit/manager/allocator/cnmallocator" |
There was a problem hiding this comment.
nit: was the alias needed here? (did it conflict, or was this just for readability?)
There was a problem hiding this comment.
for readability I added that like
|
|
||
| # cluster | ||
| github.com/docker/swarmkit cfa742c8abe6f8e922f6e4e920153c408e7d9c3b | ||
| github.com/docker/swarmkit d7d23d763a2d47ad6e540f81ab3609f6c323e9be |
There was a problem hiding this comment.
Full diff: moby/swarmkit@cfa742c...d7d23d7
relevant changes;
- Add sysctls support swarmkit#2729 Add sysctls support (relates to Add support for sysctl options in services #37701)
- Default Address Pool Code Refactor swarmkit#2732 Default Address Pool Code Refactor (relates to this PR)
|
@vdemeester PTAL |
ctelfer
left a comment
ctelfer
left a comment
There was a problem hiding this comment.
LGTM
One comment. It looks like the swarmkit vendoring also carries with it at least one other update (in this case, sysctl settings for services). It would probably be good to mention that and any other PRs that get rolled in in the description here.
|
experimental failure is a flaky test |
6 participants
Addressing few review comments as part of code refactoring.
Also moved validation logic from CLI to Moby.
Signed-off-by: selansen [email protected]
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)