allow leader election to accept namespace

[jmrodri]

• remove isRunLocal()
• change errNoNamespace value, it now returns
  "Namespace is required when running outside the cluster"
• update tests

[joelanford]

If the POD_NAME environment variable is not set, I think we could use this code. That way, we would no longer require operators to set the POD_NAME environment variable using the downward API in their deployment (as long as they don't override hostname in their pod spec).

This behavior would need to be documented.

[joelanford]

WDYT of duplicating the leaderelection.Options struct from controller-runtime and using that as a parameter to leader.Become()

Suggested change

	func Become(ctx context.Context, lockName string, ns string) error {
	func Become(ctx context.Context, opts Options) error {

[estroz]

Nit: have this take a path to make it testable (without resetting a global in tests)

Suggested change

	func getOperatorNamespace() (string, error) {
	func getOperatorNamespace(namespacePath string) (string, error) {

[mhrivnak]

This approach to leader election depends on a "lease" resource, implemented with a ConfigMap, having an owner reference to the Pod that the operator is running in. If the Pod goes away, the lease automatically goes with it, and thus someone else can become the leader.

But given that, I don't see how this is going to work if the operator itself is either running outside the cluster, or running in a different namespace than where the lease is. Mind elaborating on the goal and how it's going to work?

[joelanford]

@mhrivnak Yeah, that's totally right. I had lost track of that constraint. Given that, it doesn't make sense to accept an explicit namespace since this will only ever work in-cluster, and owner references don't work across namespace boundaries.

Do you think it's reasonable to use the pod hostname if POD_NAME isn't set (as long as we document that the pod spec hostname value should not be overridden)?

Or should we keep the hard requirement on POD_NAME?

[coveralls]

Pull Request Test Coverage Report for Build 178610974

• 3 of 7 (42.86%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-57.6%) to 42.396%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
leader/leader.go	3	7	42.86%

Totals
Change from base Build 178609315:	-57.6%
Covered Lines:	92
Relevant Lines:	217

---

💛 - Coveralls

[mhrivnak]

Do you think it's reasonable to use the pod hostname if POD_NAME isn't set (as long as we document that the pod spec hostname value should not be overridden)?

Or should we keep the hard requirement on POD_NAME?

My first implementation used the pod hostname, but that was changed after it was pointed out as not being a reliable way to identify the pod name. Based on the comments, it sounds like the hostname should be avoided.

[jmrodri]

Based on the comments above, it sounds like it would be a bad idea to accept a namespace parameter. Closing this PR.