client-go: support waiting for SharedInformerFactory shutdown #112200
Conversation
k8s-ci-robot
added
release-note
There was a problem hiding this comment.
looks great so far. Would like to see the semantics for WaitForShutdown clarified a bit more in the documentation.
I'm also wondering what the best place would be to add tests for this. I'm not seeing any existing tests for the imformer-gen code. Hopefully an approver can comment about that.
staging/src/k8s.io/code-generator/cmd/informer-gen/generators/factory.go
Outdated
Show resolved
Hide resolved
| for informerType, informer := range f.informers { | ||
| if !f.startedInformers[informerType] { | ||
| f.wg.Add(1) | ||
| informer := informer |
There was a problem hiding this comment.
nit -- this line surprised me. did the compiler yell
There was a problem hiding this comment.
No, it's worse: without it, when the goroutines run, they use the current value of the loop variable, typically the last entry in the slice.
This idiom creates a new variable for each loop iteration. I can add a comment explaining it.
There was a problem hiding this comment.
shucks thats unfortunate. maybe a brief line about it yeah
There was a problem hiding this comment.
This is a common go gotcha-- I had to think for a moment to see why the old version of the code didn't have this bug.
staging/src/k8s.io/code-generator/cmd/informer-gen/generators/factory.go
Outdated
Show resolved
Hide resolved
pohly
force-pushed
the
client-go-shared-informer-factory-shutdown
branch
from
64834e9
to
5ffd088
Compare
k8s-ci-robot
added
size/XL
|
@alexzielenski : please take another look. I prefer to keep the PR always mergeable, therefore I updated the commit. Changes since initial review: https://github.com/kubernetes/kubernetes/compare/64834e96aa00e041717c110e9ff051b1f8ddf482..5ffd0887ed858e01d106074314b759695afffe3e |
|
/retest |
|
/cc @lavalamp |
k8s-ci-robot
added
triage/accepted
There was a problem hiding this comment.
nit about the doc comment
otherwise looks good to me. ill hold off on proper /lgtm until someone more acquainted takes a look.
xref: https://kubernetes.slack.com/archives/C0EG7JC6T/p1662030952291459
staging/src/k8s.io/code-generator/cmd/informer-gen/generators/factory.go
Outdated
Show resolved
Hide resolved
|
/assign @lavalamp |
pohly
force-pushed
the
client-go-shared-informer-factory-shutdown
branch
3 times, most recently
from
29cd3f6
to
91071d8
Compare
k8s-ci-robot
added
size/XXL
staging/src/k8s.io/code-generator/cmd/informer-gen/generators/factory.go
Show resolved
Hide resolved
staging/src/k8s.io/code-generator/cmd/informer-gen/generators/factory.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/code-generator/cmd/informer-gen/generators/factory.go
Outdated
Show resolved
Hide resolved
| func (f *sharedInformerFactory) Shutdown() { | ||
| f.lock.Lock() | ||
| f.shuttingDown = true | ||
| f.lock.Unlock() |
There was a problem hiding this comment.
it really doesn't matter here, but for more complex things I usually ask people to put it in an anonymous function so that defer can be used to unlock.
|
Thanks! /approve your choice about which nits to fix |
k8s-ci-robot
added
the
approved
SharedInformerFactory starts goroutines in Start and those can be stopped by closing the stop channel. However, there was no API that waits for the goroutines. This is a problem for unit testing. A test has to return while the informers are still running, which may get flagged by tools like https://github.com/uber-go/goleak or by klog/ktesting when those informers lead to log output. While at it, more documentation gets added to address kubernetes#65036.
pohly
force-pushed
the
client-go-shared-informer-factory-shutdown
branch
from
7487bc1
to
e89d1d4
Compare
|
I took your documentation updates but left the Lock/Unlock as it was. I think it's simple enough to not need a separate function. |
|
@lavalamp: do you want to add the LGTM yourself or hand it back to @alexzielenski? |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexzielenski, lavalamp, pohly The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
Are there any plans to add this for metadata SharedInformerFactory and dynamic DynamicSharedInformerFactory? |
|
No plans, but if someone (you?) were to prepare a PR, it probably would get merged. |
Followup to kubernetes#112200, specifically kubernetes#112200 (comment).
…114434) * client-go: support `Shutdown()` for metadata and dynamic informers Followup to #112200, specifically #112200 (comment). * add comments * Defer lock
…114434) * client-go: support `Shutdown()` for metadata and dynamic informers Followup to kubernetes/kubernetes#112200, specifically kubernetes/kubernetes#112200 (comment). * add comments * Defer lock Kubernetes-commit: b99fe0d5b9896dd3fe9a2c1bc3b399a18ad080d2
…ubernetes#114434) * client-go: support `Shutdown()` for metadata and dynamic informers Followup to kubernetes#112200, specifically kubernetes#112200 (comment). * add comments * Defer lock
What type of PR is this?
/kind feature
What this PR does / why we need it:
SharedInformerFactory starts goroutines in Start and those can be stopped by
closing the stop channel. However, there was no API that waits for the
goroutines.
This is a problem for unit testing. A test has to return while the informers
are still running, which may get flagged by tools like
https://github.com/uber-go/goleak or by klog/ktesting when those informers
lead to log output.
While at it, more documentation gets added to address
#65036.
Which issue(s) this PR fixes:
Fixes #65036 (as confirmed in #65036 (comment))
Special notes for your reviewer:
Does this PR introduce a user-facing change?
/cc @alexzielenski