馃悰 Fix a race condition between leader election and recorder #1379
Conversation
This change introduces better syncronization between the leader election code and the event recorder. Running tests with -race flag, we often saw a panic on a closed channel, the channel was the one that the event recorder was using internally. After digging more through the code, it seems that we weren't properly waiting for leader election code to stop completely, but instead we were only calling the cancel() function asking the leader election to stop. With this change, during a shutdown, we now wait for leader election to finish up any internal task before we return and close an internal channel. Only after leader election signals that the channel has been closed, and Run(...) has properly returned, we return execution to the stop procedure, where the event recorder is then stopped. Signed-off-by: Vince Prignano <vincepri@vmware.com>
k8s-ci-robot
added
the
cncf-cla: yes
|
/milestone v0.9.0 |
|
@vincepri: The provided milestone is not valid for this repository. Milestones in this repository: [ Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/milestone v0.9.x |
k8s-ci-robot
added
approved
|
Nice, this looks good to me, I will wait for others to chime in. |
| cm.leaderElectionCancel() | ||
| <-cm.leaderElectionStopped |
There was a problem hiding this comment.
One concern (Probably more about the unconditional leaderElectionCancel() call than your change):
If we attempt to shut down gracefully and the Runnables do not end in time, they will end up running after we are not elected leader anymore (since we now block until that happens).
Maybe cancel the leader election only if the rest of the func has a nil error or an error that is not context.Cancelled?
There was a problem hiding this comment.
You mean checking the return err and only if it's not nil, cancel leader election? The error comes from shutdownCtx, if we don't cancel leader election, I'd assume that it'll timeout after a bit and the main.go should exit, right?
There was a problem hiding this comment.
Trying to double check the thinking, I'd also like to capture in a comment around the err check
There was a problem hiding this comment.
only if its nil, cancel leader election, if its not-nil, that means something is still running and its unsafe to cancel leader election because that might result in the thing still running after we are not leader anymore.
This should result in the binary exiting which exists all pending routines and the leader lease of us will just time out.
Signed-off-by: Vince Prignano <vincepri@vmware.com>
|
@alvaroaleman ptal |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, vincepri 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 |
This change introduces better syncronization between the leader election
code and the event recorder. Running tests with -race flag, we often saw
a panic on a closed channel, the channel was the one that the event
recorder was using internally.
After digging more through the code, it seems that we weren't properly
waiting for leader election code to stop completely, but instead we were
only calling the cancel() function asking the leader election to stop.
With this change, during a shutdown, we now wait for leader election to
finish up any internal task before we return and close an internal
channel. Only after leader election signals that the channel has been
closed, and Run(...) has properly returned, we return execution to the
stop procedure, where the event recorder is then stopped.
Signed-off-by: Vince Prignano vincepri@vmware.com