feat(sosreport): Add OVS analysis command #211
Conversation
- Add /sosreport:ovs-db command for OVS data analysis - Default mode parses text files in sos_commands/openvswitch/ (no ovsdb-tool needed) - Database mode (--db) uses ovsdb-tool for conf.db queries and custom --query - Analyzes: system info, topology, bridges, interfaces, OpenFlow flows - Shows active drop flows with match details - Shows top flows by packet count with match/actions - Shows port statistics with interface names mapped from ofport - Reports datapath flow table health from upcall.show - Reports OVS internal statistics from coverage.show - Maps pod interfaces via external_ids Assisted-by: Claude noreply@anthropic.com Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tssurya The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @tssurya. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
WalkthroughThe changes introduce a new OVS database analysis capability to the sosreport plugin. A new command Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (6 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: 3
🧹 Nitpick comments (11)
plugins/sosreport/skills/ovs-db-analysis/SKILL.md (4)
52-67: Add language specifier to code block.Per markdownlint MD040, fenced code blocks should have a language specified for proper syntax highlighting.
-``` +```text sosreport-hostname-date/
72-74: Add language specifier to code block.-``` +```text <plugin-root>/skills/ovs-db-analysis/scripts/analyze_ovs_db.py
214-219: Add language specifier to code block.-``` +```text DATAPATH FLOW TABLE HEALTH
225-292: Add language specifier to code block.-``` +```text ================================================================================plugins/sosreport/skills/ovs-db-analysis/scripts/analyze_ovs_db.py (4)
163-166: Remove unused variableindent.The variable
indentis assigned but never used. Per Ruff F841.for line in lines[1:]: - indent = len(line) - len(line.lstrip()) stripped = line.strip()
48-48: Consider explicitOptionaltype hints for clarity.Per PEP 484 and Ruff RUF013, use explicit
T | Noneinstead of implicitOptionalwith defaultNone.- def query(self, table: str, columns: List[str] = None, where: List = None) -> List[Dict]: + def query(self, table: str, columns: List[str] | None = None, where: List | None = None) -> List[Dict]:
1116-1117: Use explicitOptionaltype hint.-def analyze_from_database(db_path: Path, sosreport_path: Path = None, +def analyze_from_database(db_path: Path, sosreport_path: Path | None = None, include_flows: bool = True):
112-115: Edge case:is_dropmay produce false positives.The check
'drop' in self.actions.lower()could match actions containing "drop" as a substring in other contexts (e.g., hypothetical action names). Consider a more precise check.@property def is_drop(self) -> bool: """Check if this flow drops packets""" - return self.actions.strip() in ['drop', ''] or 'drop' in self.actions.lower() + actions_lower = self.actions.strip().lower() + return actions_lower in ['drop', ''] or actions_lower == 'drop' or ',drop' in actions_lower or 'drop,' in actions_lowerAlternatively, use word boundary matching if more precision is needed.
plugins/sosreport/README.md (1)
100-103: Consider showing all options in usage line.The usage line omits
--dband--flows-onlyoptions that are documented in the detailed command docs. For consistency withovs-db.md, consider including them.**Usage:** ```bash -/sosreport:ovs-db <sosreport-path> [--query <json>] +/sosreport:ovs-db <sosreport-path> [--db] [--flows-only] [--query <json>]</blockquote></details> <details> <summary>plugins/sosreport/commands/ovs-db.md (2)</summary><blockquote> `74-86`: **Add language specifier to code block.** For consistency with markdownlint MD040. ```diff -``` +```text sosreport-hostname-date/
91-93: Add language specifier to code block.-``` +```text <plugin-root>/skills/ovs-db-analysis/scripts/analyze_ovs_db.py
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
PLUGINS.md(1 hunks)docs/data.json(2 hunks)plugins/sosreport/README.md(2 hunks)plugins/sosreport/commands/ovs-db.md(1 hunks)plugins/sosreport/skills/ovs-db-analysis/SKILL.md(1 hunks)plugins/sosreport/skills/ovs-db-analysis/scripts/analyze_ovs_db.py(1 hunks)
🧰 Additional context used
🪛 LanguageTool
plugins/sosreport/commands/ovs-db.md
[grammar] ~382-~382: Use a hyphen to join words.
Context: ...db` file. - --db: Optional. Database only mode - analyze conf.db only (requir...
(QB_NEW_EN_HYPHEN)
[style] ~382-~382: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...l. Database only mode - analyze conf.db only (requires ovsdb-tool). - **--flows-only...
(ADVERB_REPETITION_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
plugins/sosreport/skills/ovs-db-analysis/SKILL.md
52-52: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
72-72: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
214-214: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
225-225: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.8)
plugins/sosreport/skills/ovs-db-analysis/scripts/analyze_ovs_db.py
48-48: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
48-48: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
65-65: subprocess call: check for execution of untrusted input
(S603)
66-66: Starting a process with a partial executable path
(S607)
78-78: Consider moving this statement to an else block
(TRY300)
165-165: Local variable indent is assigned to but never used
Remove assignment to unused variable indent
(F841)
437-437: f-string without any placeholders
Remove extraneous f prefix
(F541)
576-576: f-string without any placeholders
Remove extraneous f prefix
(F541)
663-663: f-string without any placeholders
Remove extraneous f prefix
(F541)
683-683: f-string without any placeholders
Remove extraneous f prefix
(F541)
734-734: f-string without any placeholders
Remove extraneous f prefix
(F541)
780-780: f-string without any placeholders
Remove extraneous f prefix
(F541)
782-782: f-string without any placeholders
Remove extraneous f prefix
(F541)
784-784: f-string without any placeholders
Remove extraneous f prefix
(F541)
934-934: f-string without any placeholders
Remove extraneous f prefix
(F541)
994-994: f-string without any placeholders
Remove extraneous f prefix
(F541)
1034-1034: Uses of tarfile.extractall()
(S202)
1039-1039: Uses of tarfile.extractall()
(S202)
1042-1042: Uses of tarfile.extractall()
(S202)
1071-1071: subprocess call: check for execution of untrusted input
(S603)
1072-1072: Starting a process with a partial executable path
(S607)
1100-1100: f-string without any placeholders
Remove extraneous f prefix
(F541)
1116-1116: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
1121-1121: Starting a process with a partial executable path
(S607)
1132-1132: f-string without any placeholders
Remove extraneous f prefix
(F541)
1246-1246: Starting a process with a partial executable path
(S607)
1280-1280: Starting a process with a partial executable path
(S607)
🔇 Additional comments (10)
plugins/sosreport/skills/ovs-db-analysis/scripts/analyze_ovs_db.py (3)
295-339: LGTM: Flow parsing logic is well-structured.The regex-based parsing of OpenFlow entries handles the common format correctly, with appropriate fallbacks for missing fields.
606-694: LGTM: OpenFlow analysis with OVN internal table filtering.Good implementation that correctly filters OVN internal tables (44, 64-87) from active drop detection to avoid false positives in troubleshooting output.
746-784: LGTM: Datapath health analysis with clear thresholds.The flow table health check at 70%/90% thresholds with clear status indicators is practical for troubleshooting.
PLUGINS.md (1)
280-280: LGTM: New command entry follows existing format.The
/sosreport:ovs-dbcommand is documented consistently with other plugin commands.plugins/sosreport/README.md (2)
96-134: LGTM: Comprehensive command documentation.The new ovs-db command section provides clear explanations of analysis capabilities, prerequisites, and examples.
145-145: LGTM: Skill table entry added correctly.The OVS DB Analysis skill is properly documented with link to SKILL.md.
plugins/sosreport/commands/ovs-db.md (2)
129-251: LGTM: Detailed output example.The example output clearly demonstrates what users can expect from the analysis, including all sections (system info, topology, OpenFlow, port stats, datapath health).
379-384: LGTM: Clear argument documentation.Arguments are well-documented with required/optional markers and descriptions.
docs/data.json (2)
390-395: Command documentation correctly formatted and aligned with feature specification.The new
ovs-dbcommand entry follows the established pattern, with argument hints and synopsis accurately reflecting the documented modes (text parsing, database mode with--db, flow filtering, and custom queries).
412-416: Skill entry correctly positioned and structured.The new
ovs-db-analysisskill is properly integrated into the sosreport plugin skills list, with a matching id and descriptive name consistent with other skills in the plugin.
| The `ovs-db` command analyzes Open vSwitch data collected in sosreports. It operates in three modes: | ||
|
|
||
| 1. **Default mode**: Full analysis - conf.db + all text files (requires ovsdb-tool) | ||
| 2. **Database mode** (`--db`): Database only - analyze conf.db (requires ovsdb-tool) | ||
| 3. **Text files mode** (`--flows-only`): Text files only - no ovsdb-tool needed! |
There was a problem hiding this comment.
Inconsistency: "three modes" but four are listed.
Line 16 states "three modes" but the modes section (lines 43-47) lists four distinct modes. Update for consistency.
-The `ovs-db` command analyzes Open vSwitch data collected in sosreports. It operates in three modes:
+The `ovs-db` command analyzes Open vSwitch data collected in sosreports. It operates in four modes:
1. **Default mode**: Full analysis - conf.db + all text files (requires ovsdb-tool)
2. **Database mode** (`--db`): Database only - analyze conf.db (requires ovsdb-tool)
-3. **Text files mode** (`--flows-only`): Text files only - no ovsdb-tool needed!
+3. **Text files mode** (`--flows-only`): Text files only - no ovsdb-tool needed
+4. **Query mode** (`--query`): Run custom OVSDB JSON queries (requires ovsdb-tool)📝 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.
| The `ovs-db` command analyzes Open vSwitch data collected in sosreports. It operates in three modes: | |
| 1. **Default mode**: Full analysis - conf.db + all text files (requires ovsdb-tool) | |
| 2. **Database mode** (`--db`): Database only - analyze conf.db (requires ovsdb-tool) | |
| 3. **Text files mode** (`--flows-only`): Text files only - no ovsdb-tool needed! | |
| The `ovs-db` command analyzes Open vSwitch data collected in sosreports. It operates in four modes: | |
| 1. **Default mode**: Full analysis - conf.db + all text files (requires ovsdb-tool) | |
| 2. **Database mode** (`--db`): Database only - analyze conf.db (requires ovsdb-tool) | |
| 3. **Text files mode** (`--flows-only`): Text files only - no ovsdb-tool needed | |
| 4. **Query mode** (`--query`): Run custom OVSDB JSON queries (requires ovsdb-tool) |
🤖 Prompt for AI Agents
In plugins/sosreport/commands/ovs-db.md around lines 16 to 20, the introduction
says "three modes" but the modes section enumerates four; update the phrasing
for consistency by either changing "three modes" to "four modes" or adjusting
the listed modes to be three (remove or merge one), and ensure the subsequent
descriptions and any flags (--db, --flows-only, etc.) remain accurate after the
change.
| if path.suffix == '.xz': | ||
| import lzma | ||
| with lzma.open(path, 'rb') as xz: | ||
| with tarfile.open(fileobj=xz) as tar: | ||
| tar.extractall(extract_dir) | ||
| elif path.suffix == '.gz': | ||
| import gzip | ||
| with gzip.open(path, 'rb') as gz: | ||
| with tarfile.open(fileobj=gz) as tar: | ||
| tar.extractall(extract_dir) | ||
| else: | ||
| with tarfile.open(path) as tar: | ||
| tar.extractall(extract_dir) | ||
|
|
There was a problem hiding this comment.
Security: tarfile.extractall() is vulnerable to path traversal attacks.
When extracting archives from untrusted sources (like sosreports from unknown systems), tarfile.extractall() without a filter can allow malicious archives to write files outside the target directory via path traversal (CVE-2007-4559). Python 3.12+ supports a filter parameter; for older versions, validate members before extraction.
if path.suffix == '.xz':
import lzma
with lzma.open(path, 'rb') as xz:
with tarfile.open(fileobj=xz) as tar:
- tar.extractall(extract_dir)
+ tar.extractall(extract_dir, filter='data')
elif path.suffix == '.gz':
import gzip
with gzip.open(path, 'rb') as gz:
with tarfile.open(fileobj=gz) as tar:
- tar.extractall(extract_dir)
+ tar.extractall(extract_dir, filter='data')
else:
with tarfile.open(path) as tar:
- tar.extractall(extract_dir)
+ tar.extractall(extract_dir, filter='data')Note: The filter='data' parameter requires Python 3.12+. For compatibility with older Python versions, consider adding a helper function:
def safe_extract(tar, path):
"""Extract tar safely, preventing path traversal."""
for member in tar.getmembers():
member_path = os.path.join(path, member.name)
if not os.path.abspath(member_path).startswith(os.path.abspath(path) + os.sep):
raise Exception(f"Attempted path traversal in tar file: {member.name}")
tar.extractall(path)🧰 Tools
🪛 Ruff (0.14.8)
1034-1034: Uses of tarfile.extractall()
(S202)
1039-1039: Uses of tarfile.extractall()
(S202)
1042-1042: Uses of tarfile.extractall()
(S202)
| This skill provides detailed analysis of Open vSwitch (OVS) data collected via sosreport. It operates in three modes: | ||
|
|
||
| 1. **Default mode**: Full analysis - conf.db + all text files (requires ovsdb-tool, falls back to text files if not available) | ||
| 2. **Database mode (`--db`)**: Database only - analyze conf.db (requires ovsdb-tool) | ||
| 3. **Text files mode (`--flows-only`)**: Text files only - no ovsdb-tool needed | ||
| 4. **Query mode (`--query`)**: Run custom OVSDB JSON queries (requires ovsdb-tool) |
There was a problem hiding this comment.
Inconsistency: "three modes" but four are listed.
Line 8 states "three modes" but lines 10-13 actually list four modes (Default, Database, Text files, Query).
-This skill provides detailed analysis of Open vSwitch (OVS) data collected via sosreport. It operates in three modes:
+This skill provides detailed analysis of Open vSwitch (OVS) data collected via sosreport. It operates in four modes:📝 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.
| This skill provides detailed analysis of Open vSwitch (OVS) data collected via sosreport. It operates in three modes: | |
| 1. **Default mode**: Full analysis - conf.db + all text files (requires ovsdb-tool, falls back to text files if not available) | |
| 2. **Database mode (`--db`)**: Database only - analyze conf.db (requires ovsdb-tool) | |
| 3. **Text files mode (`--flows-only`)**: Text files only - no ovsdb-tool needed | |
| 4. **Query mode (`--query`)**: Run custom OVSDB JSON queries (requires ovsdb-tool) | |
| This skill provides detailed analysis of Open vSwitch (OVS) data collected via sosreport. It operates in four modes: | |
| 1. **Default mode**: Full analysis - conf.db + all text files (requires ovsdb-tool, falls back to text files if not available) | |
| 2. **Database mode (`--db`)**: Database only - analyze conf.db (requires ovsdb-tool) | |
| 3. **Text files mode (`--flows-only`)**: Text files only - no ovsdb-tool needed | |
| 4. **Query mode (`--query`)**: Run custom OVSDB JSON queries (requires ovsdb-tool) |
🤖 Prompt for AI Agents
In plugins/sosreport/skills/ovs-db-analysis/SKILL.md around lines 8 to 13, the
header claims "three modes" but four modes are enumerated; update the text to
match the actual list by changing "three modes" to "four modes" (or
alternatively remove one of the listed modes if intended), and ensure the
enumerated list and the introductory sentence are consistent.
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
|
/test lint |
|
@tssurya: The specified target(s) for Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
lgtm. are we planning to add comamnds to gather the sos report or only analyze it? |
Only analyzing gathered data has been done so far.. But gathering is also a great idea! perhaps I can open an issue tracker for this? |
2 participants
Assisted-by: Claude noreply@anthropic.com
What this PR does / why we need it:
Special notes for your reviewer:
Sample output looks like this:
https://privatebin.corp.redhat.com/?b2dc140eeec0ae5c#EbwL16shrcf9vLkAJFUXjjTqdteXyWNhW9Bdtj9iWt7s
Command used to test:
python3 ai-helpers/plugins/sosreport/skills/ovs-db-analysis/scripts/analyze_ovs_db.py ~/Downloads/sosreport-worker-blah
Checklist:
Summary by CodeRabbit
Release Notes
New Features
/sosreport:ovs-dbcommand for comprehensive Open vSwitch data analysis from sosreport, supporting full analysis, database-only, text-files-only, and custom query modes for infrastructure troubleshooting.Documentation
✏️ Tip: You can customize this high-level summary in your review settings.