-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
operator, hive, k8s: don't call workerpool.New from hive constructors #24419
Conversation
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>
fc9e4c2
to
c93a471
Compare
/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.
Awesome thanks! Looks like it'd be good to extend operator/cmd/root_test.go
with the goleak check the same way as in daemon/cmd/cells_test.go
. Would you mind seeing if it'd be trivial to add it?
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. |
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>
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 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