operator, hive, k8s: don't call workerpool.New from hive constructors #24419
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
tklauser
added
the
release-note/misc
workerpool.New spawns a goroutine [1]. Per the hive guidelines [2], spawning of goroutines must not be performed from constructors, but rather via the Start hook. Fix an occurrence of this anti-pattern in the operator's identity GC constructor. Also add a missing goleak check to TestIdentitiesGC so spawning of goroutines in the constructor will be detected in the future. [1] https://github.com/cilium/workerpool/blob/054fdc524a07dfe6cc4123aba9bd76186605d960/workerpool.go#L68 [2] https://docs.cilium.io/en/latest/contributing/development/hive/#guidelines Fixes: b115951 ("operator: Refactor cilium identities GC to a cell") Signed-off-by: Tobias Klauser <tobias@cilium.io>
workerpool.New spawns a goroutine [1]. Per the hive guidelines [2], spawning of goroutines must not be performed from constructors, but rather via the Start hook. Fix two occurrences of this anti-pattern in hive examples. [1] https://github.com/cilium/workerpool/blob/054fdc524a07dfe6cc4123aba9bd76186605d960/workerpool.go#L68 [2] https://docs.cilium.io/en/latest/contributing/development/hive/#guidelines Signed-off-by: Tobias Klauser <tobias@cilium.io>
tklauser
force-pushed
the
pr/tklauser/hive-workerpool-new-onstart
branch
from
fc9e4c2
to
c93a471
Compare
|
/test |
joamaki
approved these changes
Mar 17, 2023
squeed
approved these changes
Mar 17, 2023
maintainer-s-little-helper
bot
added
ready-to-merge
tklauser
added a commit
that referenced
this pull request
Mar 17, 2023
Add a goleak check the same way as in the daemon's TestAgentCell. Reference: #24419 (review) Suggested-by: Jussi Maki <jussi@isovalent.com> Signed-off-by: Tobias Klauser <tobias@cilium.io>
Sure, it was trivial to add. I've opened #24431 doing that. |
tklauser
added a commit
that referenced
this pull request
Mar 21, 2023
Add a goleak check the same way as in the daemon's TestAgentCell. Reference: #24419 (review) Suggested-by: Jussi Maki <jussi@isovalent.com> Signed-off-by: Tobias Klauser <tobias@cilium.io>
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.
workerpool.Newspawns a goroutine [1]. Per the hive guidelines [2], spawning of goroutines must not be performed from constructors, but rather via the Start hook. Fix three occurrences of this anti-pattern. See commits for details.[1] https://github.com/cilium/workerpool/blob/054fdc524a07dfe6cc4123aba9bd76186605d960/workerpool.go#L68
[2] https://docs.cilium.io/en/latest/contributing/development/hive/#guidelines