Skip to content

fix: make the leader lease renew deadline configurable - #3256

Merged
rollandf merged 1 commit into
Mellanox:masterfrom
stav-inbar:leader_lease
Oct 6, 2026
Merged

rollandf merged 1 commit into
Mellanox:masterfrom
stav-inbar:leader_lease

Conversation

@stav-inbar

Copy link
Copy Markdown
Collaborator

A brief control-plane outage was enough to kill the operator. With the controller-runtime default renew deadline of 10s, a kube-apiserver restart that refused connections for a few seconds left the leader unable to renew its lease, so the manager returned "leader election lost", main exited, and the single-replica Deployment went into CrashLoopBackOff. Each restart then had to wait out the stale lease before reconciling again, delaying NicClusterPolicy by minutes for an outage that lasted seconds.

Add --leader-lease-renew-deadline so the operator can ride out an outage instead of standing down, and default it to 60s in both the Helm chart and the kustomize manifests. Leader election requires the lease duration to exceed the renew deadline or it refuses to start, so the lease duration is derived as the deadline plus 5s rather than exposed as a second flag that could be set to an invalid combination. Leaving the flag unset keeps the controller-runtime defaults, which is what happens on any deployment that does not pass it. Helm users can override it through operator.leaderElection.renewDeadline.

Note the trade-off this makes: a leader that dies without releasing its lease is now replaced after about 65s instead of about 15s, so operator restarts and upgrades pause reconciliation for that long. That is the cost of not treating a transient API error as fatal.

Fixes #3133

@copy-pr-bot

copy-pr-bot Bot commented Sep 17, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@greptile-apps

greptile-apps Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[High risk] Adds configurable leader election timing to the operator.

The PR appears safe to merge; no outstanding finding or new actionable issue remains.

Summary

The PR makes the leader lease renew deadline configurable, defaults it to 60s in Helm and Kustomize, and derives a lease duration five seconds longer. It also rejects deadlines below 3s before creating the manager.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  V[Helm value or Kustomize argument] --> F[Renew deadline flag]
  F --> O[Manager options]
  O --> R[Renew deadline]
  O --> L[Lease duration: deadline + 5s]
Loading

Reviews (3) · Last reviewed commit: "fix: make the leader lease renew deadlin..."

Comment thread config/manager/manager.yaml
Comment thread main.go Outdated
@rollandf

Copy link
Copy Markdown
Member

@tginer PTAL

@rollandf

Copy link
Copy Markdown
Member

/retest-all

@heyvister1

Copy link
Copy Markdown
Collaborator

/retest-nic_operator_kind

4 similar comments
@heyvister1

Copy link
Copy Markdown
Collaborator

/retest-nic_operator_kind

@heyvister1

Copy link
Copy Markdown
Collaborator

/retest-nic_operator_kind

@heyvister1

Copy link
Copy Markdown
Collaborator

/retest-nic_operator_kind

@heyvister1

Copy link
Copy Markdown
Collaborator

/retest-nic_operator_kind

@stav-inbar

Copy link
Copy Markdown
Collaborator Author

/retest-nic_operator_helm

1 similar comment
@stav-inbar

Copy link
Copy Markdown
Collaborator Author

/retest-nic_operator_helm

@rollandf rollandf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thanks!

@almaslennikov

Copy link
Copy Markdown
Collaborator

@stav-inbar please rebase

@rollandf
rollandf requested a review from almaslennikov October 6, 2026 08:43
A brief control-plane outage was enough to kill the operator. With the
controller-runtime default renew deadline of 10s, a kube-apiserver restart
that refused connections for a few seconds left the leader unable to renew
its lease, so the manager returned "leader election lost", main exited, and
the single-replica Deployment went into CrashLoopBackOff. Each restart then
had to wait out the stale lease before reconciling again, delaying
NicClusterPolicy by minutes for an outage that lasted seconds.
Add --leader-lease-renew-deadline so the operator can ride out an outage
instead of standing down, and default it to 60s in both the Helm chart and
the kustomize manifests. Both kustomize paths have to carry the flag: the
default overlay replaces the manager's argument list rather than merging
into it, so setting it only in the base left `make deploy` on the 10s
default. Leader election requires the lease duration to exceed the renew
deadline or it refuses to start, so the lease duration is derived as the
deadline plus 5s rather than exposed as a second flag that could be set to
an invalid combination. Leaving the flag unset keeps the controller-runtime
defaults, which is what happens on any deployment that does not pass it.
Helm users can override it through operator.leaderElection.renewDeadline.
That same validation also requires the deadline to exceed the retry period
scaled by a jitter factor, 2.4s with the defaults in use, and it runs when
the elector is built rather than when the manager is created. A value below
it therefore let the operator come up and only then exit, which is the crash
loop this flag exists to prevent, so the deadline is now rejected below 3s
before the manager is built.
Registering the command line flags moved into its own function, and the
manager configuration is passed around as a struct, because the added check
put main over the statement limit. Neither changes behavior.
Note the trade-off this makes: a leader that dies without releasing its
lease is now replaced after about 65s instead of about 15s, so operator
restarts and upgrades pause reconciliation for that long. That is the cost
of not treating a transient API error as fatal.

Fixes Mellanox#3133

Signed-off-by: Stav Inbar <sinbar@nvidia.com>
@rollandf
rollandf merged commit 8e2188a into Mellanox:master Oct 6, 2026
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fine tuning leader election parameters

4 participants