Fix: don't create sandbox when bridge is none to avoid docker restart hang.

[payall4u]

From #42461

- What I did
If we create container with docker daemon disable bridge, we'll create needless sandbox.
And when the container be removed, the sandbox will be stayed here. Only when dockerd be restarted, those sandbox would be deleted by sandboxCleanup(). 4000 such sandbox will cost nearly 10 minutes. It means restarting docker daemon will hang more then 10 minutes.

func (daemon *Daemon) allocateNetwork(container *container.Container) (retErr error) {
	var (
		start          = time.Now()
		controller     = daemon.netController
	)

	// container.Config.NetworkDisabled never be assigned.
	if container.Config.NetworkDisabled && container.HostConfig.NetworkMode.IsContainer() {
		return nil
	}
	// ...
        // container.Config.NetworkDisabled will be true here (in connectToNetwork).
	// when we create set NetworkDisabled, we won't delete sandbox after deleting container.
	if nConf, ok := container.NetworkSettings.Networks[defaultNetName]; ok {
		cleanOperationalData(nConf)
		if err := daemon.connectToNetwork(container, defaultNetName, nConf.EndpointSettings, updateSettings); err != nil {
			return err
		}

	}

	networks := make(map[string]*network.EndpointSettings)
	for n, epConf := range container.NetworkSettings.Networks {
		if n == defaultNetName {
			continue
		}

		networks[n] = epConf
	}
	// len of networks is 0, we'll create sandbox here
	if len(networks) == 0 {
	}
}

So I think we should assign the NetworkDisabled earlier.
- How I did it
I fix the bug by setting NetworkDisabled correctly.

- How to verify it

1. Update the docker config set "bridge": "none"
2. Use for i in $(seq 1 4000); do docker run --rm ubuntu; done to create many sandbox.
3. Restart docker and wait.(We can verify it by read network local-kv.db directly.)

- Description for the changelog

Setting NetworkDisabled correctly to avoid restart docker hang.

- A picture of a cute animal (not mandatory but encouraged)
🐧🐧🐧

[payall4u]

ping @AkihiroSuda @fuweid

[thaJeztah]

I'm a bit confused; wouldn't the container still need a sandbox?

And when the container be removed, the sandbox will be stayed here

Do you know why it's not removed? Feels like that may be the actual bug?

[payall4u]

I'm a bit confused; wouldn't the container still need a sandbox?

And when the container be removed, the sandbox will be stayed here

Do you know why it's not removed? Feels like that may be the actual bug?

Summary

When we set bridge to "none" and create a container, we'll get:

1. create container
2. allocate network <=== we'll set Container.Config.NetworkDisabled to true here
3. create sandbox however

When we delete the container, it will:

1. release network <=== if Container.Config.NetworkDisabled is true jump to step 3
2. delete sandbox
3. delete container

Detail

Create container

func (daemon *Daemon) allocateNetwork(container *container.Container) (retErr error) {

	if nConf, ok := container.NetworkSettings.Networks[defaultNetName]; ok {
		// set container.Config.NetworkDisabled here
		if err := daemon.connectToNetwork(container, defaultNetName, nConf.EndpointSettings, updateSettings); err != nil {
			return err
		}
	}

	// Create its network sandbox now if not present
	if len(networks) == 0 {
		if nil == daemon.getNetworkSandbox(container) {
			sb, err := daemon.netController.NewSandbox(container.ID, sbOptions...)
			if err != nil {
				return err
			}
			updateSandboxNetworkSettings(container, sb)
			defer func() {
				if retErr != nil {
					sb.Delete()
				}
			}()
		}
	}
}

func (daemon *Daemon) connectToNetwork(container *container.Container, idOrName string, ...) (err error) {
	// ...
	if containertypes.NetworkMode(idOrName).IsBridge() &&
		daemon.configStore.DisableBridge {
		container.Config.NetworkDisabled = true
		return nil
	}
	// ...
}

Delete container

When deleting the container, we'll check container.Config.NetworkDisabled. If the value is true, we won't delete the sandbox.

func (daemon *Daemon) releaseNetwork(container *container.Container) {
	if container.HostConfig.NetworkMode.IsContainer() || container.Config.NetworkDisabled {
		return
	}
	// ...
	sb, err := daemon.netController.SandboxByID(sid)
	// ...
	if err := sb.Delete(); err != nil {
		logrus.Errorf("Error deleting sandbox id %s for container %s: %v", sid, container.ID, err)
	}
	// ...
}

@thaJeztah Sorry for my late response.

[thaJeztah]

release network <=== if Container.Config.NetworkDisabled is true jump to step 3

Is the sandbox ID set in that case? Because if that's the case, removing the || container.Config.NetworkDisabled would fix that; looking at the code;

moby/daemon/container_operations.go

Lines 1017 to 1027 in a773178

	if container.HostConfig.NetworkMode.IsContainer() || container.Config.NetworkDisabled {
	return
	}
	sid := container.NetworkSettings.SandboxID
	settings := container.NetworkSettings.Networks
	container.NetworkSettings.Ports = nil
	if sid == "" {
	return
	}

If the sandbox-ID is not set (empty), it would already return early. and if it's set, it would clean it up. If the sandbox ID is set in this case, I think that's something that should be done (even with this fix applied)

Looking further; I wonder if there's other situations that are overlooked for which things should be skipped; thinking of host mode networking; I see there's some handling in daemon.initializeNetworking(), but it does not return before daemon.allocateNetwork() is called;

moby/daemon/container_operations.go

Lines 978 to 989 in a773178

	if container.HostConfig.NetworkMode.IsHost() {
	if container.Config.Hostname == "" {
	container.Config.Hostname, err = os.Hostname()
	if err != nil {
	return err
	}
	}
	}
	if err := daemon.allocateNetwork(container); err != nil {
	return err
	}

[thaJeztah]

I noticed that container.NetworkSettings is a pointer, so could (potentially) be nil; perhaps we need a check for that below, to prevent a panic if that ever happens (not sure if possible)

[payall4u]

I noticed that container.NetworkSettings is a pointer, so could (potentially) be nil; perhaps we need a check for that below, to prevent a panic if that ever happens (not sure if possible)

I think that container.NetworkSettings is never nil, unless someone update the container config on disk and restart docker daemon. It's always good to check.

[payall4u]

Ping @thaJeztah