Skip to content

Test pre commit - #7

Closed
Oleg Kunitsyn (hiddenmarten) wants to merge 6 commits into
cozystack:mainfrom
hiddenmarten:test-pre-commit
Closed

Oleg Kunitsyn (hiddenmarten) wants to merge 6 commits into
cozystack:mainfrom
hiddenmarten:test-pre-commit

Conversation

@hiddenmarten

Copy link
Copy Markdown
Contributor

No description provided.

@hiddenmarten
Oleg Kunitsyn (hiddenmarten) marked this pull request as draft March 10, 2024 19:33
@hiddenmarten
Oleg Kunitsyn (hiddenmarten) deleted the test-pre-commit branch March 10, 2024 19:34
Terry Tang (saymololy) pushed a commit to saymololy/etcd-operator that referenced this pull request Nov 2, 2026
Architectural change — Observed/ProgressDeadline locking pattern
================================================================
The cluster controller now reconciles toward EtcdCluster.status.observed,
not directly against spec. Spec changes are noticed but not acted on
until the in-flight target is met (or the deadline expires). This
addresses two distinct race conditions the reviewer flagged:

  - cozystack#1 (scale-up immediately followed by scale-down): in the old code,
    a user reverting spec.replicas after MemberAdd but before the new
    pod's MemberID is populated would orphan the etcd member. The
    locking pattern means the second scale doesn't take effect until
    the first one has settled.

  - cozystack#5 (mid-bootstrap replica change): if spec.replicas changed between
    the first and last bootstrap member's creation, the --initial-cluster
    flags would diverge and etcd couldn't form. observed.replicas locks
    the bootstrap target size at the first reconcile.

User-facing escape hatch: status.progressDeadline. Patch it to a past
time via `kubectl patch --subresource=status` to abort a stuck reconcile
(documented in README).

Specific reviewer fixes
=======================
  cozystack#1: removeMemberFromEtcd now falls back to MemberList lookup by name
      (or peer URL) when status.MemberID is empty. Belt-and-suspenders
      with the locking pattern.

  cozystack#2: ensurePVC refuses to reuse a same-named PVC owned by a different
      EtcdMember UID. Same-named scale-down/up cycles can't bind the
      new pod to the prior member's data dir.

  cozystack#3: MemberReady=True is now gated on status.MemberID being populated.
      A pod that's PodReady but whose memberID we haven't learned yet
      reports Ready=False, Reason=DiscoveringMemberID.

  cozystack#4: tryDiscoverCluster logs etcd-client and MemberList errors and
      sets Available=False, Reason=ClusterUnreachable on the cluster
      so users can see what's blocking the bootstrap.

  cozystack#6: isBroken predicate is wired into the steady-state path; the
      result is exposed as status.brokenMembers. The predicate still
      always returns false (auto-replacement policy is not yet decided)
      but it has a tested call site instead of being dead code.

  cozystack#7: README is no longer kubebuilder boilerplate. Documents the
      v1alpha2 model, the locking pattern, the abort-stuck-reconcile
      recipe, and supported/unsupported features.

  cozystack#8: config/samples/kustomization.yaml added so `kubectl apply -k
      config/samples/` works as advertised.

Recommended (non-blocking) items also addressed:
  - cluster-controller RBAC trimmed to get;list;watch on etcdclusters
    (only status uses update;patch).
  - ensureService documents that drift on existing Services is
    intentionally not reconciled.

Test infrastructure
===================
Introduced an EtcdClusterClient interface and EtcdClientFactory so
tests can inject a fake etcd. SetupWithManager defaults the factory
to DefaultEtcdClientFactory in production. All 13 new unit tests use
controller-runtime's fake client + an in-memory fakeEtcd to drive the
reconcilers and assert on K8s + etcd state, with no envtest dependency.

Verified end-to-end on a live cluster (bootstrap → scale 3→5 → scale
5→3 with consistent etcd member list → cluster deletion).

Co-Authored-By: Claude Opus 4.6 (1M context) <[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
…edGeneration

cozystack#1 (scaleUp not crash-safe): MemberAdd is idempotent now. Before calling
it, scaleUp does a MemberList and checks whether a member with our peerURL
is already registered (the symptom of a crash between MemberAdd and
Create(EtcdMember) on a previous reconcile). If so, skip MemberAdd and
proceed to creating the CR. MemberAdd stays in the cluster controller.

cozystack#2 (bootstrap consistency on resume): added a test asserting that when
bootstrap() resumes with some EtcdMembers already present, the
InitialCluster value computed for the missing members exactly matches the
one stored on the existing CRs. Belt-and-suspenders for the locking
pattern; previously implicit, now pinned.

cozystack#3 (concurrent scale-down race): Reconcile checks whether any EtcdMember
already has DeletionTimestamp set before deciding to issue another delete.
If one is in flight, requeue and wait. The cluster controller stays the
decision-maker; the EtcdMember finalizer remains the actuator that calls
MemberRemove. This eliminates the race where two finalizers run
MemberRemove against stale snapshots of the etcd cluster.

cozystack#4 (Status().Update errors swallowed): tryDiscoverCluster now checks the
return value. Conflicts requeue; other errors propagate so
controller-runtime's backoff applies.

cozystack#5 (status churn while parked): two changes work together. handleDeadline
Exceeded now returns ctrl.Result{} (no requeue) once it has set the
terminal conditions — the watch on the EtcdCluster resource is enough to
wake us up if the user edits spec to recover. setClusterCondition returns
a "changed" bool by comparing against the existing condition; callers
skip Status().Update when nothing changed. ResourceVersion stays stable
across no-op terminal reconciles (verified by test).

cozystack#7 (storage doc drift): README's "What's not supported" now calls out
that spec.storage edits don't resize existing PVCs. Filed cozystack#2 to track
the resize feature.

cozystack#8 (ObservedGeneration on conditions): setClusterCondition stamps
cluster.Generation; setMemberCondition (new helper) stamps
member.Generation. All cluster-side conditions now go through
setClusterCondition; all member-side conditions through
setMemberCondition.

UID alignment: pod runs as 65532 (distroless nonroot convention; matches
the operator's own pod and kubebuilder default) instead of an arbitrary
60000. The official quay.io/coreos/etcd image runs as root by default;
fsGroup ensures the data dir is writable as 65532.

Tests: 7 new unit tests pinning each fix. All 21 tests pass.
Verified end-to-end on dev4 — bootstrap completes with conditions
carrying observedGeneration=1 and Progressing=False/Reconciled.

Co-Authored-By: Claude Opus 4.6 (1M context) <[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
Recommendation from review round 7: a user reading status.clusterToken
during a deadline-exceeded recovery flow might think they need to
"fix" it. Spell out that the field is set once at bootstrap and never
reset — none of the recovery paths touch it.

Two non-blocking review items get follow-up issues instead:
  - cozystack#7: surface a NoMembers condition when scale-up runs against an
    empty member list post-bootstrap (self-inflicted-wound case but
    silent retry is bad UX)
  - cozystack#8: tighten ensurePod to UID-check existing Pods, mirroring the
    posture ensurePVC already takes

Co-Authored-By: Claude Opus 4.6 (1M context) <[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.

1 participant