lets controllerManager.start() return instead of blocking

[mhrivnak]

It always gets called as a goroutine, so nothing expects it to block. This
change slightly simplifies the code and lets the goroutine die instead of
keeping it around forever.

[mhrivnak]

The diff looks more complex than it is. It just:

1. removes the select from the bottom
2. removes the inner func() block

Everything else is untouched except moving the indentation left.

When viewing it, if you click "Diff settings" and select "Hide whitespace changes", you'll see there's not much to it.

[droot]

Looks good to me. Would like @JoelSpeed to take a look just to ensure this change plays nicely with the leaderElection code that we have in manager.Start().

[droot]

Another thing I noticed that while using leader election, leaderElector passes its own stop channel to cm.start, so not sure if that leads to unexpected behavior. Also, the code for leader election in cm.Start sort of looks complex with multiple go routines and select blocks. /cc @JoelSpeed @mhrivnak @DirectXMan12

[JoelSpeed]

LGTM

Would like @JoelSpeed to take a look just to ensure this change plays nicely with the leaderElection code that we have in manager.Start()

This change shouldn't affect the behaviour of leader election in either client-go 8 or 9 (checking for futures sake). In both the OnStartedLeading is just started in a go-routine and the code doesn't care whether it blocks or not.

Another thing I noticed that while using leader election, leaderElector passes its own stop channel to cm.start, so not sure if that leads to unexpected behavior.

I don't think this does. When the leader elector loses leader it closes the channel and sends an error which in turn causes cm.Start to return the error, exiting the controller. If stop passed to cm.Start is closed then the function returns as well in theory closing the controller, we don't at this point then close the leader electors stop channel which I guess could cause problems, this could be avoided with a little refactor though. Want me to do that and make sure we ignore the leader elector's stop channel?

Also, the code for leader election in cm.Start sort of looks complex with multiple go routines and select blocks.

Yeah it's not pretty, could probably be made a lot nicer if I'd just used an else for the leader elector stuff, I'll raise a PR once to tidy it up a bit

[droot]

I don't think this does. When the leader elector loses leader it closes the channel and sends an error which in turn causes cm.Start to return the error, exiting the controller. If stop passed to cm.Start is closed then the function returns as well in theory closing the controller, we don't at this point then close the leader electors stop channel which I guess could cause problems, this could be avoided with a little refactor though. Want me to do that and make sure we ignore the leader elector's stop channel?

Took a quick look a the leader elector lib, ignoring the leader elector's stop channel is fine as long as we exit when we stop leading (which we do as you explained above). I saw you made another PR, will take a look.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: mhrivnak

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

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