From 22c02e79815dac269d5ab0f98b5a506b4dbc0292 Mon Sep 17 00:00:00 2001 From: as51340 Date: Tue, 28 Jul 2026 16:05:00 +0200 Subject: [PATCH 1/2] feat: coordinator scale-down with leadership-safe Raft removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Carries out a lowered `coordinators` count, the last scale `14-topology-scale-up.md` accepted at admission but held back. Removing a coordinator means removing a Raft member, and Raft refuses to remove its own leader (`RAFT_CANNOT_REMOVE_LEADER`) — while a StatefulSet sheds only its highest ordinals, so the leader may well sit on one of them. Retiring ordinals are `[spec.coordinators, liveStatefulSet.spec.replicas)`, and because the count must stay odd a shrink always retires an even number of members, so the surviving Raft cluster keeps an odd membership throughout. `YIELD LEADERSHIP` is the lever, and it is the one command whose outcome the planner cannot predict: it must be issued on the current leader — the connection the controller already holds — and it names no successor, because `yield_leadership()` is called without one and NuRaft's election decides. So it is always a plan's **last** command and terminal: the controller stops after it and requeues to re-observe under whichever coordinator won, asking again if the election happened to pick another retiring member. Everything the planner can still order safely goes out ahead of it in that same pass — the retiring coordinators that are not the leader, and the whole data-instance retirement. `REMOVE COORDINATOR` is therefore never aimed at the observed leader. Raft membership is given up before the pods are, so no removed member's vote outlives its pod: the shrink is applied in the one place a replica count is ever lowered, at the end of the registration phase once the plan is empty. The readiness gate stays strict, retiring pods included. `Converged` is False with reason `RetirementInProgress` — now naming the retiring members of both roles — or `LeadershipTransferInProgress` while a yield is pending. A coordinator removed from Raft keeps running and keeps its state on purpose: NuRaft fires `RemovedFromCluster`, stops it campaigning after two election timeouts, and never calls `system_exit`, so the container does not die and the readiness gate is not tripped. It appends nothing, so its log stays a prefix of the leader's and cannot diverge, and a later `ADD COORDINATOR` is accepted unconditionally — a re-added coordinator on a retained volume is in the same position as one whose pod crashed and stayed down. No PVC wipe, no removal bookkeeping, no re-add guard. Its stale view is already handled by `13-coordinator-leader-required.md`. `ScaleInProgress` goes away with this: `applied` is `max(declared, current)`, so a mismatch between the two is now exactly a retirement in flight, and the reason became unreachable rather than merely unused. Tests: planner cases for the leader on a retiring ordinal, on a survivor and outside the retiring set, two coordinators retiring at once, an already-removed retiring member, a retiring leader with nothing else to order, and a retiring coordinator alongside retiring data instances; `RetiringCoordinators` bounds in both directions plus its declared-form equality; envtest specs for the removal order, the yield and the pass that removes under the new leader, the raised-back count, and both roles retiring in one edit. The fake cluster gains `REMOVE COORDINATOR` and `YIELD LEADERSHIP`, refusing a removal aimed at its own leader, so a plan that skipped the yield fails the suite loudly. The scaling e2e container forces leadership onto `coordinator_4`, drops the count to 3, and asserts both members leave the Raft cluster before their pods are shed, that leadership lands on a survivor, that the cluster converges, and that the retired claims are kept by the default retention policy. No CRD or RBAC change, so the chart is untouched. --- CLAUDE.md | 8 +- README.md | 12 +- api/v1alpha1/memgraphcluster_types.go | 22 +-- config/samples/v1alpha1_memgraphcluster.yaml | 17 +- examples/minimal-cluster.yaml | 5 +- internal/controller/fake_memgraph_test.go | 68 ++++++++ .../controller/memgraphcluster_controller.go | 157 +++++++++++------- .../memgraphcluster_controller_test.go | 141 ++++++++++++++-- internal/memgraph/bolt.go | 10 ++ internal/memgraph/client.go | 21 ++- internal/memgraph/queries.go | 9 + internal/memgraph/queries_test.go | 16 ++ internal/planner/planner.go | 107 ++++++++++-- internal/planner/planner_test.go | 151 +++++++++++++++++ internal/resources/topology.go | 49 +++++- internal/resources/topology_test.go | 79 +++++++++ test/e2e/memgraphcluster_test.go | 123 ++++++++++++++ 17 files changed, 874 insertions(+), 121 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ae7d638..e47cad8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -49,10 +49,10 @@ CI (`.github/workflows/`) runs `make lint-config`, `make lint`, `make test-unit` The PRD defines seven modules with two pure cores and one mock seam. Keep this separation — it's what makes the logic testable without a cluster: -1. **API types** (`api/v1alpha1/`) — spec/status with kubebuilder + CEL markers. Replica counts (coordinators, data instances) are **mutable**, bounded by schema floors alone (coordinators ≥ 3 and odd, data instances ≥ 1) — enforced by the CRD, not a webhook. Raising a count grows the cluster; lowering one is accepted but not yet carried out (see `specs/operator-mvp/issues/15`, `16`). Defaults are declared as CRD schema defaults *and* mirrored as Go constants in `memgraphcluster_types.go` so resource builders behave correctly on specs that never passed admission (unit tests). +1. **API types** (`api/v1alpha1/`) — spec/status with kubebuilder + CEL markers. Replica counts (coordinators, data instances) are **mutable in both directions**, bounded by schema floors alone (coordinators ≥ 3 and odd, data instances ≥ 1) — enforced by the CRD, not a webhook. Defaults are declared as CRD schema defaults *and* mirrored as Go constants in `memgraphcluster_types.go` so resource builders behave correctly on specs that never passed admission (unit tests). 2. **Resource builders** — pure functions: spec in, desired Kubernetes objects out (one StatefulSet per role + headless Services; per-pod identity such as coordinator ID and advertised addresses derived from pod ordinals). No API calls, no side effects. Tested golden-style. -3. **Memgraph HA client** — a narrow Go interface (show instances, register instance, add coordinator, set main) over the Bolt driver. Everything above depends on the interface, never the driver — this is the mock seam. -4. **Registration planner** — pure diff: declared topology + observed `SHOW INSTANCES` in, ordered registration commands out (empty when converged). Reconciliation semantics live here: read-before-write, idempotent, re-issue only missing registrations. A MAIN is promoted only when the cluster has none — at bootstrap, and after the planner itself demoted a data instance that is retiring; a MAIN that is staying is never overridden, because failover belongs to the Raft coordinators. A lowered `dataInstances` count is the one removal the planner drives: `DEMOTE INSTANCE` the retiring MAIN, promote a survivor, `UNREGISTER INSTANCE` the retiring members, and only then does the controller shed their pods (see `specs/operator-mvp/issues/15`). +3. **Memgraph HA client** — a narrow Go interface (show instances, register instance, add coordinator, set main, demote/unregister instance, remove coordinator, yield leadership) over the Bolt driver. Everything above depends on the interface, never the driver — this is the mock seam. +4. **Registration planner** — pure diff: declared topology + observed `SHOW INSTANCES` in, ordered registration commands out (empty when converged). Reconciliation semantics live here: read-before-write, idempotent, re-issue only missing registrations. A MAIN is promoted only when the cluster has none — at bootstrap, and after the planner itself demoted a data instance that is retiring; a MAIN that is staying is never overridden, because failover belongs to the Raft coordinators. A lowered count is the one removal the planner drives: `DEMOTE INSTANCE` the retiring MAIN, promote a survivor, `UNREGISTER INSTANCE` the retiring data instances, `REMOVE COORDINATOR` the retiring coordinators, and only then does the controller shed their pods (see `specs/operator-mvp/issues/15`, `16`). One command breaks the pure-diff mould: `YIELD LEADERSHIP`, needed because Raft refuses to remove its own leader. It names no successor, so it is always a plan's **last** command and terminal — the controller requeues and re-observes under whichever coordinator won the election. 5. **Controller** (`internal/controller/`) — fetch CR, server-side-apply builder output, gate on pod readiness, run planner against the HA client, write status/conditions. 6. **Operator install chart** (`charts/memgraph-operator/`) — lives in this repo, cross-published to `memgraph.github.io/helm-charts` at release. Its `crds/` and `rbac/manager-rules.yaml` are **generated** (`make chart-sync`, verified by `make chart-verify`): the manager's ClusterRole comes from the `+kubebuilder:rbac` markers, so tightening or widening the controller's permissions means editing the markers, never the chart. The e2e suite installs the operator through this chart, so every scenario runs under the RBAC users get. The chart's `version` and its `appVersion` (the operator image tag) move **independently**: tag `v` releases the operator, `chart-` releases the chart alone — see `docs/releasing.md`. 7. **E2E harness** (`test/e2e/`, build tag `e2e`) — multi-node KinD with real Memgraph images. @@ -63,6 +63,6 @@ Test philosophy (from the PRD): assert external behavior, never internal call or - Spec knob names mirror the HA Helm chart's vocabulary where the concept carries over (e.g. the `secrets.name` / `secrets.licenseKey` / `secrets.organizationKey` block) — check the chart before inventing a name. - No secret material in spec or status; secrets are consumed by reference only. -- Storage is never deleted by the operator: no finalizer-based cleanup; PVC retention (deletion *and* scale-down) maps to the StatefulSet PVC retention policy (default `Retain`). The only cluster members the operator removes are the data instances a lowered `dataInstances` count retires; coordinators are never removed (`REMOVE COORDINATOR` arrives with `specs/operator-mvp/issues/16`). +- Storage is never deleted by the operator: no finalizer-based cleanup; PVC retention (deletion *and* scale-down) maps to the StatefulSet PVC retention policy (default `Retain`). The only cluster members the operator removes are the ones a lowered replica count retires; a coordinator removed from Raft keeps running and keeps its state on purpose, which is what makes re-growing onto a retained volume safe. - Workload pods: non-root uid 101 / gid 103, seccomp RuntimeDefault, all capabilities dropped. - Log messages follow Kubernetes style: capital first letter, no trailing period, past tense, object type named (see AGENTS.md for examples). diff --git a/README.md b/README.md index bdfb2fb..d9d791f 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ The MVP is deliberately "provision, bootstrap, observe". It does: - bootstrap HA: add the coordinators, register the data instances, and promote the initial MAIN once; - re-register continuously: every reconcile compares `SHOW INSTANCES` on the coordinator leader against the declared topology and issues only the missing registrations, so an instance that loses its registration state (say, after being rescheduled onto a fresh node) rejoins without human action; - **grow a live cluster**: raise `coordinators` or `dataInstances` (both in one edit if you like, in any step size) and the added pods are provisioned and registered by the same diff that restores a lost registration — no manual `ADD COORDINATOR` or `REGISTER INSTANCE`; -- **shrink the data instances**: lower `dataInstances` and the instances above the new count are retired — MAIN moved off them if one of them holds it, then `UNREGISTER INSTANCE`, and only then are their pods shed, so the coordinators never expect an instance whose pod is gone; +- **shrink a live cluster**: lower `dataInstances` or `coordinators` and the members above the new count are retired before their pods are shed — a data instance has MAIN moved off it if it holds it and is then `UNREGISTER INSTANCE`d, a coordinator is `REMOVE COORDINATOR`ed out of the Raft cluster — so the coordinators never expect an instance whose pod is gone, and no removed member's pod outlives its vote; - report the observed MAIN, the registered member counts, and the readiness and convergence conditions on the resource's status. Scaling is one edit, and `Converged` tells you when it is finished: @@ -185,17 +185,17 @@ kubectl patch mgc memgraph -n memgraph --type=merge -p '{"spec":{"coordinators": kubectl wait --namespace memgraph --for=condition=Converged memgraphcluster/memgraph --timeout=10m ``` -A scale-down reports `Converged=False` with reason `RetirementInProgress`, naming the instances on their way out, until their pods are gone. Two things to know about it: +Both counts have a floor the schema enforces at creation and on every update: `coordinators` must stay odd and at or above three, `dataInstances` at or above one. A scale-down reports `Converged=False` with reason `RetirementInProgress`, naming the members on their way out, until their pods are gone. Three things to know about it: -- **A retiring pod that cannot become ready blocks its own removal.** The operator only touches the cluster when every pod of both StatefulSets is ready, and until the shrink is applied the retiring pods still belong to the data StatefulSet. So an instance that is stuck (crash-looping, unschedulable, wedged in a snapshot restore) keeps its own retirement waiting, and the resource reports `WorkloadsNotReady` rather than the operator writing to a cluster whose state it only half knows. Fix the pod, or delete it if it is genuinely unrecoverable, and the retirement continues. -- **The claims of a retired instance follow `spec.storage.retentionPolicy`**, the same knob that decides what happens to storage when the cluster is deleted — `Retain` (the default) keeps them, so a shrink made by accident loses no data, and re-raising the count reattaches them. +- **A retiring pod that cannot become ready blocks its own removal.** The operator only touches the cluster when every pod of both StatefulSets is ready, and until the shrink is applied the retiring pods still belong to their StatefulSet. So a member that is stuck (crash-looping, unschedulable, wedged in a snapshot restore) keeps its own retirement waiting, and the resource reports `WorkloadsNotReady` rather than the operator writing to a cluster whose state it only half knows. Fix the pod, or delete it if it is genuinely unrecoverable, and the retirement continues. +- **The claims of a retired member follow `spec.storage.retentionPolicy`**, the same knob that decides what happens to storage when the cluster is deleted — `Retain` (the default) keeps them, so a shrink made by accident loses no data, and re-raising the count reattaches them. A coordinator removed from Raft keeps running and keeps its state on purpose, which is what makes re-growing onto a retained volume safe: it is in the same position as one whose pod crashed and stayed down, and a later `ADD COORDINATOR` brings it back in. +- **A coordinator shrink may have to wait for a Raft election.** Raft refuses to remove its own leader, and a StatefulSet sheds only its highest ordinals, so a leader sitting in the retiring range is asked to `YIELD LEADERSHIP` first — which cannot name a successor. The resource reports `Converged=False` with reason `LeadershipTransferInProgress` while that is pending, and the operator asks again if the election happens to pick another retiring coordinator. What it does not do yet: -- **Scaling the coordinators down.** `coordinators` must stay odd and at or above three, `dataInstances` at or above one — all enforced at creation and on every update. Lowering `coordinators` is accepted by admission but not carried out: dropping a coordinator means removing a Raft member, which the operator does not do yet, so it holds the StatefulSet at its current size and reports `Converged=False` with reason `ScaleInProgress` until the count is raised back. - **Failover.** The operator promotes a MAIN only when the cluster has none: once at bootstrap, and once more when it demotes an instance that is retiring. It never overrides a MAIN that is staying — leadership belongs to the Raft coordinators, so two control systems never fight over which instance is MAIN. - **Other day-2 operations**: orchestrated or rolling version upgrades, backup and restore, storage-mode changes. -- **Removing coordinators**: there is no `REMOVE COORDINATOR`, and no finalizer-based storage cleanup — deleting storage is left entirely to the StatefulSet's own retention policy. +- **Deleting storage**: the operator owns no finalizer and runs no cleanup of its own — deleting a volume is left entirely to the StatefulSet's own retention policy. - **External access** of any kind — no LoadBalancer, NodePort, ingress or gateway. Access is in-cluster (or `kubectl port-forward`) only; the approach is expected to change, so it was deliberately deferred rather than shipped and broken later. - **TLS**, for Bolt or intra-cluster traffic. - **Bolt authentication** — the operator connects to the coordinators unauthenticated, so clusters must not enable auth yet. diff --git a/api/v1alpha1/memgraphcluster_types.go b/api/v1alpha1/memgraphcluster_types.go index 7146d7d..d78027b 100644 --- a/api/v1alpha1/memgraphcluster_types.go +++ b/api/v1alpha1/memgraphcluster_types.go @@ -163,18 +163,20 @@ const ( // declared topology. ReasonAllInstancesRegistered = "AllInstancesRegistered" - // ReasonScaleInProgress is set when registration has converged but a - // StatefulSet still runs a different number of replicas than the spec - // declares, so the declared topology is not fully realized yet. - ReasonScaleInProgress = "ScaleInProgress" - - // ReasonRetirementInProgress is set while a lowered dataInstances count is - // being carried out: the instances beyond the declared count are still - // members of the cluster, or their pods are still being shed. The message - // names them, so a scale-down that stalls says which instance it is waiting - // on. + // ReasonRetirementInProgress is set while a lowered count of either role is + // being carried out: the members beyond the declared count are still part of + // the cluster, or their pods are still being shed. The message names them, so + // a scale-down that stalls says which member it is waiting on. ReasonRetirementInProgress = "RetirementInProgress" + // ReasonLeadershipTransferInProgress is set while a lowered coordinators + // count is waiting on Raft leadership to move: Raft refuses to remove its own + // leader, so a retiring coordinator holding leadership is asked to yield it + // first. YIELD LEADERSHIP cannot name a successor, so the operator re-observes + // the cluster under whichever coordinator won the election and may have to ask + // again — which is exactly what this reason means when it persists. + ReasonLeadershipTransferInProgress = "LeadershipTransferInProgress" + // ReasonMainElected is set when a data instance is observed as MAIN. ReasonMainElected = "MainElected" diff --git a/config/samples/v1alpha1_memgraphcluster.yaml b/config/samples/v1alpha1_memgraphcluster.yaml index 24bf81c..8bba63b 100644 --- a/config/samples/v1alpha1_memgraphcluster.yaml +++ b/config/samples/v1alpha1_memgraphcluster.yaml @@ -11,14 +11,15 @@ spec: # odd so the Raft quorum cannot split, and at least three — a quorum of one # cannot survive losing itself. # - # Lowering dataInstances shrinks the cluster: the instances above the new count - # are retired (MAIN moved off them, then UNREGISTER INSTANCE) before their pods - # are shed, reported as Converged=False with reason RetirementInProgress. Note - # that the operator only touches the cluster while every pod is ready, so a - # retiring pod that cannot become ready blocks its own removal. Lowering - # coordinators is not supported yet: it is accepted at admission but the - # operator holds the StatefulSet at its current size and reports - # Converged=False with reason ScaleInProgress. + # Lowering either count shrinks the cluster: the members above the new count are + # retired before their pods are shed — a data instance has MAIN moved off it and + # is then UNREGISTER INSTANCEd, a coordinator is REMOVE COORDINATORed out of the + # Raft cluster — reported as Converged=False with reason RetirementInProgress. + # Two notes. The operator only touches the cluster while every pod is ready, so + # a retiring pod that cannot become ready blocks its own removal. And Raft + # refuses to remove its own leader, so a leader in the retiring range is asked + # to YIELD LEADERSHIP first, reported as LeadershipTransferInProgress while that + # is pending. coordinators: 3 dataInstances: 2 # repository carries the registry host and image path only — the version diff --git a/examples/minimal-cluster.yaml b/examples/minimal-cluster.yaml index 3098272..9c13fe0 100644 --- a/examples/minimal-cluster.yaml +++ b/examples/minimal-cluster.yaml @@ -14,8 +14,9 @@ spec: # Raise either count later to grow the cluster: the operator provisions the # new pods and registers them, no manual registration involved. The # coordinator count must be odd so the Raft quorum cannot split, and at least - # three. Lowering dataInstances retires the instances above the new count — - # unregistered before their pods go; lowering coordinators is not supported yet. + # three. Lower either count and the members above it are retired first — a data + # instance unregistered, a coordinator removed from the Raft cluster — before + # their pods go. coordinators: 3 dataInstances: 2 image: diff --git a/internal/controller/fake_memgraph_test.go b/internal/controller/fake_memgraph_test.go index e3bee3f..69734fb 100644 --- a/internal/controller/fake_memgraph_test.go +++ b/internal/controller/fake_memgraph_test.go @@ -88,6 +88,23 @@ func (f *fakeMemgraph) setInstances(instances []memgraph.Instance) { f.instances = slices.Clone(instances) } +// setLeader moves Raft leadership onto the named coordinator, which is how a spec +// parks it where a scale-down cannot remove it: on an ordinal the shrink retires. +func (f *fakeMemgraph) setLeader(name string) { + f.mu.Lock() + defer f.mu.Unlock() + for i, instance := range f.instances { + if !strings.HasPrefix(instance.Name, "coordinator_") { + continue + } + role := memgraph.RoleFollower + if instance.Name == name { + role = memgraph.RoleLeader + } + f.instances[i].Role = role + } +} + // setStaleView makes the coordinator at the given Bolt address answer // SHOW INSTANCES with its own view instead of the cluster's. func (f *fakeMemgraph) setStaleView(address string, instances []memgraph.Instance) { @@ -240,6 +257,57 @@ func (c *fakeClient) UnregisterInstance(_ context.Context, name string) error { }) } +// RemoveCoordinator drops the coordinator with the given Raft ID from the cluster +// view and — as Raft does — refuses the current leader, so a plan that aims a +// removal at the leader fails the suite loudly instead of quietly working. +func (c *fakeClient) RemoveCoordinator(_ context.Context, id int32) error { + name := fmt.Sprintf("coordinator_%d", id) + return c.execute(fmt.Sprintf("REMOVE COORDINATOR %d", id), func() error { + for i, instance := range c.cluster.instances { + if instance.Name != name { + continue + } + if instance.IsLeader() { + return fmt.Errorf("fake memgraph: %s is the leader", name) + } + c.cluster.instances = slices.Delete(c.cluster.instances, i, i+1) + return nil + } + return fmt.Errorf("fake memgraph: coordinator %s is not a member", name) + }) +} + +// YieldLeadership moves leadership off the coordinator serving this connection to +// the lowest-numbered remaining member, standing in for the election NuRaft runs. +// A test cannot rely on which coordinator wins — that is the point of the command +// — only on leadership having moved, which is what the operator has to converge +// around. +func (c *fakeClient) YieldLeadership(context.Context) error { + self, err := c.selfName() + if err != nil { + return err + } + return c.execute("YIELD LEADERSHIP", func() error { + successor := -1 + for i, instance := range c.cluster.instances { + if strings.HasPrefix(instance.Name, "coordinator_") && instance.Name != self { + successor = i + break + } + } + if successor < 0 { + return fmt.Errorf("fake memgraph: %s is the only coordinator, so leadership cannot be yielded", self) + } + for i, instance := range c.cluster.instances { + if instance.Name == self { + c.cluster.instances[i].Role = memgraph.RoleFollower + } + } + c.cluster.instances[successor].Role = memgraph.RoleLeader + return nil + }) +} + func (c *fakeClient) Close(context.Context) error { c.cluster.mu.Lock() defer c.cluster.mu.Unlock() diff --git a/internal/controller/memgraphcluster_controller.go b/internal/controller/memgraphcluster_controller.go index 3ea5e6d..79d90f8 100644 --- a/internal/controller/memgraphcluster_controller.go +++ b/internal/controller/memgraphcluster_controller.go @@ -191,50 +191,68 @@ type replicaCounts struct { data roleReplicas } -// scaleMessage describes the roles whose StatefulSet does not run the declared -// number of replicas, and is empty once both do — which is what widens -// Converged from "registration matches the declared topology" to "the declared -// topology is actually running". -func (c replicaCounts) scaleMessage() string { - var pending []string - for _, role := range []roleReplicas{c.coordinators, c.data} { - if role.applied != role.declared { - pending = append(pending, fmt.Sprintf("StatefulSet %s runs %d replica(s) while %d are declared", - role.name, role.applied, role.declared)) - } +// retirementMessage names the members a lowered count is shedding, and is empty +// when none are. It is non-empty for exactly as long as the retirement is +// unfinished: the retiring sets are derived from the replica counts the +// operator's own StatefulSets still run, so they empty only once the shrink that +// removes those pods has been applied. +func retirementMessage(topology planner.Topology) string { + var retiring []string + if names := coordinatorNames(topology.RetiringCoordinators); len(names) > 0 { + retiring = append(retiring, "coordinator(s) "+strings.Join(names, ", ")) + } + if names := instanceNames(topology.RetiringDataInstances); len(names) > 0 { + retiring = append(retiring, "data instance(s) "+strings.Join(names, ", ")) + } + if len(retiring) == 0 { + return "" } - return strings.Join(pending, "; ") + return "Retiring " + strings.Join(retiring, " and ") + " before their pods are shed" } -// retirementMessage names the data instances a lowered count is shedding, and is -// empty when none are. It is non-empty for exactly as long as the retirement is -// unfinished: the retiring set is derived from the replica count the operator's -// own StatefulSet still runs, so it empties only once the shrink that removes -// those pods has been applied. -func retirementMessage(topology planner.Topology) string { - if len(topology.RetiringDataInstances) == 0 { - return "" +func coordinatorNames(coordinators []memgraph.CoordinatorSpec) []string { + names := make([]string, 0, len(coordinators)) + for _, coordinator := range coordinators { + names = append(names, coordinator.Name()) } - names := make([]string, 0, len(topology.RetiringDataInstances)) - for _, instance := range topology.RetiringDataInstances { + return names +} + +func instanceNames(instances []memgraph.DataInstanceSpec) []string { + names := make([]string, 0, len(instances)) + for _, instance := range instances { names = append(names, instance.Name) } - return "Retiring data instance(s) " + strings.Join(names, ", ") + - " before their pods are shed" + return names +} + +// yieldedLeader is the retiring coordinator a plan ends by moving Raft leadership +// off, or the empty string when the plan does not do that. A yield is always the +// plan's last command, because nothing after it could be planned: the election +// picks the successor, so the pass stops there and the next one observes the +// cluster under whoever won. +func yieldedLeader(commands []planner.Command) string { + if len(commands) == 0 { + return "" + } + yield, ok := commands[len(commands)-1].(planner.YieldLeadership) + if !ok { + return "" + } + return yield.Leader } // replicaCounts resolves the replica count to apply per role: the declared count // while the cluster grows or holds its size, and deliberately the current count // while a lowered count would shrink it. Shedding pods means removing members // from the Memgraph cluster first — the coordinators otherwise keep expecting -// instances whose pods are gone — so this rule never shrinks anything, which -// keeps it free of any knowledge about the cluster's state. +// instances whose pods are gone, and a removed coordinator's vote must be given +// up before its pod is — so this rule never shrinks anything, which keeps it free +// of any knowledge about the cluster's state. // -// Lowering the data-instance count is carried out at the end of the registration +// A lowered count of either role is carried out at the end of the registration // phase instead, once the retiring members have actually left the cluster (see -// reconcileRegistration). Lowering the coordinator count is not carried out at -// all yet: the size is held and the mismatch reported, rather than acting on half -// of a scale-down the operator cannot finish. +// reconcileRegistration). func (r *MemgraphClusterReconciler) replicaCounts( ctx context.Context, cluster *memgraphcomv1alpha1.MemgraphCluster, @@ -292,11 +310,12 @@ func (r *MemgraphClusterReconciler) currentReplicas( // removal, and the resource reports WorkloadsNotReady rather than the operator // acting on a half-known cluster. // -// This is also where a data-instance scale-down finishes. Once the plan comes -// back empty — meaning the retiring instances have left the cluster — the data -// StatefulSet is applied at the declared count, shedding their pods. That is the -// one place the operator ever lowers a replica count, so the coordinators never -// see a registered instance's pod disappear. +// This is also where a scale-down finishes. Once the plan comes back empty — +// meaning the retiring instances have been unregistered and the retiring +// coordinators have left the Raft cluster — the shrinking role's StatefulSet is +// applied at the declared count, shedding their pods. That is the one place the +// operator ever lowers a replica count, so the coordinators never see a registered +// instance's pod disappear, and no removed member's pod outlives its vote. func (r *MemgraphClusterReconciler) reconcileRegistration( ctx context.Context, cluster *memgraphcomv1alpha1.MemgraphCluster, @@ -324,6 +343,7 @@ func (r *MemgraphClusterReconciler) reconcileRegistration( // The members a lowered count is shedding are the ordinals the operator's own // previous apply still runs beyond the declared count, so the range is bounded // by what the operator itself created. + topology.RetiringCoordinators = resources.RetiringCoordinators(cluster, replicas.coordinators.applied) topology.RetiringDataInstances = resources.RetiringDataInstances(cluster, replicas.data.applied) // Whether a retirement is in flight is decided once per pass, from the // topology alone: it is what both the condition and the shrink below key off. @@ -356,15 +376,13 @@ func (r *MemgraphClusterReconciler) reconcileRegistration( latest := observe(topology, observed) commands := planner.Plan(topology, observed) if len(commands) == 0 { - // No retiring instance is a member of the cluster any more — the plan - // would carry an UNREGISTER INSTANCE otherwise — so their pods can go. + // No retiring member belongs to the cluster any more — the plan would + // carry an UNREGISTER INSTANCE or a REMOVE COORDINATOR otherwise — so + // their pods can go. if retiring != "" { - if err := r.applyDesired(ctx, cluster, - resources.DataStatefulSet(cluster, replicas.data.declared)); err != nil { + if err := r.shedRetiredPods(ctx, cluster, topology, replicas); err != nil { return ctrl.Result{}, err } - log.Info("Shrank the data StatefulSet to the declared replica count", - "statefulset", replicas.data.name, "replicas", replicas.data.declared) if statusErr := r.writeStatus(ctx, cluster, latest, readyOrNot(latest.main), notConvergedCondition(memgraphcomv1alpha1.ReasonRetirementInProgress, retiring), ); statusErr != nil { @@ -375,20 +393,6 @@ func (r *MemgraphClusterReconciler) reconcileRegistration( return ctrl.Result{RequeueAfter: requeueAfterRegistration}, nil } - // Registration matches the declared topology. It is only converged once - // the StatefulSets run the declared replica counts too, so a scale the - // operator is holding back keeps the condition False and says which - // role and by how much. - if pending := replicas.scaleMessage(); pending != "" { - log.Info("Held a StatefulSet short of the declared replica count", "reason", pending) - if statusErr := r.writeStatus(ctx, cluster, latest, readyOrNot(latest.main), - notConvergedCondition(memgraphcomv1alpha1.ReasonScaleInProgress, pending), - ); statusErr != nil { - return ctrl.Result{}, statusErr - } - return ctrl.Result{RequeueAfter: resyncInterval}, nil - } - // Converged, but keep re-observing: a registration a pod loses later // produces no watch event, so drift is only caught by resyncing. log.Info("Confirmed cluster registration is converged") @@ -405,12 +409,21 @@ func (r *MemgraphClusterReconciler) reconcileRegistration( // serving stays Ready while a lost registration is restored; a fresh // bootstrap has no MAIN yet, so Ready is False until one is elected. A // retirement in flight is named as such — it is the more specific operation, - // and the one whose pending members a user wants to see. + // and the one whose pending members a user wants to see. A pending leadership + // yield is more specific still: it is the one step whose outcome nobody can + // predict, so a scale-down circling it says so rather than looking stuck on + // the removal it cannot reach yet. reason := memgraphcomv1alpha1.ReasonRegistrationInProgress message := fmt.Sprintf("Issuing %d registration command(s) to converge the cluster", len(commands)) if retiring != "" { reason, message = memgraphcomv1alpha1.ReasonRetirementInProgress, retiring } + if yielded := yieldedLeader(commands); yielded != "" { + reason = memgraphcomv1alpha1.ReasonLeadershipTransferInProgress + message = fmt.Sprintf( + "Retiring coordinator %s holds Raft leadership, which cannot be removed: yielding it to another member", + yielded) + } inProgress := notConvergedCondition(reason, message) if statusErr := r.writeStatus(ctx, cluster, latest, readyOrNot(latest.main), inProgress); statusErr != nil { return ctrl.Result{}, statusErr @@ -428,6 +441,38 @@ func (r *MemgraphClusterReconciler) reconcileRegistration( return ctrl.Result{RequeueAfter: requeueAfterRegistration}, nil } +// shedRetiredPods applies the shrinking roles' StatefulSets at their declared +// replica counts — the one place the operator ever lowers a replica count. It is +// reached only after the plan came back empty, so every pod it sheds belongs to a +// member that has already left the Memgraph cluster: an unregistered data +// instance, or a coordinator whose Raft vote is gone. +func (r *MemgraphClusterReconciler) shedRetiredPods( + ctx context.Context, + cluster *memgraphcomv1alpha1.MemgraphCluster, + topology planner.Topology, + replicas replicaCounts, +) error { + log := logf.FromContext(ctx) + for _, role := range []struct { + retiring int + replicas roleReplicas + build func(*memgraphcomv1alpha1.MemgraphCluster, int32) *appsv1.StatefulSet + }{ + {len(topology.RetiringCoordinators), replicas.coordinators, resources.CoordinatorStatefulSet}, + {len(topology.RetiringDataInstances), replicas.data, resources.DataStatefulSet}, + } { + if role.retiring == 0 { + continue + } + if err := r.applyDesired(ctx, cluster, role.build(cluster, role.replicas.declared)); err != nil { + return err + } + log.Info("Shrank a StatefulSet to the declared replica count", + "statefulset", role.replicas.name, "replicas", role.replicas.declared) + } + return nil +} + // observation is everything a reconcile pass observed about the cluster that // reaches the resource's status: which data instance is MAIN, and how many of // each role's declared members are registered. It is observation only — no diff --git a/internal/controller/memgraphcluster_controller_test.go b/internal/controller/memgraphcluster_controller_test.go index cfeb6ca..e77a45d 100644 --- a/internal/controller/memgraphcluster_controller_test.go +++ b/internal/controller/memgraphcluster_controller_test.go @@ -764,14 +764,13 @@ var _ = Describe("MemgraphCluster Controller", func() { Expect(converged.Reason).To(Equal(memgraphcomv1alpha1.ReasonAllInstancesRegistered)) }) - // Lowering the coordinator count is the scale the operator still cannot - // realize: removing a Raft member is not implemented, so the StatefulSet is - // held at its current size and the resource says so rather than the - // operator acting on half of a scale-down it cannot finish. - It("should report Converged False while a lowered coordinator count is held back", func() { + // grownToFive drives the cluster to a converged five-coordinator topology, + // which is the only shape a coordinator shrink can start from: the count must + // stay odd and at or above three, so five is the smallest cluster with members + // to drop. It returns the command count the shrink assertions start from. + grownToFive := func() int { + GinkgoHelper() bootstrapped() - - By("growing the coordinators so there is a member to drop") setCounts(5, 2) reconcileCluster(resourceName) markWorkloadsReady(resourceName) @@ -779,29 +778,141 @@ var _ = Describe("MemgraphCluster Controller", func() { reconcileCluster(resourceName) Expect(apimeta.IsStatusConditionTrue(status().Conditions, memgraphcomv1alpha1.ConditionConverged)).To(BeTrue()) - grown := len(fake.executedCommands()) + return len(fake.executedCommands()) + } + + // Raft membership is given up before the pods are, so no removed member's pod + // outlives its vote. With the leader on a survivor that is the whole shrink: + // removing a follower needs no leadership dance. + It("should remove retiring coordinators from Raft and only then shed their pods", func() { + baseline := grownToFive() setCounts(3, 2) reconcileCluster(resourceName) + leader := coordinatorAddress(0) + Expect(sinceBootstrap(baseline)).To(Equal([]string{ + leader + ": REMOVE COORDINATOR 4", + leader + ": REMOVE COORDINATOR 5", + }), "both retiring members leave the Raft cluster in one pass under a surviving leader") Expect(replicas(coordinatorSuffix)).To(Equal(int32(5)), - "a lower declared count must never shrink the applied StatefulSet") + "a pass with pending commands must never lower the replica count") converged := convergedCondition() Expect(converged.Status).To(Equal(metav1.ConditionFalse)) - Expect(converged.Reason).To(Equal(memgraphcomv1alpha1.ReasonScaleInProgress)) - Expect(converged.Message).To(ContainSubstring(resourceName + coordinatorSuffix)) - Expect(sinceBootstrap(grown)).To(BeEmpty(), - "the coordinators the lowered count drops stay registered: removal is not implemented") + Expect(converged.Reason).To(Equal(memgraphcomv1alpha1.ReasonRetirementInProgress)) + Expect(converged.Message).To(ContainSubstring("coordinator_4"), + "the condition must name the coordinators being retired") - // Raising the count back matches what is running, which converges - // again without touching the cluster. + By("shedding the pods once the members have left the Raft cluster") + reconcileCluster(resourceName) + Expect(replicas(coordinatorSuffix)).To(Equal(int32(3))) + Expect(sinceBootstrap(baseline)).To(HaveLen(2), "the removals are not re-issued") + Expect(convergedCondition().Reason).To(Equal(memgraphcomv1alpha1.ReasonRetirementInProgress)) + + By("reporting the shrink as finished once the StatefulSet runs the declared count") + reconcileCluster(resourceName) + s := status() + Expect(s.Coordinators).To(Equal(int32(3))) + Expect(s.Main).To(Equal("instance_0"), "shrinking the coordinators does not move MAIN") + Expect(apimeta.IsStatusConditionTrue(s.Conditions, + memgraphcomv1alpha1.ConditionConverged)).To(BeTrue()) + }) + + // A StatefulSet sheds its highest ordinals, so the Raft leader may well sit on + // one of them — and Raft refuses to remove its own leader. The plan then ends + // with YIELD LEADERSHIP and nothing after it, because the election picks the + // successor: the pass stops there and the next one removes under whoever won. + // The fake refuses a removal aimed at its leader, so a plan that skipped the + // yield would fail this spec rather than quietly working. + It("should yield leadership off a retiring coordinator before removing it", func() { + baseline := grownToFive() + + By("parking Raft leadership on the coordinator the shrink retires") + fake.setLeader("coordinator_4") + + setCounts(3, 2) + reconcileCluster(resourceName) + + retiringLeader := coordinatorAddress(3) + Expect(sinceBootstrap(baseline)).To(Equal([]string{ + retiringLeader + ": REMOVE COORDINATOR 5", + retiringLeader + ": YIELD LEADERSHIP", + }), "the yield comes last, after the removal the planner could still order safely") + converged := convergedCondition() + Expect(converged.Status).To(Equal(metav1.ConditionFalse)) + Expect(converged.Reason).To(Equal(memgraphcomv1alpha1.ReasonLeadershipTransferInProgress)) + Expect(converged.Message).To(ContainSubstring("coordinator_4"), + "the condition must name the coordinator being moved off leadership") + Expect(replicas(coordinatorSuffix)).To(Equal(int32(5)), + "a pass with a pending yield must never lower the replica count") + + By("removing the former leader on the next pass, under whichever coordinator won") + reconcileCluster(resourceName) + Expect(sinceBootstrap(baseline)).To(Equal([]string{ + retiringLeader + ": REMOVE COORDINATOR 5", + retiringLeader + ": YIELD LEADERSHIP", + coordinatorAddress(0) + ": REMOVE COORDINATOR 4", + })) + Expect(convergedCondition().Reason).To(Equal(memgraphcomv1alpha1.ReasonRetirementInProgress)) + + By("shedding the pods and converging once the Raft cluster is down to three") + reconcileCluster(resourceName) + Expect(replicas(coordinatorSuffix)).To(Equal(int32(3))) + reconcileCluster(resourceName) + s := status() + Expect(s.Coordinators).To(Equal(int32(3))) + Expect(apimeta.IsStatusConditionTrue(s.Conditions, + memgraphcomv1alpha1.ConditionConverged)).To(BeTrue()) + }) + + // Raising the count back to what is already running is a no-op scale: the + // members are still registered, so nothing is planned and nothing is applied. + It("should converge again when a lowered coordinator count is raised back", func() { + baseline := grownToFive() + + setCounts(3, 2) setCounts(5, 2) reconcileCluster(resourceName) + Expect(sinceBootstrap(baseline)).To(BeEmpty(), + "a shrink that was undone before it was acted on touches the cluster not at all") + Expect(replicas(coordinatorSuffix)).To(Equal(int32(5))) Expect(apimeta.IsStatusConditionTrue(status().Conditions, memgraphcomv1alpha1.ConditionConverged)).To(BeTrue()) }) + // Both counts lowered in one edit: each role's retirement is planned + // independently, and both StatefulSets shrink once the plan is empty. + It("should retire members of both roles in one edit", func() { + baseline := grownToFive() + + setCounts(3, 1) + reconcileCluster(resourceName) + + leader := coordinatorAddress(0) + Expect(sinceBootstrap(baseline)).To(Equal([]string{ + leader + ": UNREGISTER INSTANCE instance_1", + leader + ": REMOVE COORDINATOR 4", + leader + ": REMOVE COORDINATOR 5", + }), "the surviving MAIN is left alone, and each role's removals are planned on their own") + Expect(replicas(coordinatorSuffix)).To(Equal(int32(5))) + Expect(replicas(dataSuffix)).To(Equal(int32(2))) + Expect(convergedCondition().Message).To(SatisfyAll( + ContainSubstring("coordinator_4"), ContainSubstring("instance_1")), + "the condition must name the retiring members of both roles") + + reconcileCluster(resourceName) + Expect(replicas(coordinatorSuffix)).To(Equal(int32(3))) + Expect(replicas(dataSuffix)).To(Equal(int32(1))) + + reconcileCluster(resourceName) + s := status() + Expect(s.Coordinators).To(Equal(int32(3))) + Expect(s.DataInstances).To(Equal(int32(1))) + Expect(apimeta.IsStatusConditionTrue(s.Conditions, + memgraphcomv1alpha1.ConditionConverged)).To(BeTrue()) + }) + // The whole point of the shrink: the member beyond the declared count leaves // the cluster before its pod does, so the coordinators never expect an // instance whose pod is gone. diff --git a/internal/memgraph/bolt.go b/internal/memgraph/bolt.go index 6abbc13..b587bda 100644 --- a/internal/memgraph/bolt.go +++ b/internal/memgraph/bolt.go @@ -85,6 +85,16 @@ func (c *boltClient) UnregisterInstance(ctx context.Context, name string) error return err } +func (c *boltClient) RemoveCoordinator(ctx context.Context, id int32) error { + _, err := c.run(ctx, removeCoordinatorQuery(id)) + return err +} + +func (c *boltClient) YieldLeadership(ctx context.Context) error { + _, err := c.run(ctx, yieldLeadershipQuery) + return err +} + func (c *boltClient) Close(ctx context.Context) error { return c.driver.Close(ctx) } diff --git a/internal/memgraph/client.go b/internal/memgraph/client.go index 891c3fd..aa6f6bd 100644 --- a/internal/memgraph/client.go +++ b/internal/memgraph/client.go @@ -16,8 +16,9 @@ limitations under the License. // Package memgraph provides the narrow client surface the operator uses to // drive a Memgraph high-availability cluster over Bolt: show instances, add -// coordinator, register instance, set main, and — for a data instance a lowered -// replica count is retiring — demote and unregister. All higher layers depend on +// coordinator, register instance, set main, and — for the members a lowered +// replica count is retiring — demote and unregister a data instance, yield +// coordinator leadership and remove a coordinator. All higher layers depend on // the Client and Connector interfaces, never on the Bolt driver — this package // is the mock seam for testing and the only place the driver is referenced. package memgraph @@ -111,6 +112,22 @@ type Client interface { // the coordinators stop expecting it before its pod goes away. UnregisterInstance(ctx context.Context, name string) error + // RemoveCoordinator drops the coordinator with the given Raft ID from the + // Raft cluster, so its vote is gone before its pod is. Raft refuses to + // remove its own leader, so the caller must never aim this at the leader — + // YieldLeadership moves leadership away first. + // + // The removed coordinator keeps running and keeps its state: NuRaft only + // stops it from campaigning, which is what makes a later ADD COORDINATOR on + // the retained volume safe. + RemoveCoordinator(ctx context.Context, id int32) error + + // YieldLeadership makes the coordinator this client is connected to give up + // Raft leadership. It has to be issued on the leader itself and cannot name + // a successor — NuRaft's election picks one — so its outcome is not + // predictable and the caller must re-observe the cluster afterwards. + YieldLeadership(ctx context.Context) error + Close(ctx context.Context) error } diff --git a/internal/memgraph/queries.go b/internal/memgraph/queries.go index 70ce04c..9b01e2c 100644 --- a/internal/memgraph/queries.go +++ b/internal/memgraph/queries.go @@ -56,3 +56,12 @@ func demoteInstanceQuery(name string) string { func unregisterInstanceQuery(name string) string { return fmt.Sprintf("UNREGISTER INSTANCE %s", name) } + +func removeCoordinatorQuery(id int32) string { + return fmt.Sprintf("REMOVE COORDINATOR %d", id) +} + +// yieldLeadershipQuery takes no argument on purpose: Memgraph's grammar has no +// successor to name, so the coordinator it runs on hands leadership to whichever +// member NuRaft's election picks. +const yieldLeadershipQuery = "YIELD LEADERSHIP" diff --git a/internal/memgraph/queries_test.go b/internal/memgraph/queries_test.go index a3a7a99..ecb944e 100644 --- a/internal/memgraph/queries_test.go +++ b/internal/memgraph/queries_test.go @@ -78,6 +78,22 @@ func TestUnregisterInstanceQuery(t *testing.T) { } } +func TestRemoveCoordinatorQuery(t *testing.T) { + got := removeCoordinatorQuery(4) + if want := "REMOVE COORDINATOR 4"; got != want { + t.Errorf("removeCoordinatorQuery() = %q, want %q", got, want) + } +} + +// YIELD LEADERSHIP names no successor: the coordinator it runs on is the subject, +// and NuRaft picks who takes over. A query that grew an argument would mean the +// planner could suddenly predict the outcome, so the shape is pinned. +func TestYieldLeadershipQuery(t *testing.T) { + if want := "YIELD LEADERSHIP"; yieldLeadershipQuery != want { + t.Errorf("yieldLeadershipQuery = %q, want %q", yieldLeadershipQuery, want) + } +} + func TestInstanceFromRecord(t *testing.T) { record := &db.Record{ Keys: []string{ diff --git a/internal/planner/planner.go b/internal/planner/planner.go index e5f48b4..2be75c5 100644 --- a/internal/planner/planner.go +++ b/internal/planner/planner.go @@ -23,10 +23,17 @@ limitations under the License. // the Raft coordinators; the planner never overrides a MAIN that is staying. // // Members a lowered replica count is retiring are the one thing the planner -// removes, and it can order the whole removal in a single pass because it -// predicts every intermediate state: a retiring MAIN is demoted, a survivor is -// promoted in its place, and the retiring members are then unregistered — so no -// UNREGISTER INSTANCE is ever aimed at a MAIN, which Memgraph would refuse. +// removes, and it orders each removal so that Memgraph never has to refuse it: a +// retiring MAIN is demoted and a survivor promoted in its place before any +// UNREGISTER INSTANCE, and a retiring coordinator is only ever removed from Raft +// while it is not the leader. +// +// One command breaks the pure-diff mould: YIELD LEADERSHIP, which moves +// coordinator leadership off a retiring coordinator so it can be removed at all. +// It cannot name a successor, so its outcome is the one thing the planner cannot +// predict — which is why it is always the last command of a plan. Everything the +// planner can still order safely goes out ahead of it in the same pass, and the +// caller re-observes the cluster under whichever coordinator won the election. package planner import ( @@ -48,9 +55,10 @@ type Topology struct { // longer declared. They are empty while a cluster grows or holds its size. // // A retiring data instance is demoted if it holds MAIN and then - // unregistered, so the cluster stops expecting it before its pod goes. - // RetiringCoordinators is still always empty: removing a Raft member is not - // implemented yet, so a plan issues no command for one. + // unregistered, so the cluster stops expecting it before its pod goes. A + // retiring coordinator is removed from the Raft cluster, which Raft only + // allows for a member that is not the leader — so leadership is yielded away + // from a retiring leader first. RetiringCoordinators []memgraph.CoordinatorSpec RetiringDataInstances []memgraph.DataInstanceSpec } @@ -136,14 +144,63 @@ func (c UnregisterInstance) String() string { return "UNREGISTER INSTANCE " + c.Name } +// RemoveCoordinator drops a retiring coordinator from the Raft cluster, so its +// vote is gone before its pod is. It is never aimed at the observed leader: Raft +// refuses to remove its own leader, which is what YieldLeadership is for. +// +// The removed coordinator keeps running and keeps its state — NuRaft only stops +// it campaigning — so nothing here has to be undone before a raised count adds +// it back on its retained volume. +type RemoveCoordinator struct { + Coordinator memgraph.CoordinatorSpec +} + +// Run implements Command. +func (c RemoveCoordinator) Run(ctx context.Context, client memgraph.Client) error { + return client.RemoveCoordinator(ctx, c.Coordinator.ID) +} + +func (c RemoveCoordinator) String() string { + return fmt.Sprintf("REMOVE COORDINATOR %d", c.Coordinator.ID) +} + +// YieldLeadership hands Raft leadership away from the retiring coordinator that +// currently holds it, which is the only way it can then be removed. It runs on +// the leader — the connection the caller already holds — and cannot name a +// successor, so the plan it ends says nothing about who takes over: the caller +// re-observes the cluster and plans again under the new leader. +type YieldLeadership struct { + // Leader is the retiring coordinator giving leadership up. It is carried for + // the sake of whoever is watching the scale-down; the query itself has no + // argument, and no successor can be named. + Leader string +} + +// Run implements Command. +func (c YieldLeadership) Run(ctx context.Context, client memgraph.Client) error { + return client.YieldLeadership(ctx) +} + +func (c YieldLeadership) String() string { + return "YIELD LEADERSHIP" +} + // Plan diffs the declared topology against the observed instances and returns // the commands still needed, in execution order: coordinators before data // instances (registration requires a formed Raft cluster), then the retirement // of the members a lowered count sheds — demote a retiring MAIN, promote a -// survivor in its place, unregister every retiring member. The promotion sits -// between the two so that no UNREGISTER INSTANCE is ever aimed at an observed -// MAIN, and so the cluster is MAIN-less only for the few milliseconds between -// two queries of the same pass. +// survivor in its place, unregister every retiring data instance, remove every +// retiring coordinator from Raft. The promotion sits between the demotion and the +// unregistrations so that no UNREGISTER INSTANCE is ever aimed at an observed +// MAIN, and so the cluster is MAIN-less only for the few milliseconds between two +// queries of the same pass. +// +// A retiring coordinator that holds Raft leadership cannot be removed at all, so +// the plan ends with YIELD LEADERSHIP instead and stops there — the retiring +// coordinators that are not the leader still go out ahead of it in that same +// pass. Nothing follows a yield, because nothing after it could be planned: the +// election picks the next leader, and the caller has to observe the cluster again +// to learn who won. // // Instances the cluster knows but the topology neither declares nor retires are // left untouched: the retiring set is bounded by the operator's own prior apply, @@ -183,9 +240,37 @@ func Plan(declared Topology, observed []memgraph.Instance) []Command { commands = append(commands, UnregisterInstance{Name: instance.Name}) } } + + leader := leaderName(observed) + yieldFrom := "" + for _, coordinator := range declared.RetiringCoordinators { + if coordinator.Name() == leader { + // Raft refuses to remove its own leader, so this one waits for the + // yield below to move leadership to another member. + yieldFrom = leader + continue + } + if coordinatorRegistered(registered, coordinator) { + commands = append(commands, RemoveCoordinator{Coordinator: coordinator}) + } + } + if yieldFrom != "" { + commands = append(commands, YieldLeadership{Leader: yieldFrom}) + } return commands } +// leaderName is the coordinator the observed view reports as Raft leader, or the +// empty string when it reports none. +func leaderName(observed []memgraph.Instance) string { + for _, instance := range observed { + if instance.IsLeader() { + return instance.Name + } + } + return "" +} + // Registered reports how many of the declared coordinators and data instances // the observed cluster has registered. It is pure observation for the CR's // status, and it shares Plan's definition of "registered" — so a role's count diff --git a/internal/planner/planner_test.go b/internal/planner/planner_test.go index 26c8e0a..bab10e1 100644 --- a/internal/planner/planner_test.go +++ b/internal/planner/planner_test.go @@ -39,6 +39,11 @@ const ( thirdInstance = "instance_2" ) +// fourthCoordinator is the lowest-numbered coordinator a shrink from five to +// three retires, and the one the cases below park Raft leadership on: a +// StatefulSet sheds its highest ordinals, so the leader may well sit on one. +const fourthCoordinator = "coordinator_4" + // declaredTopology is the canonical 3-coordinator, 2-data-instance fixture // the cases below diff observed cluster states against. func declaredTopology() planner.Topology { @@ -70,6 +75,26 @@ func mixedTopology() planner.Topology { return topology } +// shrunkCoordinators is a cluster whose coordinators count was lowered from five +// to three — the smallest coordinator shrink the schema floors allow, and an even +// number of members either way — while its StatefulSet still runs all five pods: +// coordinator_4 and coordinator_5 are Raft members on their way out. +func shrunkCoordinators() planner.Topology { + topology := topologyOf(3, 2) + for id := int32(4); id <= 5; id++ { + topology.RetiringCoordinators = append(topology.RetiringCoordinators, coordinatorSpec(id)) + } + return topology +} + +// retiringBothRoles is one edit lowering both counts: the coordinators shrink from +// 5 to 3 while the data instances shrink from 3 to 2. +func retiringBothRoles() planner.Topology { + topology := shrunkCoordinators() + topology.RetiringDataInstances = append(topology.RetiringDataInstances, dataInstanceSpec(2)) + return topology +} + func topologyOf(coordinators int32, dataInstances int) planner.Topology { topology := planner.Topology{} for id := int32(1); id <= coordinators; id++ { @@ -115,6 +140,22 @@ func observedCoordinator(id int32, role string) memgraph.Instance { } } +// observedCoordinators is the Raft membership a leader reports: one row per given +// coordinator ID, the one named by leaderID reporting itself leader. Which +// coordinator holds leadership is what decides whether a shrink can remove +// members at all, so every retirement case below states it outright. +func observedCoordinators(leaderID int32, ids ...int32) []memgraph.Instance { + view := make([]memgraph.Instance, 0, len(ids)) + for _, id := range ids { + role := memgraph.RoleFollower + if id == leaderID { + role = memgraph.RoleLeader + } + view = append(view, observedCoordinator(id, role)) + } + return view +} + func observedDataInstance(i int, role string) memgraph.Instance { spec := dataInstanceSpec(i) return memgraph.Instance{ @@ -440,6 +481,116 @@ func TestPlan(t *testing.T) { planner.UnregisterInstance{Name: thirdInstance}, }, }, + // Coordinators the count drops are removed from Raft outright when the + // leader is a survivor: their votes are gone before their pods are, and + // removing a follower needs no leadership dance. + { + name: "retiring coordinators are removed from Raft under a surviving leader", + observed: append(observedCoordinators(1, 1, 2, 3, 4, 5), + observedDataInstance(0, memgraph.RoleMain), + observedDataInstance(1, memgraph.RoleReplica), + ), + declared: ptr.To(shrunkCoordinators()), + want: []planner.Command{ + planner.RemoveCoordinator{Coordinator: coordinatorSpec(4)}, + planner.RemoveCoordinator{Coordinator: coordinatorSpec(5)}, + }, + }, + // Raft refuses to remove its own leader, so a leader on a retiring ordinal + // is asked to yield — last in the plan, with nothing after it, because the + // election picks the successor and only a fresh observation can say who won. + // The other retiring member still goes in this same pass: removing a + // follower is safe and predictable. + { + name: "a retiring leader yields last, after every removal it can still order", + observed: append(observedCoordinators(4, 1, 2, 3, 4, 5), + observedDataInstance(0, memgraph.RoleMain), + observedDataInstance(1, memgraph.RoleReplica), + ), + declared: ptr.To(shrunkCoordinators()), + want: []planner.Command{ + planner.RemoveCoordinator{Coordinator: coordinatorSpec(5)}, + planner.YieldLeadership{Leader: fourthCoordinator}, + }, + }, + // Read-before-write: a coordinator already gone from the Raft membership gets + // no removal, so a pass that crashed between two removals re-plans to just + // the rest of the work. + { + name: "an already-removed retiring coordinator is not removed again", + observed: append(observedCoordinators(1, 1, 2, 3, 4), + observedDataInstance(0, memgraph.RoleMain), + observedDataInstance(1, memgraph.RoleReplica), + ), + declared: ptr.To(shrunkCoordinators()), + want: []planner.Command{ + planner.RemoveCoordinator{Coordinator: coordinatorSpec(4)}, + }, + }, + // Nothing is left to order ahead of the yield: the plan is the yield alone, + // and the removal of the leader itself waits for the next pass. + { + name: "a retiring leader with nothing else to remove plans only the yield", + observed: append(observedCoordinators(4, 1, 2, 3, 4), + observedDataInstance(0, memgraph.RoleMain), + observedDataInstance(1, memgraph.RoleReplica), + ), + declared: ptr.To(shrunkCoordinators()), + want: []planner.Command{ + planner.YieldLeadership{Leader: fourthCoordinator}, + }, + }, + // Leadership on a coordinator the topology neither declares nor retires — one + // a human added — is left where it is: it is not in the way of any removal. + { + name: "a leader outside the retiring set is not asked to yield", + observed: append(observedCoordinators(6, 1, 2, 3, 4, 5, 6), + observedDataInstance(0, memgraph.RoleMain), + observedDataInstance(1, memgraph.RoleReplica), + ), + declared: ptr.To(shrunkCoordinators()), + want: []planner.Command{ + planner.RemoveCoordinator{Coordinator: coordinatorSpec(4)}, + planner.RemoveCoordinator{Coordinator: coordinatorSpec(5)}, + }, + }, + // Both roles shrinking in one edit: the data instances are retired first + // (MAIN moved off the one going away), then the Raft members are removed. + { + name: "both roles retire in one pass under a surviving leader", + observed: append(observedCoordinators(1, 1, 2, 3, 4, 5), + observedDataInstance(0, memgraph.RoleReplica), + observedDataInstance(1, memgraph.RoleReplica), + observedDataInstance(2, memgraph.RoleMain), + ), + declared: ptr.To(retiringBothRoles()), + want: []planner.Command{ + planner.DemoteInstance{Name: thirdInstance}, + planner.SetInstanceToMain{Name: firstInstance}, + planner.UnregisterInstance{Name: thirdInstance}, + planner.RemoveCoordinator{Coordinator: coordinatorSpec(4)}, + planner.RemoveCoordinator{Coordinator: coordinatorSpec(5)}, + }, + }, + // The same edit with leadership in the way: the data-instance retirement is + // fully ordered and issues in this pass regardless — only the removal of the + // leader itself has to wait behind the yield. + { + name: "a retiring leader does not hold up the data-instance retirement", + observed: append(observedCoordinators(4, 1, 2, 3, 4, 5), + observedDataInstance(0, memgraph.RoleReplica), + observedDataInstance(1, memgraph.RoleReplica), + observedDataInstance(2, memgraph.RoleMain), + ), + declared: ptr.To(retiringBothRoles()), + want: []planner.Command{ + planner.DemoteInstance{Name: thirdInstance}, + planner.SetInstanceToMain{Name: firstInstance}, + planner.UnregisterInstance{Name: thirdInstance}, + planner.RemoveCoordinator{Coordinator: coordinatorSpec(5)}, + planner.YieldLeadership{Leader: fourthCoordinator}, + }, + }, // The retiring range is bounded by the operator's own prior apply, which is // what keeps an instance a human registered out of it — even one at a // higher ordinal than everything the operator ever ran. diff --git a/internal/resources/topology.go b/internal/resources/topology.go index 05f74f5..7105de4 100644 --- a/internal/resources/topology.go +++ b/internal/resources/topology.go @@ -51,13 +51,7 @@ func DeclaredTopology(cluster *memgraphcomv1alpha1.MemgraphCluster) planner.Topo DataInstances: make([]memgraph.DataInstanceSpec, 0, spec.dataInstances), } for ordinal := range spec.coordinators { - fqdn := podFQDN(cluster, CoordinatorName(cluster), spec, ordinal) - topology.Coordinators = append(topology.Coordinators, memgraph.CoordinatorSpec{ - ID: ordinal + 1, - BoltServer: hostPort(fqdn, spec.ports.bolt), - CoordinatorServer: hostPort(fqdn, spec.ports.coordinator), - ManagementServer: hostPort(fqdn, spec.ports.management), - }) + topology.Coordinators = append(topology.Coordinators, coordinator(cluster, spec, ordinal)) } for ordinal := range spec.dataInstances { topology.DataInstances = append(topology.DataInstances, dataInstance(cluster, spec, ordinal)) @@ -65,6 +59,29 @@ func DeclaredTopology(cluster *memgraphcomv1alpha1.MemgraphCluster) planner.Topo return topology } +// RetiringCoordinators is the coordinators a lowered coordinators count is +// shedding: pod ordinals [declared, applied), where applied is the replica count +// the operator's own previous apply left on the coordinator StatefulSet. It is +// empty while a cluster grows or holds its size. +// +// Because the count must stay odd, a shrink always retires an even number of +// coordinators, so the surviving Raft cluster keeps an odd membership throughout. +func RetiringCoordinators( + cluster *memgraphcomv1alpha1.MemgraphCluster, + applied int32, +) []memgraph.CoordinatorSpec { + spec := normalize(cluster.Spec) + if applied <= spec.coordinators { + return nil + } + + retiring := make([]memgraph.CoordinatorSpec, 0, applied-spec.coordinators) + for ordinal := spec.coordinators; ordinal < applied; ordinal++ { + retiring = append(retiring, coordinator(cluster, spec, ordinal)) + } + return retiring +} + // RetiringDataInstances is the data instances a lowered dataInstances count is // shedding: pod ordinals [declared, applied), where applied is the replica count // the operator's own previous apply left on the data StatefulSet. It is empty @@ -89,6 +106,24 @@ func RetiringDataInstances( return retiring } +// coordinator describes the coordinator running on the given pod ordinal, as the +// pod itself advertises it. Retiring coordinators are described the same way as +// declared ones: they are members of the Raft cluster under the ID and addresses +// the operator added them with, whether or not the spec still declares them. +func coordinator( + cluster *memgraphcomv1alpha1.MemgraphCluster, + spec normalizedSpec, + ordinal int32, +) memgraph.CoordinatorSpec { + fqdn := podFQDN(cluster, CoordinatorName(cluster), spec, ordinal) + return memgraph.CoordinatorSpec{ + ID: ordinal + 1, + BoltServer: hostPort(fqdn, spec.ports.bolt), + CoordinatorServer: hostPort(fqdn, spec.ports.coordinator), + ManagementServer: hostPort(fqdn, spec.ports.management), + } +} + // dataInstance describes the data instance running on the given pod ordinal, as // the pod itself advertises it. Retiring instances are described the same way as // declared ones: they are registered under the addresses the operator registered diff --git a/internal/resources/topology_test.go b/internal/resources/topology_test.go index 05abf2a..32e26e7 100644 --- a/internal/resources/topology_test.go +++ b/internal/resources/topology_test.go @@ -146,6 +146,85 @@ func TestRetiringDataInstances(t *testing.T) { } } +// TestRetiringCoordinators covers the range a lowered coordinators count sheds. +// The bounds matter more here than for data instances: every member in the range +// loses a Raft vote, so a range that reached past what the operator applied would +// try to remove a coordinator a human added. +func TestRetiringCoordinators(t *testing.T) { + cases := []struct { + name string + cluster *memgraphcomv1alpha1.MemgraphCluster + applied int32 + want []string + }{ + { + name: "a cluster holding its size retires nothing", + cluster: minimalCluster(), + applied: 3, + }, + { + name: "a growing cluster retires nothing", + cluster: coordinatorsCluster(5), + applied: 3, + }, + // The count must stay odd, so a shrink always retires an even number of + // coordinators and the surviving Raft membership stays odd throughout. + { + name: "both ordinals above the declared count retire at once", + cluster: coordinatorsCluster(3), + applied: 5, + want: []string{"coordinator_4", "coordinator_5"}, + }, + { + name: "a larger shrink retires every ordinal above the declared count", + cluster: coordinatorsCluster(3), + applied: 7, + want: []string{"coordinator_4", "coordinator_5", "coordinator_6", "coordinator_7"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var names []string + for _, coordinator := range resources.RetiringCoordinators(tc.cluster, tc.applied) { + names = append(names, coordinator.Name()) + } + if diff := cmp.Diff(tc.want, names); diff != "" { + t.Errorf("RetiringCoordinators() mismatch (-want +got):\n%s", diff) + } + }) + } +} + +// A retiring coordinator is a Raft member under the ID and addresses the operator +// added it with, so it must be described exactly as the declared coordinator on +// the same ordinal was — otherwise the plan would aim REMOVE COORDINATOR at the +// wrong ID. +func TestRetiringCoordinatorMatchesItsDeclaredForm(t *testing.T) { + // The tuned cluster (non-default ports and cluster domain) declares three + // coordinators. Lowering the count is not possible below three, so the declared + // form is taken from a five-coordinator variant of the same spec. + grown := tunedCluster() + grown.Spec.Coordinators = ptr.To(int32(5)) + declared := resources.DeclaredTopology(grown).Coordinators + + shrunk := tunedCluster() + shrunk.Spec.Coordinators = ptr.To(int32(3)) + got := resources.RetiringCoordinators(shrunk, int32(len(declared))) + + if diff := cmp.Diff(declared[3:], got); diff != "" { + t.Errorf("RetiringCoordinators() mismatch (-want +got):\n%s", diff) + } +} + +// coordinatorsCluster is the minimal cluster with a different coordinators count, +// the spec side of a coordinator scale. +func coordinatorsCluster(coordinators int32) *memgraphcomv1alpha1.MemgraphCluster { + cluster := minimalCluster() + cluster.Spec.Coordinators = ptr.To(coordinators) + return cluster +} + // dataInstancesCluster is the minimal cluster with a lowered dataInstances count, // the spec side of a scale-down. func dataInstancesCluster(dataInstances int32) *memgraphcomv1alpha1.MemgraphCluster { diff --git a/test/e2e/memgraphcluster_test.go b/test/e2e/memgraphcluster_test.go index cecb7e7..6b6fa23 100644 --- a/test/e2e/memgraphcluster_test.go +++ b/test/e2e/memgraphcluster_test.go @@ -59,6 +59,11 @@ const ( // roleMain is the MAIN data-instance role reported in the SHOW INSTANCES role // column. roleMain = "main" + + // roleLeader is the Raft leader coordinator role reported in the same column. + // Which coordinator holds it decides whether a shrink can remove a member at + // all: Raft refuses to remove its own leader. + roleLeader = "leader" ) // example is the parsed quickstart manifest and the source of truth for the @@ -536,6 +541,90 @@ spec: Expect(claims).To(ContainElement(fmt.Sprintf("lib-storage-%s-data-2", scalingClusterName)), "whenScaled follows spec.storage.retentionPolicy, which defaults to Retain") }) + + // The coordinator shrink, against the 5-coordinator cluster the specs above left + // behind (Ordered), with Raft leadership deliberately parked on a coordinator + // that has to go — the case the whole safety argument is about: Raft returns + // RAFT_CANNOT_REMOVE_LEADER for its own leader, and a StatefulSet sheds only its + // highest ordinals, so the operator has to move leadership out of the retiring + // range before it can remove anything there. + It("retires the coordinator holding Raft leadership and only then sheds its pod", func() { + // The topology the data shrink left running, and the one this spec drops to. + // Three is the floor, so a shrink from five retires an even number of members + // and the surviving Raft cluster keeps an odd membership throughout. + running := grown.withTopology(5, 2) + shrunk := grown.withTopology(3, 2) + retiring := []string{"coordinator_4", "coordinator_5"} + + By("forcing Raft leadership onto a coordinator the shrink retires") + // Retried as a whole: YIELD LEADERSHIP names no successor, so each attempt + // hands leadership to whichever member NuRaft's election picks. + Eventually(func(g Gomega) { + g.Expect(running.makeLeader(retiring[0])).To(Succeed()) + view, err := running.leaderView() + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(coordinatorLeaderOf(view)).To(Equal(retiring[0])) + }, 10*time.Minute, 10*time.Second).Should(Succeed()) + + By("lowering the coordinator count on the live cluster") + cmd := exec.Command("kubectl", "patch", "memgraphcluster", scalingClusterName, + "-n", scalingNamespace, "--type=merge", "-p", + fmt.Sprintf(`{"spec":{"coordinators":%d}}`, shrunk.coordinators)) + _, err := utils.Run(cmd) + Expect(err).NotTo(HaveOccurred(), "the operator must accept a lowered coordinator count") + + By("waiting for both retiring members to leave the Raft cluster while their pods are still there") + Eventually(func(g Gomega) { + // The replica count is read before the membership view for the reason the + // data shrink above spells out: a view read afterwards that still lists a + // retiring member proves it held a vote at a moment its pod had already + // been scaled away, which is the order the operator must never produce. + replicas, err := shrunk.replicas("coordinator") + g.Expect(err).NotTo(HaveOccurred()) + + // Read through the five-coordinator view: while leadership is still moving + // it may sit on a retiring ordinal, which the shrunk cluster does not scan. + view, err := running.leaderView() + g.Expect(err).NotTo(HaveOccurred()) + names := instanceNames(view) + + for _, name := range retiring { + if replicas != "5" && slices.Contains(names, name) { + // Not something to retry: the ordering this catches is broken for + // good by the time it is observable. + StopTrying(fmt.Sprintf( + "the coordinator StatefulSet was scaled to %s replicas while %s was still a Raft member", + replicas, name)).Now() + } + g.Expect(names).NotTo(ContainElement(name)) + } + }, 10*time.Minute, 5*time.Second).Should(Succeed()) + + By("waiting for their pods to be shed") + Eventually(func(g Gomega) { + g.Expect(shrunk.replicas("coordinator")).To(Equal("3")) + g.Expect(shrunk.podExists("coordinator", 3)).To(BeFalse()) + g.Expect(shrunk.podExists("coordinator", 4)).To(BeFalse()) + }, 5*time.Minute, 5*time.Second).Should(Succeed()) + + By("confirming the shrunk cluster is registered, converged, and led by a survivor") + Eventually(shrunk.verifyRegistered, 10*time.Minute, 10*time.Second).Should(Succeed()) + shrunk.awaitConverged(5 * time.Minute) + coordinators, _ := shrunk.registeredCounts() + Expect(coordinators).To(Equal("3")) + view, err := shrunk.leaderView() + Expect(err).NotTo(HaveOccurred()) + Expect(coordinatorLeaderOf(view)).To(BeElementOf("coordinator_1", "coordinator_2", "coordinator_3")) + + By("confirming the retired coordinators' claims are kept by the default retention policy") + claims, err := listPVCs(scalingNamespace) + Expect(err).NotTo(HaveOccurred()) + for _, ordinal := range []int{3, 4} { + Expect(claims).To(ContainElement( + fmt.Sprintf("lib-storage-%s-coordinator-%d", scalingClusterName, ordinal)), + "whenScaled follows spec.storage.retentionPolicy, which defaults to Retain") + } + }) }) // listAdoptedPVCs returns the names of the PersistentVolumeClaims a @@ -914,6 +1003,40 @@ func (c clusterUnderTest) makeMain(name string) error { return nil } +// makeLeader nudges Raft leadership toward the named coordinator by yielding it on +// whichever coordinator currently holds it, which is how a spec parks leadership +// where a scale-down cannot remove it. +// +// One call is an attempt, not a guarantee: YIELD LEADERSHIP takes no successor, so +// NuRaft's election decides who takes over. Callers retry until the target wins. It +// is a no-op when the target already holds leadership. +func (c clusterUnderTest) makeLeader(name string) error { + pod, view, err := c.leaderPod() + if err != nil { + return err + } + if coordinatorLeaderOf(view) == name { + return nil + } + cmd := exec.Command("kubectl", "exec", pod, "-n", c.namespace, "-c", "memgraph", "--", + "bash", "-c", "echo 'YIELD LEADERSHIP;' | mgconsole") + if _, err := utils.Run(cmd); err != nil { + return fmt.Errorf("yielding leadership on %s: %w", pod, err) + } + return nil +} + +// coordinatorLeaderOf returns the coordinator a view reports as Raft leader, or the +// empty string when it reports none. +func coordinatorLeaderOf(view []instanceRow) string { + for _, instance := range view { + if instance.role == roleLeader { + return instance.name + } + } + return "" +} + // mainOf returns the name of the data instance a view reports as MAIN, or the // empty string when it reports none. func mainOf(view []instanceRow) string { From b775c433f0ce12105440e752854a2941bfe2d145 Mon Sep 17 00:00:00 2001 From: as51340 Date: Wed, 29 Jul 2026 09:21:04 +0200 Subject: [PATCH 2/2] fix: Finding out who is the leader --- test/e2e/memgraphcluster_test.go | 167 +++++++++++++++---------------- test/utils/names.go | 47 +++++++++ 2 files changed, 125 insertions(+), 89 deletions(-) create mode 100644 test/utils/names.go diff --git a/test/e2e/memgraphcluster_test.go b/test/e2e/memgraphcluster_test.go index 6b6fa23..5e7ebcf 100644 --- a/test/e2e/memgraphcluster_test.go +++ b/test/e2e/memgraphcluster_test.go @@ -146,10 +146,10 @@ func loadExample() *memgraphcomv1alpha1.MemgraphCluster { func (c clusterUnderTest) declaredInstances() []string { names := make([]string, 0, c.coordinators+c.dataInstances) for ordinal := range c.coordinators { - names = append(names, fmt.Sprintf("coordinator_%d", ordinal+1)) + names = append(names, utils.CoordinatorName(ordinal)) } for ordinal := range c.dataInstances { - names = append(names, fmt.Sprintf("instance_%d", ordinal)) + names = append(names, utils.DataInstanceName(ordinal)) } return names } @@ -558,12 +558,16 @@ spec: By("forcing Raft leadership onto a coordinator the shrink retires") // Retried as a whole: YIELD LEADERSHIP names no successor, so each attempt - // hands leadership to whichever member NuRaft's election picks. + // hands leadership to whichever member NuRaft nominates. Either retiring + // member satisfies the precondition — what the spec is about is leadership + // sitting inside the retiring range, not on one particular ordinal, and + // insisting on one would make the wait depend on which peer NuRaft happens + // to nominate. Eventually(func(g Gomega) { - g.Expect(running.makeLeader(retiring[0])).To(Succeed()) + g.Expect(running.makeLeader(retiring...)).To(Succeed()) view, err := running.leaderView() g.Expect(err).NotTo(HaveOccurred()) - g.Expect(coordinatorLeaderOf(view)).To(Equal(retiring[0])) + g.Expect(coordinatorLeaderOf(view)).To(BeElementOf(retiring)) }, 10*time.Minute, 10*time.Second).Should(Succeed()) By("lowering the coordinator count on the live cluster") @@ -760,80 +764,44 @@ func (c clusterUnderTest) podExists(component string, ordinal int32) (bool, erro // wipeInstanceRegistration unregisters the named data instance on the // coordinator leader, simulating registration state a pod loses when it is // rescheduled onto a fresh node. UNREGISTER INSTANCE must run on the leader — -// only it holds the authoritative cluster view — so the leader is located the -// same way leaderView does: the coordinator that reports a MAIN. +// only it holds the authoritative cluster view — which leaderPod locates. func wipeInstanceRegistration(name string) error { - var errs []error - for ordinal := range coordinatorCount { - pod := fmt.Sprintf("%s-coordinator-%d", clusterName, ordinal) - view, err := quickstartCluster.showInstances(pod) - if err != nil { - errs = append(errs, err) - continue - } - isLeader := false - for _, instance := range view { - if instance.role == roleMain { - isLeader = true - break - } - } - if !isLeader { - continue - } - cmd := exec.Command("kubectl", "exec", pod, "-n", clusterNamespace, "-c", "memgraph", "--", - "bash", "-c", fmt.Sprintf("echo 'UNREGISTER INSTANCE %s;' | mgconsole", name)) - if _, err := utils.Run(cmd); err != nil { - return fmt.Errorf("unregistering %s on %s: %w", name, pod, err) - } - return nil + pod, _, err := quickstartCluster.leaderPod() + if err != nil { + return fmt.Errorf("no coordinator leader found to unregister %s: %w", name, err) + } + cmd := exec.Command("kubectl", "exec", pod, "-n", clusterNamespace, "-c", "memgraph", "--", + "bash", "-c", fmt.Sprintf("echo 'UNREGISTER INSTANCE %s;' | mgconsole", name)) + if _, err := utils.Run(cmd); err != nil { + return fmt.Errorf("unregistering %s on %s: %w", name, pod, err) } - return fmt.Errorf("no coordinator leader found to unregister %s: %w", name, errors.Join(errs...)) + return nil } // removeCoordinatorRegistration removes a follower coordinator from the Raft // cluster on the coordinator leader, simulating a coordinator that fell out of // the cluster view (e.g. rescheduled onto a fresh node). REMOVE COORDINATOR -// mutates Raft membership, so it must run on the leader — located the same way -// leaderView does: the coordinator that reports a MAIN. A follower is chosen -// (never the leader itself) so the leader keeps the authoritative view it needs -// to accept the removal and observe the operator's re-ADD. It returns the -// instance name of the coordinator it removed. +// mutates Raft membership, so it must run on the leader — which leaderPod +// locates. A follower is chosen (never the leader itself): Raft refuses to remove +// its own leader, and the leader keeps the authoritative view it needs to accept +// the removal and observe the operator's re-ADD. It returns the instance name of +// the coordinator it removed. func removeCoordinatorRegistration() (string, error) { - var errs []error - for ordinal := range coordinatorCount { - pod := fmt.Sprintf("%s-coordinator-%d", clusterName, ordinal) - view, err := quickstartCluster.showInstances(pod) - if err != nil { - errs = append(errs, err) - continue - } - isLeader := false - for _, instance := range view { - if instance.role == roleMain { - isLeader = true - break - } - } - if !isLeader { - continue - } - // The leader hosts coordinator_ordinal+1; remove a different - // coordinator so the leader keeps quorum and its authoritative view. - leaderID := ordinal + 1 - removeID := 1 - if leaderID == 1 { - removeID = 2 - } - name := fmt.Sprintf("coordinator_%d", removeID) - cmd := exec.Command("kubectl", "exec", pod, "-n", clusterNamespace, "-c", "memgraph", "--", - "bash", "-c", fmt.Sprintf("echo 'REMOVE COORDINATOR %d;' | mgconsole", removeID)) - if _, err := utils.Run(cmd); err != nil { - return "", fmt.Errorf("removing coordinator %d on %s: %w", removeID, pod, err) - } - return name, nil + pod, view, err := quickstartCluster.leaderPod() + if err != nil { + return "", fmt.Errorf("no coordinator leader found to remove a coordinator: %w", err) + } + ordinal := int32(0) + if coordinatorLeaderOf(view) == utils.CoordinatorName(ordinal) { + ordinal = 1 } - return "", fmt.Errorf("no coordinator leader found to remove a coordinator: %w", errors.Join(errs...)) + name := utils.CoordinatorName(ordinal) + cmd := exec.Command("kubectl", "exec", pod, "-n", clusterNamespace, "-c", "memgraph", "--", + "bash", "-c", fmt.Sprintf("echo 'REMOVE COORDINATOR %d;' | mgconsole", ordinal+1)) + if _, err := utils.Run(cmd); err != nil { + return "", fmt.Errorf("removing %s on %s: %w", name, pod, err) + } + return name, nil } // dumpDiagnosticsOnFailure dumps everything needed to debug a broken cluster @@ -947,36 +915,57 @@ type instanceRow struct { role string } -// leaderView returns the SHOW INSTANCES view of the first coordinator that -// reports a MAIN. Only the coordinator leader health-checks data instances and -// reports their roles (followers show them as unknown), so a view containing a -// MAIN is the leader's authoritative view. +// leaderView returns the coordinator leader's SHOW INSTANCES view, the +// authoritative one: only the leader health-checks the data instances it reports +// on. func (c clusterUnderTest) leaderView() ([]instanceRow, error) { _, view, err := c.leaderPod() return view, err } -// leaderPod locates the coordinator leader — the coordinator whose view reports a -// MAIN — and returns its pod name together with that view. Management queries a -// test issues by hand have to run there: only the leader holds the authoritative -// cluster state and accepts a mutation of it. +// leaderPod locates the coordinator leader and returns its pod name together with +// its view. Management queries a test issues by hand have to run there: only the +// leader holds the authoritative cluster state and accepts a mutation of it — a +// follower rejects YIELD LEADERSHIP, SET INSTANCE TO MAIN and REMOVE COORDINATOR +// outright. +// +// The leader is read out of the role column, never inferred from a view reporting +// a MAIN: a coordinator forwards SHOW INSTANCES to the leader and answers with the +// leader's view, so every coordinator reports the MAIN and only the role column +// says who holds Raft leadership. That same forwarding is why one read is enough — +// the view a follower returns is already the authoritative one, and only the pod to +// send mutations to has to be looked up from it. func (c clusterUnderTest) leaderPod() (string, []instanceRow, error) { var errs []error for ordinal := range c.coordinators { - pod := fmt.Sprintf("%s-coordinator-%d", c.name, ordinal) + pod := c.coordinatorPod(ordinal) view, err := c.showInstances(pod) if err != nil { errs = append(errs, err) continue } - if mainOf(view) != "" { - return pod, view, nil + leader := coordinatorLeaderOf(view) + if leader == "" { + errs = append(errs, fmt.Errorf("%s reports no coordinator leader among %d instances", + pod, len(view))) + continue } - errs = append(errs, fmt.Errorf("%s reports no MAIN among %d instances", pod, len(view))) + leaderOrdinal, err := utils.CoordinatorOrdinal(leader) + if err != nil { + errs = append(errs, fmt.Errorf("%s named %s as leader: %w", pod, leader, err)) + continue + } + return c.coordinatorPod(leaderOrdinal), view, nil } return "", nil, errors.Join(errs...) } +// coordinatorPod is the pod the coordinator with the given StatefulSet ordinal +// runs in. +func (c clusterUnderTest) coordinatorPod(ordinal int32) string { + return fmt.Sprintf("%s-coordinator-%d", c.name, ordinal) +} + // makeMain moves MAIN onto the named data instance by hand, which is how a spec // arranges for the instance a scale-down retires to be the one holding MAIN. // @@ -1003,19 +992,19 @@ func (c clusterUnderTest) makeMain(name string) error { return nil } -// makeLeader nudges Raft leadership toward the named coordinator by yielding it on -// whichever coordinator currently holds it, which is how a spec parks leadership -// where a scale-down cannot remove it. +// makeLeader nudges Raft leadership toward one of the named coordinators by +// yielding it on whichever coordinator currently holds it, which is how a spec +// parks leadership where a scale-down cannot remove it. // // One call is an attempt, not a guarantee: YIELD LEADERSHIP takes no successor, so -// NuRaft's election decides who takes over. Callers retry until the target wins. It -// is a no-op when the target already holds leadership. -func (c clusterUnderTest) makeLeader(name string) error { +// NuRaft decides who takes over. Callers retry until one of the targets wins. It is +// a no-op when a target already holds leadership. +func (c clusterUnderTest) makeLeader(names ...string) error { pod, view, err := c.leaderPod() if err != nil { return err } - if coordinatorLeaderOf(view) == name { + if slices.Contains(names, coordinatorLeaderOf(view)) { return nil } cmd := exec.Command("kubectl", "exec", pod, "-n", c.namespace, "-c", "memgraph", "--", diff --git a/test/utils/names.go b/test/utils/names.go new file mode 100644 index 0000000..c0bd73b --- /dev/null +++ b/test/utils/names.go @@ -0,0 +1,47 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package utils + +import "fmt" + +// The names a cluster member appears under in SHOW INSTANCES, derived from its +// StatefulSet pod ordinal. They are spelled out here rather than taken from the +// operator's own packages on purpose: a test that asked production code what it +// named an instance could never catch it renaming one. + +// CoordinatorName is the SHOW INSTANCES name of the coordinator in the pod with +// the given ordinal: ordinal N registers as coordinator_N+1, because Raft +// coordinator IDs start at one. +func CoordinatorName(ordinal int32) string { + return fmt.Sprintf("coordinator_%d", ordinal+1) +} + +// CoordinatorOrdinal is the inverse of CoordinatorName: the ordinal of the pod +// running the coordinator a view names. +func CoordinatorOrdinal(name string) (int32, error) { + var id int32 + if _, err := fmt.Sscanf(name, "coordinator_%d", &id); err != nil { + return 0, fmt.Errorf("parsing coordinator name %q: %w", name, err) + } + return id - 1, nil +} + +// DataInstanceName is the SHOW INSTANCES name of the data instance in the pod +// with the given ordinal: ordinal N registers as instance_N. +func DataInstanceName(ordinal int32) string { + return fmt.Sprintf("instance_%d", ordinal) +}