🐛 Fix runnable race in manager

[abursavich]

In controllerManager, startLeaderElectionRunnables doesn't protect its read of leaderElectionRunnables and write of started. This may cause a Runnable to be run twice:

• startNonLeaderElectionRunnables sets started to true
• Add appends the runnable to leaderElectionRunnables, sees that that started is true, and starts the runnable
• startLeaderElectionRunnables starts all of the runnables in leaderElectionRunnables

See https://travis-ci.org/kubernetes-sigs/controller-runtime/jobs/541066866 for an example.

[abursavich]

I should note that there's another subtle bug with adding leader election runnables, which I noticed when reading this code but this change does not fix.

If a runnable is added after the manager has started, the runnable is unconditionally started. In the case that the runnable should wait for leader election, it will start early (without leadership) and then it will be started again when the manager wins election.

[DirectXMan12]

Thank you for fixing this. Do you mind also fixing the additional bug you mentioned?

[DirectXMan12]

otherwise looks good. I don't suppose you've found a good way of testing this behavior? races are very hard to test

[abursavich]

I would assume that running the existing tests with the race detector would've caught this. Otherwise, no testing ideas.

I can probably get to the other issue in the next few days. I had planned on tackling them separately to keep the diffs smaller and easier to review.

[DirectXMan12]

I can probably get to the other issue in the next few days

Ack. Can you file a separate issue then, just so we don't lose track of it?

I would assume that running the existing tests with the race detector would've caught this

Ah, yeah, need to make sure that's on.

[DirectXMan12]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abursavich, DirectXMan12

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:

• OWNERS [DirectXMan12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment