Skip to content

Fix CR api group name - #16

Merged
Kirill Ilin (sircthulhu) merged 4 commits into
mainfrom
crds
Mar 13, 2024
Merged

Kirill Ilin (sircthulhu) merged 4 commits into
mainfrom
crds

Conversation

@sircthulhu

Copy link
Copy Markdown
Collaborator

Fixes from comments #15

  • Fix API group name for custom resources
  • remove unnecessary comment lines

@sircthulhu
Kirill Ilin (sircthulhu) marked this pull request as draft March 13, 2024 09:45
@sircthulhu
Kirill Ilin (sircthulhu) marked this pull request as ready for review March 13, 2024 09:50
@sircthulhu Kirill Ilin (sircthulhu) linked an issue Mar 15, 2024 that may be closed by this pull request
@yimtun

yimtun commented Oct 23, 2024

Copy link
Copy Markdown

Fixes from comments #15

  • Fix API group name for custom resources
  • remove unnecessary comment lines

Is this modified manually? If so, doesn't that create inconsistency with what is described in the PROJECT file?

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]>
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.

Write basic etcd-operator spec

6 participants