libnet: Controller: cache networks & endpoints in-memory#49736

akerouanton · 2025-04-03T09:38:58Z

Related to Unable to remove network "has active endpoints" #17217
Related to docker network rm networkname has error has active endpoints #42119

- What I did

Maintain a cache of Endpoint and Network in libnet Controller.
Use that cache to determine if any --config-from network, or endpoint is referencing a network before deleting it.
Remove the endpointCnt struct.

Libnetwork is afflicted by long-standing data consistency issues caused by its persistence layer creating new instances of Network, Endpoint and Sandbox every time an object is loaded from the datastore or its in-memory cache. See for instance the following code that ends with a CopyTo:

moby/libnetwork/datastore/cache.go

Lines 134 to 149 in 2e92272

    
           func (c *cache) get(kvObject KVObject) error { 
        
           	kmap, err := c.kmap(kvObject) 
        
           	if err != nil { 
        
           		return err 
        
           	} 
        
           	c.mu.Lock() 
        
           	defer c.mu.Unlock() 
        
           	o, ok := kmap[Key(kvObject.Key()...)] 
        
           	if !ok { 
        
           		return ErrKeyNotFound 
        
           	} 
        
           	return o.CopyTo(kvObject) 
        
           }

For instance, this led to #47186.

Fixing the data persistence layer itself is particularly hard as it touches virtually all code paths in libnetwork. We need to be especially cautious with mutex locking -- libnetwork makes heavy use of mutexes, but since two concurrent API calls referencing the same object result in to two different instances of that object living in memory, the concurrency model might be broken (unknowingly).

By maintaining a separate cache of all objects in libnet's Controller, we'll be able to slowly iterate on all code paths, and verify each one through static analysis, without touching the data persistence layer.

This PR specifically adds an Endpoint and a Network cache to the Controller. This allows us to delete the struct endpointCnt which was used to ref count networks (either when they were referenced by config-from networks, or by endpoints) to prevent deleting networks in use.

Instead, (*Network).delete() looks for endpoints / config-from networks that reference the network being deleted from the cache maintained in the Controller.

- How I did it

See individual commits.

- How to verify it

Through unit and integration tests.

- Human readable description for the release notes

- Improve how network-endpoint relationships are tracked to reduce the chance of the "has active endpoints" error to be wrongfully returned.

akerouanton · 2025-04-03T09:53:18Z

internal/maputil/maputil.go

@@ -0,0 +1,11 @@
+package maputil


CI is failing because of that:

Suggested change

package maputil

//go:build go1.22

package maputil

robmry

LGTM!

Just some suggestions/nits ... and should it have a changelog comment, saying this might solve a long-standing issue with a "has active endpoints" on network deletion?

robmry · 2025-04-03T16:26:48Z

libnetwork/sandbox_store.go

 					}
 				}
 			}
+			c.cacheEndpoint(ep)


Looks like getEndpointFromStore will already have cached it, so this should be moved to the if err != nil block above?

Yikes! That's where it was originally 🙈

robmry · 2025-04-03T16:33:22Z

libnetwork/endpoint_store.go

