Add CME support/ Slot migration testing support - #15
Conversation
Signed-off-by: Tracy <yuningt@amazon.com>
Signed-off-by: Tracy <yuningt@amazon.com>
Signed-off-by: Tracy <yuningt@amazon.com>
Signed-off-by: Tracy <yuningt@amazon.com>
Add ClusterTestCase.migrate_slot() so cluster tests can move a slot (and its keys) between live nodes without hand-writing the CLUSTER SETSLOT / GETKEYSINSLOT / MIGRATE protocol each time. - ClusterTestCase: migrate_slot (mark IMPORTING/MIGRATING, batch-move keys with MIGRATE KEYS, announce the new owner on every primary), get_slot_owner, wait_for_slot_owner (polls until all nodes agree, no fixed sleeps) - ClusterNodeHandle primitives: count_keys_in_slot, start_importing_slot, start_migrating_slot, assign_slot_owner, get_slot_owner_id, is_primary - Ownership is announced only on primaries (CLUSTER SETSLOT is rejected on replicas; they learn from their primary) - Multi-DB migration supported via dbs=...; requires cluster-databases > 1 - tests/test_slot_migration.py: single-key, many-key drain, multi-DB, caller-DB isolation, cluster-wide ownership, empty slot, and replica shards - README: slot migration subsection under the CME docs Built on the ClusterTestCase/ClusterNodeHandle helpers from valkey-io#12. Signed-off-by: Fanta Niakate <niakatf@amazon.com>
zackcam
left a comment
There was a problem hiding this comment.
Looks good to me at a high level overall!
| target = next(n for n in self.nodes if n.nodeid != source.nodeid) | ||
| return source, target | ||
|
|
||
| def test_migrate_slot_moves_key(self): |
There was a problem hiding this comment.
could maybe combine this and the test below mostly achieve the same purpose with the seocnd one being just more keys
Combine test_migrate_slot_moves_key into test_migrate_slot_moves_keys — the many-key test already covers the single-key path; keep a value-integrity assertion so a specific key's value is still verified after migration. Signed-off-by: Fanta Niakate <niakatf@amazon.com>
| for node in self.nodes: | ||
| if node.is_primary(): | ||
| node.assign_slot_owner(slot, target.nodeid) |
There was a problem hiding this comment.
I think this does not guarantee that target.assign_slot_owner() executes first since valkey explicitly requires target-first finalization so the target’s replicas persist the new topology before the source releases the slot. If the target fails during an out-of-order handoff, the slot can become ownerless.
1. Send SETSLOT NODE to the target primary.
2. The target replicates the topology change to its replicas.
3. The target applies the ownership change and returns success.
4. Only then finalize the slot on the source and let the change propagate elsewhere.
There was a problem hiding this comment.
yea, I made migrate_slot now be able to announce the new owner on the target, then the source, then the remaining primaries, so the target persists the topology before the source releases the slot.
|
|
||
| To test slot migration, use `migrate_slot(source, target, slot)` to move a slot and its keys from one node to another, then `wait_for_slot_owner(slot, target)` to wait until every node agrees on the new owner. `get_slot_owner(slot)` returns the node that currently owns a slot. | ||
|
|
||
| ``` |
There was a problem hiding this comment.
nit: does it need to import any fn here like your above example?
There was a problem hiding this comment.
Added the ClusterTestCase and key_slot imports to the example to match the CME one above
chinguyen21
left a comment
There was a problem hiding this comment.
LGTM, just some minor suggestions
Address review feedback: - migrate_slot now announces the new owner on the target first, then the source, then the remaining primaries. Valkey expects target-first finalization so the target (and its replicas) persist the new topology before the source releases the slot; an out-of-order handoff could leave the slot ownerless if the target failed mid-migration. - README slot-migration example now shows the ClusterTestCase and key_slot imports, matching the CME example above it. Signed-off-by: Fanta Niakate <niakatf@amazon.com>
Add slot migration testing support
This PR adds a helper so cluster tests can move a slot (and the keys in it) from one node to another on a live cluster, without hand-writing the actual
CLUSTER SETSLOT / GETKEYSINSLOT / MIGRATEprotocol each time.What's Added
ClusterTestCase.migrate_slot(source, target, slot, dbs=(0,)) — runs the manual migration protocol end to end: mark the slot IMPORTING on the target andMIGRATINGon the source, batch-move every key withMIGRATE ... KEYS, then announce the new owner on every primary.ClusterTestCase.get_slot_owner(slot)— the node that currently owns a slot.ClusterTestCase.wait_for_slot_owner(slot, owner) — polls until every node agrees on the new owner (no fixed sleeps).ClusterNodeHandle:count_keys_in_slot, start_importing_slot, start_migrating_slot, assign_slot_owner, get_slot_owner_id, is_primary.tests/test_slot_migration.py— 7 tests: single-key move, many-key drain (250 keys), multi-DB, caller-DB isolation, cluster-wide ownership, empty slot, andreplica shards.
Why this is needed
Slot migration is a core cluster operation (adding/removing nodes, rebalancing), but the framework has no helper for it — modules that want to test it hand-roll ~20 lines of low-level CLUSTER commands today (e.g. Search's
test_multidb_slot_migration_CME). This gives them one call and handles the fiddly parts (batch key move, draining slots with >100 keys, per-DB selection, primary-only ownership announcement).How to use
Once a slot's owner is known, migrating it is two calls:
self.migrate_slot(source_node, target_node, slot)# move slot + its keysself.wait_for_slot_owner(slot, target_node)# wait until the cluster agreesUse
self.get_slot_owner(slot)to find the current owner, andpass dbs=(0, 1, ...)to migrate keys across multiple databases. Migrating any DB other than 0, requires the cluster to be started withcluster-databases > 1(plain cluster mode only has DB 0).Design notes
SETSLOT/MIGRATEprotocol (what Search's real migration test uses today) rather than nativeCLUSTER MIGRATESLOTS, which has no current consumer.CLUSTER SETSLOTis rejected on replicas (they learn from their primary).Test plan