Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure no duplicates for docker networks, fixes #5193 #5508

Merged
merged 4 commits into from Nov 10, 2023

Conversation

stasadev
Copy link
Member

@stasadev stasadev commented Nov 7, 2023

The Issue

Summary:

Sometimes when you start a project, multiple project networks are created (e.g. ddev-your-project_default) which prevents the project from starting.

Error response from daemon: network ddev-your-project_default is ambiguous (16 matches found on name)

The project network is internal (created from docker-compose.yaml) in v1.22.4, and we cannot manipulate its state.
But we can make it external (created programmatically from DDEV, see #5305) in v1.22.5 and control it however we want.
But we can create it programmatically from DDEV (see #5533) in v1.22.5 and control it however we want.
We can only check for network duplicates, as this is fixed in Docker v25+.

This set of PRs are intended to fix the need for user to do manually docker network rm, because the project cannot be started when there are duplicate networks.

An attempt has been made to fix this on the Docker side (but it will only be available with Docker 25):

To make things easier for DDEV users, we can manually create networks for DDEV projects and automatically remove duplicates.

Code changes for this issue are made in:

How This PR Solves The Issue

DDEV should remove Docker networks by ID, not by name. This way users won't need to worry about duplicate networks, because they will be removed automatically on ddev start and ddev poweroff - regardless of what users run.

Manual Testing Instructions

Download the binary below, this is a special broken binary that creates duplicate networks:

# use the binary from above for ddev start, the more you do it, the more duplicates will be created
ddev start
ddev start
ddev start

# then run poweroff (networks will not be removed)
ddev poweroff

# see a lot of duplicates for ddev_default and ddev-your-project_default
docker network ls

# switch the binary to the one in the second comment (from the bot) and run

ddev start

# you will see a lot of these:

Network ddev_default removed
Network ddev_default removed

You seem to have a new DDEV version. 
During an upgrade it's important to `ddev poweroff`.
May I do `ddev poweroff` before continuing?
This does no harm and loses no data. [Y/n] (yes): y

# Answer "yes", and all duplicates will be removed on ddev poweroff

Network ddev-your-project_default removed 
Network ddev-your-project_default removed 
Network ddev-your-project_default removed
Network ddev_default removed
Network ddev-your-project_default removed 
Network ddev-your-project_default removed

# Also you can repeat the same steps but this time answer "no" and then run ddev start, the duplicates will also be removed

May I do `ddev poweroff` before continuing?
This does no harm and loses no data. [Y/n] (yes): n

ddev start

Starting project... 
Network ddev-your-project_default removed 
Network ddev-your-project_default removed 
Network ddev-your-project_default removed

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

@github-actions github-actions bot added the bugfix label Nov 7, 2023
Copy link

github-actions bot commented Nov 7, 2023

@stasadev stasadev marked this pull request as ready for review November 7, 2023 21:02
@stasadev stasadev requested a review from a team as a code owner November 7, 2023 21:02
@jonaseberle
Copy link
Collaborator

jonaseberle commented Nov 8, 2023

I never had the duplicate networks on Linux (docker v24.0.5), but with your "broken" build I can reproduce.
The current master shows many warnings then:
On ddev start (v1.22.5-alpha1-3-g0225d6f48):

multiple networks with name "ddev_default" were found. Use network ID as name to avoid ambiguity
Failed to start shop.lash-lift.de: composeCmd failed to run 'COMPOSE_PROJECT_NAME=ddev-shoplash-liftde docker-compose -f /home/jonas/src/github.com/lmwa/lashlift-shopware-project/.ddev/.ddev-docker-compose-full.yaml up -d', action='[]', err='exit status 1', stdout='', stderr='multiple networks with name "ddev_default" were found. Use network ID as name to avoid ambiguity'

On ddev poweroff (v1.22.5-alpha1-3-g0225d6f48):

Unable to remove network ddev_default: API error (400): network ddev_default is ambiguous (5 matches found based on name)
Unable to remove network ddev-shoplash-liftde_default: API error (400): network ddev-shoplash-liftde_default is ambiguous (3 matches found based on name)

With this PR (v1.22.5-alpha1-4-g8ccbe6ba1) they get removed on ddev start. So that works for me.


But I am not sure if network removal should be in NetExists(). It seems a bit overloaded to me in such a seemingly simple function.
I think it would be nicer in EnsureNetwork() (to prevent duplicates being created - if we still have that problem on docker for Mac) and/or the RemoveNetwork...(name) functions (they could really remove all networks with that name to clean up from potential stray networks from older versions).

EDIT: We also have the network labels that we added in 1.22.4. I am not sure if we are using them in ddev poweroff correctly already.

@stasadev
Copy link
Member Author

stasadev commented Nov 8, 2023

@jonaseberle Thank you!

I am not sure if network removal should be in NetExists(). It seems a bit overloaded to me in such a seemingly simple function.

I agree, I thought just one "for" loop for networks would be good, but now it's not clear what's going on with the function name. I will create a new function to remove duplicates (or move it somewhere, don't know yet).

I need to think more about RemoveNetwork...(name), showing Network ddev-your-project_default removed is not helpful for duplicates, maybe I can suppress this output in this case so the user doesn't even have to worry about it.

We also have the network labels that we added in 1.22.4. I am not sure if we are using them in ddev poweroff correctly already.

We use them to remove all leftovers from Docker, so if something goes wrong it'll still be cleaned up here.

It's true that currently duplicates cannot be removed in v1.22.4 (because of the check for network name, not ID), but with my change in this PR it will finally be handled properly.

@stasadev stasadev force-pushed the 20231107_stasadev_ambiguous_docker_network branch from 8ccbe6b to c6b9e44 Compare November 9, 2023 19:46
@stasadev
Copy link
Member Author

stasadev commented Nov 9, 2023

I made a new function RemoveNetworkDuplicates, and tried to simplify the code.

The first post of this PR now has a summary where I've gathered all the information about this issue in one place.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

The code is looking great, but when I ddev restart I get errors like this:

$ ddev restart d10
Restarting project d10...
 Container ddev-d10-db  Stopped
 Container ddev-d10-web  Stopped
 Container ddev-d10-db  Stopped
 Container ddev-d10-web  Stopped
 Container ddev-d10-db  Removed
 Container ddev-d10-web  Removed
Unable to remove network ddev-d10_default: API error (403): error while removing network: network ddev-d10_default id 169ec45f14920cef6a487cc8be020647f9d81e69187a53d7d37464b61aa103fd has active endpoints

This is using orbstack, I doubt that matters.

@rfay
Copy link
Member

rfay commented Nov 10, 2023

That problem was due to an abandoned container left by a removed add-on (xhgui). I don't think it has anything to do with this.

Fix is in

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

I manually tested with the broken ddev that creates extra networks, and it works perfectly with the manual testing technique.

I looked at the code and it's elegant and nice. Thanks! 💯

@rfay rfay merged commit fe3abca into ddev:master Nov 10, 2023
20 checks passed
@stasadev stasadev deleted the 20231107_stasadev_ambiguous_docker_network branch November 10, 2023 20:37
stasadev added a commit to stasadev/ddev that referenced this pull request Nov 15, 2023
stasadev added a commit to stasadev/ddev that referenced this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants