New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add helm setting for leader election #3051
Conversation
Build Failed 😱 Build Id: aca33ce0-c17a-4671-a2d4-5f5585500dc0 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Derives the replica number for agones-controller based on current helm and feature flag settings. | ||
*/}} | ||
{{- define "agones.controller.replica" -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot this was landing all in controller.yaml
, any reason we can't just inline the logic rather than adding the helper? I think if you inline it at e.g. line 16 in controller.yaml, you don't even need a define
, it just becomes a matter of setting $controllerReplicas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it to inline, please take a look.
@@ -13,6 +13,7 @@ | |||
# limitations under the License. | |||
|
|||
{{- $featureGates := include "agones.featureGates" . | fromYaml }} | |||
{{- $controllerReplica := include "agones.controller.replica" . }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would call this controllerReplicas
(plural), or just replicas
. You're in controller.yaml
, the controller
prefix is kind of redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, changed to just replicas
Build Failed 😱 Build Id: 30d8ce46-ff70-492d-927f-28500727646d To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really close!
@@ -13,6 +13,10 @@ | |||
# limitations under the License. | |||
|
|||
{{- $featureGates := include "agones.featureGates" . | fromYaml }} | |||
{{- $replicas := .Values.agones.controller.replicas }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we talked offline about just passing this through, but since you rely on this being an int (which I wasn't thinking about during standup), you probably need to cast it here - and maybe validate it's > 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to ensure that replicas is an int here? It would fail the gt
checks if it isn't an int.
But I guess that would mean relying on the helm check to give an typing error instead of us stating the error earlier. This also applies to validating if replicas
is greater than 0, it will also give an typing error when comparing with a 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the build break you just had was because it wasn't cast to an int already - that's why I was suggesting it here (it probably comes in as string?) Maybe I'm wrong. Whichever way works - happy if helm validates it and we can skip code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see I see. You are correct, there is a need to cast to int. This also means that we would need to check if values is 0 once it's casted. What is the behavior we want when the value is set to 0? Do we want it to default to 1?
Build Failed 😱 Build Id: de679343-bf8f-4bf5-8930-e74f304c30fe To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
2b07577
to
b0a5e7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Fix some nits, take off the WIP and I'll get it merged.
Build Failed 😱 Build Id: a82c6dc9-21ed-4e47-9af3-cdb38f9c4956 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 98393d84-ebed-4e59-be6e-54c75d552448 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Your code LGTM, now just get it past the @agones-bot guantlet. |
Build Failed 😱 Build Id: 01e92e44-3ca1-4f84-9c59-a35173b13ad2 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 531063f6-822d-41d7-8883-a21c9a5a9576 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: cf376173-59b9-4d00-a477-7a4e71158fd0 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build failed due to a different error than prev build:
|
Build Failed 😱 Build Id: 9684ff40-2fed-4068-a28f-7bbbcb920136 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Removed |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chiayi, zmerlynn The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Build Failed 😱 Build Id: 778e84ad-d139-4e49-8290-271d56aee164 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Head branch was pushed to by a user without write access
New changes are detected. LGTM label has been removed. |
Build Failed 😱 Build Id: 6126f0bc-347c-41b3-adbf-9136006a4c56 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 4a14afa0-98aa-4588-9d3e-d077b5929dc8 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 0292aa4b-1c27-45c6-a056-726772cea240 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: eb3493d2-5274-4654-bbf1-8b3ab53e550e To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 0d17de03-dac8-4156-a3bb-0890a0339f5e 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:
|
Build Succeeded 👏 Build Id: aeadbc83-eced-40cf-bb03-ec454a39a3d6 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:
|
* Adds `replicas` and `pdb` settings to `agones.controller` - both are only relevant when `SplitControllerAndExtensions` is enabled. * Derives `--leader-election` and PDB presence from the `replicas` setting: Leader election can be disabled by setting `replicas: 1`.
What type of PR is this?
/kind feature
What this PR does / Why we need it:
This PR adds helm config for controller leader election
Which issue(s) this PR fixes:
Work on #2797
Special notes for your reviewer: