Constructing a Broadcaster already starts a watch which runs in the
background. Shutdown must be called to avoid leaking the goroutine. Providing
a context was supposed to remove the need to call Shutdown, but that did not
actually work because the logic for "must check for cancellation" was
accidentally inverted.
While at it, structured log output also gets tested together with checking for
goroutine leaks.
Kubernetes-commit: ff779f1cb56cf896405e52f7923188b99b88bb00
Using StartRecordingToSinkWithContext instead of StartRecordingToSink and
StartLogging instead of StartStructuredLogging has several advantages:
- Spawned goroutines no longer get stuck for extended periods of
time during shutdown when passing in a context that gets canceled.
- Log output can be directed towards a specific logger instead of the global
default, for example one which writes to a testing.T instance.
- The new methods return an error when something went wrong instead of
merely recording the error.
That last point is the reason for deprecating the old methods instead of merely
adding new alternatives.
Setting a context when constructing an EventBroadcaster makes calling Shutdown
optional. It can also be used to specify the logger.
Both EventRecorder interfaces in tools/events and tools/record now have a
WithLogger helper. Using that method is optional, but recommended to support
contextual logging properly. Without it, errors that occur while emitting an
event are not associated with the caller.
Kubernetes-commit: 27a68aee3a48340f7c14235f7fc24aa69aaeb8f6
When Shutdown was called, delivery of each pending event would still be retried
12 times with a delay of ~10s between each retry. In apiserver integration
tests that caused the goroutine to linger long after the corresponding
apiserver of the test was shut down.
Kubernetes-commit: 15b01af9c18a0840d71e2bb7dff4d8c29b158aad
- Introduce PassiveRateLimiter which implements all methods of previous RateLimiter except Accept() and Wait()
- Change RateLimiter interface to extend PassiveRateLimiter by additionally implementing Accept() and Wait()
- Make client-go/tools/record use PassiveRateLimiter
Refactor EventSourceObjectSpamFilter, EventAggregator, EventCorrelator
- EventSourceObjectSpamFilter, EventAggregator, EventCorrelator use clock.PassiveClock now.
- This won't be a breaking change because even if a clock.Clock is passed, it still implements the clock.PassiveClock interface.
- Extend clock.PassiveClock through Clock.
- Replace pacakge local implementation of realClock with clock.RealClock
- In flowcontrol/throttle.go split tokenBucketRateLimiters to use Clock and clock.PassiveClock.
- Migrate client-go/tools/record tests from using IntervalClock to using SimpleIntervalClock (honest implementation of clock.PassiveClock)
Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>
Kubernetes-commit: ac5c55f0bd853fcf883d9b8e1f5ef728a2fb5309
This changes the event recorder to use the equivalent of a select
statement instead of a goroutine to record events.
Previously, we used a goroutine to make event recording non-blocking.
Unfortunately, this writes to a channel, and during shutdown we then
race to write to a closed channel, panicing (caught by the error
handler, but still) and making the race detector unhappy.
Instead, we now use the select statement to make event emitting
non-blocking, and if we'd block, we just drop the event. We already
drop events if a particular sink is overloaded, so this just moves the
incoming event queue to match that behavior (and makes the incoming
event queue much longer).
This means that, if the user uses `Eventf` and friends correctly (i.e.
ensure they've returned by the time we call `Shutdown`), it's
now safe to call Shutdown. This matches the conventional go guidance on
channels: the writer should call close.
Kubernetes-commit: e90e67bd002e70a525d3ee9045b213a5d826074d
