[mellanox_firmware] PCC collector (opt in) - #104
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds optional PCC collection to the Mellanox firmware suite. The collector queries PPCC registers through ChangesPCC collection flow
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sos/report/mellanox_firmware_suite/tools/base_tool.py`:
- Around line 49-52: The cache early-return in execute_cmd is skipping the
required side-effect of writing artifacts when filename or subdir is provided;
modify execute_cmd so that when filename or subdir is set it does not
short-circuit on a cache hit but still calls _collect_cmd_output (or an
equivalent helper) to create/write the file into the requested subdir, while
still returning the cached stdout when appropriate; use the existing
cache_key/self.ctx.cache logic to retrieve stdout but ensure
_collect_cmd_output(cmd, output, filename, subdir) is invoked before returning
if filename or subdir are non-empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cf45634c-fd07-4972-a83c-11ebe05c891d
📒 Files selected for processing (6)
sos/report/mellanox_firmware_suite/collectors/collector_manager.pysos/report/mellanox_firmware_suite/collectors/pcc_collector.pysos/report/mellanox_firmware_suite/tools/MFT/mlxreg.pysos/report/mellanox_firmware_suite/tools/MSTFlint/mstreg.pysos/report/mellanox_firmware_suite/tools/base_tool.pysos/report/plugins/mellanox_firmware.py
| if get_cached and cache_key in self.ctx.cache: | ||
| return self.ctx.cache[cache_key] | ||
|
|
||
| rc, output = self._run_command(cmd, timeout, filename) | ||
| rc, output = self._run_command(cmd, timeout, filename, subdir=subdir) |
There was a problem hiding this comment.
Don't short-circuit archival calls on cache hits.
When filename/subdir is provided, execute_cmd() has a required side effect: it must write the collected artifact. The cache guard returns before _collect_cmd_output() runs, so a repeated call for the same command can hand back cached stdout and never create the requested file in the requested subdirectory.
Suggested fix
- if get_cached and cache_key in self.ctx.cache:
+ if filename is None and get_cached and cache_key in self.ctx.cache:
return self.ctx.cache[cache_key]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if get_cached and cache_key in self.ctx.cache: | |
| return self.ctx.cache[cache_key] | |
| rc, output = self._run_command(cmd, timeout, filename) | |
| rc, output = self._run_command(cmd, timeout, filename, subdir=subdir) | |
| if filename is None and get_cached and cache_key in self.ctx.cache: | |
| return self.ctx.cache[cache_key] | |
| rc, output = self._run_command(cmd, timeout, filename, subdir=subdir) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sos/report/mellanox_firmware_suite/tools/base_tool.py` around lines 49 - 52,
The cache early-return in execute_cmd is skipping the required side-effect of
writing artifacts when filename or subdir is provided; modify execute_cmd so
that when filename or subdir is set it does not short-circuit on a cache hit but
still calls _collect_cmd_output (or an equivalent helper) to create/write the
file into the requested subdir, while still returning the cached stdout when
appropriate; use the existing cache_key/self.ctx.cache logic to retrieve stdout
but ensure _collect_cmd_output(cmd, output, filename, subdir) is invoked before
returning if filename or subdir are non-empty.
There was a problem hiding this comment.
Pre-existing execute_cmd cache behavior from the modular collector refactor — this PR only adds subdir passthrough and doesn't change caching. Not a regression, and PCC isn't affected: each ppcc_get has a unique cache key.
Out of scope for this PR.
There was a problem hiding this comment.
@DanGoldberg, understood — thanks for the context. Since each ppcc_get uses a unique cache key the early-return path won't be hit for those calls, so there's no practical regression here.
The general concern (cache hit suppressing a file-write side-effect) still exists for any future caller that reuses a cache key with a filename/subdir argument, so it would be worth addressing eventually. Would you like me to open a follow-up GitHub issue to track it?
(ﾐ ^ ᆺ ^ ﾐ)
🧠 Learnings used
Learnt from: anton-metsner
Repo: NVIDIA/doca-sosreport PR: 91
File: sos/report/mellanox_firmware_suite/collectors/cable_collector.py:54-111
Timestamp: 2026-01-22T22:39:45.230Z
Learning: In the sos plugin framework (sos/report/plugins), directory creation is handled by the setup flow and path helper methods (e.g., plugin.archive.get_archive_path() and plugin._make_command_filename()). Do not add explicit os.makedirs() calls before writing files to paths produced by these methods, as directories will be created automatically when needed. This applies to Python files under the sos/report directory and its plugins.
Calculation was made by 4 minutes per devices
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sos/report/plugins/mellanox_firmware.py (1)
80-93: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTimeout warning threshold doesn't account for
pccbeing opt-in.
collect_pcc_info()incollector_manager.pyonly runsPccCollectorper device whenplugin.get_option("pcc", default=False)is true. But thisexpected_timeoutcalculation unconditionally assumes ~4 min/device (device_count * 240), which is the PCC-inclusive estimate. For the default case (pcc=False), the actual expected per-device time is far lower (previously ~180s), so this will now emit misleading "timeout may be too low" warnings even when PCC collection isn't running at all.Consider branching the multiplier on whether the
pccoption is enabled:🐛 Proposed fix
`@property` def timeout(self): base_timeout = super().timeout device_count = len([d for d in self.device_contexts if d.primary]) - expected_timeout = device_count * 240 + per_device_seconds = 240 if self.get_option("pcc") else 180 + expected_timeout = device_count * per_device_seconds if base_timeout < expected_timeout: self._log_warn( f"Plugin timeout {base_timeout}s may be too low for " f"{device_count} device(s). Expected ~{expected_timeout}s " - f"(~4 minutes per device)." + f"(~{per_device_seconds // 60} minutes per device)." ) return base_timeout🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sos/report/plugins/mellanox_firmware.py` around lines 80 - 93, The timeout warning in the timeout property of MellanoxFirmwarePlugin is using a PCC-inclusive per-device estimate even when pcc is disabled, which makes the warning inaccurate by default. Update the expected_timeout calculation to branch on the plugin’s pcc option (via get_option("pcc", default=False)) so the warning uses the higher 240s/device only when PccCollector runs, and a lower baseline estimate otherwise; keep the warning/logging logic in the timeout property and _log_warn call unchanged apart from the adjusted threshold.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@sos/report/plugins/mellanox_firmware.py`:
- Around line 80-93: The timeout warning in the timeout property of
MellanoxFirmwarePlugin is using a PCC-inclusive per-device estimate even when
pcc is disabled, which makes the warning inaccurate by default. Update the
expected_timeout calculation to branch on the plugin’s pcc option (via
get_option("pcc", default=False)) so the warning uses the higher 240s/device
only when PccCollector runs, and a lower baseline estimate otherwise; keep the
warning/logging logic in the timeout property and _log_warn call unchanged apart
from the adjusted threshold.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5362f727-6199-4a80-bea4-9c946e3fb71b
📒 Files selected for processing (3)
sos-nvdebug.confsos-nvidia.confsos/report/plugins/mellanox_firmware.py
✅ Files skipped from review due to trivial changes (2)
- sos-nvdebug.conf
- sos-nvidia.conf
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sos/report/mellanox_firmware_suite/collectors/collector_manager.py (1)
33-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider merging the two device-context loops.
collect_pcc_info()iteratesself.device_contextstwice — once forPccCollector, once forDocaPccCollector. Since both collectors are independently gated and there's no ordering dependency evident here, a single loop running both collectors perctxwould be simpler and avoid double iteration.♻️ Proposed refactor
def collect_pcc_info(self): if not self.plugin.get_option("pcc", default=False): return for ctx in self.device_contexts: PccCollector().run(self.plugin, ctx) - - for ctx in self.device_contexts: DocaPccCollector().run(self.plugin, ctx)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sos/report/mellanox_firmware_suite/collectors/collector_manager.py` around lines 33 - 41, Merge the two iterations in collect_pcc_info into a single loop over self.device_contexts, invoking both PccCollector().run and DocaPccCollector().run for each ctx. Preserve the existing pcc option guard and collector invocation order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sos/report/mellanox_firmware_suite/collectors/doca_pcc_collector.py`:
- Around line 45-56: Update _collect so the side-effecting PCC “set” exec_cmd
runs only when allow_system_changes is enabled, using the plugin’s existing
change-gating mechanism. Preserve the current failure logging and early-return
behavior when the gated command is executed, while skipping the set step
entirely when changes are not allowed.
---
Nitpick comments:
In `@sos/report/mellanox_firmware_suite/collectors/collector_manager.py`:
- Around line 33-41: Merge the two iterations in collect_pcc_info into a single
loop over self.device_contexts, invoking both PccCollector().run and
DocaPccCollector().run for each ctx. Preserve the existing pcc option guard and
collector invocation order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 90f17b56-fdd3-4e00-ac2f-71058fa546bb
📒 Files selected for processing (2)
sos/report/mellanox_firmware_suite/collectors/collector_manager.pysos/report/mellanox_firmware_suite/collectors/doca_pcc_collector.py
This reverts commit 2b63e94. The doca_pcc tool is going to be deprecated and doesn't and won't support new devices
|
This feature was tested on various machine types with up to 10 NICs. Functional review was done with Yaniv Serlin. |
1 participant
Add a new PCC collector.
This collection is disabled by default and requires
adding "-k mellanox_firmware.pcc=true" to the command line
to enable it.
Key changes:
a dedicated directory within mellanox_firmware output directory
to reduce clutter
Signed-off-by: Dan Goldberg dgoldberg@nvidia.com
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines
Summary by CodeRabbit
New Features
Improvements