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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion pkg/ddevapp/app_compose_template.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,6 @@ networks:
external: true
default:
name: ${COMPOSE_PROJECT_NAME}_default
external: true
{{ if .IsGitpod }}{{/* see https://github.com/ddev/ddev/issues/3766 */}}
driver_opts:
com.docker.network.driver.mtu: 1440
Expand Down
9 changes: 4 additions & 5 deletions pkg/ddevapp/ddevapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -1028,6 +1028,10 @@ func (app *DdevApp) Start() error {

app.DockerEnv()
dockerutil.EnsureDdevNetwork()
// "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()
Comment on lines +1031 to +1034
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.


if err = dockerutil.CheckDockerCompose(); err != nil {
util.Failed(`Your docker-compose version does not exist or is set to an invalid version.
Expand Down Expand Up @@ -2557,11 +2561,6 @@ func (app *DdevApp) Stop(removeData bool, createSnapshot bool) error {
}
}

// Remove current project network
// Working around duplicate network creation problem
// see https://github.com/ddev/ddev/pull/5508
dockerutil.RemoveNetworkWithWarningOnError(os.Getenv("COMPOSE_PROJECT_NAME") + "_default")

return nil
}

Expand Down
5 changes: 1 addition & 4 deletions pkg/ddevapp/poweroff.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,7 @@ func PowerOff() {
// Remove global DDEV default network
dockerutil.RemoveNetworkWithWarningOnError(dockerutil.NetName)

// Remove all external networks created with DDEV
// This is needed for compatibility if we decide to undo the changes
// made for the external project networks
// see https://github.com/ddev/ddev/pull/5508
// Remove all networks created with DDEV
removals, err := dockerutil.FindNetworksWithLabel("com.ddev.platform")
if err == nil {
for _, network := range removals {
Expand Down
41 changes: 27 additions & 14 deletions pkg/dockerutil/dockerutils.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type ComposeCmdOpts struct {
}

// EnsureNetwork will ensure the Docker network for DDEV is created.
func EnsureNetwork(client *docker.Client, name string) error {
func EnsureNetwork(client *docker.Client, name string, labels map[string]string) error {
// Pre-check for network duplicates
RemoveNetworkDuplicates(client, name)

Expand All @@ -50,7 +50,7 @@ func EnsureNetwork(client *docker.Client, name string) error {
Name: name,
Driver: "bridge",
Internal: false,
Labels: map[string]string{"com.ddev.platform": "ddev"},
Labels: labels,
}
_, err := client.CreateNetwork(netOptions)
if err != nil {
Expand All @@ -61,24 +61,36 @@ func EnsureNetwork(client *docker.Client, name string) error {
return nil
}

// EnsureNetworkWithFatal creates or ensures the provided network exists or
// EnsureDdevNetwork creates or ensures the DDEV network exists or
// exits with fatal.
func EnsureNetworkWithFatal(name string) {
func EnsureDdevNetwork() {
// Ensure we have the fallback global DDEV network
client := GetDockerClient()
err := EnsureNetwork(client, name)
err := EnsureNetwork(client, NetName, map[string]string{"com.ddev.platform": "ddev"})
if err != nil {
log.Fatalf("Failed to ensure Docker network %s: %v", name, err)
log.Fatalf("Failed to ensure Docker network %s: %v", NetName, err)
}
}

// EnsureDdevNetwork creates or ensures the DDEV network exists or
// EnsureProjectNetwork creates or ensures the project network exists or
// exits with fatal.
func EnsureDdevNetwork() {
// Ensure we have the fallback global DDEV network
EnsureNetworkWithFatal(NetName)
// Ensure we have the current project network
if os.Getenv("COMPOSE_PROJECT_NAME") != "" {
EnsureNetworkWithFatal(os.Getenv("COMPOSE_PROJECT_NAME") + "_default")
func EnsureProjectNetwork() {
if os.Getenv("COMPOSE_PROJECT_NAME") == "" {
log.Fatalf("dockerutil.EnsureProjectNetwork() must be called after app.DockerEnv()")
}
networkName := os.Getenv("COMPOSE_PROJECT_NAME") + "_default"
client := GetDockerClient()
err := EnsureNetwork(client, networkName, map[string]string{
"com.ddev.platform": "ddev",
// add docker-compose labels needed for "docker compose up"
"com.docker.compose.network": "default",
"com.docker.compose.project": os.Getenv("COMPOSE_PROJECT_NAME"),
"com.docker.compose.version": func() string {
version, _ := GetDockerComposeVersion()
return strings.TrimPrefix(version, "v")
}()})
if err != nil {
log.Fatalf("Failed to ensure Docker network %s: %v", networkName, err)
}
}

Expand All @@ -95,7 +107,8 @@ func NetworkExists(netName string) bool {
func RemoveNetwork(netName string) error {
client := GetDockerClient()
networks, _ := client.ListNetworks()
var err error
// the loop below may not contain such a network
var err error = &docker.NoSuchNetwork{ID: netName}
Comment on lines +110 to +111
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.

// loop through all networks because there may be duplicates
// and delete only by ID - it's unique, but the name isn't
for _, network := range networks {
Expand Down
8 changes: 2 additions & 6 deletions pkg/testcommon/testcommon.go
Original file line number Diff line number Diff line change
Expand Up @@ -262,12 +262,8 @@ func ClearDockerEnv() {

// ContainerCheck determines if a given container name exists and matches a given state
func ContainerCheck(checkName string, checkState string) (bool, error) {
// Ensure we have Docker network
client := dockerutil.GetDockerClient()
err := dockerutil.EnsureNetwork(client, dockerutil.NetName)
if err != nil {
log.Fatal(err)
}
// Ensure we have DDEV network
dockerutil.EnsureDdevNetwork()
Comment on lines +265 to +266
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.


c, err := dockerutil.FindContainerByName(checkName)
if err != nil {
Expand Down