Skip to content

Include all networks in ContainerCreate call if API >= 1.44#11429

Merged
ndeloof merged 1 commit intodocker:mainfrom
laurazard:multiple-networks-api
Feb 7, 2024
Merged

Include all networks in ContainerCreate call if API >= 1.44#11429
ndeloof merged 1 commit intodocker:mainfrom
laurazard:multiple-networks-api

Conversation

@laurazard
Copy link
Copy Markdown
Member

@laurazard laurazard commented Jan 30, 2024

Previously, we included the container's primary network in the ContainerCreate API call, and then connected the created container to each extra network one-by-one, by calling NetworkConnect.

However, starting API version 1.44 (moby/moby#45906), the ContainerCreate endpoint now takes multiple networks, allowing us to just include all the network configurations there and skip connecting the container to each extra network separately.

What I did

Change composeService.createMobyContainer to include all the network configurations for the container in the ContainerCreate call and skip connecting the container to networks individually, when API version >= 1.44.

Also added some tests to make sure we don't break this for <1.44.

(a good callout here is that currently, afaik, Compose isn't being tested against different Engine versions in CI, so it may be worth it to do that soon/wait on that before merging this PR)

(not mandatory) A picture of a cute animal, if possible in relation to what you did

Screenshot 2024-01-30 at 17 54 10

@laurazard laurazard self-assigned this Jan 30, 2024
@laurazard laurazard force-pushed the multiple-networks-api branch 2 times, most recently from 7d4030d to 39a5531 Compare January 30, 2024 17:59
laurazard
laurazard commented Jan 30, 2024
View reviewed changes
Comment thread pkg/compose/convergence.go Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 30, 2024 •
edited
Loading

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (6ef55a5) 58.09% compared to head (0fbdb93) 58.08%.

Files Patch % Lines
pkg/compose/convergence.go 66.66% 4 Missing and 2 partials ⚠️
pkg/compose/create.go 89.28% 2 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #11429      +/-   ##
==========================================
- Coverage   58.09%   58.08%   -0.02%     
==========================================
  Files         136      136              
  Lines       11543    11565      +22     
==========================================
+ Hits         6706     6717      +11     
- Misses       4177     4185       +8     
- Partials      660      663       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@laurazard laurazard force-pushed the multiple-networks-api branch from 39a5531 to 02cd50c Compare January 30, 2024 18:49
@laurazard laurazard requested review from glours, milas and ndeloof February 5, 2024 02:25
ndeloof
ndeloof reviewed Feb 5, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as a general feature but I'm not a big fan we cover such an improvement with unit tests relying on API mocks. As we start relying on 1.25 features we need to run real tests on multiple engine versions

Comment thread pkg/compose/convergence.go Outdated
@ndeloof ndeloof mentioned this pull request Feb 5, 2024
Merged
@laurazard
Copy link
Copy Markdown
Member Author

laurazard commented Feb 5, 2024

Yup! I wrote in the PR test:

(a good callout here is that currently, afaik, Compose isn't being tested against different Engine versions in CI, so it may be worth it to do that soon/wait on that before merging this PR)

Probably a good change regardless :')

milas
milas reviewed Feb 6, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Want to rebase to pick up #11459? and then we can mergeeee

@laurazard laurazard force-pushed the multiple-networks-api branch from 02cd50c to ee8d82f Compare February 7, 2024 11:32
@laurazard
Copy link
Copy Markdown
Member Author

laurazard commented Feb 7, 2024

Sorry, just saw this @milas! Rebased :)

@laurazard laurazard requested review from milas and ndeloof February 7, 2024 11:33
@laurazard
Copy link
Copy Markdown
Member Author

laurazard commented Feb 7, 2024

cc @glours if you want to TAL

@laurazard laurazard force-pushed the multiple-networks-api branch from ee8d82f to b834cc3 Compare February 7, 2024 11:37
glours
glours approved these changes Feb 7, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me 👍

@laurazard
Include all networks in ContainerCreate call if API >= 1.44
0fbdb93 
Previously, we included the container's primary network in the
ContainerCreate API call, and then connected the created container
to each extra network one-by-one, by calling NetworkConnect.

However, starting API version 1.44, the ContainerCreate endpoint now
takes multiple EndpointsConfigs, allowing us to just include all the
network configurations there and skip connecting the container to each
extra network separately.

Signed-off-by: Laura Brehm <[email protected]>
@laurazard laurazard force-pushed the multiple-networks-api branch from b834cc3 to 0fbdb93 Compare February 7, 2024 12:46
@ndeloof ndeloof enabled auto-merge (rebase) February 7, 2024 13:25
@laurazard
Copy link
Copy Markdown
Member Author

laurazard commented Feb 7, 2024

It's failing on a ddev test:

Failed to start 004: ddev-router failed to become ready; debug with 'docker logs ddev-router'; logOutput=, err=health check timed out: labels map[com.docker.compose.service:ddev-router] timed out without becoming healthy, status=

which I think might be flaky, but I don't have permissions to restart the test to check.
@ndeloof @glours can you TAL?

@glours
Copy link
Copy Markdown
Contributor

glours commented Feb 7, 2024

CI was stuck, I restarted the whole workflow

@ndeloof ndeloof merged commit aaa7ef6 into docker:main Feb 7, 2024
@glours
Copy link
Copy Markdown
Contributor

glours commented Feb 7, 2024

Ok we forgot to change the checks since we introduced the tests against 2 Docker engine versions @ndeloof 😅

@dnhn dnhn mentioned this pull request Feb 15, 2024
Merged
@xtaixe xtaixe mentioned this pull request Feb 16, 2024
Closed
@akerouanton akerouanton mentioned this pull request Feb 16, 2024
Merged
@robmry robmry mentioned this pull request Dec 17, 2024
Merged
@robmry robmry mentioned this pull request May 1, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ndeloof ndeloof ndeloof approved these changes

@glours glours glours approved these changes

@milas milas Awaiting requested review from milas

Assignees

@laurazard laurazard

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@laurazard @glours @ndeloof @milas