-
Notifications
You must be signed in to change notification settings - Fork 537
fix: disable leader election for single replica deployment #2767
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -8,7 +8,7 @@ metadata: | |||||
| app.kubernetes.io/component: "gpu-operator" | ||||||
| nvidia.com/gpu-driver-upgrade-drain.skip: "true" | ||||||
| spec: | ||||||
| replicas: 1 | ||||||
| replicas: {{ .Values.operator.replicas | default 1 }} | ||||||
| selector: | ||||||
| matchLabels: | ||||||
| app.kubernetes.io/component: "gpu-operator" | ||||||
|
|
@@ -39,7 +39,9 @@ spec: | |||||
| imagePullPolicy: {{ .Values.operator.imagePullPolicy }} | ||||||
| command: ["gpu-operator"] | ||||||
| args: | ||||||
| {{- if gt (int (.Values.operator.replicas | default 1)) 1 }} | ||||||
| - --leader-elect | ||||||
| {{- end }} | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens when two gpu-operator controllers come up at same time during upgrade (old controller and new controller)?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my opinion leader election is critical under multiple replicas deployed. With a single replica and a very brief overlap period, acting without a leader seems fairly safe. The tests I performed resulted in:
Basically the two pods were briefly trying to do the same idempotent work and the old pod was terminated in a 10-15 second-window. I replicated the idea implemented by NVIDIA in Mellanox/network-operator#2304 but the network operator AI agent bot also raised your concern in my identical PR in the NNO, see Mellanox/network-operator#3056
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Concurrent reconciles by different versions of the operator are almost never a good thing, even if they are idempotent in theory. It can be a source of unexpected bugs and we are sensitive to unnecessary daemonset rollouts (especially triggering unnecessary driver upgrades). So this change does not look safe to me. Instead of disabling leader election, how about setting a larger gpu-operator/bundle/manifests/gpu-operator-certified.clusterserviceversion.yaml Lines 1081 to 1082 in 10ee5b3
|
||||||
| {{- if .Values.operator.logging.develMode }} | ||||||
| - --zap-devel | ||||||
| {{- else }} | ||||||
|
|
||||||
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.
This also needs to be added to values.yaml (maybe commented) so that users can find this.