+// maintained by the Controller.
+//
+// This method is thread-safe.
+func (c *Controller) deleteEndpoint(ep *Endpoint) error {


The name might be a bit misleading - deleteEndpoint doesn't actually delete the endpoint, it forgets it ... the caller should already have deleted it from the Sandbox etc? Perhaps deleteStoredEndpoint or something like that?

At some point, it might be good to put the cache in a separate package to protect its map/mutex, then the call would be something like c.epStore.Delete(ep) would also work. But, as you noted offline, that'd be a bigger job because Endpoint/Network would create a circular dependency, or the cache would need to be implemented with a generic.

(upsertEndpoint is clearer, but ditto for deleteNetwork.)

Renamed upsertEndpoint into storeEndpoint, and deleteEndpoint into deleteStoredEndpoint (and followed the same pattern for networks).

robmry · 2025-04-03T16:47:31Z

internal/maputil/maputil.go

+
+package maputil
+
+func FilterValues[K comparable, V any](in map[K]V, fn func(V) bool) []V {


The callers only want a count, the slice always gets discarded after a len() ... so maybe CountValues, and countEndpoints/countNetworks - and we can add findXYZ funcs if it turns out they're needed?

Then you could get rid of these warnings for findEndpoints/findNetworks, because there'd be no danger ...

// This method is thread-safe, but do not use it unless you're sure your code // uses the returned endpoints in thread-safe way (see the comment on // Controller.endpoints).

and we can add findXYZ funcs if it turns out they're needed?

I didn't want to change ActiveEndpointsError in this PR to make clear it's focused on data persistence.

But once this one is merged, I'll open another PR to change it, and findNetworks / findEndpoints will be needed. So, to avoid needless code churn, I think it's fine to add these here.

The `Controller`'s store is used by: - `deleteFromStore` - `getEndpointFromStore` - `getEndpointsFromStore` - `updateToStore` - … and other methods that can't store / delete / retrieve an Endpoint Calls to `updateToStore` and `deleteFromStore` have been replaced with `upsertEndpoint` and `deleteEndpoint`. Both `getEndpointFromStore` and `getEndpointsFromStore` call `cacheEndpoint` to ensure endpoints loaded from the datastore are kept in-memory. Finally, `sandboxRestore` was instantiating `Endpoint` itself. These are cached too. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

The `Controller`'s store is used by: - `deleteFromStore` - `getNetworks` - `getNetworksFromStore` - `updateToStore` - … and other methods that can't store / delete / retrieve a Network Calls to `updateToStore` and `deleteFromStore` have been replaced with `upsertNetwork` and `deleteNetwork`. Both `getNetworks` and `getNetworksFromStore` call `cacheNetwork` to ensure networks loaded from the datastore are kept in-memory. Finally, `sandboxRestore` was instantiating `Network` itself. These are cached too. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

endpointCnt is a refcounter used to track how many endpoints use a network, and how many networks references a config-only network. It's stored separately from the network. This is only used to determine if a network can be removed. This commit removes the `endpointCnt` struct and all its references. The refcounter is replaced by two lookups in the newly introduced `networks` and `endpoints` caches added to the `Controller`. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

vvoland · 2025-04-07T09:59:31Z

libnetwork/endpoint_store_test.go

+	assert.Equal(t, len(found), 2)
+	assert.Equal(t, found[0], ep1)
+	assert.Equal(t, found[1], ep2)


nit: could use assert.Check

vvoland · 2025-04-07T09:59:48Z

libnetwork/endpoint_store_test.go

+	assert.Equal(t, len(found), 1)
+	assert.Equal(t, found[0], ep2)


vvoland · 2025-04-07T11:46:29Z

libnetwork/endpoint_store_test.go

+	"gotest.tools/v3/assert"
+)
+
+func TestEndpointStore(t *testing.T) {


I just hit some flakiness:
https://github.com/moby/moby/actions/runs/14308075196/job/40096446556?pr=49746#step:7:2040

cc @akerouanton

Yup, hit some flakiness in #49762 (comment) as well

akerouanton added area/networking Networking kind/refactor PR's that refactor, or clean-up code labels Apr 3, 2025

akerouanton added this to the 28.1.0 milestone Apr 3, 2025

akerouanton requested review from robmry and thaJeztah April 3, 2025 09:38

akerouanton self-assigned this Apr 3, 2025

akerouanton commented Apr 3, 2025

View reviewed changes

akerouanton force-pushed the cache-endpoint-in-memory branch 4 times, most recently from 99867d9 to 40f7c61 Compare April 3, 2025 15:14

robmry approved these changes Apr 3, 2025

View reviewed changes

akerouanton added the impact/changelog label Apr 4, 2025

akerouanton force-pushed the cache-endpoint-in-memory branch 3 times, most recently from e49ce34 to baf40a2 Compare April 4, 2025 07:27

akerouanton added 2 commits April 4, 2025 10:03

akerouanton force-pushed the cache-endpoint-in-memory branch from baf40a2 to 1169763 Compare April 4, 2025 08:03

akerouanton force-pushed the cache-endpoint-in-memory branch from 1169763 to 51d7f95 Compare April 4, 2025 09:21

akerouanton requested a review from vvoland April 4, 2025 09:24

vvoland approved these changes Apr 7, 2025

View reviewed changes

vvoland merged commit da3b31f into moby:master Apr 7, 2025
148 checks passed

akerouanton deleted the cache-endpoint-in-memory branch April 7, 2025 10:37

vvoland reviewed Apr 7, 2025

View reviewed changes

This was referenced Apr 7, 2025

libnet: de-flake TestEndpointStore and TestNetworkStore #49764

Merged

libnet: add ep name in 'has active endpoints' error #49773

Merged

docker network rm networkname has error has active endpoints #42119

Open

akerouanton mentioned this pull request Apr 15, 2025

Partially revert "libnet: remove struct endpointCnt" #49808

Closed

robmry mentioned this pull request Apr 15, 2025

Store an endpoint count for networks, for downgrade #49812

Merged

akerouanton mentioned this pull request Apr 25, 2025

[BUG] Network removal fails with "network has active endpoints" error during docker-compose down docker/compose#12770

Closed

robmry mentioned this pull request Apr 30, 2025

Active endpoints error error #49901

Merged

thaJeztah mentioned this pull request May 7, 2025

"endpoint with name XX already exists in network" can't disconnect container from bridge docker/cli#1891

Closed

robmry mentioned this pull request Sep 27, 2025

Error (Unable to complete atomic operation, key modified) deleting object [endpoint_count ], retrying #51054

Closed

	func (c *cache) get(kvObject KVObject) error {
	kmap, err := c.kmap(kvObject)
	if err != nil {
	return err
	}

	c.mu.Lock()
	defer c.mu.Unlock()

	o, ok := kmap[Key(kvObject.Key()...)]
	if !ok {
	return ErrKeyNotFound
	}

	return o.CopyTo(kvObject)
	}


		package maputil

		func FilterValues[K comparable, V any](in map[K]V, fn func(V) bool) []V {

		assert.Equal(t, len(found), 1)
		assert.Equal(t, found[0], ep2)

Conversation

akerouanton commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robmry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

akerouanton commented Apr 3, 2025 •

edited

Loading