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

Add watch request timeout to prevent watch request hang #5732

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 36 additions & 7 deletions pkg/search/proxy/store/multi_cluster_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -334,15 +334,44 @@ func (c *MultiClusterCache) Watch(ctx context.Context, gvr schema.GroupVersionRe
if cache == nil {
continue
}
w, err := cache.Watch(ctx, options)
if err != nil {

// The following logic adds a 30-second timeout to prevent watch requests to member clusters from hanging,
// which could cause the client watch to hang.
watchChan := make(chan watch.Interface, 1)
errChan := make(chan error, 1)

go func(cluster string) {
w, err := cache.Watch(ctx, options)
Copy link
Member

Choose a reason for hiding this comment

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

If this watcher is created after 30s, then it seems no way to stop it, is it leak?

Copy link
Member Author

@xigang xigang Oct 28, 2024

Choose a reason for hiding this comment

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

If the watcher times out after 30 seconds during creation, it will trigger a time.After timeout, return an error, and call cancel to stop the watcher goroutine.

Copy link
Member

Choose a reason for hiding this comment

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

ok

if err != nil {
select {
case errChan <- fmt.Errorf("failed to start watch for resource %v in cluster %q: %v", gvr.String(), cluster, err):
case <-ctx.Done():
}
return
}

select {
case watchChan <- w:
case <-ctx.Done():
w.Stop()
}
}(cluster)

select {
case w := <-watchChan:
mux.AddSource(w, func(e watch.Event) {
setObjectResourceVersionFunc(cluster, e.Object)
addCacheSourceAnnotation(e.Object, cluster)
})
case err := <-errChan:
// If the watch request fails, return the error, and the client will retry.
return nil, err
case <-time.After(30 * time.Second):
Copy link
Member

Choose a reason for hiding this comment

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

It seems wait 30s for each cluster. Should we wait all clusters paralleled?

Copy link
Member Author

@xigang xigang Oct 29, 2024

Choose a reason for hiding this comment

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

It seems wait 30s for each cluster. Should we wait all clusters paralleled?

@ikaven1024 There’s no issue here; as long as a single cache.Watch times out, the Watch request will return with an error and end. There’s no problem with that here.😄

Copy link
Member

Choose a reason for hiding this comment

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

It seems wait 30s for each cluster. Should we wait all clusters paralleled?

@ikaven1024 There’s no issue here; as long as a single cache.Watch times out, the Watch request will return with an error and end. There’s no problem with that here.😄

While if every cluster create watching takes 20s, not timeout, the total time spends 20s * N.

// If the watch request times out, return an error, and the client will retry.
return nil, fmt.Errorf("timeout waiting for watch for resource %v in cluster %q", gvr.String(), cluster)
Copy link
Contributor

Choose a reason for hiding this comment

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

@xigang Hi, if a watch request is hanging and causes a timeout, will the hanging watch request continue to exist in the subprocess?

Copy link
Member Author

Choose a reason for hiding this comment

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

@zhzhuang-zju Yes, there is this issue. When a watch request times out, the goroutine needs to be terminated.

Copy link
Member

Choose a reason for hiding this comment

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

Good point! Then that case we have to cancel the context passed to cache.Watch().

Copy link
Member

Choose a reason for hiding this comment

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

So this patch intends to terminate the hanging by raising an error after a period of time. Is this the idea?

Copy link
Member

Choose a reason for hiding this comment

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

Another question:
Before starting the Watch, we tried to get the cache of that cluster, I'm curious why this cache still exists even after the cluster is gone. Do we have a chance to clean the cache?

cache := c.cacheForClusterResource(cluster, gvr)
if cache == nil {
continue
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Another question: Before starting the Watch, we tried to get the cache of that cluster, I'm curious why this cache still exists even after the cluster is gone. Do we have a chance to clean the cache?

cache := c.cacheForClusterResource(cluster, gvr)
if cache == nil {
continue
}

@RainbowMango When the member cluster goes offline but the Cluster resources in the control plane are not deleted, it can prevent the offline clusters in the ResourceRegistry from being removed, resulting in the resource cache being retained for a short time.

Copy link
Member Author

@xigang xigang Oct 27, 2024

Choose a reason for hiding this comment

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

@xigang Hi, if a watch request is hanging and causes a timeout, will the hanging watch request continue to exist in the subprocess?

@RainbowMango @zhzhuang-zju Fixed, please take a look.

case <-ctx.Done():
return nil, ctx.Err()
}

mux.AddSource(w, func(e watch.Event) {
setObjectResourceVersionFunc(cluster, e.Object)
addCacheSourceAnnotation(e.Object, cluster)
})
}
mux.Start()
return mux, nil
Expand Down