Conversation
fregataa
added a commit
that referenced
this pull request
Mar 31, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the agent’s container network-stat collection in CGROUP mode to avoid unreliable per-thread namespace switching by reading /proc/<container_pid>/net/dev directly.
Changes:
- Replace
netstat_ns()/setns()/psutil-based namespace reads with a/proc/<pid>/net/devparser (read_proc_net_dev()). - Update
MemoryPluginCGROUP-mode net stat collection to use container PID fromcontainer.show()["State"]["Pid"]. - Refactor unit tests to target PID-based net stat collection and remove the old namespace-switching test coverage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/ai/backend/agent/docker/intrinsic.py |
Introduces /proc/<pid>/net/dev parsing and switches MemoryPlugin CGROUP net stats to use container PID instead of SandboxKey/netns. |
tests/unit/agent/test_docker_intrinsic.py |
Updates tests for PID-based net stats, adds PID validation cases, and removes netns work tests (but still contains outdated netns-based fixtures/tests that need updating). |
Comments suppressed due to low confidence (1)
tests/unit/agent/test_docker_intrinsic.py:196
memory_cgroup_contextmockscurrent_loop().run_in_executorto always return0, butsysfs_impl()now awaitsrun_in_executor(..., read_proc_net_dev, pid)and unpacks a(rx, tx)tuple. With the current mock this will raise during tuple-unpacking. Adjust the mock to call the provided function (fn(*args)) or return a(0, 0)tuple forread_proc_net_devwhile still returning an int forget_scratch_size.
patch(
"ai.backend.agent.docker.intrinsic.read_proc_net_dev",
return_value=(0, 0),
),
patch(
"ai.backend.agent.docker.intrinsic.current_loop",
) as mock_loop,
):
mock_container_instance = AsyncMock()
mock_container_instance.show.return_value = mock_container_data
mock_container_cls.return_value = mock_container_instance
mock_loop.return_value.run_in_executor = AsyncMock(return_value=0)
yield ctx
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fregataa
added a commit
that referenced
this pull request
Mar 31, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fregataa
added a commit
that referenced
this pull request
Apr 1, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fregataa
added a commit
that referenced
this pull request
Apr 1, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
seedspirit
reviewed
Apr 2, 2026
Contributor
seedspirit
left a comment
There was a problem hiding this comment.
The logic seems fine, but I think we'll need @HyeockJinKim's review as well.
Comment on lines
+583
to
+618
| def test_parses_standard_format(self, tmp_path: Path) -> None: | ||
| """Parses standard /proc/net/dev format: skips headers, excludes lo, | ||
| returns correct rx/tx sums.""" | ||
| net_dev = tmp_path / "net_dev" | ||
| net_dev.write_text(self.SAMPLE_NET_DEV) | ||
| with patch( | ||
| "ai.backend.agent.docker.intrinsic.Path", | ||
| return_value=net_dev, | ||
| ): | ||
| result = read_proc_net_dev(42) | ||
| assert result.rx_bytes == 50000 | ||
| assert result.tx_bytes == 80000 | ||
|
|
||
| def test_sums_multiple_interfaces(self, tmp_path: Path) -> None: | ||
| """Sums rx/tx bytes across all non-loopback interfaces.""" | ||
| net_dev = tmp_path / "net_dev" | ||
| net_dev.write_text(self.MULTI_IFACE_NET_DEV) | ||
| with patch( | ||
| "ai.backend.agent.docker.intrinsic.Path", | ||
| return_value=net_dev, | ||
| ): | ||
| result = read_proc_net_dev(42) | ||
| assert result.rx_bytes == 40000 # 10000 + 30000 | ||
| assert result.tx_bytes == 60000 # 20000 + 40000 | ||
|
|
||
| def test_loopback_only_returns_zero(self, tmp_path: Path) -> None: | ||
| """When only loopback is present, returns (0, 0).""" | ||
| net_dev = tmp_path / "net_dev" | ||
| net_dev.write_text(self.LO_ONLY_NET_DEV) | ||
| with patch( | ||
| "ai.backend.agent.docker.intrinsic.Path", | ||
| return_value=net_dev, | ||
| ): | ||
| result = read_proc_net_dev(42) | ||
| assert result.rx_bytes == 0 | ||
| assert result.tx_bytes == 0 |
Contributor
There was a problem hiding this comment.
I think this test can be generalized with parametrize
…d of using setns Replace netstat_ns()/netstat_ns_work() namespace-switching approach with direct /proc/[container_pid]/net/dev reads. The old approach was unreliable because setns() only changes the calling thread's namespace, but psutil.net_io_counters() reads /proc/self/net/dev which reflects the process-level (host) namespace, causing inflated network statistics. The new read_proc_net_dev() function reads the container's /proc entry directly using the container PID from Docker inspect (State.Pid), which works correctly from any thread without namespace switching. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…at_ns - Remove TestNetstatNsWork class (tested dead code that used setns) - Replace TestMemoryPluginNamespaceValidation with TestMemoryPluginContainerPidValidation (PID=0, valid PID, OSError cases) - Update _SysfsMocks to use State.Pid + read_proc_net_dev instead of SandboxKey + netstat_ns - Add TestReadProcNetDev unit tests: standard format parsing, multi-interface sum, loopback exclusion, nonexistent PID error - Update run_in_executor mocks to delegate to actual functions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…d-self/net/dev When container PID is unavailable (PID=0), fall back to SandboxKey + setns() + /proc/thread-self/net/dev instead of skipping net stats. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… type Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ainer PID Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
netstat_ns()+setns()with direct/proc/[container_pid]/net/devreading for container network stats in cgroup modeProcessPoolExecutorunavailable → thread pool fallback →setns()changes thread namespace butpsutilreads/proc/self/net/dev(process-level = host namespace)netstat_ns(),netstat_ns_work(), unused imports (multiprocessing,ProcessPoolExecutor,nsenter)Fixes BA-5519
Test plan
docker statsoutput/proc/[container_pid]/net/devshows only container interfaces (lo, eth0)🤖 Generated with Claude Code