Repository navigation
Fix CR api group name - #16
Merged
Merged
Conversation
Kirill Ilin (sircthulhu)
marked this pull request as draft
March 13, 2024 09:45
Kirill Ilin (sircthulhu)
marked this pull request as ready for review
March 13, 2024 09:50
George Gaál (gecube)
approved these changes
Mar 13, 2024
Kirill-Garbar
approved these changes
Mar 13, 2024
Kunitsyn Oleg (genestack-okunitsyn)
approved these changes
Mar 13, 2024
Sergey Shevchenko (sergeyshevch)
approved these changes
Mar 13, 2024
Is this modified manually? If so, doesn't that create inconsistency with what is described in the PROJECT file? |
19 of 29 tasks
Terry Tang (saymololy)
pushed a commit
to saymololy/etcd-operator
that referenced
this pull request
Nov 2, 2026
* api: add StorageMedium enum and PodUID status Add the API surface for memory-backed storage: - StorageMedium enum on EtcdClusterSpec, mirrored to EtcdMemberSpec and Status.Observed. "" means PVC (default, unchanged behaviour); "Memory" means tmpfs emptyDir whose lifetime is bound to the Pod. - Status.PodUID on EtcdMember, used by a later commit to detect Pod loss (tmpfs gone) for memory-backed members and trigger replacement. No behavioural change in this commit — fields are added but not yet read by either controller. The kubebuilder Enum marker is the only validation; deeper rules (StorageMedium immutability, replicas=0 ban, storage size required) belong in an admission webhook (issue cozystack#15) and are not added to the controller. Documented operational caveats (no PDB / anti-affinity / resource limits yet) reference issue cozystack#16 from the field godoc so a user reading the CRD documentation lands on the right tracking issue. Signed-off-by: Timofei Larkin <[email protected]> * controllers/cluster: propagate StorageMedium Echo EtcdCluster.spec.storageMedium onto new EtcdMember CRs at both seed-create and learner-create sites. Snapshot it into status.observed during the locking-pattern update; include it in the spec-vs-observed equality check so a mid-flight medium edit is locked out like any other spec change. No behavioural branching on StorageMedium yet — the member controller's volume-source switch lands in a follow-up commit. Signed-off-by: Timofei Larkin <[email protected]> * controllers/member: tmpfs volume + skip PVC + track PodUID For StorageMedium=Memory: - buildPod emits EmptyDir{Medium:Memory, SizeLimit:&storage} instead of a PVC-backed volume. SizeLimit caps tmpfs allocation; container memory limits and other production hardening are tracked in cozystack#16. - ensurePVC short-circuits, leaving Status.PVCName empty. The PVC owner check still applies to PVC-backed members. - updateStatus only writes Status.PVCName for PVC-backed members. Pod-UID tracking (used by the upcoming Pod-loss detection commit): ensurePod records Pod.UID into Status.PodUID on both find-existing and create-new paths; updateStatus keeps it in sync on each reconcile; ensurePodAbsent (the dormant path) clears it so the deliberate-absence state is distinguishable from a Pod-loss event. No callers of Status.PodUID yet — that wiring lands next. Signed-off-by: Timofei Larkin <[email protected]> * controllers/member: self-delete memory member on pod loss When a memory-backed member's recorded Pod is gone or has been replaced by a Pod with a different UID, the tmpfs is gone with it — the data cannot be recovered. Recreating a fresh Pod would silently fail to rejoin the cluster (member ID is in raft state but the WAL is empty), leaving the member permanently wedged. Self-delete the EtcdMember CR instead. The existing finalizer flow removes the member from etcd via MemberRemove (when peers are reachable) and GC takes the rest. The cluster controller's existing scale-up gap- fill creates a replacement EtcdMember with a fresh GenerateName and fresh etcd member ID. If quorum is already lost across multiple simultaneous Pod losses, MemberRemove will fail and the surviving members stay in Terminating until quorum returns — the correct outcome for memory clusters past the point of no return. Signed-off-by: Timofei Larkin <[email protected]> * controllers: isBroken predicate fires on memory-member pod loss Replace the stub with a real check for memory-backed members: a member whose Pod UID was recorded but whose Pod is no longer observed is broken (the tmpfs is gone with the Pod and the data cannot be recovered). The member controller already self-deletes such members in the next reconcile pass; this predicate's job is to surface the count on EtcdCluster.status.brokenMembers during the brief observation window before that delete fires, so operators get a stable signal even when the CR is in the process of being torn down. PVC-backed auto-replacement stays a stub — that's a richer policy question (corruption, irrecoverable crashloop) that this PR doesn't decide. Signed-off-by: Timofei Larkin <[email protected]> * controllers: tests for memory-backed storage Cover the new mechanics: - buildPod with StorageMedium=Memory emits EmptyDir{Medium:Memory, SizeLimit:storage} and zero PVC volumes. Negative guard: an empty StorageMedium still produces a PVC-backed volume so the default path is not silently flipped. - ensurePVC short-circuits for memory members (no PVC created; Status.PVCName stays empty). - ensurePod captures Pod.UID into Status.PodUID via the Get path (the Create path can't be tested reliably — controller-runtime's fake client does not auto-assign UIDs and the steady-state reconcile uses Get anyway). - A reconcile against a memory member with a recorded PodUID and no live Pod self-deletes the EtcdMember and does NOT recreate the Pod. - A reconcile against a memory member with a stable Pod (UID matches) is left alone — otherwise the reconciler would churn itself to death. - updateStatus leaves Status.PVCName empty for memory members so the cluster controller's Paused message can't claim "data preserved on PVC data-X" when there is no PVC. - isBroken table-test pins the predicate across the four relevant cases: memory-with-lost-Pod (broken), memory-with-Pod (healthy), memory-no-UID-yet (not broken on first reconcile), and PVC-backed (stub stays false). - Cluster controller propagates StorageMedium onto the seed EtcdMember and into Status.Observed.StorageMedium; specEqualsObserved treats a medium mismatch as "spec differs" so the locking pattern protects against a mid-flight flip. Signed-off-by: Timofei Larkin <[email protected]> * docs: memory-backed storage - README: bullet under "What's supported", flagging the wedge with replicas=0 and the missing production defaults. - concepts.md: new "Storage" section with the PVC vs Memory trade-off table, the Pod-loss detection model, and the known limitations (PDB/anti-affinity/limits not auto-emitted yet → cozystack#16; replicas=0 combination wedges until webhook → cozystack#15). The "no automatic broken-member replacement" bullet now correctly notes that memory members do auto-replace; only PVC-backed isBroken stays a stub. - operations.md: Memory-backed clusters section with verification recipe (jq the Pod's volume + check mount inside the container), the three things to pre-deploy for a production memory cluster (PodDisruptionBudget, anti-affinity, container memory limits via LimitRange), how Pod-loss auto-replacement plays out, and an explicit "don't pause" warning. - installation.md: a sample memory-backed EtcdCluster manifest with pointers to concepts/operations for the caveats. Signed-off-by: Timofei Larkin <[email protected]> * Address review of feat/memory-storage Two blockers from review: 1. docs/operations.md "Broken member" section still said isBroken is a stub. After this PR it's a real predicate for memory members (even though it doesn't observably surface — see below). Reword to qualify "PVC-backed members" and point at the Memory-backed clusters section for the auto-replacement story. 2. docs/concepts.md and controllers/helpers.go both promised that Status.BrokenMembers would surface a brief non-zero window during memory pod-loss. The controller flow makes this not true in practice: the member-controller's loss path self-deletes the EtcdMember in the same reconcile pass, and the cluster-controller's updateStatus iterates the running set (filterRunningMembers excludes DeletionTimestamp != nil), so the predicate is never consulted for the lost member. Walk the claim back rather than adding sub-reconcile Status writes purely for cosmetic visibility: the brief window would be too short for any kubectl-poll or scrape to catch and would not change any operator action. Update both the doc paragraph and the isBroken comment to be honest about the predicate's current placeholder role. Add TestBrokenMembers_StaysZeroAcrossMemoryPodLoss as the integration contract: with both controllers exercised across a memory pod-loss + member-deletion cycle, BrokenMembers stays at 0. The dedicated unit test for the predicate's pure logic (TestIsBroken_MemoryMemberWithLostPodIsBroken) stays — it pins the intent for the future hook the predicate exists to be. Non-blocking note worth addressing in this PR: add a Detection latency paragraph to docs/operations.md noting that node-NotReady failures are gated by kube-controller-manager's --pod-eviction-timeout (default 5 min), so unannounced node failures surface as up to 5 minutes of degraded quorum before auto-replacement kicks in. Tuning that timeout is outside the operator's control; documenting the budget matters because the rest of the runbook leads readers to expect ~seconds. Signed-off-by: Timofei Larkin <[email protected]> * controllers: honest Paused message for memory-backed clusters When a user pauses a memory-backed cluster (replicas: 0 — not yet gated by an admission webhook, see cozystack#15), the scale-down code path runs unchanged: the survivor is flipped to spec.dormant=true and its Pod is deleted. findDormantMember returns the survivor and the existing Paused branch publishes: Available=False/Paused: cluster is paused (spec.replicas=0); data is preserved on PVC data-<name> This is actively false — the PVC never existed (ensurePVC was skipped for the memory member), the tmpfs evaporated with the Pod, and the cluster cannot be resumed. Operators reading the condition message (directly, via runbooks, or via dashboards keyed on .status.conditions[?(@.type=='Available')].message) would be misled about durability the cluster doesn't have. Tighten the Paused branch to special-case the memory medium with a message that names tmpfs and points the user at recreating the cluster. The condition stays Available=False/Paused — the cluster is paused, the wedge is real — only the message changes. The admission webhook in cozystack#15 will supersede this guard by rejecting replicas=0+memory at create/update time; until that lands, this is the cheap, honest fallback. Signed-off-by: Timofei Larkin <[email protected]> * docs+test: README catchup, drop fragile three-phase test README's "What's not supported" line was the only doc still claiming "No automatic broken-member replacement (the predicate is stubbed)". After the previous review pass walked back the BrokenMembers surfacing claim and the operations / concepts docs landed on "memory members auto-replace on Pod loss; PVC-backed isBroken stays a stub; BrokenMembers reads 0 in practice", the README slipped through. Bring it in line — no behaviour change. Drop TestBrokenMembers_StaysZeroAcrossMemoryPodLoss. The contract it pinned (BrokenMembers stays 0 across a memory pod-loss cycle) is already covered by TestIsBroken_MemoryMemberWithLostPodIsBroken (the predicate's truth table) plus TestReconcile_MemoryMemberDeletesSelfOn PodLoss (the member-controller self-delete path). The three-phase test duplicated those, depended on the current reconcile ordering, and would break whenever isBroken is refined with better liveness gates — which is its intended evolution per the helpers.go comment. Signed-off-by: Timofei Larkin <[email protected]> --------- Signed-off-by: Timofei Larkin <[email protected]>
Terry Tang (saymololy)
pushed a commit
to saymololy/etcd-operator
that referenced
this pull request
Nov 2, 2026
* api+controllers: EtcdMember.Status.IsVoter + role-label constants Add Status.IsVoter to EtcdMember (the carrier bit between the cluster controller's MemberList view and the member controller's Pod label maintenance) and the matching LabelRole / RoleVoter constants in helpers.go. No behavioural change yet — the cluster controller writes IsVoter from MemberList in the next commit, and the member controller propagates that to the Pod's etcd.lllamnyp.su/role=voter label after that. This addition keeps the controller chain clean: cluster owns CR-level state (writes Status.IsVoter), member owns Pod-level state (reads Status.IsVoter, patches the role label). No cluster-controller-to-Pod patches across the layering boundary. Signed-off-by: Timofei Larkin <[email protected]> * controllers: write IsVoter, propagate to Pod label, emit PDB Three pieces, layered through the existing controller chain: 1. Cluster controller writes Status.IsVoter: - syncIsVoter() consumes etcd's MemberList response and patches each EtcdMember.Status.IsVoter to match etcd's !IsLearner view. Match key is the peer URL (not Member.Name, which is empty until the etcd process announces itself post-Pod-start). Idempotent. - Called from promotePendingLearner — the existing site that already fetches MemberList in the current==desired steady-state branch. - Seed creation pre-stamps Status.IsVoter=true with a follow-up r.Status().Update(). The seed is never a learner, so this skips the 1-2 reconcile wait for the first MemberList round to discover that fact, closing the gap where the seed Pod would otherwise exist unprotected by the PDB. 2. Member controller propagates IsVoter to the Pod label: - buildPod() adds etcd.lllamnyp.su/role=voter at creation when Status.IsVoter is already set (the seed's pre-stamp case). - ensurePod() now calls reconcileRoleLabel() after the Get-existing branch — idempotent Patch that adds or strips the role label to match Status.IsVoter. Handles the steady-state case where the cluster controller flips IsVoter on an existing Pod (post-promote) and the lag is one reconcile cycle. - Cluster controller stays out of Pod patches entirely. The role label is the only cross-layer signal and it flows: MemberList → cluster patches CR.Status.IsVoter → member patches Pod label. 3. PDB resource: - reconcilePDB() emits a per-cluster PodDisruptionBudget owned by the EtcdCluster. Selector keys on LabelCluster + LabelRole= RoleVoter; only voter-labelled Pods are protected, so learner evictions don't consume budget and learner-only Pod kills aren't blocked. - maxUnavailable = (voterCount - 1) / 2 (integer-divided, auto- floored). For voterCount=0 the PDB is deleted: pre-bootstrap, paused, or wedged state — nothing to protect. - Called from the end of updateStatus, after the existing broken- count surface. RBAC marker for policy/v1 podisruptionbudgets and for etcdmembers/status (needed for syncIsVoter's patches). - SetupWithManager gains Owns(&policyv1.PodDisruptionBudget{}) so PDB drift (user/automation deletion) triggers reconcile. Race characterisation: scale-up holds N voters in PDB while etcd has N+1 briefly (next reconcile catches up); scale-down's MemberRemove makes etcd N-1 while PDB still reflects N (k8s PDB controller counts DeletionTimestamp != nil Pods as unavailable, so the budget naturally accounts for the in-flight removal). Both windows are one reconcile wide and quorum-preserving. Closes part of cozystack#16 (PDB). Signed-off-by: Timofei Larkin <[email protected]> * controllers: tests for IsVoter, role label, and PDB Cluster controller: - pdbMaxUnavailable table: 0-7 voters covering the floor (1,2 → 0; 3,4 → 1; 5,6 → 2; 7 → 3). - reconcilePDB creates a PDB whose Selector restricts to LabelRole= RoleVoter (the central correctness property — without it, learner evictions would consume budget). - reconcilePDB updates MaxUnavailable in-place when voter count changes (3 → 5 transitions the budget 1 → 2). - reconcilePDB deletes the PDB when voterCount drops to 0; idempotent on subsequent calls. - syncIsVoter patches Status.IsVoter from a MemberList snapshot, by peer URL (not Name — Name is empty in the post-MemberAdd window). Covers four cases: up-patch, down-patch, no-op when already correct, and skip when member is not yet in etcd's list. - Bootstrap pre-stamps Status.IsVoter=true on the seed (end-to-end through Reconcile, not just a unit test of the helper). Member controller: - ensurePod applies LabelRole=RoleVoter when Status.IsVoter=true on an existing Pod that lacks the label (steady-state propagation). - ensurePod strips LabelRole when Status.IsVoter=false on a Pod that still has a stale label (the inverse — important for the future case where a member could transition from voter back to learner). - buildPod includes LabelRole=RoleVoter at create time when Status.IsVoter is already set (the seed pre-stamp's create-time benefit). Negative side: IsVoter=false omits the label entirely rather than emitting an empty string that would match a permissive selector. Signed-off-by: Timofei Larkin <[email protected]> * docs: PodDisruptionBudget section + walk back "no PDB yet" callouts Concepts.md: new ## PodDisruptionBudget section before ## Conditions. Covers the selector (LabelCluster + LabelRole=RoleVoter), the (n-1)/2 budget formula, the two-hop voter detection path (cluster controller writes Status.IsVoter → member controller patches Pod label), the seed pre-stamp shortcut, the two transient races (scale-up over- permits by N→N+1 lag, scale-down's Terminating-Pod accounting), and the zero-voters → delete-PDB behaviour. Operations.md: new ## Draining nodes (PodDisruptionBudget) section before ## Recipes. Shows the kubectl get pdb output, the eviction- denied error users will see, how to identify voter Pods by label, and why drains might block during scale events. Walked back the "no PDB yet, tracked in cozystack#16" callouts in the same files (and in installation.md, README.md, and the StorageMedium type godoc). The PodDisruptionBudget is now auto-emitted on every cluster; cozystack#16 still tracks the remaining production gaps (anti-affinity, container memory limits). Signed-off-by: Timofei Larkin <[email protected]> * controllers: route MemberList around learners via IsVoter The cluster controller's memberEndpoints helper and the member controller's discoverMemberID peer filter both previously used MemberReady=True as a proxy for "voter". The original assumption was that Ready transitively implies non-learner because the member can only become Ready after its MemberID was discovered via a voter peer. That assumption is wrong: a learner whose Pod is Running and which the operator successfully looked up via a voter is itself Ready=True even though it remains a learner until MemberPromote completes. clientv3 then round-robins MemberList across the returned endpoints and hits "rpc not supported for learner" with non-trivial probability, wedging promote during the scale-up window. Status.IsVoter (introduced in this PR for the PDB selector) is the precise signal: it's set true only when etcd's MemberList confirms IsLearner=false. Switch both call sites to filter on IsVoter instead. The unfiltered-fallback path is preserved for pre-bootstrap cases where no voter has been observed yet. Surfaced on dev4 smoke: 3-replica PVC cluster wedged after seed + learner came up, operator's promote MemberList kept hitting the learner endpoint. Tests in TestDiscoverMemberID_* were updated to set Status.IsVoter on the voter fixtures, matching the new contract. isMemberReady was dropped (no remaining callers). Signed-off-by: Timofei Larkin <[email protected]> * controllers/member: grant Pod patch RBAC for role label reconcileRoleLabel was added in this PR to keep the Pod's role=voter label in sync with Status.IsVoter, but the member controller's RBAC marker only listed get;list;watch;create;delete for pods — patch was missing. The smoke test on dev4 surfaced this as repeated: pods "pdb-test-Xxxxx" is forbidden: User "...controller-manager" cannot patch resource "pods" in API group "" Add patch to the verb list, regenerate role.yaml. Tests don't exercise RBAC (the fake client doesn't enforce it), so the smoke is what catches this — the regression check is "operator's pods reconcile without 'forbidden' errors", which is operationally observable. (Plus an incidental gofmt fix to a comment alignment in TestSyncIsVoter_PatchesFromMemberList that `make manifests` triggered.) Signed-off-by: Timofei Larkin <[email protected]> * api+controllers: expose /scale on EtcdMember for PDB The PodDisruptionBudget controller traverses each protected Pod's controllerRef looking for the /scale subresource on the parent — and fails closed when it's missing: status: conditions: - reason: SyncFailed message: etcdmembers.lllamnyp.su does not implement the scale subresource expectedPods: 0 disruptionsAllowed: 0 With expectedPods=0 the PDB doesn't actually gate any disruption, even though it advertises maxUnavailable=1. The smoke test on dev4 caught this — kubectl get pdb showed the right MaxUnavailable but ALLOWED DISRUPTIONS stuck at 0 with the SyncFailed condition above. Each EtcdMember represents exactly one Pod, so /scale's contract is degenerate: spec.replicas locked to 1 (validation Min=Max=1), status .replicas pinned to 1, status.selector exposes a label expression matching the one Pod. The /scale subresource exists only to satisfy the PDB controller's traversal; it's not a tunable knob. Cluster controller doesn't need to set spec.replicas explicitly — the +kubebuilder:default=1 marker means the apiserver fills it in on EtcdMember Create. Member controller writes the status fields in updateStatus. Existing TestUpdateStatus_NoChurnInSteadyState was updated to pre-set Status.Replicas/Selector so the test fixture matches what a steady-state member looks like. Signed-off-by: Timofei Larkin <[email protected]> * docs+test: tighten PDB doc accuracy from review Five reader-facing accuracy fixes flagged in /branch-review of PR cozystack#19: 1. docs/concepts.md (Transient races, scale-up arm): the quorum formula in the worked example used the ceiling-form `⌈(N+1)/2⌉+1`, which coincidentally matches the floor-form for even M but understates quorum for odd M (M=3 ceiling says 3, actual is 2). Etcd's write quorum is `⌊M/2⌋+1`; rewrite the parenthetical to use the floor form and state the M=N+1 case directly. 2. docs/concepts.md (Transient races, scale-down arm): the doc said the k8s PDB controller "counts DeletionTimestamp != nil Pods as already unavailable". That's not the mechanism — the PDB controller's `currentHealthy` counts Pods whose `Ready` condition is True, regardless of DeletionTimestamp. The Terminating Pod stops counting toward the budget's healthy total once kubelet flips Ready to False at the start of graceful shutdown. End conclusion is unchanged; the explanation is corrected. 3. docs/concepts.md (Where the role=voter label comes from): "Two reconcile hops" undercounted by one. Cluster writes IsVoter (hop 1) → member patches Pod label (hop 2) → cluster's next pass picks up the new label via reconcilePDB (hop 3). State the full end-to-end path and the cycle count. 4. docs/operations.md (Why drains might block during scale events): "millisecond-wide race window" was wrong. Steady-state RequeueAfter is 30s, so the race window is one reconcile cycle wide (up to ~30s in the worst case). Soften the wording to match. 5. controllers/etcdmember_controller_test.go: rename TestDiscoverMemberID_ExcludesNonReadyPeers → TestDiscoverMemberID_ExcludesNonVoterPeers and rewrite its docstring. The test's substance was updated to use Status.IsVoter in the previous commit; the name and docstring still described the older Ready-based filter. Signed-off-by: Timofei Larkin <[email protected]> --------- Signed-off-by: Timofei Larkin <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes from comments #15