deployment watcher: fix goroutine leak when job is purged #20348
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The deployment watcher on the leader makes blocking queries to detect when the set of active deployments changes. It takes the resulting list of deployments and adds or removes watchers based on whether the deployment is active. But when a job is purged, the deployment will be deleted. This unblocks the query but the query result only shows the remaining deployments. When the query unblocks, ensure that all active watchers have a corresponding deployment in state. If not, remove the watcher so that the goroutine stops. Fixes: #19988
tgross
force-pushed
the
19988-deploymentwatcher-goroutine-leak
branch
from
0c69e47 to
004fb71
Compare
tgross
added
backport/1.5.x
| @@ -191,6 +190,21 @@ func (w *Watcher) watchDeployments(ctx context.Context) { | |||
| w.remove(d) | |||
| } | |||
| } | |||
|
|
|||
| // Ensure we're not tracking deployments that have been deleted because | |||
There was a problem hiding this comment.
If you check for the number of watchers and number of deployments, you could avoid always checking even when there are no changes
There was a problem hiding this comment.
Good catch. That also made me realize that we're not locking the read on w.watchers here, so I need to fix that.
There was a problem hiding this comment.
I don't think we can't optimize this with a length check after all. Imagine the following scenario:
- watchers for deployment A and B exist, len(deployments) == 2, len(watchers) == 2
- A gets purged, B completes (is no longer active), C gets added
- after we call
add/remove, we have len(deployments) == 2, len(watchers) == 2 - so now checking the lengths would be equal and we'd skip removing A
I've pulled this block of code out to a function to make the locking nicer, and I've added a note about why we can't optimize it with the length check.
This was referenced Apr 11, 2024
Merged
Merged
tgross
added a commit
that referenced
this pull request
Apr 11, 2024
The deployment watcher on the leader makes blocking queries to detect when the set of active deployments changes. It takes the resulting list of deployments and adds or removes watchers based on whether the deployment is active. But when a job is purged, the deployment will be deleted. This unblocks the query but the query result only shows the remaining deployments. When the query unblocks, ensure that all active watchers have a corresponding deployment in state. If not, remove the watcher so that the goroutine stops. Fixes: #19988
tgross
added a commit
that referenced
this pull request
Apr 11, 2024
…#20348) (#20359) The deployment watcher on the leader makes blocking queries to detect when the set of active deployments changes. It takes the resulting list of deployments and adds or removes watchers based on whether the deployment is active. But when a job is purged, the deployment will be deleted. This unblocks the query but the query result only shows the remaining deployments. When the query unblocks, ensure that all active watchers have a corresponding deployment in state. If not, remove the watcher so that the goroutine stops. Fixes: #19988 Co-authored-by: Tim Gross <[email protected]>
tgross
added a commit
that referenced
this pull request
Apr 12, 2024
The test added in #20348 for the deployment watcher was backported to 1.5.x but is hitting a compilation error because the signature of a state store method was changed.
tgross
added a commit
that referenced
this pull request
Apr 12, 2024
The test added in #20348 for the deployment watcher was backported to 1.5.x but is hitting a compilation error because the signature of a state store method was changed.
philrenaud
pushed a commit
that referenced
this pull request
Apr 18, 2024
The deployment watcher on the leader makes blocking queries to detect when the set of active deployments changes. It takes the resulting list of deployments and adds or removes watchers based on whether the deployment is active. But when a job is purged, the deployment will be deleted. This unblocks the query but the query result only shows the remaining deployments. When the query unblocks, ensure that all active watchers have a corresponding deployment in state. If not, remove the watcher so that the goroutine stops. Fixes: #19988
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The deployment watcher on the leader makes blocking queries to detect when the set of active deployments changes. It takes the resulting list of deployments and adds or removes watchers based on whether the deployment is active. But when a job is purged, the deployment will be deleted. This unblocks the query but the query result only shows the remaining deployments.
When the query unblocks, ensure that all active watchers have a corresponding deployment in state. If not, remove the watcher so that the goroutine stops.
Fixes: #19988