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

vai: document resync period #463

Merged
merged 1 commit into from
Jan 28, 2025
Merged

vai: document resync period #463

merged 1 commit into from
Jan 28, 2025

Conversation

moio
Copy link
Contributor

@moio moio commented Jan 24, 2025

Currently all Vai informers are started with a zero interval period, meaning they never force resync all objects.

It was suggested this should be made configurable and aligned with the behavior of other informers in lasso (rancher/lasso#65 (comment), part of rancher/rancher#47825)

Actually, there was a misunderstanding on the exact consequences of resync. This PR is to document exactly why.

Note also discussion in #463 (comment)

@tomleb
Copy link
Contributor

tomleb commented Jan 24, 2025

I initially thought the resync was doing an actual LIST+WATCH resync with the apiserver, but it is simply taking whatever is in its cache and re-running handlers with it. (Confirmed by adding print statements in the various Store.Add, Store.Update, etc call and not seeing them with a custom configured interval of 2 seconds)

Given that there aren't any handlers registered with the vai informer, do we actually need this now? Or is it just a "future-proofing" kind of change?

@moio
Copy link
Contributor Author

moio commented Jan 24, 2025

I initially thought the resync was doing an actual LIST+WATCH resync with the apiserver, but it is simply taking whatever is in its cache and re-running handlers with it. (Confirmed by adding print statements in the various Store.Add, Store.Update, etc call and not seeing them with a custom configured interval of 2 seconds)

Whoa.

I have to admit I was under the wrong assumption you mentioned to this day. Probably misguided by a document that circulated years ago as part of the Rancher developer onboarding: https://github.com/aiyengar2/k8s-docs/blob/main/docs/controllers/02_informers.md#introduction-to-the-informer-pattern

I checked client-go's code and indeed it seems like the periodic resync is only used to notify event handlers and setting the internal Reflector resync time. And all Reflector does is to call Resync on its associated Store - but is a no-op on "simple" stores like ours (which is modeled after ThreadSafeStore).

So that only leaves event handlers, no periodic re-listing.

Then this change is completely useless.

I will use this PR as an occasion to document the behavior well.

@moio moio force-pushed the uniform_resync_time branch from e1ec011 to 26adbfa Compare January 24, 2025 14:59
@moio moio changed the title vai: align default resync period with lasso vai: document resync period Jan 24, 2025
@moio moio force-pushed the uniform_resync_time branch from 26adbfa to fd1d41c Compare January 27, 2025 08:34
Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@moio moio merged commit ae4153b into rancher:main Jan 28, 2025
1 check passed
@moio moio deleted the uniform_resync_time branch January 28, 2025 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants