-
Notifications
You must be signed in to change notification settings - Fork 825
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 leader election feature to agones-controller
#3025
Conversation
Build Failed 😱 Build Id: b57b23a3-75d0-433a-80d7-eed5198019e8 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
cmd/controller/main.go
Outdated
@@ -433,6 +521,22 @@ type httpServer struct { | |||
http.ServeMux | |||
} | |||
|
|||
func buildConfig(kubeconfig string) (*rest.Config, error) { |
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.
figure out if we need this? we presumably construct a client elsewhere and that would need similar logic?
Build Failed 😱 Build Id: 89d1e47b-0f4a-4993-80be-83f85012b9ea To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
c91457f
to
ae2a5c3
Compare
Build Failed 😱 Build Id: d27d4de6-044f-40ab-9b72-cee323a97b6c To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 534f5263-129f-4fac-a5b9-bee19e0fa7f4 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 85cefdce-d12c-47a8-b4d0-4ac9670e0aac To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 6aed74b5-9b8a-4558-90aa-9ee2fe165470 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 12fc2ca5-599a-4ea1-ae04-56c015edb82a To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
install/helm/agones/values.yaml
Outdated
@@ -94,6 +94,7 @@ agones: | |||
failureThreshold: 3 | |||
timeoutSeconds: 1 | |||
allocationBatchWaitTime: 500ms | |||
replicas: 1 |
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 think we should derive this somehow from the feature gate status, but allow it to be forced one way or the other. Here's my thought:
We add a boolean --leader-elected
flag to controller to enable/disable leader election. This acts as the ultimate chicken-switch.
This is not a typed structure, so we could have "replicas: derived-from-feature-gate" here, and then later, we apply the following logic:
- If replicas is not an int:
- If
SplitControllerAndExtensions
is enabled:- derive replicas as 2
- Else (
SplitControllerAndExtensions
is disabled):- derive replicas as 1
- If
- Else (replicas is an int)
- replicas is the value set
- if
SplitControllerAndExtensions
is not enabled:- validate replicas == 1
Then, using the above logic, if derived replicas is >1, we enable --leader-election
and enable the PDB.
If you want to add a function for derived replicas, you can define it in helpers and use it elsewhere (it's a little fussy because it effectively defines a template, but it works). Example:
agones/install/helm/agones/templates/_helpers.tpl
Lines 25 to 41 in 5ddbe60
{{/* | |
Create a default fully qualified app name. | |
We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec). | |
If release name contains chart name it will be used as a full name. | |
*/}} | |
{{- define "agones.fullname" -}} | |
{{- if .Values.fullnameOverride -}} | |
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}} | |
{{- else -}} | |
{{- $name := default .Chart.Name .Values.nameOverride -}} | |
{{- if contains $name .Release.Name -}} | |
{{- .Release.Name | trunc 63 | trimSuffix "-" -}} | |
{{- else -}} | |
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}} | |
{{- end -}} | |
{{- end -}} | |
{{- end -}} |
Build Failed 😱 Build Id: 00cf887c-a8fe-4700-9f7d-a28e239cac92 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: c5fc03ce-c25a-4f7e-a75f-1edc2d4c59d7 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! Mostly just nits this time around.
Build Succeeded 👏 Build Id: e8cfd135-a2db-4ef9-96f8-5a2af7c0f88b 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: f2eca970-b964-41db-990b-3862be55af9c 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:
|
logger.Info("Shut down agones controllers") | ||
<-ctx.Done() | ||
logger.Info("Shut down agones controllers") | ||
}) |
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.
Nice, exactly what I was hoping to see.
[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 |
62d1bf2
to
a506de5
Compare
New changes are detected. LGTM label has been removed. |
Build Failed 😱 Build Id: 5d5b3027-cb86-4b73-8786-4635c7389169 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
Build Failed 😱 Build Id: 2b5a9971-8fa4-45eb-9887-3f4b49c4545f To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: b0ff3cf4-4f6c-4dd9-9913-6f85d1763310 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 --leader-election flag, which enabled leader election amongst controller replicas. Helm changes to follow.
WIP PR.
What type of PR is this?
/kind feature
What this PR does / Why we need it:
Leader Election feature for agones-controller
Which issue(s) this PR fixes:
Work on #2797
Special notes for your reviewer: