local_cluster tool #30323
local_cluster tool #30323
Conversation
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ ⚪ Ya make output | Test bloat | Test bloat
⚪ Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
Run Extra TestsRun additional tests for this PR. You can customize:
|
|
🟢 |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new command-line tool called local_cluster that allows developers to run a standalone YDB cluster (ydbd) locally in their development environment. The tool simplifies local testing by starting a cluster with default ports and a MIRROR_3DC configuration.
Key Changes
- New
local_clusterPython CLI tool that wraps the existing test harness to start a local YDB cluster - New
DefaultFirstNodePortAllocatorclass that assigns default ports (grpc=2135, mon=8765, ic=19001) to the first node and offset ports for additional nodes - Command-line interface supporting custom working directory and binary path specification
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| ydb/tests/tools/ya.make | Adds local_cluster to the list of tools to be built |
| ydb/tests/tools/local_cluster/ya.make | Build configuration for the new local_cluster Python program |
| ydb/tests/tools/local_cluster/main.py | Main implementation of the local cluster management tool with CLI interface |
| ydb/tests/library/harness/kikimr_port_allocator.py | Adds DefaultFirstNodePortAllocator class and default port constants to support predictable port assignment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def get_slot_port_allocator(self, slot_index): | ||
| # For slots, use offset starting from a large number to avoid conflicts | ||
| slot_offset = 10000 + (slot_index - 1) * PORT_OFFSET_STEP |
There was a problem hiding this comment.
[nitpick] In the slot port allocator, the offset calculation 10000 + (slot_index - 1) * PORT_OFFSET_STEP could result in port conflicts if there are many nodes. For example, if there are more than 800 nodes (8 * PORT_OFFSET_STEP = 80 added to a base port like 2135 would give 2215, and 10000 + 0 * 10 = 10000, but they'd still be separate). However, a more concerning issue is that slots starting at port 10000+ might conflict with other services. Consider documenting the expected range of node and slot counts to avoid conflicts, or using a more robust conflict avoidance strategy.
| for node_id, node in self.cluster.nodes.items(): | ||
| if not node.daemon.process.poll() is None: | ||
| logger.error("ydbd process for node %d terminated unexpectedly", node_id) | ||
| break |
There was a problem hiding this comment.
The break statement on line 102 only exits the inner for loop, not the outer while True loop. When a node process terminates unexpectedly, the code logs an error but continues the infinite loop, repeatedly logging the same error message every second. Consider either exiting the method after detecting a terminated process or adding proper handling to stop the cluster.
| break | |
| self.stop() | |
| sys.exit(1) |
| error_msg = "Failed to start cluster.\n\n" | ||
| error_msg += "\nOriginal error:\n" | ||
| error_msg += str(e) |
There was a problem hiding this comment.
[nitpick] The error message construction is redundant. Line 67 says "Failed to start cluster.\n\n" followed by line 68 which adds "\nOriginal error:\n". This results in three newlines between the two parts of the message. Consider simplifying to: error_msg = "Failed to start cluster.\n\nOriginal error:\n" + str(e)
| error_msg = "Failed to start cluster.\n\n" | |
| error_msg += "\nOriginal error:\n" | |
| error_msg += str(e) | |
| error_msg = "Failed to start cluster.\n\nOriginal error:\n" + str(e) |
| # Check that processes are still alive | ||
| if self.cluster and self.cluster.nodes: | ||
| for node_id, node in self.cluster.nodes.items(): | ||
| if not node.daemon.process.poll() is None: |
There was a problem hiding this comment.
The comparison not node.daemon.process.poll() is None should be simplified to node.daemon.process.poll() is not None for better readability. The is not operator should come after the method call, not before None.
| if not node.daemon.process.poll() is None: | |
| if node.daemon.process.poll() is not None: |
| '--working-dir', | ||
| type=str, | ||
| default=None, | ||
| help='Working directory for cluster data storage (default: temporary directory)' |
There was a problem hiding this comment.
The help text says "(default: temporary directory)" but the actual default is .ydbd_working_dir in the current directory (line 35). The help text should be updated to reflect this, for example: "(default: .ydbd_working_dir in current directory)".
| help='Working directory for cluster data storage (default: temporary directory)' | |
| help='Working directory for cluster data storage (default: .ydbd_working_dir in current directory)' |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
2 participants
New tool to run standalone ydbd in local environment
Possible next_steps: