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: make project network external, fixes #5193 #5305

Merged

Conversation

stasadev
Copy link
Member

@stasadev stasadev commented Aug 29, 2023

Summary

The Issue

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

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

It doesn't happen all the time, but it's really annoying.

How This PR Solves The Issue

The idea is that if the project's docker network is created before doing the usual stuff, it can prevent the problem from occurring.

Let's make the project's docker network external (just like the ddev_default global network).

Manual Testing Instructions

This PR must not break existing features to be considered something valuable to merge.

There is no reliable way to test this, because the issue is related to docker-compose (not DDEV itself), and it occurs randomly.

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

@github-actions
Copy link

github-actions bot commented Aug 29, 2023

@@ -550,7 +560,7 @@ func ComposeCmd(composeFiles []string, action ...string) (string, string, error)
// Container (or Volume) ... Creating or Created or Stopping or Starting or Removing
// Container Stopped or Created
// No resource found to remove (when doing a stop and no project exists)
ignoreRegex := "(^ *(Network|Container|Volume) .* (Creat|Start|Stopp|Remov)ing$|^Container .*(Stopp|Creat)(ed|ing)$|Warning: No resource found to remove$|Pulling fs layer|Waiting|Downloading|Extracting|Verifying Checksum|Download complete|Pull complete)"
ignoreRegex := "(^ *(Network|Container|Volume) .* (Creat|Start|Stopp|Remov)ing$|^Container .*(Stopp|Creat)(ed|ing)$|Warning: No resource found to remove|Pulling fs layer|Waiting|Downloading|Extracting|Verifying Checksum|Download complete|Pull complete)"
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed $ from Warning: No resource found to remove$ because there was extra output from docker-compose with this command:

$ project=test-5305 && mkdir $project && cd $project && ddev config --auto --omit-containers=db && ddev start && ddev stop && ddev restart && ddev delete --omit-snapshot --yes && cd .. && rm -rf $project
...
Starting test-5305...
...
Project test-5305 has been stopped. 
Restarting project test-5305... 
Warning: No resource found to remove for project "ddev-test-5305".
...

@rfay rfay force-pushed the 20230829_stasadev_make_project_network_external branch from d30e558 to 48e9f9a Compare August 30, 2023 22:40
@rfay
Copy link
Member

rfay commented Aug 30, 2023

Rebased to hopefully fix colima problems.

@rfay rfay force-pushed the 20230829_stasadev_make_project_network_external branch from 48e9f9a to baeb897 Compare August 31, 2023 03:14
@rfay
Copy link
Member

rfay commented Aug 31, 2023

Rebased again hoping for colima success.

@jonaseberle
Copy link
Collaborator

It seems we are working around a docker bug here if the implicit project networks get duplicated with the same name (which should not be possible!).

I haven't found regressions so far.

But maybe help me understand: Where do we create that external project network now that we are not letting docker-compose doing that? It must be in our code but I have not found it.

@rfay
Copy link
Member

rfay commented Aug 31, 2023

I haven't looked closely at this, but another way to handle this, instead of using external network, is to just remove any dangling network before running docker-compose up. That would leave the network responsibility in the docker-compose yaml and be less intrusive.

@stasadev
Copy link
Member Author

stasadev commented Aug 31, 2023

Where do we create that external project network now that we are not letting docker-compose doing that?

I put the creation of the project's network inside EnsureDdevNetwork() - where ddev_default is created.

another way to handle this, instead of using external network, is to just remove any dangling network before running docker-compose up

The problem is that there is no dangling network, the duplicates are created only when docker-compose up is running.

A comment from the related issue:

All 16 created in the same second with different subnets.

@jonaseberle
Copy link
Collaborator

Thanks @stasadev for the explanation!

@stasadev
Copy link
Member Author

stasadev commented Sep 5, 2023

I have been using this PR for a week and have not noticed any problems.

During this time, I did not see duplicate networks.

Again, this is a workaround, but it would be nice to have it here.

@stasadev stasadev marked this pull request as ready for review September 5, 2023 07:08
@stasadev stasadev requested a review from a team as a code owner September 5, 2023 07:08
@rfay rfay force-pushed the 20230829_stasadev_make_project_network_external branch from baeb897 to 898568b Compare September 6, 2023 21:06
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 this and looked at networks before and after and it seemed right to me.

I added just a single commit to add links to the PRs and issues so this won't be incomprehensible in the future when somebody stumbles across it in the code.

@stasadev would you mind testing this version for another week, as it now has an updated docker-compose in it?

@stasadev
Copy link
Member Author

stasadev commented Sep 6, 2023

Sure, I will use it for another week.

Meanwhile, yesterday I went to the stable v1.22.1 to confirm that I have this problem with networks - yes, I still see duplicate networks.

@rfay
Copy link
Member

rfay commented Sep 6, 2023

I sure wish we understood why this can happen to you so often, but is not often-reported.

@rfay
Copy link
Member

rfay commented Sep 7, 2023

I'm sure you're already doing this, but when testing make sure to use the "native" bundled version of docker-compose.

@stasadev
Copy link
Member Author

stasadev commented Sep 7, 2023

I sure wish we understood why this can happen to you so often, but is not often-reported.

Added a new comment here, hard to tell, but the problem may come from the Docker Engine.

