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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃悰 Start the Cache if the Manager has already started #1681
馃悰 Start the Cache if the Manager has already started #1681
Conversation
Welcome @jsanda! |
Hi @jsanda. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@alvaroaleman if my changes look reasonable I can add a new test in manager_test.go. |
@@ -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 { | |||
cm.startRunnable(hasCache) | |||
if !hasCache.GetCache().WaitForCacheSync(cm.internalCtx) { |
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.
This will never timeout, right? Does that match the standard path, where we start the caches before the cm
is started?
/ok-to-test
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.
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
.
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.
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.
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.
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
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.
Looks good, but please also add a testcase for "Adding a cluster after manager was started results in a working cache"
@@ -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 { | |||
cm.startRunnable(hasCache) | |||
if !hasCache.GetCache().WaitForCacheSync(cm.internalCtx) { |
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.
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
@@ -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 { |
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.
This variable may only be accessed after acquiring Nevermind, we acquire that lock in the beginning of cm.mu
. Also warning, #1689 changes some of the lock handling so that might end up conflictingAdd
so this is fine
Add a test that adds a cluster to the manager after the manager has already started. Verify that the cluster is started and its cache is sycned. Added the startClusterAfterManager struct which is a basically just a hook to verify that the cluster is started.
@alvaroaleman I added a test. I added a fake cluster impl as a hook to verify the cluster is started. It feels a bit clunky. Open to suggestions if you have a better/simpler approach. |
return c.informer.Start(ctx) | ||
} | ||
|
||
func (c *startClusterAfterManager) GetCache() cache.Cache { |
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.
You don't need to implement anything other than GetCache
and Start
for this test
Only GetCache and Start methods are neeed for new test that adds a cluster to the manage after the manager has already started.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, jsanda The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
thanks! |
Fixes #1673.
This is needed to better support multi-cluster controllers where you want to dynamically add Clusters after the Manager has already started and have their caches started and synced.