OCPBUGS-100060: gate etcd member restarts on quorum and control plane node state - #1663
OCPBUGS-100060: gate etcd member restarts on quorum and control plane node state#1663mkowalski wants to merge 1 commit into
Conversation
The static pod installer restarts the etcd member on a node when rolling out
a new revision. The quorum checks gate only revision creation, so an
installer pod could restart a member while another control plane node was
simultaneously down for a machine-config reboot: the rebooting node keeps
reporting Ready until it actually goes down, and the cached member health
(60s TTL) can also be stale. With 2 of 3 members down the cluster is left
without an etcd leader for ~2 minutes and the whole API becomes unavailable
(OCPBUGS-100060).
Wire the new library-go installer precondition to a member restart safety
check: before creating an installer pod for a node, require that
* fresh (non-cached) member health reports the cluster quorum fault
tolerant, and
* no control plane node other than the target is cordoned or not ready -
the cordon that precedes an MCO drain/reboot is visible minutes before
the node goes down, closing the race that member health alone cannot.
Existing topology exemptions are preserved: unsafe/single-node skips the
check entirely and Two Node with Fencing keeps its fault tolerance
exception, consistent with CheckSafeToScaleCluster.
The vendored library-go change (WithInstallerPrecondition) is included ad
interim and will be replaced by a proper bump once
openshift/library-go#2387 merges.
Assisted-By: Claude Fable 5
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@mkowalski: This pull request references Jira Issue OCPBUGS-100060, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe quorum checker gains per-node restart-safety evaluation using scaling strategy, etcd health, quorum tolerance, and control plane node status. Operator startup wires this check into static pod installation using a non-cached etcd client. ChangesMember restart safety
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant StaticPodInstaller
participant QuorumCheck
participant EtcdClient
participant NodeLister
StaticPodInstaller->>QuorumCheck: Check target node restart safety
QuorumCheck->>EtcdClient: Evaluate member health and quorum tolerance
QuorumCheck->>NodeLister: List control plane nodes
QuorumCheck-->>StaticPodInstaller: Return safety decision and reason
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ipi-upgrade-ovn-ipv6 10 |
|
@mkowalski: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/f6e49270-8b52-11f1-8a66-3fe54ab832c2-0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/ceohelpers/quorum_check.go`:
- Around line 61-102: Update IsSafeToRestartMember after nodeLister.List to
reject an empty node list instead of falling through to the safe result. Return
an unsafe result with a clear explanation that no control-plane nodes were
found, while preserving the existing error handling and per-node
cordon/readiness checks for non-empty lists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 00ce89df-3d1e-4142-aa30-a77a5754ebe9
⛔ Files ignored due to path filters (2)
vendor/github.com/openshift/library-go/pkg/operator/staticpod/controller/installer/installer_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/staticpod/controllers.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (3)
pkg/operator/ceohelpers/quorum_check.gopkg/operator/ceohelpers/quorum_check_test.gopkg/operator/starter.go
| func (c *QuorumCheck) IsSafeToRestartMember(ctx context.Context, targetNodeName string) (bool, string, error) { | ||
| scalingStrategy, err := GetBootstrapScalingStrategy(c.operatorClient, c.namespaceLister, c.infraLister) | ||
| if err != nil { | ||
| return false, "", fmt.Errorf("IsSafeToRestartMember failed to get bootstrap scaling strategy: %w", err) | ||
| } | ||
| if scalingStrategy == UnsafeScalingStrategy { | ||
| return true, "", nil | ||
| } | ||
|
|
||
| // the cluster must currently tolerate the loss of one member | ||
| memberHealth, err := c.etcdClient.MemberHealth(ctx) | ||
| if err != nil { | ||
| return false, "", fmt.Errorf("IsSafeToRestartMember couldn't determine member health: %w", err) | ||
| } | ||
| // Two Node OpenShift with Fencing protects etcd via pacemaker; treat it as an exception to the fault | ||
| // tolerance rule, consistent with CheckSafeToScaleCluster. | ||
| if err := etcdcli.IsQuorumFaultTolerantErr(memberHealth); err != nil && | ||
| !(len(memberHealth) == 2 && (scalingStrategy == TwoNodeScalingStrategy || scalingStrategy == DelayedTwoNodeScalingStrategy)) { | ||
| return false, err.Error(), nil | ||
| } | ||
|
|
||
| // no control plane node other than the target may be cordoned or not ready. Member health alone is not | ||
| // enough: a node that is about to reboot keeps reporting healthy until it actually goes down, while the | ||
| // cordon that precedes its drain is visible minutes in advance. | ||
| nodes, err := c.nodeLister.List(labels.Everything()) | ||
| if err != nil { | ||
| return false, "", fmt.Errorf("IsSafeToRestartMember failed to list control plane nodes: %w", err) | ||
| } | ||
| for _, node := range nodes { | ||
| if node.Name == targetNodeName { | ||
| continue | ||
| } | ||
| if node.Spec.Unschedulable { | ||
| return false, fmt.Sprintf("control plane node %q is cordoned, likely about to be drained and rebooted; restarting the etcd member on %q now could lose quorum", node.Name, targetNodeName), nil | ||
| } | ||
| if !isNodeReady(node) { | ||
| return false, fmt.Sprintf("control plane node %q is not ready; restarting the etcd member on %q now could lose quorum", node.Name, targetNodeName), nil | ||
| } | ||
| } | ||
|
|
||
| return true, "", nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Empty node list silently treated as "safe" — fail-open gap.
If c.nodeLister.List(labels.Everything()) returns zero nodes (e.g., the control-plane node informer hasn't synced yet right after operator restart, or a lister/selector misconfiguration), the loop at Line 89 never executes and the function falls through to return true, "", nil at Line 101. That silently approves a restart with zero visibility into other control-plane nodes' state — precisely the failure mode (stale/missing state hiding an imminent reboot) this PR is meant to close for OCPBUGS-100060.
🛡️ Proposed fix: guard against an empty/unsynced node list
nodes, err := c.nodeLister.List(labels.Everything())
if err != nil {
return false, "", fmt.Errorf("IsSafeToRestartMember failed to list control plane nodes: %w", err)
}
+ if len(nodes) == 0 {
+ return false, "", fmt.Errorf("IsSafeToRestartMember found no control plane nodes; node lister may not be synced yet")
+ }
for _, node := range nodes {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *QuorumCheck) IsSafeToRestartMember(ctx context.Context, targetNodeName string) (bool, string, error) { | |
| scalingStrategy, err := GetBootstrapScalingStrategy(c.operatorClient, c.namespaceLister, c.infraLister) | |
| if err != nil { | |
| return false, "", fmt.Errorf("IsSafeToRestartMember failed to get bootstrap scaling strategy: %w", err) | |
| } | |
| if scalingStrategy == UnsafeScalingStrategy { | |
| return true, "", nil | |
| } | |
| // the cluster must currently tolerate the loss of one member | |
| memberHealth, err := c.etcdClient.MemberHealth(ctx) | |
| if err != nil { | |
| return false, "", fmt.Errorf("IsSafeToRestartMember couldn't determine member health: %w", err) | |
| } | |
| // Two Node OpenShift with Fencing protects etcd via pacemaker; treat it as an exception to the fault | |
| // tolerance rule, consistent with CheckSafeToScaleCluster. | |
| if err := etcdcli.IsQuorumFaultTolerantErr(memberHealth); err != nil && | |
| !(len(memberHealth) == 2 && (scalingStrategy == TwoNodeScalingStrategy || scalingStrategy == DelayedTwoNodeScalingStrategy)) { | |
| return false, err.Error(), nil | |
| } | |
| // no control plane node other than the target may be cordoned or not ready. Member health alone is not | |
| // enough: a node that is about to reboot keeps reporting healthy until it actually goes down, while the | |
| // cordon that precedes its drain is visible minutes in advance. | |
| nodes, err := c.nodeLister.List(labels.Everything()) | |
| if err != nil { | |
| return false, "", fmt.Errorf("IsSafeToRestartMember failed to list control plane nodes: %w", err) | |
| } | |
| for _, node := range nodes { | |
| if node.Name == targetNodeName { | |
| continue | |
| } | |
| if node.Spec.Unschedulable { | |
| return false, fmt.Sprintf("control plane node %q is cordoned, likely about to be drained and rebooted; restarting the etcd member on %q now could lose quorum", node.Name, targetNodeName), nil | |
| } | |
| if !isNodeReady(node) { | |
| return false, fmt.Sprintf("control plane node %q is not ready; restarting the etcd member on %q now could lose quorum", node.Name, targetNodeName), nil | |
| } | |
| } | |
| return true, "", nil | |
| } | |
| func (c *QuorumCheck) IsSafeToRestartMember(ctx context.Context, targetNodeName string) (bool, string, error) { | |
| scalingStrategy, err := GetBootstrapScalingStrategy(c.operatorClient, c.namespaceLister, c.infraLister) | |
| if err != nil { | |
| return false, "", fmt.Errorf("IsSafeToRestartMember failed to get bootstrap scaling strategy: %w", err) | |
| } | |
| if scalingStrategy == UnsafeScalingStrategy { | |
| return true, "", nil | |
| } | |
| // the cluster must currently tolerate the loss of one member | |
| memberHealth, err := c.etcdClient.MemberHealth(ctx) | |
| if err != nil { | |
| return false, "", fmt.Errorf("IsSafeToRestartMember couldn't determine member health: %w", err) | |
| } | |
| // Two Node OpenShift with Fencing protects etcd via pacemaker; treat it as an exception to the fault | |
| // tolerance rule, consistent with CheckSafeToScaleCluster. | |
| if err := etcdcli.IsQuorumFaultTolerantErr(memberHealth); err != nil && | |
| !(len(memberHealth) == 2 && (scalingStrategy == TwoNodeScalingStrategy || scalingStrategy == DelayedTwoNodeScalingStrategy)) { | |
| return false, err.Error(), nil | |
| } | |
| // no control plane node other than the target may be cordoned or not ready. Member health alone is not | |
| // enough: a node that is about to reboot keeps reporting healthy until it actually goes down, while the | |
| // cordon that precedes its drain is visible minutes in advance. | |
| nodes, err := c.nodeLister.List(labels.Everything()) | |
| if err != nil { | |
| return false, "", fmt.Errorf("IsSafeToRestartMember failed to list control plane nodes: %w", err) | |
| } | |
| if len(nodes) == 0 { | |
| return false, "", fmt.Errorf("IsSafeToRestartMember found no control plane nodes; node lister may not be synced yet") | |
| } | |
| for _, node := range nodes { | |
| if node.Name == targetNodeName { | |
| continue | |
| } | |
| if node.Spec.Unschedulable { | |
| return false, fmt.Sprintf("control plane node %q is cordoned, likely about to be drained and rebooted; restarting the etcd member on %q now could lose quorum", node.Name, targetNodeName), nil | |
| } | |
| if !isNodeReady(node) { | |
| return false, fmt.Sprintf("control plane node %q is not ready; restarting the etcd member on %q now could lose quorum", node.Name, targetNodeName), nil | |
| } | |
| } | |
| return true, "", nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/operator/ceohelpers/quorum_check.go` around lines 61 - 102, Update
IsSafeToRestartMember after nodeLister.List to reject an empty node list instead
of falling through to the safe result. Return an unsafe result with a clear
explanation that no control-plane nodes were found, while preserving the
existing error handling and per-node cordon/readiness checks for non-empty
lists.
|
Payload aggregate results (10 runs, all rebooted 3/3 masters):
Baseline for context: ~8% of master-updating runs hit the quorum-loss race (expected ~0.8 occurrences in 10 runs), so the strongest evidence here is behavioral: the precondition demonstrably delays installer pods 5-6 times per upgrade exactly when another master is cordoned/rebooting, without ever blocking rollout progress. Residual oauth-api disruption of 0-10s in these runs is the separate OCPBUGS-100065 mechanism (PRs #2730/#2732, not included in this payload). This comment was generated using AI. Please verify before acting on it. |
|
@mkowalski: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
Second of two PRs for OCPBUGS-100060 (companion to openshift/library-go#2387): etcd quorum loss during upgrades when the revision installer restarts an etcd member while MCO is simultaneously rebooting another master.
Incident evidence (both runs: 2/3 members down → ~2min leaderless → cluster-wide API outage, aggregated apiservers returning
429 storage is (re)initializing):The rebooting node keeps reporting
Readyuntil it's actually down (in one runNotReadywas only recorded after the reboot finished), and the cached member health (60s TTL) can be stale — so existing checks pass. The reliable early signal is the cordon that precedes the MCO drain by minutes.Change:
ceohelpers.QuorumCheckergainsIsSafeToRestartMember(ctx, targetNode): fresh (non-cached) member health must be quorum fault-tolerant, and no control-plane node other than the target may be cordoned or NotReady. Topology exemptions preserved (unsafe/SNO skips entirely; TNF keeps its pacemaker exception, consistent withCheckSafeToScaleCluster).WithInstallerPreconditionimmediately before each installer pod (which restarts the member); when unmet the installer controller emitsInstallerPreconditionNotMetand requeues (15s) — installs are only delayed, never aborted, resuming when the other master returns Ready.starter.gofor DualReplica).go modbump once openshift/library-go#2387 merges.Test plan
gofmt,go vet,go build ./...go test ./pkg/operator/ceohelpers/— newTestQuorumCheck_IsSafeToRestartMember(all-healthy allows; other-node cordoned blocks; target-itself cordoned allows; other-node NotReady blocks; non-fault-tolerant quorum blocks); existing tests pass429 storage is (re)initializingburstsThis PR was generated using AI. Please verify before acting on it.
Summary by CodeRabbit
New Features
Bug Fixes