I'm sure you're already doing this, but when testing make sure to use the "native" bundled version of docker-compose.

Yes, I use the bundled version.


Tried to grab the latest artifacts in the second comment, but they are not updated.

Something is wrong with Add download link to PR after PR Build.

Found the latest binaries here.

@rfay
Copy link
Member

rfay commented Sep 11, 2023

We'll do a point release this week, so if you think this is going to be OK we'll pull when you're ready again. Or we can wait for the next point release.

We could also wait for moby/moby#46251 but that one won't roll out to all environments for quite some time, even though it will show up sooner on Linux and Docker Desktop.

@stasadev
Copy link
Member Author

I've been testing it for a few days and haven't noticed any issues with docker-compose v2.21.0.

But let's postpone this PR for another point release.

I see activity in moby/moby#46251 and want to check it out, hopefully it will be merged soon (I'll build Docker from master).

@rfay
Copy link
Member

rfay commented Sep 11, 2023

Thanks! I'm going to move this back to draft then, and we'll rebase as time goes on.

@rfay rfay marked this pull request as draft September 11, 2023 15:13
@rfay
Copy link
Member

rfay commented Sep 11, 2023

I did find one minor piece of fallout with this, but it has to do with going back to regular version after testing this one:

failed to start d10: composeCmd failed to run 'COMPOSE_PROJECT_NAME=ddev-d10 docker-compose -f /Users/rfay/workspace/d10/.ddev/.ddev-docker-compose-full.yaml up -d', action='[up -d]', err='exit status 1', stdout='', stderr='time="2023-09-11T11:25:04-06:00" level=warning msg="a network with name ddev-d10_default exists but was not created by compose.\nSet external: true to use an existing network" network ddev-d10_default was found but has incorrect label com.docker.compose.network set to ""'

People who understand what's going on know to delete the network, but it would be baffling for others. Not sure if or what we could do or if we should do anything.

@stasadev
Copy link
Member Author

Ah yes, that could be a problem. DDEV offers to do ddev poweroff only after the upgrade.

But I can't guarantee that project will be stopped before switching the version.

This requires adding extra code to check and remove the external network at ddev start.

But again, that won't prevent this error from occurring if someone decides to downgrade.

@rfay
Copy link
Member

rfay commented Sep 11, 2023

Yeah, let's just think about it. One thing we could do now is

  • Always tag networks like we already do on containers and should start doing on images
  • ddev poweroff can delete all tagged networks.

@stasadev
Copy link
Member Author

Yes, looks like part of a larger task with Docker resources management.
So many things to control, but it would definitely improve the user experience.

@stasadev
Copy link
Member Author

I see activity in moby/moby#46251 and want to check it out, hopefully it will be merged soon (I'll build Docker from master).

My update here:

I'm not testing anything here yet, despite it being merged into master.

The PR has not yet been merged into the https://github.com/moby/moby/commits/24.0 branch.

I tried a build from master, but ddev start is very slow for me there. I believe the Arch Linux build is not optimized for it (the master branch is something like what we'll see in Docker 25).

@stasadev
Copy link
Member Author

I was going to do the same thing with labels, and on a larger scale (with images and other docker entities).

But I haven't had time to do it yet. @jonaseberle we think alike here 😄

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.

So sad to say this after as long as you've been working on this, but it will introduce a situation where if we have any kind of problem with the release it's introduced in, we won't easily be able to tell people "just go back to previous" without quite a bit of pain.

So let's introduce a simple all-network cleanup before we introduce this, and clean up all known networks and all "ddev" tagged networks. So with that little item added in the upcoming release, we can be confident that we can offer people backward compatibility. Then we can get this into the next one.

This also gives @jonaseberle, a total expert on networks, time to use these artifacts, which is also a win.

@rfay
Copy link
Member

rfay commented Oct 14, 2023

Put this one back to draft mode again, just do I don't get confused along the way :)

@stasadev
Copy link
Member Author

I will rebase and resolve conflicts here later

rfay pushed a commit that referenced this pull request Oct 21, 2023
@stasadev stasadev force-pushed the 20230829_stasadev_make_project_network_external branch from b206979 to 32ed132 Compare October 23, 2023 12:30
@stasadev
Copy link
Member Author

Fixed conflicts and rebased.

@stasadev stasadev added this to the v1.22.5 milestone Oct 24, 2023
@stasadev stasadev force-pushed the 20230829_stasadev_make_project_network_external branch from 32ed132 to 841632e Compare October 26, 2023 16:02
@stasadev
Copy link
Member Author

stasadev commented Oct 26, 2023

Another rebase after v1.22.4 release.

I am going to check it out after tests and I believe we can finally merge it.

Thanks to @rfay for the careful planning and @jonaseberle for the testing and ideas on how to make it better.

@stasadev stasadev marked this pull request as ready for review October 26, 2023 16:06
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.

Given the amount of effort on this one and good checking I think it should be good to go now, can be pulled when you think it's good.

@stasadev stasadev merged commit 989c173 into ddev:master Oct 27, 2023
19 checks passed
@stasadev stasadev deleted the 20230829_stasadev_make_project_network_external branch October 27, 2023 15:05
@rfay
Copy link
Member

rfay commented Nov 1, 2023

I also did a prerelease that can be used for testing this, https://github.com/ddev/ddev/releases/tag/v1.22.5-alpha1

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