Fix: Duplicate event on network disconnect

[AmirBuddy]

• This fixes Duplicate Events for Network Disconnect in Docker Events Command #48797

- What I did
I modified the DisconnectFromNetwork function in daemon/container_operations.go to remove redundant code that was causing duplicate network disconnect events.

- How I did it
By identifying and removing the part of the code that was generating an additional event when a network disconnection occurred.

- How to verify it

1. Build the modified Docker from my branch.
2. Run a container and disconnect it from a network.
3. Use the docker events command to check that only one network disconnect event is generated for each disconnection.

- Description for the changelog

Fix duplicate network disconnect events

[thaJeztah]

Thanks!

Had a quick peek, and look like the other event may be coming from here;

moby/daemon/container_operations.go

Lines 916 to 928 in dc22579

	func (daemon *Daemon) tryDetachContainerFromClusterNetwork(network *libnetwork.Network, ctr *container.Container) {
	if !ctr.Managed && daemon.clusterProvider != nil && network.Dynamic() {
	if err := daemon.clusterProvider.DetachNetwork(network.Name(), ctr.ID); err != nil {
	log.G(context.TODO()).WithError(err).Warn("error detaching from network")
	if err := daemon.clusterProvider.DetachNetwork(network.ID(), ctr.ID); err != nil {
	log.G(context.TODO()).WithError(err).Warn("error detaching from network")
	}
	}
	}
	daemon.LogNetworkEventWithAttributes(network, events.ActionDisconnect, map[string]string{
	"container": ctr.ID,
	})
	}

As the other one is related to cluster (swarm) networks, we may have to double check if there's a codepath where we would now miss the event (to be sure).

[AmirBuddy]

Thank you for the insight!

I reviewed the tryDetachContainerFromClusterNetwork function as suggested and tested both non-Swarm and Swarm network disconnections:

1. Non-Swarm Mode: The modified version resolves the issue of duplicate network disconnect events. Only one event is now generated, as expected.

2. Swarm Mode: I also verified that both the original and modified versions work as intended, with no redundant network disconnect events triggered in Swarm mode. I think it’s safe to say Swarm mode is unchanged, which is what we wanted.

From these tests, it looks like the changes effectively remove the duplicate event issue in non-Swarm networks without impacting Swarm functionality.

Please let me know if there’s anything else I may have missed that I should check.

[akerouanton]

I think we should move the event sent by tryDetachContainerFromClusterNetwork up by one level (ie. into disconnectFromNetwork and releaseNetwork). But let's keep this for a follow-up pr. Thanks!

[thaJeztah]

Don't merge yet; still want to have a peek as well; at least, want to look at the tryDetachContainerFromClusterNetwork again, as I notice it's firing an event regardless if there actually was a network attachment (if not, it logs an error, and falls through). In deed wondering if a different spot for the event would work.

[AmirBuddy]

Thanks, @thaJeztah! I see the concern with tryDetachContainerFromClusterNetwork firing events unnecessarily. Moving the event trigger up a level, as @akerouanton suggested, would better separate detachment and logging, similar to how connectToNetwork handles it. This can be addressed in a follow-up PR.

[robmry]

If DisconnectFromNetwork fails to find the network and force, or the container's not running ...

moby/daemon/container_operations.go

Lines 1020 to 1032 in 64ca1c0

	if !ctr.Running || (err != nil && force) {
	if ctr.RemovalInProgress || ctr.Dead {
	return errRemovalContainer(ctr.ID)
	}
	// In case networkName is resolved we will use n.Name()
	// this will cover the case where network id is passed.
	if n != nil {
	networkName = n.Name()
	}
	if _, ok := ctr.NetworkSettings.Networks[networkName]; !ok {
	return fmt.Errorf("container %s is not connected to the network %s", ctr.ID, networkName)
	}
	delete(ctr.NetworkSettings.Networks, networkName)

disconnectFromNetwork -> tryDetachContainerFromClusterNetwork isn't called, so it won't send an event.

But, at the moment, the event will still be sent by DisconnectFromNetwork.

Sending an event for removal of a stopped container, or from a non-existent network, doesn't sound right. So, I think the change is good ... but, it is an extra change.

[robmry]

If DisconnectFromNetwork fails to find the network and force, or the container's not running ...

Sorry, that wasn't quite right - the event will only be sent for disconnect from a network that does exist (for a stopped container).

[thompson-shaun]

@thaJeztah - final review :D

[thaJeztah]

LGTM

sorry, this one dropped of my radar, but looks like there's been enough eyes on this to make sure it's correct.

Thank you so much for contributing!