-
Notifications
You must be signed in to change notification settings - Fork 635
flake: wait for xds sync #13233
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
base: main
Are you sure you want to change the base?
flake: wait for xds sync #13233
Conversation
The alternative is to sleep for 20ms. This is more like what Istio pilot does in its 'EnsureSynced' function. Will prevent the TestXDSUpdate flake seen in https://github.com/kgateway-dev/kgateway/actions/runs/20791862732/job/59715696984?pr=13232 Signed-off-by: David L. Chandler <[email protected]>
* Move our 'update inbound' to be earlier; the old logic was too late so we could see 0 inbound updates Signed-off-by: John Howard <[email protected]>
| go s.sendPushes(stopCh) | ||
| for _, reg := range s.registrations { | ||
| go reg.Start(stopCh) | ||
| reg.Start(stopCh) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this means there's no blocking startup?
| return CollectionRegistration{ | ||
| Start: start, | ||
| HasSynced: synced, | ||
| HasSynced: synced.Load, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So now HasSynced doesn't mean the krt collection has synced? It's tied to the WaitUntilSynced handler. Maybe we should rename HasSynced so it's less confusing with other places we use the collection has synced ex.
| func (c *AgwCollections) HasSynced() bool { |
| synced := func() bool { | ||
| return nc.HasSynced() | ||
| go func() { | ||
| handler.WaitUntilSynced(stop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, should we wrap this in a timeout or a stop channel? Is WaitUntilSynced blocking until the krt collection reports that it has synced all resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@howardjohn do we need this or is it fine as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aitUntilSynced(stop) will exit early if stop chan is closed
| } | ||
| pushChannel <- &pr | ||
| s.InboundUpdates.Inc() | ||
| s.pushChannel <- &pr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to buffer the pushChannel now that we have the longer WaitUntilSynced? It's probably not a problem but maybe worth adding a future todo here.
Description
The alternative is to sleep for 20ms. This is more like what Istio pilot does in its 'EnsureSynced' function.
Will prevent the TestXDSUpdate flake seen in https://github.com/kgateway-dev/kgateway/actions/runs/20791862732/job/59715696984?pr=13232
Change Type
/kind flake
Changelog
Additional Notes