Refactor endpoint management

[joestringer]

Review commit-by-commit.

This PR aims to address some technical debt relating to the EndpointManager. Despite the naming, up until this PR the management of endpoints has been handled from pkg/endpoint, with the call flow being Daemon -> Endpoint.Expose() / Endpoint.Delete() -> EndpointManager.UpdateReferences() / EndpointManager.RemoveReferences() and friends. This required defining an EndpointManager type inside the Endpoint, and passing it through as a parameter just so that Endpoint could call EndpointManager at the right time. This relationship is the opposite of what one might expect, and leads to subsequent difficulties when trying to allow other subsystems to interact with the set of available endpoints (because they cannot rely on the EndpointManager to manage endpoints, they must pull in complexity from granular dependencies on the Endpoint package.) Many of the details of these interfaces were previously exposed to the callers, increasing complexity across the codebase. This PR aims to reduce such abstraction leakage to make the code simpler and more reliable.

This PR moves the endpoint lifecycle more towards this:

• Endpoint constructor
• EndpointManager.AddEndpoint() to make the endpoint findable from other subsystems.
  • Indirectly calls Endpoint.Start() to handle events from other subsystems.
• If shutting down the agent, Endpoint.Stop() gracefully stops handling events without initiating the delete process.
• EndpointManager.RemoveEndpoint() to prevent other subsystems from finding the endpoint.
  • As part of this, first hide the endpoint from other subsystems by removing it from the EndpointManager, then call endpoint.Delete() which does Endpoint.Stop() to stop handling ongoing events.

There are no functional changes intended by this PR, but the ordering of some operations has been shuffled around slightly, in particular the locking management between Endpoint vs. EndpointManager during endpoint deletion and the order of removing the endpoint from the EndpointManager vs. stopping the goroutines that handle events from other subsystems.

This was initiated while trying to figure out a clean way to port #14541 to the master branch.

[joestringer]

Incidentally, I noticed that this path is actually broken. If we intend to change any of the Identifiers from this path, then this code is not actually removing the previous identifiers, it's only creating new references in the endpointmanager for the endpoint. This could lead to stale references using old identifiers.

I don't think this is actually a viable use of the config API today so I didn't bother to fix it, and also I was trying to keep the scope of this PR relatively small so I can move on to the PR that I want to build on top of these changes.

[joestringer]

test-me-please

[joestringer]

test-gke

[joestringer]

test-gke

[joestringer]

test-me-please

[joestringer]

test-me-please

[joestringer]

test-me-please

[joestringer]

Hit flakes #12511 (net-next), #13011 (kernel-4.9). Otherwise green on CI.

[christarazi]

🎉 This is great! One of the more satisfying PRs to see. LGTM overall, very clean commits and well-documented history. A few minor questions.

[joestringer]

test-me-please

[joestringer]

test-me-please

[tklauser]

This PR is nicely structured and with the details in the commit messages made it really convenient to review also for someone not (yet) deeply familiar with the code 🚀

A few questions and nit-pick suggestions for follow-up PRs inline, nothing blocking.

[christarazi]

Last question for me is do we need the Introduce Disconnect() commit any more? For review, it served a nice purpose to ease inspection, but now endpoint: Rearrange endpoint Unexpose vs Stop essentially removes it. It is a helpful for history purposes, so maybe a middle-ground would be to include the context in the latter commit also.

[joestringer]

Yeah I debated whether to fold that commit back into the earlier commits to remove the churn in the middle. Now that I had a fully green CI run I feel a bit more confident that I won't need to throw away the extra patch on top, so maybe I should touch that up. I can roll in a couple of minor bits of feedback from @tklauser at the same time.

[joestringer]

I did some cleanup / shuffling around based on the above feedback. Here's everything except for the revert of the WaitEndpointRemoved() function rename patch which I changed my mind about, WaitEndpointRemoved is back for use in unit tests:

diff --git a/pkg/endpoint/endpoint.go b/pkg/endpoint/endpoint.go
index f4986e21cfab..2c544a6c618a 100644
--- a/pkg/endpoint/endpoint.go
+++ b/pkg/endpoint/endpoint.go
@@ -2076,6 +2076,9 @@ type IPReleaser interface {

 // Delete cleans up all resources associated with this endpoint, including the
 // following:
+// * all goroutines managed by this Endpoint (EventQueue, Controllers)
+// * removal from the endpointmanager, resulting in new events not taking effect
+// on this endpoint
 // * cleanup of datapath state (BPF maps, proxy configuration, directories)
 // * releasing IP addresses allocated for the endpoint
 // * releasing of the reference to its allocated security identity
@@ -2089,9 +2092,8 @@ func (e *Endpoint) Delete(ipam IPReleaser, conf DeleteConfig) []error {
        // return here. Ignore the request if the endpoint is already
        // disconnected.
        if err := e.lockAlive(); err != nil {
-               return errs
+               return []error{}
        }
-
        e.setState(StateDisconnecting, "Deleting endpoint")

        // If dry mode is enabled, no changes to BPF maps are performed
diff --git a/pkg/endpoint/endpoint_test.go b/pkg/endpoint/endpoint_test.go
index bec949f149f4..45a9661b84f9 100644
--- a/pkg/endpoint/endpoint_test.go
+++ b/pkg/endpoint/endpoint_test.go
@@ -18,7 +18,6 @@ package endpoint

 import (
        "context"
-       "net"
        "reflect"
        "testing"
        "time"
@@ -720,9 +719,3 @@ func BenchmarkEndpointGetModel(b *testing.B) {
                e.GetModel()
        }
 }
-
-type ipReleaserDummy struct{}
-
-func (i *ipReleaserDummy) ReleaseIP(ip net.IP) error {
-       return nil
-}
diff --git a/pkg/endpointmanager/manager.go b/pkg/endpointmanager/manager.go
index 811e64e30083..adcddfee76ea 100644
--- a/pkg/endpointmanager/manager.go
+++ b/pkg/endpointmanager/manager.go
@@ -305,9 +305,6 @@ func (mgr *EndpointManager) ReleaseID(ep *endpoint.Endpoint) error {
 // unexpose removes the endpoint from the endpointmanager, so subsequent
 // lookups will no longer find the endpoint.
 func (mgr *EndpointManager) unexpose(ep *endpoint.Endpoint) {
-       mgr.mutex.Lock()
-       defer mgr.mutex.Unlock()
-
        // Fetch the identifiers; this will only fail if the endpoint is
        // already disconnected, in which case we don't need to proceed with
        // the rest of cleaning up the endpoint.
@@ -318,6 +315,9 @@ func (mgr *EndpointManager) unexpose(ep *endpoint.Endpoint) {
        }
        previousState := ep.GetState()

+       mgr.mutex.Lock()
+       defer mgr.mutex.Unlock()
+
        // This must be done before the ID is released for the endpoint!
        mgr.removeIDLocked(ep.ID)
        mgr.RemoveIPv6Address(ep.IPv6)
@@ -346,6 +346,14 @@ func (mgr *EndpointManager) RemoveEndpoint(monitor regeneration.Owner, ipam endp
        return ep.Delete(ipam, conf)
 }

+// WaitEndpointRemoved waits until all operations associated with Remove of
+// the endpoint have been completed.
+// Note: only used for unit tests, to avoid ep.Delete()
+func (mgr *EndpointManager) WaitEndpointRemoved(ep *endpoint.Endpoint) {
+       mgr.unexpose(ep)
+       ep.Stop()
+}
+
 // RemoveAll removes all endpoints from the global maps.
 func (mgr *EndpointManager) RemoveAll() {
        mgr.mutex.Lock()

EDIT: Ah, I was a little overzealous: this bit doesn't make sense. I can fix it up before merge or send a simple follow-up to remove these lines:

+// * removal from the endpointmanager, resulting in new events not taking effect
+// on this endpoint

[joestringer]

test-me-please