Repository navigation
Pre-commit via make - #6
Merged
Merged
Conversation
Oleg Kunitsyn (hiddenmarten)
force-pushed
the
pre-commit-init
branch
3 times, most recently
from
March 10, 2024 13:50
90d8cd4 to
bc724c7
Compare
Oleg Kunitsyn (hiddenmarten)
marked this pull request as ready for review
March 10, 2024 19:46
Oleg Kunitsyn (hiddenmarten)
enabled auto-merge (squash)
March 10, 2024 19:46
Contributor
Author
Kirill Ilin (sircthulhu)
requested changes
Mar 11, 2024
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: 1.22.1 |
Collaborator
There was a problem hiding this comment.
We have version 1.21 in Dockerfile
Contributor
Author
There was a problem hiding this comment.
Yep, thanks
Comment on lines
-76
to
+77
| _, err = utils.Run(cmd) | ||
| // TODO: Handle CRD installation error | ||
| _, _ = utils.Run(cmd) |
There was a problem hiding this comment.
Is it still makes sense after #15 get merged?
Contributor
Author
There was a problem hiding this comment.
Yep, it's a linter validation issue.
Comment on lines
+4
to
+6
| ## Contributing | ||
| ### Prerequisites | ||
| - [pre-commit](https://pre-commit.com/#installation) |
There was a problem hiding this comment.
Could you rebase PR, please?
Contributor
Author
There was a problem hiding this comment.
Sure, done.
Oleg Kunitsyn (hiddenmarten)
force-pushed
the
pre-commit-init
branch
from
March 14, 2024 08:18
976e305 to
54bfa1d
Compare
György Gaál (gaalw)
approved these changes
Mar 14, 2024
George Gaál (gecube)
approved these changes
Mar 14, 2024
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
…status Architectural change — learner-mode scale-up ============================================ Scale-up now adds members as learners (MemberAddAsLearner) and promotes them (MemberPromote) once etcd reports they have caught up on the raft log. Voting quorum does not shift until promotion, so an unreachable joiner can never destabilise the existing cluster (reviewer blocker cozystack#1 on the previous round). The flow is now: 1. Reconcile gates on every existing EtcdMember reporting Ready=True before starting another scale-up step. allMembersReady() returns false → requeue 5s. 2. scaleUp first asks etcd for the member list. If any member is a learner, MemberPromote — on success, Requeue; on ErrLearnerNotReady, RequeueAfter 5s (just wait). 3. Otherwise, MemberAddAsLearner + create EtcdMember CR. Crash-safety check still skips MemberAddAsLearner when the peerURL is already in etcd's list (handles operator restart between add and CR creation). 4. Critical subtlety found via live smoke: the iteration that adds the *last* learner reaches current==desired before that learner gets promoted. The steady-state branch now calls promotePendingLearner too — same logic, no extra add. Without this the final member stays a learner forever. Other reviewer blockers ======================= cozystack#2 — discoverMemberID falls back to PeerURL matching when etcd reports a member with empty Name (the window between MemberAddAsLearner and the joiner reporting in). Mirrors what resolveMemberID already did. cozystack#3 — Reconcile short-circuits on cluster.DeletionTimestamp. No more creating Services for a Terminating cluster. cozystack#4 — setMemberCondition returns a bool, mirroring setClusterCondition. updateStatus tracks "changed" across PodName/PVCName/MemberID and condition writes; if nothing changed, Status().Update is skipped. ResourceVersion stable across no-op steady-state reconciles (verified by test). cozystack#5 — gofmt -w on previously drifted files. cozystack#6 — README documents the learner-add-then-promote flow. EtcdClusterClient interface gains MemberAddAsLearner / MemberPromote. fakeEtcd tracks IsLearner state and exposes addLearnerCalls / promoteCalls for assertion. Tests ===== 44 unit tests, six new: - TestScaleUp_WaitsForAllMembersReady (cozystack#1 gate) - TestScaleUp_AddsAsLearner (cozystack#1 happy path) - TestScaleUp_PromotesExistingLearner (cozystack#1 second half) - TestReconcile_PromotesLearnerWhenCurrentEqualsDesired (post-final add — caught only by live smoke testing) - TestReconcile_ShortCircuitsOnDeletion (cozystack#3) - TestDiscoverMemberID_FallsBackToPeerURL (cozystack#2) - TestUpdateStatus_NoChurnInSteadyState (cozystack#4) Verified end-to-end on dev4: 5-member cluster bootstraps, learners appear briefly in etcdctl member list with IS LEARNER=true, then get promoted. Final steady state has IS LEARNER=false for all 5. 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.