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

wait for informer caches to sync before running controllers #2299

Merged
merged 2 commits into from Mar 4, 2020

Conversation

skriss
Copy link
Member

@skriss skriss commented Feb 25, 2020

Signed-off-by: Steve Kriss krisss@vmware.com

closes #616 (already closed)

@carlisia @nrb @ashish-amarnath wanted to get your input on this change before making it for all the controllers. This moves the logic that waits for informers' caches to sync out of each individual controller, and into the main server process, before running any controllers. You can see #616 for a little more detail.

I want to do this because it makes some other refactoring that I'd like to do (basically #131) easier by not requiring each struct that uses an informer/lister to wait for cache sync before using it.

If it looks good to you, I'll continue changing all the other controllers!

@ashish-amarnath
Copy link
Contributor

I am onboard w/ the idea here 👍

@skriss
Copy link
Member Author

skriss commented Feb 26, 2020

@ashish-amarnath FYI, I'm hoping that this followed by #131 puts us in a better position to contemplate a potential migration to kubebuilder, since most of the processing logic will have been extracted from our controllers and put into libraries. It also sets us up to more easily move to using worker pods (one per backup/restore) if we decide to go that route.

@skriss skriss changed the title WIP wait for informer caches to sync before running controllers wait for informer caches to sync before running controllers Feb 26, 2020
@skriss
Copy link
Member Author

skriss commented Feb 26, 2020

I went ahead and updated all the controllers.

Note we'll hold this until after releasing v1.3, so I'm leaving it as a draft.

@skriss skriss added this to the v1.4 milestone Feb 26, 2020
carlisia
carlisia previously approved these changes Feb 26, 2020
Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

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

This is a great change to add, and the implementation seems fine to me.

@skriss
Copy link
Member Author

skriss commented Mar 2, 2020

ready for review/merge now that v1.3.0 has been tagged.

Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
@skriss
Copy link
Member Author

skriss commented Mar 4, 2020

@nrb @ashish-amarnath gentle reminder that this is ready for review/merge.

Copy link
Contributor

@ashish-amarnath ashish-amarnath left a comment

Choose a reason for hiding this comment

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

lgtm!

@ashish-amarnath ashish-amarnath merged commit cc848fb into vmware-tanzu:master Mar 4, 2020
@skriss skriss deleted the refactor-cache-sync branch March 4, 2020 22:26
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.

Use shared informer factory's WaitForCacheSync in server.go
4 participants