Use procfs for CPU and process collection - #292
salemmoustafa wants to merge 4 commits into
Conversation
github-actions
Bot
added
documentation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Four unresolved moderate issues remain in procfs process collection.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (3)
What changed in this PR
Ports process collection from rocm-smi/top to /proc, removing KFD analysis and updating configuration, tests, and documentation.
Changes:
- Adds procfs-based CPU and process sampling.
- Removes KFD fields, limits, and obsolete configuration.
- Updates tests, fixtures, and plugin documentation.
| File | Changes and review notes |
|---|---|
test/unit/plugin/test_process_collector.py |
Updates procfs collection and parsing tests. |
test/unit/plugin/test_process_analyzer.py |
Updates analyzer tests. |
test/unit/plugin/test_analyzer_args_build_from_model.py |
Updates argument construction tests. |
test/functional/fixtures/process_plugin_config.json |
Removes obsolete KFD configuration. |
nodescraper/plugins/inband/process/processdata.py |
Removes KFD data from the process model. |
nodescraper/plugins/inband/process/process_collector.py |
Implements procfs collection. Four unresolved moderate issues remain: robustly parse names containing parentheses, rank only currently running processes, exclude guest CPU fields from totals, and apply the PID fallback when name lookup returns an empty value. |
nodescraper/plugins/inband/process/process_analyzer.py |
Analyzes aggregate CPU usage without KFD data. |
nodescraper/plugins/inband/process/collector_args.py |
Adds sampling interval configuration. |
nodescraper/plugins/inband/process/analyzer_args.py |
Removes the KFD limit argument. |
docs/PLUGIN_DOC.md |
Documents the new procfs behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
salemmoustafa
changed the title
|
|
||
|
|
||
| class ProcessAnalyzerArgs(AnalyzerArgs): | ||
| max_kfd_processes: int = Field( |
There was a problem hiding this comment.
why are we removing this information?
There was a problem hiding this comment.
It was intentionally removed to match the upstream change. AMD SMI now owns that functionality, so ProcessPlugin is limited to CPU process collection
| return None | ||
|
|
||
| # guest and guest_nice are already included in user and nice. | ||
| return sum(values[:8]), values[3] + values[4] |
There was a problem hiding this comment.
A disk-wait node that used to trip max_cpu_usage can now look fine. If the goal is “same check, better source,” idle should be values[3] only. If iowait-as-idle is intentional, that should be specified for cpu_usage / max_cpu_usage.
There was a problem hiding this comment.
Excluding I/O-wait was intentional, I'll update field and threshold descriptions to document this behavior explicitly.
| self, top_n_process: int, sample_interval_seconds: float | ||
| ) -> tuple[Optional[float], list[tuple[str, str]]]: | ||
| """Collect aggregate CPU usage and top process CPU shares.""" | ||
| stat1 = self._run_sut_cmd(self.CMD_PROC_STAT) |
There was a problem hiding this comment.
i think we dont want this in the artifacts cause it will bloat a lot, so lets set log_artifact=False for both places where this is called
| if stat1.exit_code != 0: | ||
| return None, [] | ||
| dump1 = self._run_sut_cmd(self.CMD_PROCESS_STAT) | ||
| if dump1.exit_code != 0: |
There was a problem hiding this comment.
if this fails, something should be logged as a warning, self.logger.WARNING(...) see how other places do it. Same below
| process_data.cpu_usage = cpu_usage | ||
| process_data.processes = processes | ||
| self._log_event( | ||
| category="PROCESS_READ", |
There was a problem hiding this comment.
this cannot be a string, you either place a new enum in EventCategory or use an existing one, throughout
Summary
Replaces the ProcessPlugin's
topandrocm-smicollection with distro-independent/procsampling. Removes KFD process analysis and updates configuration, tests, and documentation.Test plan
pytest test/unitpytest test/functional(if applicable) — focused ProcessPlugin test passed locally; full suite passed in CIpre-commit run --all-filesChecklist