Repository navigation
Test pre commit - #7
Closed
Oleg Kunitsyn (hiddenmarten) wants to merge 6 commits into
Closed
Oleg Kunitsyn (hiddenmarten) wants to merge 6 commits into
Oleg Kunitsyn (hiddenmarten) wants to merge 6 commits into
Conversation
Oleg Kunitsyn (hiddenmarten)
marked this pull request as draft
March 10, 2024 19:33
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]>
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.
No description provided.