-
Notifications
You must be signed in to change notification settings - Fork 36
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
chore: replicas using values.yaml + enable leader election #501
Conversation
…tion Signed-off-by: Rajat Vig <[email protected]>
@@ -42,6 +43,7 @@ spec: | |||
- -logLevel={{ .Values.controller.logLevel }} | |||
- --extProcImage={{ .Values.extProc.repository }}:{{ .Values.extProc.tag | default .Chart.AppVersion }} | |||
- --extProcLogLevel={{ .Values.extProc.logLevel }} | |||
- --enableLeaderElection=true |
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.
Can we make it configurable ?
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.
That is possible though with > 1 replica, having leader election is required. Do you think it would be better to have that as a conditional here, i.e., always turn it on if there is > 1 replica but not add it if there is only 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 it won't hurt to enable this by default ?
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.
is this for the ai-controller ? then you'd have to do extra work in code, to detect the leader election and based on that, only allow the leader to generate the Gateway API resources
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.
yeah controller and i believe it's already handled assuming leader election = true (i don't remember precisely;)), and the current default is true:
ai-gateway/cmd/controller/main.go
Line 50 in f3ab8bb
true, |
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.
Would I be correct in assuming then that the change is safe to do and this pull request looks fine?
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.
yeah I think so, but deferring to @yuzisun for the final approval
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.
@rajatvig You can set the default to true in values.yaml, otherwise there is no way to disable it if needed.
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.
Have changed the code per suggestion.
Commit Message
Set deployment replicas based off the
values.yaml
and enable leader election for the controller.Signed-off-by: Rajat Vig [email protected]