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

volumewatcher: stop watcher goroutines when there's no work #7909

Merged
merged 1 commit into from
May 11, 2020

Conversation

tgross
Copy link
Member

@tgross tgross commented May 8, 2020

Fixes #7837

The watcher goroutines will be automatically started if a volume has updates, but when idle we shouldn't keep a goroutine running and taking up memory.

In addition to the unit test updates, I've tested this out on a real-world cluster and everything seems to work out ok.

@tgross tgross added this to the 0.11.2 milestone May 8, 2020
@tgross tgross force-pushed the volumewatcher_reduce_goroutines branch from bed9b46 to 72675b7 Compare May 8, 2020 20:53
@tgross tgross marked this pull request as ready for review May 8, 2020 20:55
@tgross tgross requested review from langmartin and shoenig May 8, 2020 20:55
Copy link
Contributor

@langmartin langmartin left a comment

Choose a reason for hiding this comment

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

I think this looks good. The idea is that since we're just collecting the claim, we'll:

  • block until notify is called (or shutdown)
  • read the one claim (or shutdown)
  • exit
    so there's no way to notify twice, but we don't need to do that, because we're just using the channel to facilitate the one delay. I've got all that right?

@tgross
Copy link
Member Author

tgross commented May 11, 2020

so there's no way to notify twice, but we don't need to do that, because we're just using the channel to facilitate the one delay. I've got all that right?

Yes, with the addition that checkpointing will re-trigger the notify, ensuring that if we error-out we retry from the point of the checkpoint.

@tgross
Copy link
Member Author

tgross commented May 11, 2020

Hm, TestVolumeWatch_StartStop is failing here but was passing for me locally. There may still be something racy there I need to fix. It just can take a little while to quiesce, bumping up the require.Eventually timeout a bit makes it pass reliably.

The watcher goroutines will be automatically started if a volume has
updates, but when idle we shouldn't keep a goroutine running and
taking up memory.
@tgross tgross force-pushed the volumewatcher_reduce_goroutines branch from 72675b7 to 7e33619 Compare May 11, 2020 13:13
@tgross
Copy link
Member Author

tgross commented May 11, 2020

Remaining test failure is a flake being fixed in #7913

@tgross tgross merged commit 3d6c088 into master May 11, 2020
@tgross tgross deleted the volumewatcher_reduce_goroutines branch May 11, 2020 13:32
@github-actions
Copy link

github-actions bot commented Jan 6, 2023

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reduce running goroutines for volumewatcher
2 participants