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

KIC 2.0: handle sync-period flag #1309

Closed
rainest opened this issue May 11, 2021 · 3 comments · Fixed by #1506
Closed

KIC 2.0: handle sync-period flag #1309

rainest opened this issue May 11, 2021 · 3 comments · Fixed by #1506
Assignees
Labels
area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. priority/low

Comments

@rainest
Copy link
Contributor

rainest commented May 11, 2021

--sync-period sets the duration (10m by default) between full resource checks (i.e. when the controller repopulates all resources into the cache, in case it missed an event). In 1.x, we pass it to the informer factories.

#1274 originally had a hard-coded version of this with a stub implementation, but it was removed from the final version of the PR. Unsure where we're tracking the follow-up.

@mflendrich
Copy link
Contributor

After discussion with @shaneutt it seems likely that the k8s API contract and the controller-runtime implementation give us sufficient guarantees as to not dropping events. But we're not satisfied with our current confidence level, so the scope of this issue becomes now to:

  • prove or disprove that the --sync-period flag is not necessary in KIC 2.x
  • document dropping of the --sync-period flag, if this be the case.

@mflendrich
Copy link
Contributor

As @hbagdi stated, KIC 1.x, by performing a sync every X minutes, has a side effect of cleaning up resources (e.g. consumers) that have been created by the user outside of the source of truth (the k8s api), e.g. in the UI.

We probably want to keep that functionality, or be very explicit about changing behavior here. Today the sync-period flag seems like the only known way to achieve existing behavior.

@shaneutt shaneutt assigned shaneutt and unassigned shaneutt Jul 7, 2021
@shaneutt
Copy link
Contributor

shaneutt commented Jul 7, 2021

After code diving controller-runtime and reviewing and hardening my understanding about controller-runtime's implementation of client-go, I did find that controller runtime does technically handle this for us as we were suspecting however I don't particularly like their defaults (10hour resyncs) given our historical 10m resyncs.

We don't have solid metric or performance information to show how hard resyncing every 10 minute hits us, however given no data on the matter I expect the correct decision for the moment will be to use the same period as V1 and will make a PR to that effect, where we can make improvements forward if we discover new evidence to motivate us to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. priority/low
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants