Proposal: Add Node to OnStreamClosed and OnDeltaStreamClosed of Callbacks

[tony612]

When we implement a control plane, we may need to watch resources like k8s Ingress/Gateway using k8s informer. To save cpu and memory, we only want to watch specific the resources we care about. Then we can start watching when we get a request and stop watching when the stream closes. With current Callbacks interface, we can start watching in (the first) OnStreamRequest(take sotw for an example) using Node in DiscoveryRequest, but we can't stop watching in OnStreamClosed because the params of doesn't have the Node.

So I propose adding Node to OnStreamClosed and OnDeltaStreamClosed, like:

# sotw
type Callbacks interface {
	OnStreamClosed(int64, *corev3.Node)
}

# delta
type Callbacks interface {
	OnDeltaStreamClosed(int64, *corev3.Node)
}

Any idea about the proposal? Or maybe there are other solutions to achieve this?

If this is a good idea, I can send a PR for this. But this will be a breaking change :(

[atollena]

This would be particularly handy when using SnapshotCache. The way we do that is typically to keep two mapping inside every implementations of the Callbacks interface:

1. a mapping of stream -> node id (a map[streamKey]stream, where streamKey is a stream ID + a boolean indicating whether it is a delta stream) which is created on the first request on a stream (in OnStreamRequest). You can access it in OnStreamClosed to do cleanup and also access the node associated to this stream.
2. a mapping of node id -> count (map[string]int) which counts the number of active streams for this node id. You can also add more information inside the mapping, such as the node metadata for this node ID.

In our control plane, a lot of implementation of Callbacks eventually need this kind of bookkeeping, and it would make sense for go-control-plane to offer a pattern for this.

That being said I think we can do better than just passing the node to OnStreamClosed and its delta variant: you'd still need to keep the count of stream for a given node (the second mapping). I'd rather suggest something in the form of an interface like:

type NodeCallbacks struct {
    OnNodeAdded: func (node *node) // typically calls SetSnapshot for this node
    OnNodeRemoved: func (node *node) // (or OnNodeRemoved: func(nodeId string)) // typically clears snapshots
}

Then offer a something to implement the xDS callbacks via this interface:

type NodeTrackingCallback struct {
        map[streamKey]stream
        map[string]int
}

type NewNodeTrackingCallback(callbacks NodeCallbacks) NodeTrackingCallback {
        ...
}

// + implementation of xds.Callacks that calls OnNodeAdded/OnNodeRemoved appropriately.

This would significantly decrease the complexity of basic control plane functionalities that just call SetSnapshot when the first stream for a node ID is opened, and ClearSnapshot when the last stream with this node ID is closed.

[alecholmez]

I think this is a reasonable addition to the callbacks interface. If you'd like to open a PR that makes this change i'd be happy to review.

[tony612]

@atollena I'm using a similar solution to track connected nodes like yours(map & callbacks).

OnNodeAdded & OnNodeRemoved will be a more direct way. But they are very different from the current callbacks, which are all about the lifetime of the streams. If we request more features in the future, we need to add more and more callbacks. Maybe we should discuss more. @alecholmez Any idea for the new NodeTrackingCallback by atollena?

@alecholmez I can open a PR for this soon.

[alecholmez]

I've been actually thinking about this quite a lot recently and I'm thinking go-control-plane could benefit from a better node management system. It seems quite a few control-plane implementations are having to manage their own node association layers so moving that logic upstream might be beneficial to a large user base. If you guys want to open a PR for that logic I'd be more than happy to review that. I would just recommend to make sure its generic enough where it can support different cache implementations.

On the other hand we may not even want to overload the callbacks. It could very well be worth introducing a new subsystem to track the proxy registration. What do you guys think about that? I think having the callbacks stay pretty tied to the stream lifecycle is rather user friendly.

[tony612]

@alecholmez I agree that we need to manage nodes in a better way without overloading the callbacks. I don't have a mature idea now but will post it if I have something to discuss.