OCPBUGS-105882: Remove dead etcd members during a revision rollout

[hasbro17]

What

Remove an etcd member whose backing Node is gone and that etcd reports
unhealthy, even while a static-pod revision rollout is in progress. This runs
ahead of the revision-stability gate; the quorum-sensitive scale-down and
learner-removal paths still wait for revisions to stabilize.

This supersedes #1684. That PR moved orphan removal ahead of the revision gate
but only for members with no Machine, so it missed the case where the Machine is
still present as a tombstone pending deletion. Keying on Node absence instead of
Machine absence covers both.

Why

After a control-plane node is replaced, the stale (unhealthy) etcd member keeps
the etcd-endpoints configmap churning, which keeps rolling new revisions, which
keeps IsRevisionStable() false. ClusterMemberRemovalController.sync() bailed
out early whenever revisions were unstable, so the member was never removed and
the churn sustained itself. The stale member was both the cause of the
instability and the thing the instability blocked us from removing.

The fix keys removal on Node absence rather than Machine absence. A revision
rollout never deletes the Node object (it only reinstalls the static pod), so:

• a member with no Node has no running etcd pod and is genuinely dead, safe to
  remove regardless of revision state;
• a member that is only transiently unhealthy mid-rollout still has its Node and
  is left alone.

The scenario this addresses is the assisted-installer late-binding master
replacement flow: a manual bind/unbind of the host through the Kube API (the
Agent CR). The old Node and Machine are torn down out of band, leaving the
member as a true orphan (no Machine and no Node) or a half-orphan (Machine still
present as a tombstone pending deletion); both are now removed.

MachineHealthCheck / CPMS remediation is unaffected: there the etcd PreDrain
hook keeps the Machine and the Node alive until the member is first removed, so
the member still has a Node and continues to be handled by the gated scale-down
path.

