libnetwork: fix graceful service endpoint removal

[corhere]

- What I did

• Fixed Established tcp sessions seem to hang / drop when trying to bring down container cleanly after docker service remove #51651

- How I did it

When a container is stopped, new connections to a published port will not be routed to that container but any existing connections are permitted to continue uninterrupted for the duration of the container's grace period. Unfortunately recent fixes to overlay networks introduced a regression: existing connections routed over the service mesh to containers on remote nodes are dropped immediately when the container is stopped, irrespective of the grace period.

Fix the handling of NetworkDB endpoint table events so that the endpoint is disabled in the load balancer when a service endpoint transitions to ServiceDisabled instead of deleting the endpoint and re-adding it. And fix the other bugged state transitions with the help of a unit test which exhaustively covers all permutations of endpoint event.

- How to verify it
New unit test.

- Human readable description for the release notes

- Fixed a regression where established network connections could be disrupted during a container's shutdown grace period.

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

[Copilot]

Pull request overview

This PR fixes a regression in overlay network handling where established network connections were being disrupted immediately when a container stopped, rather than being allowed to continue for the duration of the container's grace period. The fix ensures that when a service endpoint is disabled (during graceful shutdown), the load balancer backend is marked as disabled rather than being deleted and re-added, allowing existing connections to complete.

Changes:

• Refactored handleEpTableEvent from a Controller method to a standalone function with a serviceBinder interface for improved testability
• Modified EquivalentTo to ignore the ServiceDisabled field, enabling detection of state transitions where only the disabled status changes
• Updated endpoint event handling logic to call rmServiceBinding with fullRemove=false when transitioning to disabled state, preserving backends for graceful shutdown

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
daemon/libnetwork/agent.go	Core fix implementing graceful service endpoint removal by properly handling ServiceDisabled transitions and using fullRemove=false for graceful shutdown
daemon/libnetwork/agent_test.go	Comprehensive unit test coverage for all permutations of endpoint events (insert/update/delete/replace) across services and containers with all ServiceDisabled combinations

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[thaJeztah]

LGTM

@robmry @akerouanton PTAL

[thaJeztah]

No good answer to this one; just that these conditions start to become ... complex, and potentially brittle. I guess we could abstract it or have some intermediate var to describe the condition, but don't think that makes it much better 🤷

[vvoland]

I see epRec == nil || !equivalent is used in 3 different expressions.

Perhaps it could be extracted to a separate var like:

epDeletedOrReplaced := epRec == nil || !equivalent`

and then used instead?

That would be a bit more easier to parse IMO.

[corhere]

The brittleness is a concern I share, hence the exhaustive unit test coverage. And while there are a lot of clauses in the predicate, they line up beat-for-beat with a prose description of the algorithm. E.g., for the commented-on line: "if the entry was not previously disabled and has been replaced, deleted, or disabled, remove the container's endpoint from DNS."

I see epRec == nil || !equivalent is used in 3 different expressions.

For two of the three, the expressions take the form of epRec == nil || /* dereference epRec */ which I would prefer to leave open-coded to make it obvious by inspection that there is no risk of a nil-dereference. And with the necessary complexity of the predicates, I think that abstracting away some of the clauses adds to the cognitive load more than expressing the same idea as an open-coded idiom. The reader has to know that epRec == nil means deletion either way since there are bare epRec == nil conditions elsewhere in the function. An implementation with epDeletedOrReplaced would contain more concepts for the reader to keep track of than my current implementation.

[thaJeztah]

Perhaps it could be extracted to a separate var like:

That's what I considered as well for a bit, but;

...I think that abstracting away some of the clauses adds to the cognitive load...

This was my conclusion as well. Having a epDeletedOrReplaced makes the call-site look nice, but doesn't make the conditions less complex, so to understand "what does epDeletedOrReplaced .. mean?" you'd still have to navigate to its declaration.

So yes, "do I like the complex conditions?" no, but "does abstracting it make it better?" ... also no (not really).

[thaJeztah]

Nit; and no need to update (unless there's other reasons to update the PR); we mostly converged on using tc ("test case") for these, so would be good to use that as name for consistency.

Suggested change

	for _, tt := range tests {
	for _, tc := range tests {

[robmry]

nit: moby normally only has two groups of imports, should perhaps stick with that?

[thaJeztah]

Ah, yes, I spotted that as well; OTOH, I think there's a PR (somewhere) to switch to using 3 groups, as it's slightly more idiomatic (IIRC), so I thought "might as well switch this one over already", but I'm fine either way 😅