fix(test): keep test DDS domains out of the kernel ephemeral port range - #597
Open
bburda wants to merge 4 commits into
Open
fix(test): keep test DDS domains out of the kernel ephemeral port range#597bburda wants to merge 4 commits into
bburda wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adjusts the workspace’s test DDS domain allocation to avoid Linux’s ephemeral UDP port range (reducing “address in use” startup flakiness), centralizes per-package domain pools, and adds both configure-time and runtime validation to keep the scheme correct across build/test machines.
Changes:
- Centralize per-package DDS domain pools in
ROS2MedkitTestDomain.cmake, validate the full table against the kernel ephemeral range, and add a per-package runtime guard (test_dds_domain_allocation). - Update package
CMakeLists.txtfiles to request domains by package name and register the allocation guard; fix places whereRESOURCE_LOCKproperties would otherwise overwrite the per-domain lock. - Plumb the shared secondary pool into Python via
MEDKIT_SECONDARY_DOMAINSand document the new scheme.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_plugins/ros2_medkit_sovd_service_interface/CMakeLists.txt | Switch to package-based test domain init; add allocation guard test. |
| src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt | Switch to package-based test domain init; add allocation guard test. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/CMakeLists.txt | Use package pool + guard; avoid overwriting per-domain locks; remove hard-coded ROS_DOMAIN_ID for e2e. |
| src/ros2_medkit_plugins/ros2_medkit_graph_provider/CMakeLists.txt | Switch to package-based test domain init; add allocation guard test. |
| src/ros2_medkit_log_bridge/CMakeLists.txt | Switch to package-based test domain init; add allocation guard test. |
| src/ros2_medkit_integration_tests/ros2_medkit_test_utils/constants.py | Read shared secondary domains from MEDKIT_SECONDARY_DOMAINS instead of hard-coded IDs. |
| src/ros2_medkit_integration_tests/CMakeLists.txt | Use package pool + guard; assign per-test domains via medkit_set_test_domain; append secondary lock. |
| src/ros2_medkit_gateway/CMakeLists.txt | Switch to package-based test domain init; add allocation guard test. |
| src/ros2_medkit_fault_reporter/CMakeLists.txt | Switch to package-based test domain init; add allocation guard test. |
| src/ros2_medkit_fault_manager/CMakeLists.txt | Switch to package-based test domain init; add allocation guard test. |
| src/ros2_medkit_discovery_plugins/ros2_medkit_topic_beacon/CMakeLists.txt | Switch to package-based test domain init; add allocation guard test. |
| src/ros2_medkit_discovery_plugins/ros2_medkit_param_beacon/CMakeLists.txt | Switch to package-based test domain init; add allocation guard test. |
| src/ros2_medkit_diagnostic_bridge/CMakeLists.txt | Switch to package-based test domain init; add allocation guard test. |
| src/ros2_medkit_cmake/scripts/check_test_domains.py | New runtime validator that inspects generated CTest properties and checks domain safety + locking. |
| src/ros2_medkit_cmake/README.md | Document the new ROS2MedkitTestDomain workflow and constraints. |
| src/ros2_medkit_cmake/design/index.rst | Add design notes for test-domain allocation, reuse, and validation approach. |
| src/ros2_medkit_cmake/CMakeLists.txt | Install the runtime validator script alongside the CMake modules. |
| src/ros2_medkit_cmake/cmake/ROS2MedkitTestDomain.cmake | Implement safe-band table, table validation, pool wrapping, secondary pool export, and allocation guard macro. |
| src/ros2_medkit_action_status_bridge/CMakeLists.txt | Switch to package-based test domain init; add allocation guard test. |
Suppressed comments (1)
src/ros2_medkit_cmake/cmake/ROS2MedkitTestDomain.cmake:277
- The comment says "Do not call add_launch_test directly", but the repo now legitimately calls add_launch_test directly in ros2_medkit_integration_tests and then calls medkit_set_test_domain to append ROS_DOMAIN_ID + lock. The module comment should reflect this supported usage to avoid confusion.
# This is the required way to add a launch test: it makes it impossible to add
# one without domain isolation (a launch test left on the default domain 0 sees
# every other node on the machine).
# Do not call add_launch_test directly.
| message(FATAL_ERROR "medkit_reserve_test_domain called before medkit_init_test_domains") | ||
| endif() | ||
| list(GET _MEDKIT_DOMAIN_POOL ${_MEDKIT_DOMAIN_NEXT} ${DOMAIN_VAR}) | ||
| set(${LOCK_VAR} "medkit_dds_domain_${${DOMAIN_VAR}}") |
Comment on lines
+136
to
+141
| # medkit_set_test_domain wraps around its pool and two tests can end up on the | ||
| # same domain. That is safe because the same call also puts the domain's | ||
| # RESOURCE_LOCK on the test, and CTest never runs two tests holding one lock at | ||
| # the same time - a stale participant from an earlier test cannot leak into a | ||
| # later one's graph discovery, because the earlier one has finished. | ||
| # Multi-gateway tests take their EXTRA domains from the shared secondary pool, |
RTPS gives a DDS domain the UDP slice [7400 + 250*d, 7400 + 250*d + 249], and both CycloneDDS and Fast-DDS bind inside it without SO_REUSEPORT. The kernel hands out ephemeral ports from net.ipv4.ip_local_port_range, 32768-60999 by default, which maps back to domains 102 to 214. Most of the domains we handed to tests sat in that band. When another process on the machine got one of those ports first, every node on the domain failed to start with "failed to bind to ANY:<port>: address in use", and the test reported "Launch stopped before the active tests finished", which says nothing about the cause. Re-carve the allocation into 1-100 and 215-231, the domains whose whole slice sits outside that range. Domain 0 is left for the developer shell, and 232 is dropped because its slice runs past 65535. That is 117 domains for more test slots than that, so a package pool now wraps around and two of its tests can share a domain. medkit_set_test_domain also puts a medkit_dds_domain_<id> RESOURCE_LOCK on the test, so CTest never runs two of them at the same time. The lock is load-bearing: scripts/test.sh runs ctest -j $(nproc), so tests inside one package really do run concurrently. Move the pools into one table in ROS2MedkitTestDomain.cmake. A package names itself instead of repeating a range, so the allocation can no longer drift between the module and a CMakeLists.txt. The last two literals go with it: graph_watchdog no longer hardcodes ROS_DOMAIN_ID=89 in its launch tests, and the secondary pool reaches Python through MEDKIT_SECONDARY_DOMAINS instead of a number in constants.py. Add two checks so the constraint is enforced instead of documented: - at configure time the module validates the whole table, not just the pool being asked for: every slice outside the ephemeral range, pools pairwise disjoint, domain 0 rejected. The range is read from /proc/sys/net/ipv4/ip_local_port_range and widened to at least the Linux default, so a host with a narrow range cannot accept an allocation that breaks on a stock machine. - test_dds_domain_allocation reads the generated CTest properties back and checks them against the live kernel range on the machine that runs the tests. That is a different job from the one that builds them, and it also catches a hand-written ROS_DOMAIN_ID and a resource lock dropped by a property overwrite. Both properties are appended now, so three call sites that set RESOURCE_LOCK after the domain assignment were changed to set_property(... APPEND ...).
The package registers no linter, so flake8 never saw this file and it landed with its import groups out of order: a third-party import placed after requests, and a stdlib import in its own trailing group. Only the order changes. The import set is identical and the AST outside the import statements compares equal to the previous version.
bburda
force-pushed
the
fix/test-domain-ports
branch
from
August 6, 2026 13:23
2daf0b8 to
26ff82e
Compare
The package compiled with 37 warnings under -Wall -Wextra -Wconversion. No gate fails on them, so they accumulated. slugify() and short_hash_hex() iterated a std::string as 'for (unsigned char c : s)'. The element type is char, so every iteration performed an implicit char -> unsigned char conversion and -Wsign-conversion fired. The unsigned char is required: std::isalnum and std::toupper are undefined for a negative char. The conversion is now explicit, matching address_space_browser.cpp in this package. readAccessLevel() and readUserAccessLevel() already return the underlying uint8_t of the bitmask, so the static_cast<uint8_t> around them was a no-op that tripped -Wuseless-cast. Dropping it also means a future widening of that underlying type surfaces as -Wconversion instead of being cast away silently. The two test doubles aggregate-initialised LockAccessResult and LockError with fewer initialisers than members, leaving denied_code and existing_lock_id to -Wmissing-field-initializers. Both now list every member, as the graph_provider, topic_beacon and param_beacon doubles do. status_code is written as 409, the value it defaulted to. The concurrency test dropped the nodiscard result of clear_fault(). That is intentional - it races the shared pending-report buffer and asserts on no single clear - so the discard is now explicit and says why. slugify() and short_hash_hex() are on the fault_code path. Both were checked against their previous versions over every single byte and every ordered byte pair, 65792 inputs, with no difference in output.
mfaferek93
reviewed
Aug 6, 2026
| "ros2_medkit_action_status_bridge:15-17" | ||
| "ros2_medkit_diagnostic_bridge:18-20" | ||
| "ros2_medkit_fault_manager:21-32" | ||
| "ros2_medkit_opcua:33-44" |
Collaborator
There was a problem hiding this comment.
test_opcua_secured.test.py:71 still hardcodes ROS_DOMAIN_ID = '229', with a comment calling it the free slot in the old 220-229 range. After this move 229 is the first slot of the shared secondary pool that the multi-gateway tests take, so on a full run the secured test's gateway and a peer gateway can share a domain and see each other's /fault_manager/report_fault. test_dds_domain_allocation won't catch it because the domain is set inside the Python file, not as a CTest ENVIRONMENT entry. Handing it a domain via medkit_reserve_test_domain puts it back in the table.
mfaferek93
reviewed
Aug 6, 2026
| for entry in env_entries: | ||
| name, _, value = entry.partition('=') | ||
| if name == 'ROS_DOMAIN_ID': | ||
| return int(value) |
Collaborator
There was a problem hiding this comment.
CTest applies ENVIRONMENT entries in order, so the last ROS_DOMAIN_ID wins, but this returns the first. If a test ever ends up with two entries the guard validates a domain the test does not actually run on. for entry in reversed(env_entries) fixes it.
The test launched fault_manager_node with no parameters, so it took the production defaults: storage_type sqlite at /var/lib/ros2_medkit/faults.db. Only a root process can create that directory, so outside a container the node aborted at startup on an uncaught filesystem_error and never advertised a service. Every other integration test goes through create_fault_manager_node, whose storage_type defaults to memory for exactly this reason. This one bypassed it. The run then failed 249s later at 'alarm did not surface as a CONFIRMED fault', which names the alarm path. The alarm path was fine - the plugin logs 'AlarmCondition CONFIRMED: PLC_OVERPRESSURE' on the way through. There was simply nothing to report to. The readiness poll now decides the run. It used to fall through when the service never appeared, leaving the real cause to be inferred from a downstream deadline. It now fails immediately and prints the fault manager log. Seeing the service is not sufficient on its own, because the domain is shared and a name can outlive our process, so the check also requires the process to still be alive after a settle. Against a deliberately unwritable database path this reports 'exited with code 250' in 4s instead of the wrong component in 249s, on three runs out of three. Teardown also leaked. The nodes start through 'ros2 run', so the direct child is the CLI wrapper and the node is a grandchild; signalling the wrapper left the node reparented to init, still holding ROS_DOMAIN_ID 229, which this file hardcodes. Five of them had accumulated from earlier runs. Every process now starts in its own session and terminate() signals the group, so a run leaves nothing behind.
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
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.
Pull Request
Summary
Test DDS domains were allocated across 1-232. RTPS gives a domain the UDP slice
[7400 + 250*d, 7400 + 250*d + 249], and the kernel hands out ephemeral ports fromnet.ipv4.ip_local_port_range, 32768-60999 by default. That range maps back to domains 102 to214, so most of the domains we used could be taken by any other process on the machine. When
that happened the node died at startup with
failed to bind to ANY:<port>: address in use, andthe test only said
Launch stopped before the active tests finished.This moves the allocation into 1-100 and 215-231, the domains whose whole slice sits outside
that range. Domain 0 stays free for the developer shell. Domain 232 is dropped because its
slice runs past 65535.
That leaves 117 domains, fewer than the number of tests that need one, so a package pool now
wraps around and two of its tests can share a domain.
medkit_set_test_domainalso puts amedkit_dds_domain_<id>RESOURCE_LOCKon the test, and CTest never runs two tests holding onelock at the same time. The lock is not decoration:
scripts/test.shrunsctest -j $(nproc),so tests inside one package do run concurrently.
The pools also moved into a single table in
ROS2MedkitTestDomain.cmake. A package now namesitself (
medkit_init_test_domains(PACKAGE ros2_medkit_gateway)) instead of repeating a range,so the numbers cannot drift between the module and a
CMakeLists.txt. Two remaining literalswent with it:
graph_watchdogno longer hardcodesROS_DOMAIN_ID=89, and the shared secondarypool reaches Python through
MEDKIT_SECONDARY_DOMAINSinstead of a number inconstants.py.The renumbering alone would not stop this from coming back, because nothing ever checked the
allocation against the ephemeral range. Two checks now do:
pool being asked for: every slice outside the ephemeral range, pools pairwise disjoint,
domain 0 rejected, no slice past 65535. The range is read from
/proc/sys/net/ipv4/ip_local_port_rangeand then widened to at least the Linux default, so ahost with a narrow range cannot accept an allocation that breaks on a stock machine.
test_dds_domain_allocationreads the generated CTest properties back andchecks them against that machine's live kernel range. This matters because
ci.ymlbuilds andtests in separate jobs, so the machine that validated the table is not the machine that runs
the tests. It also catches a hand-written
ROS_DOMAIN_IDthat never went through the table,and a resource lock dropped by a property overwrite.
New allocation:
ros2_medkit_sovd_service_interfaceros2_medkit_param_beaconros2_medkit_topic_beaconros2_medkit_graph_providerros2_medkit_fault_reporterros2_medkit_log_bridgeros2_medkit_action_status_bridgeros2_medkit_diagnostic_bridgeros2_medkit_fault_managerros2_medkit_opcuaros2_medkit_graph_watchdogros2_medkit_gatewayros2_medkit_integration_testsTwo things to know about the trade-off:
test could never meet a later test. Now a domain comes back after the pool has been walked
once (25 tests for the gateway, 35 for the integration tests). The lock covers concurrency,
not a process that outlived its own test.
error forced a re-carve of the whole table to add one test. The cost is that an oversubscribed
pool serialises tests quietly instead of failing loudly.
Reserving the ports in CI with
net.ipv4.ip_local_reserved_portswas the other option in theissue. It was not taken. Every CI job runs in a job container, which is not privileged, so the
sysctl cannot be set there, and it would not help a developer machine either.
Issue
Type
Testing
The failure was reproduced first, on Jazzy with
rmw_cyclonedds_cppand domain 183, whosediscovery port is 53150:
ddsi_udp_create_conn: failed to bind to ANY:53150: address in use, thenrmw_create_node: failed to create domain- the same text as in the issueThe port offsets a domain really binds were measured, not assumed, with three participants on
domain 97: CycloneDDS uses offsets 0, 1, 10, 11 and Fast-DDS uses 0, 10, 11, which matches the
RTPS scheme. The code treats the whole 250-port slice as owned, which is a superset of that and
needs no assumption about how many participants a test starts.
Each guard was then checked by breaking it on purpose and confirming it fails:
claims domain 155, whose UDP slice 46150-46399 overlaps the kernel ephemeral port range 32768-60999domain 30 is allocated twiceappears 2 times in MEDKIT_DOMAIN_TABLEROS_DOMAIN_ID=183test_dds_domain_allocationfailsRESOURCE_LOCKoverwritten aftermedkit_set_test_domaintest_dds_domain_allocationfailsThe interlock itself was measured too, because the whole reuse scheme rests on it. Six tests, a
pool of two domains,
ctest -j 6, each test sleeping one second and recording its start and endtime:
in parallel with each other
To verify: build the workspace and run
colcon test.test_dds_domain_allocationis registeredin every package that uses domains, and prints the pool, the ephemeral range it read, and how
many tests it checked.
Checklist