Skip to content

Pre-commit via make - #6

Merged
Oleg Kunitsyn (hiddenmarten) merged 5 commits into
mainfrom
pre-commit-init
Mar 14, 2024
Merged

Oleg Kunitsyn (hiddenmarten) merged 5 commits into
mainfrom
pre-commit-init

Conversation

@hiddenmarten

Copy link
Copy Markdown
Contributor

No description provided.

@hiddenmarten
Oleg Kunitsyn (hiddenmarten) force-pushed the pre-commit-init branch 3 times, most recently from 90d8cd4 to bc724c7 Compare March 10, 2024 13:50
@hiddenmarten
Oleg Kunitsyn (hiddenmarten) marked this pull request as ready for review March 10, 2024 19:46
@hiddenmarten

Oleg Kunitsyn (hiddenmarten) commented Mar 10, 2024 •

Copy link
Copy Markdown
Contributor Author

- uses: actions/checkout@v4
- uses: actions/setup-go@v5
with:
go-version: 1.22.1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have version 1.21 in Dockerfile

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, thanks

@sircthulhu Kirill Ilin (sircthulhu) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Comment thread test/e2e/e2e_test.go
Comment on lines -76 to +77
_, err = utils.Run(cmd)
// TODO: Handle CRD installation error
_, _ = utils.Run(cmd)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it still makes sense after #15 get merged?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it's a linter validation issue.

Comment thread README.md Outdated
Comment on lines +4 to +6
## Contributing
### Prerequisites
- [pre-commit](https://pre-commit.com/#installation)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rebase PR, please?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done.

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

5 participants