doca_xplane: add new plugin#102
Merged
Merged
Conversation
📝 WalkthroughWalkthroughNew sosreport plugin adds DOCA XPlane diagnostics collection: copies service files and logs, conditionally runs Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as DocaXPlane Plugin
participant Sysroot as Sysroot/Filesystem
participant Client as doca-xplane-client
participant Logger as Logger
Plugin->>Sysroot: add copy specs (/opt/.../xplane, /var/log/xplane)
Plugin->>Sysroot: check executable `doca-xplane-client`
alt client available
Plugin->>Client: run `version`, `get-status`
Client-->>Plugin: status + output (raw)
Plugin->>Logger: log non-zero status and stop (if status != 0)
alt status == 0
Plugin->>Plugin: parse JSON output
Plugin->>Logger: log JSON errors and stop (if parse fails)
Plugin->>Client: register per-plane commands (for i in 0..num_planes-1)
end
else client missing
Plugin->>Logger: skip CLI collection
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.5)sos/report/plugins/doca_xplane.pyComment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/plugins/doca_xplane_client.py`:
- Around line 34-35: The code directly parses res["output"] into status and
accesses status["topologySummary"]["planes"], which can raise on invalid JSON or
missing keys; wrap json.loads(res["output"]) in a try/except catching
JSONDecodeError and other Exceptions, then validate that status is a dict and
contains "topologySummary" as a dict and "planes" as an int (or coerce/default
to 0/None) before assigning num_planes; on any parse/shape failure log a clear
warning including res["output"] and return or skip collection gracefully rather
than letting the plugin crash (refer to the variables/status keys status,
num_planes, and res["output"] to locate the change).
- Line 1: The file incorrectly imports List from ast; change the import to use
typing.List (e.g. replace "from ast import List" with "from typing import List")
or remove the import and use the native generic "list[str]" if you target Python
3.9+, so the type annotation(s) like the List[str] used in functions/classes
(see usages around the current List[str] at/near line 37) reference the proper
typing List rather than ast.List.
🪄 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: Pro
Run ID: 7251f158-8579-46bf-bb39-b85c7e8d7b5f
📒 Files selected for processing (2)
sos/report/plugins/doca_xplane.pysos/report/plugins/doca_xplane_client.py
qbarrand
changed the title
doca_xplane and doca_xplane_client
There was a problem hiding this comment.
♻️ Duplicate comments (1)
sos/report/plugins/doca_xplane_client.py (1)
32-35:⚠️ Potential issue | 🟠 MajorHarden JSON shape/type handling and avoid broad catch.
At Line 33-35,
num_planesis assumed present and range-compatible, and Line 35 catchesException. Please validatestatusshape and coerce/validateplanestointbeforerange(num_planes), while catching specific parse/type errors.Proposed fix
- try: - status = json.loads(res["output"]) - num_planes = status["topologySummary"]["planes"] - except Exception as e: - self._log_error(f"Failed to parse status: {e}") - return + try: + status = json.loads(res["output"]) + topology_summary = status.get("topologySummary", {}) + num_planes = int(topology_summary.get("planes", 0)) + if num_planes < 0: + raise ValueError("planes must be >= 0") + except (json.JSONDecodeError, TypeError, ValueError, AttributeError) as e: + self._log_error(f"Failed to parse status: {e}") + returnAlso applies to: 49-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sos/report/plugins/doca_xplane_client.py` around lines 32 - 35, The current try/except around json.loads(res["output"]) and num_planes assumes status["topologySummary"]["planes"] exists and is an int and catches all Exceptions; update the block to (1) catch specific exceptions (e.g., json.JSONDecodeError, KeyError, TypeError, ValueError) instead of a broad Exception, (2) validate that status is a dict and that "topologySummary" is a dict containing "planes", (3) coerce/validate planes to an integer (e.g., int(planes_value)) and ensure it's >=0 before using range(num_planes), and (4) apply the same validation/coercion and specific-exception handling to the other occurrence where num_planes is read (the second status/planes access) so both places use the same robust checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@sos/report/plugins/doca_xplane_client.py`:
- Around line 32-35: The current try/except around json.loads(res["output"]) and
num_planes assumes status["topologySummary"]["planes"] exists and is an int and
catches all Exceptions; update the block to (1) catch specific exceptions (e.g.,
json.JSONDecodeError, KeyError, TypeError, ValueError) instead of a broad
Exception, (2) validate that status is a dict and that "topologySummary" is a
dict containing "planes", (3) coerce/validate planes to an integer (e.g.,
int(planes_value)) and ensure it's >=0 before using range(num_planes), and (4)
apply the same validation/coercion and specific-exception handling to the other
occurrence where num_planes is read (the second status/planes access) so both
places use the same robust checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4a171c9d-509b-4f63-b743-f32d8decb98e
📒 Files selected for processing (2)
sos/report/plugins/doca_xplane.pysos/report/plugins/doca_xplane_client.py
✅ Files skipped from review due to trivial changes (1)
- sos/report/plugins/doca_xplane.py
filanov
reviewed
Apr 13, 2026
Collaborator
filanov
left a comment
filanov
left a comment
There was a problem hiding this comment.
I suggest to have a single plugin instead of spliting it
qbarrand
changed the title
doca_xplane and doca_xplane_clientdoca_xplane: add new plugin
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/plugins/doca_xplane.py`:
- Around line 65-70: The current call to self.add_cmd_output eagerly builds
num_planes * len(subcommands) commands; clamp the fan-out by introducing a
MAX_PLANES constant (or config) and replace range(num_planes) with
range(min(num_planes, MAX_PLANES)) when constructing the list comprehension that
uses CLIENT_COMMAND, subcommands and num_planes; also emit a warning/log (e.g.,
via self.logger or processLogger) when num_planes is truncated so callers know
the output is limited.
- Around line 50-55: The try/except around json.loads and extracting
status["topologySummary"]["planes"] is too broad and does not validate
num_planes; update the block to catch specific exceptions (json.JSONDecodeError,
KeyError, TypeError, ValueError) when calling json.loads and accessing status
keys, then validate num_planes (the value from
status["topologySummary"]["planes"]) is an integer >= 0 before it's consumed by
the later range(...) list comprehension; if num_planes is a numeric string
attempt int(...) conversion, otherwise call self._log_error with a clear message
and return to avoid using an invalid value.
🪄 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: Pro Plus
Run ID: 0de75a25-7130-4835-afe9-97fe050af74b
📒 Files selected for processing (1)
sos/report/plugins/doca_xplane.py
Add a plugin for DOCA XPlane. Signed-off-by: Quentin Barrand <qbarrand@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sos/report/plugins/doca_xplane.py (1)
42-44: Improve failure log context forget-status.At Line 43, the error message drops the return code/output, which makes field debugging harder. Include command status and output snippet in the log.
Proposed patch
- if res["status"] != 0: - self._log_error("Failed to get status") + if res["status"] != 0: + self._log_error( + "Failed to get status: " + f"rc={res.get('status')} output={res.get('output', '').strip()[:256]}" + ) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sos/report/plugins/doca_xplane.py` around lines 42 - 44, The current check in the doca_xplane plugin swallows useful context when the get-status command fails; update the failure logging in the block that checks res["status"] (and calls self._log_error) to include the numeric return code and a short snippet of the command output/error (e.g., res.get("output") or res.get("stderr") trimmed to ~200 chars) so the message becomes something like "Failed to get status: code=X, output='...'", and ensure you guard against missing keys when building the message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sos/report/plugins/doca_xplane.py`:
- Around line 42-44: The current check in the doca_xplane plugin swallows useful
context when the get-status command fails; update the failure logging in the
block that checks res["status"] (and calls self._log_error) to include the
numeric return code and a short snippet of the command output/error (e.g.,
res.get("output") or res.get("stderr") trimmed to ~200 chars) so the message
becomes something like "Failed to get status: code=X, output='...'", and ensure
you guard against missing keys when building the message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f5fba74-74ff-4e19-9d99-1254abb8d267
📒 Files selected for processing (1)
sos/report/plugins/doca_xplane.py
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
2 participants
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.
Add plugins for DOCA XPlane.
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines
Summary by CodeRabbit