Removing an orphan member only lowers etcd's quorum threshold and never removes
a healthy member, so it is always quorum-safe and needs no quorum check. The
Machine-API-functional precondition is retained (it is required to classify
whether a member's Machine still exists).

Testing

• New TestSync table test covering the full gate ordering: dead members
  (true orphan and half-orphan) are removed during a rollout; scale-down and
  learner removal stay gated; machine-API / bootstrap / endpoints-lag gates; and
  that an error from the dead-member removal is surfaced (not swallowed) when the
  revision is unstable.
• TestClusterMemberRemovalController updated for the generalized predicate,
  with new cases for the half-orphan and the healthy-but-unbacked error.
• make build and make verify pass.

Reference: assisted-installer late-binding unbind flow

assisted-service internal/controller/controllers/agent_controller.go:

• unbindHost — hub-side DB state only.
• cleanUnboundSpokeNode → removeSpokeResources (spoke-side):
  • deletes the Node from the spoke;
  • finds the Machine via the machine.openshift.io/machine annotation on the node;
  • annotates the Machine with machine.openshift.io/delete-machine: true and
    machine.openshift.io/exclude-node-draining: true;
  • for masters (no MachineSet): calls Delete on the Machine (and the BMH).

Assisted does not touch etcd membership/configmaps; it expects the etcd operator
to remove the member once the Node/Machine are gone. Our predicate (Node gone +
Machine absent-or-pending-deletion + member unhealthy) matches both end-states of
this flow.

Summary by CodeRabbit

• Bug Fixes

  • Improved handling of unhealthy cluster members when their associated nodes are missing or being deleted.
  • Prevented unsafe member removal during revision rollouts and while etcd membership is out of sync.
  • Added live health checks before removing orphaned members.
  • Preserved accumulated errors to improve reliability and diagnostics.

• Tests

  • Expanded coverage for bootstrap states, pending deletions, learner members, quorum health, revision stability, and endpoint synchronization.

[openshift-merge-bot]

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

[openshift-ci-robot]

@hasbro17: This pull request references Jira Issue OCPBUGS-105882, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (5.1.0) matches configured target version for branch (5.1.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

What

Remove an etcd member whose backing Node is gone and that etcd reports
unhealthy, even while a static-pod revision rollout is in progress. This runs
ahead of the revision-stability gate; the quorum-sensitive scale-down and
learner-removal paths still wait for revisions to stabilize.

This supersedes #1684. That PR moved orphan removal ahead of the revision gate
but only for members with no Machine, so it missed the case where the Machine is
still present as a tombstone pending deletion. Keying on Node absence instead of
Machine absence covers both.

Why

After a control-plane node is replaced, the stale (unhealthy) etcd member keeps
the etcd-endpoints configmap churning, which keeps rolling new revisions, which
keeps IsRevisionStable() false. ClusterMemberRemovalController.sync() bailed
out early whenever revisions were unstable, so the member was never removed and
the churn sustained itself. The stale member was both the cause of the
instability and the thing the instability blocked us from removing.

The fix keys removal on Node absence rather than Machine absence. A revision
rollout never deletes the Node object (it only reinstalls the static pod), so:

• a member with no Node has no running etcd pod and is genuinely dead, safe to
  remove regardless of revision state;
• a member that is only transiently unhealthy mid-rollout still has its Node and
  is left alone.

The scenario this addresses is the assisted-installer late-binding master
replacement flow: a manual bind/unbind of the host through the Kube API (the
Agent CR). The old Node and Machine are torn down out of band, leaving the
member as a true orphan (no Machine and no Node) or a half-orphan (Machine still
present as a tombstone pending deletion); both are now removed.

MachineHealthCheck / CPMS remediation is unaffected: there the etcd PreDrain
hook keeps the Machine and the Node alive until the member is first removed, so
the member still has a Node and continues to be handled by the gated scale-down
path.

Removing an orphan member only lowers etcd's quorum threshold and never removes
a healthy member, so it is always quorum-safe and needs no quorum check. The
Machine-API-functional precondition is retained (it is required to classify
whether a member's Machine still exists).

Testing

• New TestSync table test covering the full gate ordering: dead members
  (true orphan and half-orphan) are removed during a rollout; scale-down and
  learner removal stay gated; machine-API / bootstrap / endpoints-lag gates; and
  that an error from the dead-member removal is surfaced (not swallowed) when the
  revision is unstable.
• TestClusterMemberRemovalController updated for the generalized predicate,
  with new cases for the half-orphan and the healthy-but-unbacked error.
• make build and make verify pass.

Reference: assisted-installer late-binding unbind flow

assisted-service internal/controller/controllers/agent_controller.go:

• unbindHost — hub-side DB state only.
• cleanUnboundSpokeNode → removeSpokeResources (spoke-side):
• deletes the Node from the spoke;
• finds the Machine via the machine.openshift.io/machine annotation on the node;
• annotates the Machine with machine.openshift.io/delete-machine: true and
  machine.openshift.io/exclude-node-draining: true;
• for masters (no MachineSet): calls Delete on the Machine (and the BMH).

Assisted does not touch etcd membership/configmaps; it expects the etcd operator
to remove the member once the Node/Machine are gone. Our predicate (Node gone +
Machine absent-or-pending-deletion + member unhealthy) matches both end-states of
this flow.

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.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 71b3d894-e937-4b65-8365-3342db50496b

📥 Commits

Reviewing files that changed from the base of the PR and between 9bb66a6 and 3c0baa0.

📒 Files selected for processing (2)

• pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go
• pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller_test.go

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

---

Walkthrough

The cluster-member-removal controller now checks Machine API availability, removes eligible unhealthy members without Nodes, preserves gate errors, and keeps scale-down restrictions for unstable revisions or unsynchronized endpoints. Tests cover these paths and related member states.

Changes

Cluster member removal

Layer / File(s)	Summary
Sync gates and error propagation pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go	Machine API functionality is checked before removal. Revision and endpoint synchronization failures now preserve accumulated errors with context.
Unhealthy member removal pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go	Removal uses a Machine lookup by internal IP. Active Machines block removal, while absent or pending-deletion Machines can proceed after etcd health checks.
Sync scenario coverage pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller_test.go	Tests cover gate ordering, orphan and pending-deletion Machines, revision rollouts, endpoint lag, learners, quorum health, error propagation, and reusable fixtures.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: ⚪ Minimal · up to 3c0ba

The change narrowly enables removal of genuinely dead etcd members during revision rollouts while preserving existing gates for other removal paths; no actionable merge-blocking risk remains.

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning)

Check name	Status	Explanation	Resolution
No-Sensitive-Data-In-Logs	❌ Error	The new half-orphan path reaches klog.V(2) with memberLocator; it logs member.Name, and ETCD_NAME is derived from the node name/hostname, exposing an internal hostname.	Redact member.Name and peer URLs from removal logs. Log only a non-sensitive status and, if needed, a stable opaque member ID.
Docstring Coverage	⚠️ Warning	Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (13 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: removing dead etcd members during revision rollouts.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	The changed tests use t.Run(scenario.name) with literal, static scenario descriptions; no added title contains runtime values, generated identifiers, IPs, node names, namespaces, or timestamps.
Test Structure And Quality	✅ Passed	The changed tests use Go's testing package with synchronous in-memory fakes, not Ginkgo; they create no live cluster resources and contain no Eventually/Consistently waits.
Microshift Test Compatibility	✅ Passed	The PR adds standard Go testing unit tests only; the changed files contain no Ginkgo e2e tests, so MicroShift API compatibility checks do not apply.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	The PR changes only controller code and standard Go unit tests; no new Ginkgo e2e It/Describe/Context/When tests were added.
Topology-Aware Scheduling Compatibility	✅ Passed	The diff changes etcd member health, Node, and Machine handling only; it adds no manifests or scheduling constraints, and existing master selectors are not new scheduling behavior.
Ote Binary Stdout Contract	✅ Passed	The PR changes only controller code and unit tests; it adds no stdout writes or OTE suite setup, and its new klog calls are inside ordinary controller methods.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	The PR adds standard Go unit tests (testing, testify, t.Run) with fake clients and indexers, not Ginkgo e2e tests; no external connectivity is used.
No-Weak-Crypto	✅ Passed	The commit changes only etcd membership logic and tests; added imports and code contain no weak-crypto APIs, custom crypto, or secret/token comparisons.
Container-Privileges	✅ Passed	The diff changes only Go controller code and tests. No manifest-like files or added privilege settings such as privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, or allowPrivilegeEscalation appear.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

Some tools did not complete. Review the errors below.

🔧 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
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions

---

_{Comment @coderabbitai help to get the list of available commands.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign p0lyn0mial for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hasbro17]

That PR description is making my head hurt ☹️ even though I know what's going on. Will need to prune that.
Opus 4.8 loves a good word salad.

[hasbro17]

/pipeline required

[openshift-merge-bot]

Scheduling required tests:
/test e2e-agnostic-ovn
/test e2e-agnostic-ovn-upgrade
/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2
/test e2e-aws-ovn-single-node
/test e2e-gcp-operator
/test e2e-gcp-operator-disruptive
/test e2e-metal-ipi-ovn-ipv6
/test e2e-operator

[hasbro17]

/hold

Need to verify via the e2e scaling tests

[hasbro17]

/test e2e-aws-ovn-etcd-scaling

[tchap]

/retest

[openshift-ci]

@hasbro17: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

[hasbro17]

/hold

It would be best to get e2e coverage from the assisted-installer side.

The existing etcd vertical-scaling suite tests normal Machine deletion and replacement through CPMS or Machine API. Its pre-drain hook intentionally keeps the Node and Machine involved until the etcd member is removed.
Reproducing the assisted-installer state in the vertical-scaling suite would require provider-specific host termination and probably other convoluted and fragile steps.

This bug is triggered by the Agent late-binding unbind flow, which deletes the old host’s Node and Machine out of band while leaving its etcd membership behind.
An assisted-installer e2e can perform the actual Agent unbind flow and verify that the stale etcd member is removed afterward.

/cc @shay23bra

[shay23bra]

/hold

It would be best to get e2e coverage from the assisted-installer side.

The existing etcd vertical-scaling suite tests normal Machine deletion and replacement through CPMS or Machine API. Its pre-drain hook intentionally keeps the Node and Machine involved until the etcd member is removed. Reproducing the assisted-installer state in the vertical-scaling suite would require provider-specific host termination and probably other convoluted and fragile steps.

This bug is triggered by the Agent late-binding unbind flow, which deletes the old host’s Node and Machine out of band while leaving its etcd membership behind. An assisted-installer e2e can perform the actual Agent unbind flow and verify that the stale etcd member is removed afterward.

/cc @shay23bra

@hasbro17
The operator unit tests cover the new gate ordering and orphan/half-orphan cases. Since the existing scaling E2E does not reproduce Assisted’s late-binding unbind behavior, adding a new cross-component E2E specifically for this fix seems disproportionate. I suggest validating the actual unbind workflow through Assisted Installer QE against the fixed operator, and tracking permanent Assisted-side regression coverage separately.