libnet: remove the EndpointInfo interface #47217
Draft
akerouanton wants to merge 2 commits intomoby:masterfrom
Draft
libnet: remove the EndpointInfo interface #47217akerouanton wants to merge 2 commits intomoby:masterfrom
EndpointInfo interface #47217akerouanton wants to merge 2 commits intomoby:masterfrom
Conversation
akerouanton
added
status/2-code-review
area/networking
akerouanton
changed the title
EndpointInfo interface
Member
|
I was surprised I didn't remove this one yet, but looking at 210abfa (#46314), I see I left a comment at the time;
Wondering if there's cases where the information may not be propagated (and if there's a risk involved in removing that part). Otherwise, a big "+1" for removing. |
Member
Author
|
I'll convert this one to draft for now as there're a few CI jobs broken -- I might have missed something. |
`(*Endpoint).sbJoin()` starts by calling `(*Network).getEndpointFromStore()` before doing anything else. This latter method calls `(*Store).GetObject()` with a freshly created `*Endpoint` instance, meaning the `*Endpoint` instance returned by `getEndpointFromStore` is a totally different reference than the one already living in memory. Thus, any mutex locks on that new instance actually locks *nothing* at all. The need for these locks is also questionable, but I'll keep that for a follow-up. Another property of `GetObject()` is to fully hydrate a given object based on the "canonical" reference that lives in its cache. Since `sbJoin()` is always called on an `*Endpoint` receiver which was either just created, or just retrieved from the datastore, there's no chance that the receiver was partially hydrated. Signed-off-by: Albin Kerouanton <[email protected]>
`EndpointInfo` was introduced in moby/libnetwork@d8ba1e231. The commit's message states: > This sort of deisgn hides the libnetwork specific type details from > drivers. 9 years later it turns out the `Endpoint` struct is the only implementor of that interface, and `libnetwork` and `daemon` packages are the only consumers. This is adding an unneeded level of indirection. `(*Endpoint).Info()` is the only function that returns an `EndpointInfo`. It's basically 12 lines of cursed indirection packed together. But, as noted by thaJeztah in 210abfa, we need to make sure the `*Endpoint` receiver is always fully hydrated to get rid of it. So here we go. As the diff shows, most of the time `(*Endpoint).Info()` is called right after a call to `(*Network).Endpoints()` (sometimes through an indirection). That method in turns calls `getEndpointsFromStore`, which is calling `(*Store).List()`, which is either retrieving all objects of a given type from its internal cache (if it was populated), or retrieve them all from boltdb. In that case, `*Endpoint`s are fully hydrated. - `disconnectFromNetwork`: calls `WalkEndpoints`, which calls `(*Network).Endpoints()`, right before ; - `buildContainerAttachments`: calls `(*Network).Endpoints()` right before ; - `buildEndpointResource`: `ep` and `info` are actually both the same thing ; - `clearAttachableNetworks`: calls `(*Network).Endpoints()` right before ; - `findLBEndpointSandbox`: calls `(*Network).Endpoints()` right before ; - `addLBBackend`: calls `(*Network).Endpoints()` right before ; - `deleteLoadBalancerSandbox`: calls `(*Network).EndpointByName()`, which calls `WalkEndpoint` (see above). We're left with the following, less obvious cases: - `updateJoinInfo`: called from `(*Daemon).connectToNetwork()`, where `*Endpoint`s are created ; - `buildEndpointInfo`: called from `(*Daemon).updateEndpointNetworkSettings()`, again called from `(*Daemon).connectToNetwork()` ; Meaning, we can now safely assume `*Endpoint`s are always fully hydrated before calling its `Info()` method. We can finally ditch it! 🎉 Signed-off-by: Albin Kerouanton <[email protected]>
akerouanton
force-pushed
the
libnet-remove-EndpointInfo
branch
from
b6dc374 to
084041f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
*Endpointin the void #47216 to makeTestSRVServiceQuerypass- What I did
EndpointInfowas introduced in moby/libnetwork@d8ba1e231. The commit's message states:9 years later it turns out the
Endpointstruct is the only implementor of that interface, andlibnetworkanddaemonpackages are the only consumers. This is adding an unneeded level of indirection.(*Endpoint).Info()is the only function that returns anEndpointInfo. It's basically 12 lines of cursed indirection packed together. But, as noted by thaJeztah in 210abfa, we need to make sure the*Endpointreceiver is always fully hydrated to get rid of it. So here we go.As the diff shows, most of the time
(*Endpoint).Info()is called right after a call to(*Network).Endpoints()(sometimes through an indirection). That method in turns callsgetEndpointsFromStore, which is calling(*Store).List(), which is either retrieving all objects of a given type from its internal cache (if it was populated), or retrieve them all from boltdb. In that case,*Endpoints are fully hydrated.disconnectFromNetwork: callsWalkEndpoints, which calls(*Network).Endpoints(), right before ;buildContainerAttachments: calls(*Network).Endpoints()right before ;buildEndpointResource:epandinfoare actually both the same thing ;clearAttachableNetworks: calls(*Network).Endpoints()right before ;findLBEndpointSandbox: calls(*Network).Endpoints()right before ;addLBBackend: calls(*Network).Endpoints()right before ;deleteLoadBalancerSandbox: calls(*Network).EndpointByName(), which callsWalkEndpoint(see above).We're left with the following, less obvious cases:
updateJoinInfo: called from(*Daemon).connectToNetwork(), where*Endpoints are created ;buildEndpointInfo: called from(*Daemon).updateEndpointNetworkSettings(), again called from(*Daemon).connectToNetwork();Meaning, we can now safely assume
*Endpoints are always fully hydrated before calling itsInfo()method. We can finally ditch it! 🎉- How I did it
Static analysis of the code
- How to verify it
CI is green.