[Tests] Introduce network namespace separation for XML RPC server#71558
Open
MarekPikula wants to merge 25 commits intoproject-chip:masterfrom
Open
[Tests] Introduce network namespace separation for XML RPC server#71558MarekPikula wants to merge 25 commits intoproject-chip:masterfrom
MarekPikula wants to merge 25 commits intoproject-chip:masterfrom
Conversation
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a more robust concurrency framework for test execution, including a new WrappedProcess base class and a dedicated XmlRpcServerProcessManager to handle XML-RPC communication within network namespaces. The changes also include enhanced logging configurations and infrastructure updates to support management network namespaces on Linux. Feedback is provided regarding a discrepancy between the StartStopContextMixin docstring and its implementation of exception suppression, as well as a minor typo in a comment.
5 tasks
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a process boundary for the XML-RPC server (optionally launched inside a Linux network namespace) as groundwork for future concurrent test execution and worker isolation.
Changes:
- Add a new
chiptest.concurrentmodule providing aWrappedProcessabstraction, spawn-context helpers, and a multiprocessing-friendly cancellable queue. - Move the XML-RPC server out of the main process into a managed subprocess, bridged by a manager thread that dispatches calls back into
AppsRegister. - Extend Linux isolated networking to add a dedicated management namespace (
MGMT) and wire the test runner to startAppsRegister/XML-RPC in that namespace.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/python_testing/matter_testing_infrastructure/matter/testing/linux/namespace.py |
Adds a dedicated MGMT network namespace and maps SubprocessKind.MGMT to it. |
scripts/tests/run_test_suite.py |
Adds --log-level-rpc, wires LogConfig.level_rpc, and starts AppsRegister with a netns wrapper for the RPC server. |
scripts/tests/chiptest/worker.py |
Switches queue imports to the new chiptest.concurrent.work_queue module. |
scripts/tests/chiptest/results.py |
Switches queue imports to the new chiptest.concurrent.work_queue module. |
scripts/tests/chiptest/log_config.py |
Adds level_rpc to support independent RPC logging verbosity. |
scripts/tests/chiptest/concurrent/work_queue.py |
Extends CancellableQueue to optionally use multiprocessing.Manager primitives and adds wait helpers. |
scripts/tests/chiptest/concurrent/process.py |
Introduces WrappedProcess, shared ProcessState, and lifecycle/termination escalation logic. |
scripts/tests/chiptest/concurrent/context.py |
Adds a spawn-context helper that can install a Linux wrapper executable, plus a start/stop context mixin. |
scripts/tests/chiptest/concurrent/__init__.py |
Declares the concurrent module package. |
scripts/tests/chiptest/accessories.py |
Refactors AppsRegister XML-RPC hosting into a manager thread + namespaced subprocess, with RPC function dispatch/validation. |
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
scripts/tests/chiptest/concurrent/work_queue.py:172
wait_for_cancelled()/wait_for_closed()always route throughwait_for_mp_managed(), which polls every 0.1s even when the underlying event is a localthreading.Event(i.e. whenmp_manageris None). That introduces unnecessary wakeups/CPU churn for normal single-process usage. Consider detecting the non-Manager case and using a direct blockingEvent.wait()(or havingCancellableQueueremember whether it was created with a manager and select the appropriate wait strategy).
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
|
PR #71558: Size comparison from 04ec448 to 7fcfe7d Full report (33 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
5 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #71558 +/- ##
=======================================
Coverage 54.52% 54.52%
=======================================
Files 1588 1588
Lines 108571 108571
Branches 13365 13365
=======================================
Hits 59203 59203
Misses 49368 49368 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
|
PR #71558: Size comparison from de3c270 to f165f4c Increases above 0.2%:
Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
enkiusz
approved these changes
Apr 20, 2026
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
PR #71558: Size comparison from de3c270 to fd13126 Increases above 0.2%:
Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
antonijakubiak-samsung
left a comment
antonijakubiak-samsung
left a comment
There was a problem hiding this comment.
Hi, looks very solid for me. I have two suggestions:
- license
- save-restore context.obj.log_config.filter.msg_counter
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
PR #71558: Size comparison from adb9376 to ae94388 Full report (26 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32)
|
Contributor
|
@MarekPikula Are there any changes needed when using the test runner to execute testing like matter,js by replacing chip-tool with an other controller? ... we do like https://github.com/matter-js/matter.js/blob/69706ab580b34ddfcd8848c08744b90b103a4e17/.github/workflows/chip-matterjs-tests.yml#L104 |
Contributor
Author
Hi @Apollon77, I replied to your email regarding the test failure you mentioned. As for this PR, there should be no impact when using different controllers. I'm making every effort to ensure the changes are transparent to both controllers and tests. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
PR #71558: Size comparison from 5357019 to 473f875 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #71558: Size comparison from 46582e0 to 87bbabe Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
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.
Summary
This PR is a major step in the ongoing effort to introduce concurrent test execution. To support concurrent execution, the XML-RPC server must be namespaced so that, in the future, each worker process can run its own isolated XML-RPC server within its own namespace context. Enabling this requires introducing a namespace process boundary for the XML-RPC server, which must still communicate with the main process.
The
WrappedProcessabstraction introduced in this PR, together with its state management machinery, will be used for worker isolation in subsequent PRs. Some of the complexity added here may appear redundant in isolation, but it will be essential in the next stage. I considered introducing either XML-RPC isolation or worker process isolation first, and chose the former because it brings in less code for the initial review of the multi-process flow. A proof of concept with full concurrent test execution is available here. This PR brings in a subset of that implementation. All abstractions introduced in thechiptest.concurrentmodule will be reused in follow-up PRs.For XML-RPC isolation, the following components are required:
AppsRegister, responsible for starting the RPC server and containing the functions which are supposed to be called by it.AppsRegister.For details on
WrappedProcesslifecycle management and error handling, please refer to the docstrings in the code. I am aware that this PR introduces a substantial amount of non-trivial multiprocessing logic, so if any part of the behavior is difficult to follow, please point it out and I will improve the docstrings accordingly.Related issues
Continuation of the effort toward concurrent tests. PRs from the previous patch set:
Submitted alongside:
Testing
This PR has been tested extensively by manually exercising various failure modes with
run_test_suite.py, with exceptions injected at different points in the flow.Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines