-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
[25.0] daemon: rename: don't reload endpoint from datastore #47196
[25.0] daemon: rename: don't reload endpoint from datastore #47196
Conversation
@@ -1276,8 +1276,8 @@ func (n *Network) EndpointByName(name string) (*Endpoint, error) { | |||
return e, nil | |||
} | |||
|
|||
// EndpointByID returns the Endpoint which has the passed id. If not found, | |||
// the error ErrNoSuchEndpoint is returned. | |||
// EndpointByID should *never* be called as it's going to create a 2nd instance of an Endpoint. The first one lives in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🫠🫠🫠 So I guess we must delete this function in a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have questions. Blocking over-eager merging for the moment
yeah, let's move to draft for now |
dd873e1
to
eab40d1
Compare
Commit 8b7af1d added some code to update the DNSNames of all endpoints attached to a sandbox by loading a new instance of each affected endpoints from the datastore through a call to `Network.EndpointByID()`. This method then calls `Network.getEndpointFromStore()`, that in turn calls `store.GetObject()`, which then calls `cache.get()`, which calls `o.CopyTo(kvObject)`. This effectively creates a fresh new instance of an Endpoint. However, endpoints are already kept in memory by Sandbox, meaning we now have two in-memory instances of the same Endpoint. As it turns out, libnetwork is built around the idea that no two objects representing the same thing should leave in-memory, otherwise breaking mutex locking and optimistic locking (as both instances will have a drifting version tracking ID -- dbIndex in libnetwork parliance). In this specific case, this bug materializes by container rename failing when applied a second time for a given container. An integration test is added to make sure this won't happen again. Signed-off-by: Albin Kerouanton <albinker@gmail.com> (cherry picked from commit 80c44b4) Signed-off-by: Albin Kerouanton <albinker@gmail.com>
eab40d1
to
5295e88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
- What I did
Commit 8b7af1d added some code to update the DNSNames of all endpoints attached to a sandbox by loading a new instance of each affected endpoints from the datastore through a call to
Network.EndpointByID()
.This method then calls
Network.getEndpointFromStore()
, that in turn callsstore.GetObject()
, which then callscache.get()
, which callso.CopyTo(kvObject)
. This effectively creates a fresh new instance of an Endpoint. However, endpoints are already kept in memory by Sandbox, meaning we now have two in-memory instances of the same Endpoint.As it turns out, libnetwork is built around the idea that no two objects representing the same thing should leave in-memory, otherwise breaking mutex locking and optimistic locking (as both instances will have a drifting version tracking ID -- dbIndex in libnetwork parliance).
In this specific case, this bug materializes by container rename failing when applied a second time for a given container. An integration test is added to make sure this doesn't happen in the future.
- How I did it
This is a quick-fix; instead of loading endpoints through
Network.EndpointByID()
,Sandbox.getEndpoint()
was made public and is now used. I also added a comment onNetwork.EndpointByID()
saying this method should never be used -- and actually it should be removed in a follow-up.We should also probably look more closely to the way the datastore, its caching layer, etc... are used and implemented. (see this comment: #47195 (comment)).
- How to verify it
I added an integration test to test that
docker rename
can be called multiple times for the same container.- Description for the changelog