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: internal project network with duplicate check, revert for #5305 #5533

Merged

Conversation

stasadev
Copy link
Member

@stasadev stasadev commented Nov 13, 2023

The Issue

After switch to external project networks in #5305, there were issues with compatibility between different DDEV version, which made downgrade not smooth.

The last issue with downgrade from HEAD to v1.22.4:

$ ddev start
...
Building project images... 
Project images built in 3s. 
time="2023-11-07T08:29:14-07: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 "" 
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='[]', err='exit status 1', stdout='', stderr='time="2023-11-07T08:29:14-07: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 ""'

How This PR Solves The Issue

Move back to internal project networks, and create them before docker-compose up to ensure no duplicates.

I've refactored the code to make it clearer what's going on. In the previous PR #5305, it was unclear where and why the project network was created.

Manual Testing Instructions

Use DDEV as usual.

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

Copy link

@stasadev
Copy link
Member Author

The only difference between a network created with docker-compose up and from DDEV is this set of labels:

$ docker inspect ddev-your-project_default
...
        "Labels": {
            "com.docker.compose.network": "default",
            "com.docker.compose.project": "ddev-your-project",
            "com.docker.compose.version": "2.23.0"
        }
...

And when you have a network with these labels created programmatically, docker-compose up runs as usual.

I see this as a win because with this approach we can have a project network at an early stage, I guess there might be some kind of race condition that creates duplicates for the networks in docker-compose up.

Comment on lines +1031 to +1034
// "docker-compose up", which is called in this function later, may create
// duplicate project networks, we can create this network in advance
// see https://github.com/ddev/ddev/pull/5533
dockerutil.EnsureProjectNetwork()
Copy link
Member Author

Choose a reason for hiding this comment

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

This PR is easy to revert, you only need to remove this function call and the function itself.

Comment on lines +110 to +111
// the loop below may not contain such a network
var err error = &docker.NoSuchNetwork{ID: netName}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a fix for a bug that I introduced in #5508

When you run ddev stop or ddev poweroff several times in a row, Network <name> removed is shown each time, even if there is no such network.

Comment on lines +265 to +266
// Ensure we have DDEV network
dockerutil.EnsureDdevNetwork()
Copy link
Member Author

Choose a reason for hiding this comment

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

There is only refactoring.

@stasadev
Copy link
Member Author

There are some testing errors, I restarted them and they failed again.
Don't really know if these failures are related to this PR.

@stasadev stasadev marked this pull request as ready for review November 14, 2023 11:37
@stasadev stasadev requested a review from a team as a code owner November 14, 2023 11:37
@rfay
Copy link
Member

rfay commented Nov 14, 2023

The colima failure was Colima regression in v0.6.0, should be fixed in v0.6.2, abiosoft/colima#856

The wsl failure looks like just internet problems or upstream problems.

I'll restart both again.

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.

If you're good with it I'm good with it. Thanks again for pursuing this!

@stasadev
Copy link
Member Author

Yeah, let's pull it in. Looks pretty good considering we have version control over docker-compose.
So, if something changes with the docker-compose labels, we can handle it easily.

@rfay rfay merged commit e98ee07 into ddev:master Nov 14, 2023
24 of 26 checks passed
@stasadev stasadev deleted the 20231113_stasadev_internal_project_network branch November 14, 2023 17:11
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

2 participants