Skip to content
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

watchable: skip an update if the elements in slice have not changed, but order has #1634

Closed
arkodg opened this issue Jul 7, 2023 · 5 comments · Fixed by #1795
Closed

watchable: skip an update if the elements in slice have not changed, but order has #1634

arkodg opened this issue Jul 7, 2023 · 5 comments · Fixed by #1795
Labels
area/message-service Issues related to Gateway's message service used for communication among components. kind/enhancement New feature or request stale

Comments

@arkodg
Copy link
Contributor

arkodg commented Jul 7, 2023

Description:

  • Here is Gateway API resources list that contains many fields that hold slice values
  • This structure is reused as a watchable message and published from the provider to the gateway-api subscriber.
  • There is no guarantee that the provider will insert elements into the slice as the same order it did the last time, since the controller-runtime reconciler/cache does not guarantee this. This might cause elements to be inserted in the list out of order
  • This will cause the watchable layer to send an update to the subscriber since it thinks there is a change in the message. It relies on reflect.DeepCopy today for comparison.
  • We can probably do better by adding a custom types that implements the Equal method using cmp.Diff https://pkg.go.dev/github.com/google/go-cmp/cmp#Option and a custom sort option.
  • This increasing cpu needed for sorting but eliminates re translation in other layers such as gateway-api

Relates to #1503 (comment)
& #1365

@arkodg arkodg added the kind/enhancement New feature or request label Jul 7, 2023
@arkodg
Copy link
Contributor Author

arkodg commented Jul 7, 2023

might be good to get actual control plane obs data #700 before implementing this

@arkodg
Copy link
Contributor Author

arkodg commented Jul 7, 2023

cc @LukeShu

@arkodg arkodg added the area/message-service Issues related to Gateway's message service used for communication among components. label Jul 7, 2023
@dboslee
Copy link
Contributor

dboslee commented Jul 7, 2023

Moving the conversation to this issue as to not detract from the e2e testing issue.

we do incorporate the route name in the tcp listener (refer to #696) since the tcp listener directly maps to a cluster / destination

My particular issue is not actually the order routes are processed (although I thought this at first). It stems from the xds TCPListener being named after a particular route. Each TLSRoute gets translated into an ir.TCPListener. But many TLSRoutes and therefore ir.TCPListener may be map to a single xds TCPListener where they are added as xds FilterChains.

This ultimately leads to draining the listener when the xds TCPListener name changes. This could be due to re-ordering but even when sorted its guaranteed to be renamed when the TLSRoute that named the xds TCPListener gets deleted.

I think the fix is to have a Listener Name as before #696 but also pass through the Route name which would be used as a unique mapping from a particular filter chain to a particular cluster/desination.

@arkodg
Copy link
Contributor Author

arkodg commented Jul 8, 2023

hey @dboslee thanks for surfacing this limitation, agreed, creating a separate issue to track this work !

@github-actions
Copy link

github-actions bot commented Aug 7, 2023

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/message-service Issues related to Gateway's message service used for communication among components. kind/enhancement New feature or request stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants