Force-apply topology on reconcile and add a per-cluster sync endpoint (#395)#400
Open
bocharov wants to merge 2 commits into
Open
Force-apply topology on reconcile and add a per-cluster sync endpoint (#395)#400bocharov wants to merge 2 commits into
bocharov wants to merge 2 commits into
Conversation
Kvrocks cluster nodes do not gossip: the controller is the sole source of topology and must reliably push one complete, identical view to every node. Three gaps let a node's view drift and never recover (see apache#395): 1. The probe loop re-pushed only a node whose epoch was behind the store, so a node that had drifted at an equal epoch (a dropped push, a partial view, a stale/phantom node entry) was never repaired. 2. CLUSTERX SETNODES was sent without force, so even a re-push at an equal version is a no-op on the server (the version gate rejects a lower version and no-ops an equal one). The only prior recovery was CLUSTER RESET, which needs an empty DB and is unusable once a node holds data. 3. A push dropped by a transient error was not retried before the next tick. Changes: - SyncClusterInfo takes a force argument and, when set, sends the SETNODES force flag so the topology applies unconditionally. A forced push replaces the node's whole topology, so it also clears stale/phantom entries without a CLUSTER RESET and without data loss. Wrapped in a bounded retry-with-backoff. - The reconcile loop parses cluster_known_nodes and cluster_slots_ok from CLUSTER INFO and force-pushes a node whose peer count or slot coverage disagrees with the desired topology, not only one whose epoch lags. Coverage is compared against the desired covered-slot count; a node that omits these fields falls back to epoch-only reconcile. The node-ahead adopt branch is preserved. - New endpoint POST /namespaces/{namespace}/clusters/{cluster}/sync force-pushes the stored topology to every node and reports per-node results. It performs no CLUSTER RESET, so it is safe to run on a populated cluster. - CheckNewNodes and Shard.ToSlotsString reject a blank or port-less node address, so a half-resolved address fails loudly instead of being pushed as a phantom. Unit tests cover the CLUSTER INFO parsing, address validation, the blank-address guard, the divergence check, and the sync handler.
bocharov
force-pushed
the
feat/force-sync-and-endpoint
branch
from
July 18, 2026 00:58
998ffa0 to
3abbe98
Compare
git-hulk
self-requested a review
July 18, 2026 02:04
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #400 +/- ##
============================================
+ Coverage 43.38% 50.77% +7.39%
============================================
Files 37 45 +8
Lines 2971 4169 +1198
============================================
+ Hits 1289 2117 +828
- Misses 1544 1832 +288
- Partials 138 220 +82
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…paths Raise patch coverage of the reconcile/sync change by covering the branches the initial tests left out: - SyncClusterInfo bounded retry: exhausting the retries returns the last error, and a cancelled context short-circuits the backoff (no live server needed — the node points at a closed port). - Cluster.SyncToNodes force-pushes every node and propagates a node failure. - The reconcile loop's probe error handling: a node restoring from backup is skipped, an unreachable node increments its failure count, and an uninitialized (CLUSTERDOWN) node is force-pushed rather than counted as a failure. - The /sync handler's cluster-not-found branch. - ClusterMockNode gains an injectable GetClusterInfo error so the probe error paths above can be driven without a live node; its new hooks are covered directly from the store package. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author
|
Addressed the Codecov patch-coverage feedback in 807123c. Added unit tests for the previously-uncovered branches:
|
git-hulk
reviewed
Jul 25, 2026
Comment on lines
+425
to
+428
| host, port, err := net.SplitHostPort(node.Addr()) | ||
| if err != nil || host == "" || port == "" { | ||
| return "", fmt.Errorf("node %s has invalid address %q", node.ID(), node.Addr()) | ||
| } |
Member
There was a problem hiding this comment.
Can add a function validateAddr for node.
| // entries (e.g. empty-address ghosts) without the data-destroying CLUSTER RESET the alternative requires. | ||
| // Safe because the controller is the single writer of topology; callers must not force a version older | ||
| // than a node legitimately ahead of the store (the probe loop keeps the `node ahead` branch for that). | ||
| func (n *ClusterNode) SyncClusterInfo(ctx context.Context, cluster *Cluster, force bool) error { |
Member
There was a problem hiding this comment.
Can we add a retry policy for this with:
- max tries
- retry delay
- is force sync
Comment on lines
+250
to
+256
| func topologyDiverged(info *store.ClusterInfo, wantNodes, wantSlots int64) bool { | ||
| if info == nil || info.KnownNodes < 0 || info.SlotsOk < 0 { | ||
| return false | ||
| } | ||
| return info.KnownNodes != wantNodes || info.SlotsOk != wantSlots | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Move this to a function of ClusterInfo
Member
|
@bocharov Thanks for your contribution, a few comments inline. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Fixes #395. Kvrocks cluster nodes do not gossip — the controller is the sole source of topology and must push a complete, identical view to every node. Today a node can silently diverge and never recover:
CLUSTERX SETNODESonly when a node's epoch is behind the stored version. A node that has drifted at an equal epoch (a dropped push, a partial view, a stale/phantom node entry) is never repaired.SETNODESis sent withoutforce, so even a re-push at an equal version is a no-op on the server (the version gate rejects a lower version and no-ops an equal one). The only prior recovery wasCLUSTER RESET, which requires an empty DB and so is unusable once a node holds data. Controller's cluster_nodes push silently drops when a data node is unreachable, leaving cluster meta stale forever #395 reports exactly this: divergentcluster_nodessnapshots that never converge, with no operator-facing way to force a re-push.Changes
SyncClusterInfo(ctx, cluster, force)— whenforceis set, send the SETNODESforceflag so the topology applies unconditionally. Because SETNODES replaces the node's whole topology, a forced push also clears stale/phantom entries without aCLUSTER RESETand without data loss. Wrapped in a bounded retry-with-backoff (a push dropped by a transient blip is otherwise not retried until the next tick).Divergence detection at an equal epoch — the reconcile loop parses
cluster_known_nodesandcluster_slots_okfromCLUSTER INFOand force-pushes a node whose peer count or slot coverage disagrees with the desired topology, not only one whose epoch lags. Coverage is compared against the desired covered-slot count (not a hardcoded 16384), so a cluster that is intentionally mid-scale is not seen as drifted; a node that omits these fields falls back to epoch-only reconcile. The existing "node is ahead" adopt branch is preserved.POST /namespaces/{namespace}/clusters/{cluster}/sync— force-push the stored topology to every node and report per-node results. This is the operator-facing re-push Controller's cluster_nodes push silently drops when a data node is unreachable, leaving cluster meta stale forever #395 asks for. It performs noCLUSTER RESET, so it is safe to run on a populated cluster.Reject blank/port-less node addresses in
CheckNewNodes(the create/add-node boundary) andShard.ToSlotsString(serialization), so a half-resolved address fails loudly instead of registering a phantom, unreachable node.Safety
The controller is the single writer of topology (failover routes through it), so "newest complete view wins" holds. Force fires only when a node's version is not greater than the store's; a node legitimately ahead keeps the existing adopt-from-node branch. Verified against Apache Kvrocks 2.16.0 that
CLUSTERX SETNODES <str> <ver> forcebypasses the equal-version no-op.Tests
make testpassing. New unit tests cover theCLUSTER INFOparsing (incl. sentinel/malformed values), the divergence check, address validation, the blank-address guard, and the/synchandler. A behavioral test asserts the reconcile fix by exercising the real probe path.