api: Update gateway status to include AttachedListeners

[davidjumani]

What type of PR is this?
Add one of the following kinds:
/kind gep

What this PR does / why we need it:
Updates the status of the Gateway to include AttachedListeners which is the count of successful ListenerSet attachments to the gateway. Ref: slack thread

Does this PR introduce a user-facing change?:

Adds the `AttachedListeners` conditions to the Gateway status which is the count of successful ListenerSet attachments to the gateway

[howardjohn]

I am going to take a contrarian point of view here and say maybe we shouldn't add this. AttachedRoutes, IMO, is an extremely high-cost status field to update with extremely little value.

Our implementation spends like 20% of its CPU calculating AttachedRoutes...

If we do decide to add it though, the PR itself LGTM

[rikatz]

@howardjohn regardless of the implementation detail (which I see your point and agree about the performance caveat), I am wondering that we need a way to let the Gateway owner know that there are some ListenerSets attached to the Gateway.

Do you suggest some different approach for it?

[howardjohn]

A ListenersAttached: bool instead of int is one approach that maybe could be acceptable tradeoff.

FWIW the implementation detail is not specific to any implementation, it necessarily at least doubles the number of status writes to the API server on a route creation/deletion.

[rikatz]

ok, seems reasonable.

@robscott @youngnick for some comments, we can change for a boolean instead. I think it is important for an admin at least to know something is attached to the gateway

[davidjumani]

A ListenersAttached: bool instead of int is one approach that maybe could be acceptable tradeoff.

Thanks @howardjohn - The AttachedListenerSets status condition should suffice if this is the case

[youngnick]

I'm sorry, but the fact that implementing something can be difficult for one implementation doesn't decrease the utility of the integer field for me.

I disagree that attachedRoutes is a low-utility field, having the number there is much closer to what would be the most useful (having a list of all the routes), while still being less loaded on the apiserver than a full list.

I am happy with the integer as it currently stands.

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidjumani, youngnick

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [youngnick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[howardjohn]

It's not "difficult for 1 implementation". It's expensive for all implementations. There is no possibile way to avoid an explosion of writes as each route triggers a listenerset write

[youngnick]

Adding AttachedListeners does not materially change the number of writes compared to AttachedRoutes though, because each Route added causes one write to either a Gateway or a ListenerSet, and each ListenerSet attached to a Gateway causes one write to a Gateway. The writes aren't intended to be transitive - you don't update AttachedRoutes on the Gateway for Routes attached to a ListenerSet.

If this is not clearly specified enough, then we should clarify that.

[rikatz]

ok so are we clear on this?

• AttachedRoutes -> Reflects the routes attached directly to the Gateway. So each Route that strictly has a gateway as a parentRef should be summed to this field
• A ListenerSet also has a .status.listeners.attachedRoutes and just routes attached to that ListenerSet (eg.: using it as parentRef) are summed to its status
• AttachedListeners -> Reflects the amount of ListenerSets attached to the Gateway. does not reflect the amount of routes attached to all listenersets
• In case a route have both a Gateway and a ListenerSet as a parentRef, it should be added 1 to Gateway .status.AttachedRoutes and also to the parent ListenerSet .status.listeners[].attachedRoutes

@howardjohn IIUC this does not "explode" writes on Gateway, as we will be summing just child attached resources, but not grandchild resources.

[howardjohn]

It still means any time you add/remove a route, you need to go update the parent as well. So its not worse than Gateway, but its not better

[rikatz]

@howardjohn right, so my question is: given that nothing change (for best or worst), is this acceptable? I think the major concern was about impacting the API server in case you had to sum the routes also on the Gateway, which is not the case.

From an admin perspective, I would like to at least know how many listeners are attached, and from there I can write queries to figure out the other routes. We can also extend gwctl for these management tasks (as soon as they become more expensive API operations), but at least having an insight like we already have with attachedRoutes is desired IMO.

[howardjohn]

I wouldn't really say nothing changed. We introduced something new (ListenerSet) and applied re-applied old mistakes. So they are now new mistakes, from my opinion. If there is consensus around doing that, I am not blocking this PR.

[rikatz]

right, I don't have property or context to say from an implementation perspective what are the real problems we may have, so I wont make the final call here.

From a user experience side, IMHO a bool is not that much valuable as a count of attached resources, and that's why I am not sure we wouldn't just be exchanging one bad decision for another bad decision (what would be the point of having a bool pointer with nil/true/false other than knowing if listenerset is supported, and if there is something attached to it or not).

@robscott @shaneutt leaving this for you :)