Add cluster teardown fixes, Fix port lock leak: release all 3 locks on server exit - #14
Add cluster teardown fixes, Fix port lock leak: release all 3 locks on server exit#14Fniakate8 wants to merge 6 commits into
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>
| from conftest import resource_port_tracker | ||
| from valkey_test_case import ValkeyServerHandle | ||
|
|
||
| SERVER_PATH = f"{os.path.dirname(os.path.realpath(__file__))}/.build/binaries/{os.environ['SERVER_VERSION']}/valkey-server" |
There was a problem hiding this comment.
Should do the unstable hack here as well to allow non build.sh runs
There was a problem hiding this comment.
good call, I'll apply the same os.environ.get("SERVER_VERSION", "unstable") fallback I used in the reuse tests so these can run without build.sh too.
| if not self.external_mode: | ||
| try: | ||
| if self.args.get("cluster-enabled") == "yes": | ||
| self.client.execute_command("CLUSTER", "RESET", "HARD") |
There was a problem hiding this comment.
Looking at https://valkey.io/commands/cluster-reset/
Note that this command does not work for primaries if they hold one or more keys, in that case to completely reset a primary node keys must be removed first, e.g. by using FLUSHALL first, and then CLUSTER RESET.
What use cases do we need this hard reset?
There was a problem hiding this comment.
I hadn't accounted for the primary-with-keys case. I'll add FLUSHALL before CLUSTER RESET HARD so the reset actually succeeds on primaries that are holding data (and remove the silent except).
Use Case: it's for cluster nodes only. When a node leaves without detaching first, the other stable nodes keep trying to reach it and can trigger a failover. CLUSTER RESET HARD basically makes it forget its cluster identity so it exits cleanly, and prevents stale cluster state from leaking into a reused/restarted server in a later test.
There was a problem hiding this comment.
Update: just tested the cluster reset hard and it was redundant, i'll remove it because it would clean up only the in-memory state and the node.conf cluster identity which is later handled by shutdown nosave and existing nodes.conf deletion. so its best to remove it.
| for i in range(1, 3): | ||
| nodes[0].client.execute_command("CLUSTER", "MEET", "0.0.0.0", nodes[i].port) | ||
|
|
||
| time.sleep(2) |
There was a problem hiding this comment.
We would preferibly not sleep, we could use a wait_for in this case to verify cluster formed later on
There was a problem hiding this comment.
yea sleep is kind of like a guess. I already have code that reads how many nodes each server knows about. I can wrap it in a wait_for_equal instead of sleeping. which replaces both the sleep and verify clusters formed loop.
| # Remaining nodes should still be responsive (clean departure) | ||
| for node in nodes[1:]: | ||
| assert ( | ||
| node.client.ping() |
There was a problem hiding this comment.
I think ping would work even without cluster rest hard? Where did you see this issue mostly occurring
There was a problem hiding this comment.
yea it was theoretical input for me. ping respond regardless of cluster state, so this test doesn't reAlly prove the reset did anything. I'll change it to assert on real cluster state.
| lockfile.close() | ||
| del self.open_and_locked_files[port] | ||
|
|
||
| def release_port(self, port): |
There was a problem hiding this comment.
Nice catch this looks good!
Fniakate8
changed the title
bb699a2 to
e61b8ab
Compare
ValkeyServerHandle.exit() previously never returned its port locks to the PortTracker, so cluster tests (which use several nodes at once) could leak or exhaust ports before the tracker's context exited. - Add PortTracker.release_port() to release all 3 locks per port (base, cluster bus +10000, search coordinator +20294); safe to call twice. - Store port_tracker on ValkeyServerHandle and release its locks at the end of exit(), guarded by _ports_released so a double exit() is a no-op. - Add tests (built on the ClusterTestCase/ClusterNodeHandle helpers) that a cluster reclaims every port lock on node exit, repeated cluster creation does not exhaust ports, and double exit() is safe. Built on top of valkey-io#12 for the cluster test helpers. Signed-off-by: Fanta Niakate <niakatf@amazon.com>
e61b8ab to
d5857c5
Compare
Fniakate8
changed the title
3 participants
What's Added
PortTracker.release_port()releases all 3 port locks (base, cluster bus +10000, search coordinator +20294) immediately when a server exits, instead ofwaiting for the
PortTrackercontext manager to close at the end of the test session.ValkeyServerHandle.exit()now releases its port locks at the end of teardown.self.port_trackeris stored on ValkeyServerHandle so exit() can release ports without needing external cleanup._ports_releasedflag) makes calling exit() twice safe.Why this is needed
When cluster nodes were torn down, ports stayed locked until the PortTracker context manager exited (end of the test function). Under repeated cluster create/destroy cycles, this caused port exhaustion — get_unused_port() would fail after a few iterations because all ports were still "in use." Releasing the locks on exit() reclaims them immediately.
How to use
No changes needed in test code. The fix is in the base
ValkeyServerHandle.exit()so every server, standalone (CMD) or cluster node (CME) — releases its port locks immediately on teardown. Modules inherit it with zero code changes. A test that repeatedly creates and destroys servers/clusters within a single function will no longer leak ports.Test plan
Tests are built on the ClusterTestCase / ClusterNodeHandle helpers from #12:
test_ports_released_after_exit— creates a 3-node cluster (9 locks), exits all nodes, asserts every lock is released.test_repeated_cluster_creation_no_exhaustion— creates and destroys 5 clusters of 3 nodes in a loop, asserts no port leak between iterations.test_double_exit_safe— callsexit() twiceon the same node, asserts no crash or double-release.Note
Rebased on top of #12 to use its cluster test helpers, so the extra commits in the diff are Tracy's — they'll drop out once #12 merges into unstable. An earlier version of this PR also sent CLUSTER RESET HARD before shutdown, but I removed it: since the node is shut down with
shutdown nosave and its nodes.confis deleted right after, nothing survives teardown that the reset would clean up (verified empirically). The PR is now focused on the port-lock release fix.