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.

Sign up for GitHub

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

Jump to bottom

🐛 Start the Cache if the Manager has already started #1681

Merged
merged 3 commits into from
Oct 8, 2021
Merged

🐛 Start the Cache if the Manager has already started #1681

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c25b12
Start the Cache if the Manager has already started
jsanda Oct 4, 2021
6ea9679
verify adding cluster after mgr started results in working cache
jsanda Oct 8, 2021
10aed67
remove unnecessary methods
jsanda Oct 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions pkg/manager/internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,12 @@ func (cm *controllerManager) Add(r Runnable) error {
cm.nonLeaderElectionRunnables = append(cm.nonLeaderElectionRunnables, r)
} else if hasCache, ok := r.(hasCache); ok {
cm.caches = append(cm.caches, hasCache)
if cm.started {
Copy link
Member

Choose a reason for hiding this comment

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

This variable may only be accessed after acquiring cm.mu. Also warning, #1689 changes some of the lock handling so that might end up conflicting Nevermind, we acquire that lock in the beginning of Add so this is fine

cm.startRunnable(hasCache)
if !hasCache.GetCache().WaitForCacheSync(cm.internalCtx) {
Copy link
Member

Choose a reason for hiding this comment

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

This will never timeout, right? Does that match the standard path, where we start the caches before the cm is started?

/ok-to-test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could definitely time out. The remote cluster might not be accessible. Should the cache be started and synced with a different context? If the sync times out, I would then cancel the context. I suppose it also makes more sense to append the cache after the sync completes successfully.

I believe this matches the standard path. I can't simply call waitForCache because it returns immediately if cm.started is true. It first iterates through the caches and calls cm.startRunnable for each one. Then it iterates through the caches again and calls cache.GetCache().WaitForCacheSync(ctx) where ctx is cm.internalCtx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After looking through the code some more, and the WaitForCacheSync function in shared_informer.go in client-go in particular, I am less certain that it will time out.

Copy link
Member

Choose a reason for hiding this comment

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

What I meant was if the normal route of starting caches has a context that times out, but doesn't seem to be the case so this is fine

return fmt.Errorf("could not sync cache")
}
}
} else {
shouldStart = cm.startedLeader
cm.leaderElectionRunnables = append(cm.leaderElectionRunnables, r)
Expand Down