Have leader election use namespace from env var #3209
Conversation
| @@ -84,6 +84,7 @@ const ( | |||
| kubeconfigFlag = "kubeconfig" | |||
| allocationBatchWaitTime = "allocation-batch-wait-time" | |||
| defaultResync = 30 * time.Second | |||
| podNamespace = "pod-namespace" | |||
There was a problem hiding this comment.
I would just call this lease-namespace and make it clear it's the namespace to use for the controller lease. The helm chart can make sure it's aligned appropriately.
Strictly speaking, I don't think this needs to be in the same namespace as the pod - I think the controller just needs rights to it and for it to exist, which was the issue, AIUI.
There was a problem hiding this comment.
Would this be confusing since the namespace is the same one as the controller pods? Or should there be another env variable for leader election?
There was a problem hiding this comment.
As a note, this doesn't (and probably shouldn't?) come from helm. We can use the downward API to get this information from K8s directly:
https://kubernetes.io/docs/concepts/workloads/pods/downward-api/
There was a problem hiding this comment.
Just thinking, someone could do a yaml install to a different namespace, and expect it to work. If we use the downward API, this handles that scenario.
There was a problem hiding this comment.
I think it should be already using the downward api, looks very similar from the doc: https://github.com/googleforgames/agones/blob/main/install/helm/agones/templates/controller.yaml#L159-L162 is there a big difference between exposing it as a env variable vs a downward-api volume?
There was a problem hiding this comment.
Oh sweet - I didn't realise it was already there.
If it's already there as an env var, then you are good to go!
There was a problem hiding this comment.
If this is already plumbed into the deployment then I have no problem with the existing name - AIUI you're basically consuming an env variable that we already set here, so I guess my lease-namespace comment is ignorable.
|
Build Succeeded 👏 Build Id: 98a0fa2b-338e-41c2-a9d0-89838eb9883c The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
There was a problem hiding this comment.
Saw a small typo below, but other than that, no issues on my end here. @zmerlynn any on your end?
Other than the typo, this look good to merge?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chiayi, markmandel 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 |
|
Build Succeeded 👏 Build Id: 7e0b8ec4-3a37-4b29-b635-4dccbcaa065e The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
What type of PR is this?
/kind bug
What this PR does / Why we need it:
Currently leader election uses hardcoded namespace, this changes so that it uses the namespace from the env vars
Which issue(s) this PR fixes:
Towards #3208
Special notes for your reviewer: