fix: remove the correct member on scale-in and transfer leadership first

[xrl]

What

Two scale-in defects in reconcileClusterState, fixed together because they share the removal path:

1. Wrong member removed once ordinals reach 10. Scale-in removed memberHealth[len-1], but ClusterHealth sorts health entries lexically by endpoint — so with 11 members the "last" entry is ...-9, not ...-10. The StatefulSet always deletes the highest-ordinal pod, so etcd membership desyncs from the actual pod set. The removal target is now matched by name against the authoritative memberListResp.Members (members run with --name=$(POD_NAME), so the target is <sts>-<len-1>), via a new scaleInTargetID. This also means removal no longer requires the doomed pod to answer health probes — an unreachable top-ordinal member can still be removed.

2. Removing the leader stalls the cluster on an election. When the removal target is the current leader, the operator now transfers leadership first (etcdutils.MoveLeader, issued against the leader's own endpoint since MoveLeader must be served by the leader) to the lowest-ordinal healthy voting member. The transfer is best-effort: removing a leader is legal in etcd, so a failed transfer is logged and removal proceeds — it only exists to avoid the election stall.

Files

• internal/etcdutils/etcdutils.go — add MoveLeader(eps, transfereeID), mirroring the existing client boilerplate.
• internal/controller/utils.go — scaleInTargetID (name match against the member list), transfereeForScaleIn (walks ordinals ascending via clientEndpointForOrdinalIndex equality; skips learners, unhealthy members, and the target), removeScaleInMember (transfer-then-remove; takes the two etcd calls as parameters so tests stub them with local closures — no package-level seams).
• internal/controller/etcdcluster_controller.go — scale-in branch calls removeScaleInMember, passing etcdutils.MoveLeader/etcdutils.RemoveMember; leader ID comes from the FindLeaderStatus result already in hand.

Review

An adversarial review pass produced 4 findings; both blocking/important ones resolved (second commit): the removal target was initially re-derived by reverse-parsing ordinals out of health-probe endpoint URLs instead of using the member list the reconciler already holds (blocking — replaced with the name match, ordinalFromEndpoint deleted), and mutable package-level function seams for testing (important — replaced with function parameters). Two follow-up review passes approved with minor-only notes.

Testing

• TestScaleInTargetID: happy path, 11 members with the highest ordinal winning regardless of list order, empty member list, target name missing.
• TestTransfereeForScaleIn: lowest-ordinal voter wins; learners, unhealthy members, and the removal target are skipped; no-candidate case.
• TestRemoveScaleInMember: transfer-before-removal when the target leads, no transfer otherwise, transfer failure not blocking removal, no eligible transferee, unreachable target still removed via the member list, and an 11-member case with lexical health ordering and the leader at ordinal 10 (fails on the previous code).
• TestMoveLeader (internal/etcdutils): error on invalid endpoint and on a bogus transferee against a live embedded server.
• Full make test and make verify pass.

Descoped follow-ups

(1) No spec/CRD knob to opt out of leader transfer (best-effort transfer needs no configuration); hence no make generate/manifests/api-docs churn. (2) No refactor of the duplicated clientv3 boilerplate in etcdutils (AddMember/PromoteLearner/RemoveMember/MoveLeader) — dupl is lint-excluded for internal/*. (3) Latent bugs left untouched, each deserving its own PR: updateStatus/updateConditions index-pair memberListResp.Members[i] with lexically-sorted memberHealth[i] (etcdcluster_controller.go:434), nil-Status deref risk in FindLeaderStatus/FindLearnerStatus, and healthCheck's endpoint truncation during an interrupted scale. (4) No e2e/kind coverage for real multi-node leader transfer (unit seams cover the ordering) and no gofail failpoint additions. (5) FindLeaderStatus signature unchanged — leader ID derived from leaderStatus.Header.MemberId instead of threading a new return value through two call sites.

🤖 Generated with Claude Code

---

PR series — operability fixes, TLS & EtcdMirror

Small single-purpose PRs from live kind-cluster testing of the operator. Each stands alone unless an After is listed. → = this PR.

	PR	Lands	After
	Reviewable now — small, any order
🟢	#374	Requeue instead of swallowing the client-cert provisioning error	—
🟢	#391	Grant events.k8s.io RBAC so operator Events are actually recorded	—
🟢	#392	Correlate members[]/leaderID from one health snapshot (consistent leader)	—
🟢	#393	Propagate user-supplied altNames.ipAddresses into certificates	—
🟢	#394	Accept day-suffix validityDuration (365d, 100d12h) as documented	—
🟢	#395	Surface early reconcile errors as a Degraded condition (was empty status)	—
🟢	#379	Configurable reconcile worker pool (--max-concurrent-reconciles)	—
🟢	#369	kind-based stress/scale e2e harness (1/3/7 members, churn, quorum watcher)	—
🟢	#398	podTemplate.spec scheduling fields: topologySpread, resources, priorityClass, schedulerName	—
🟢	#397	First-class Helm chart covering the full config/ surface	—
🟢	#399	Quorum-aware PodDisruptionBudget per EtcdCluster	—
🟢→	#400	Scale-in removes the right member and transfers leadership first	—
🟢	#401	Backend quota + auto-compaction spec fields, NOSPACE alarm surfacing	—
🟢	#402	Reconcile-loop gates replace the blocking StatefulSet-ready wait	—
🟢	#403	Quorum-gated one-pod-at-a-time version upgrades via OnDelete	—
	TLS stack — in order
🟢	#376	Independent spec.tls.{peer,client} surfaces (breaking alpha API)	—
⚪	#377	TLSReady condition + TLS lifecycle Events	#376
⚪	#378	Multi-member TLS quorum e2e + PeerCANotShared	#377
	EtcdMirror — in order
🟢	#406	EtcdMirror CRD: one-way cross-cluster replication API (types + CEL)	—
🟢	#407	pkg/mirroragent replication engine (fenced checkpoint-in-target)	#406
🟢	#408	Periodic reconciliation pass wired into the engine's steady-state loop	#407
🟢	#412	mirror-agent binary: config/TLS-reload/statusz/metrics + kind e2e	#408
🟢	#413	EtcdMirror controller: Deployment rendering, statusz-driven conditions, guards, fenced finalizer	#412
🟢	#414	CRD/sample/editor+viewer-role kustomize wiring	#413
🟢	#415	Controller-side phase/condition gauges, shipped PrometheusRule + dashboard	#414
🟢	#416	Full-lifecycle kind e2e: fence overlap, compaction+prune, restart resume, cert rotation, cutover/reversal	#415
	Parked as drafts pending #363
⚪	#382	Per-cluster domain metrics on the operator /metrics endpoint	—
⚪	#384	EtcdCluster admission webhooks (consolidating with #328)	#363
⚪	#386	EtcdBackup CR → object storage (S3/GCS)	#363
⚪	#387	Automatic quorum-loss disaster recovery (bootstrap-latch guarded)	#363

🟢 ready · ⚪ draft

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xrl
Once this PR has been reviewed and has the lgtm label, please assign justinsb 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

[kubernetes-prow]

Hi @xrl. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.