feat(openshift): Add visualize-ovn-topology command #128
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mattedallo 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-merge-robot
added
the
needs-rebase
WalkthroughAdds the Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,235,255,0.45)
participant User
participant Cmd as /openshift:visualize-ovn-topology
participant Detect as detect-cluster.sh
participant Perms as check_permissions.py
participant Collect as collect_ovn_data.py
participant Analyze as analyze_placement.py
participant Utils as ovn_utils.py
end
User ->> Cmd: Invoke command
Cmd ->> Detect: discover clusters
Detect -->> Cmd: cluster list
Cmd ->> Perms: run permission checks
Perms -->> Cmd: permission summary
alt write permissions detected
Cmd -->> User: warn & request confirmation
User ->> Cmd: confirm
end
Cmd ->> Collect: collect OVN data
Collect ->> Utils: detect namespace / detect container
Collect -->> Cmd: detail files (switches, routers, LSPs, LRPs, pods)
Cmd ->> Analyze: analyze placement
Analyze -->> Cmd: placement mappings
Cmd ->> Cmd: generate Mermaid diagram
Cmd -->> User: write ovn-topology-diagram.md
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus on during review:
Pre-merge checks and finishing touches✅ Passed checks (7 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @mattedallo. 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. |
openshift-merge-robot
removed
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
plugins/openshift/skills/generating-ovn-topology/README.md (1)
115-115: Minor: Capitalize "Markdown" as proper noun.The term "Markdown" is a proper noun (the markup language name) and should be capitalized.
Apply this diff:
-The command creates a Mermaid diagram in markdown format: +The command creates a Mermaid diagram in Markdown format:plugins/openshift/commands/visualize-ovn-topology.md (1)
62-94: Add language specifiers to fenced code blocks.Fenced code blocks should declare their language for proper syntax highlighting and clarity.
Apply this diff:
- ``` + ```shell /openshift:visualize-ovn-topology ```And for the output block at lines 87–94:
- You'll be asked: - ``` + You'll be asked: + ```text File ovn-topology-diagram.md already exists. Would you like to:plugins/openshift/skills/generating-ovn-topology/SKILL.md (2)
46-66: Add language specifier to code block.The warning example block lacks a language specifier. Use
textfor plain text output.Apply this diff:
**Example of proper user communication:** - ``` + ```text ⚠️ WARNING: Write Permissions Detected
335-362: Use proper heading instead of emphasis.Line 335 uses bold text as a section header instead of a markdown heading. This improves document structure and navigation.
Apply this diff:
-**Example: Control Plane Node with Host-Network Pods** +### Example: Control Plane Node with Host-Network Pods ```mermaid
📜 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 (9)
PLUGINS.md(1 hunks)docs/data.json(1 hunks)plugins/openshift/commands/visualize-ovn-topology.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/README.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/SKILL.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/analyze-placement.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/check-permissions.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/collect-ovn-data.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh(1 hunks)
🧰 Additional context used
🪛 LanguageTool
plugins/openshift/skills/generating-ovn-topology/README.md
[uncategorized] ~115-~115: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...he command creates a Mermaid diagram in markdown format: ```markdown # OVN-Kubernetes N...
(MARKDOWN_NNP)
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/skills/generating-ovn-topology/README.md
21-21: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
plugins/openshift/commands/visualize-ovn-topology.md
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/openshift/skills/generating-ovn-topology/SKILL.md
46-46: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
335-335: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Ruff (0.14.3)
plugins/openshift/skills/generating-ovn-topology/scripts/check-permissions.py
24-35: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
45-45: subprocess call: check for execution of untrusted input
(S603)
46-53: Starting a process with a partial executable path
(S607)
58-58: Consider moving this statement to an else block
(TRY300)
98-98: subprocess call: check for execution of untrusted input
(S603)
104-104: Consider moving this statement to an else block
(TRY300)
112-112: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/collect-ovn-data.py
72-72: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
178-178: subprocess call: check for execution of untrusted input
(S603)
179-182: Starting a process with a partial executable path
(S607)
203-203: Consider moving this statement to an else block
(TRY300)
218-218: Do not catch blind exception: Exception
(BLE001)
228-228: subprocess call: check for execution of untrusted input
(S603)
229-232: Starting a process with a partial executable path
(S607)
267-267: Consider moving this statement to an else block
(TRY300)
269-269: Do not catch blind exception: Exception
(BLE001)
276-276: subprocess call: check for execution of untrusted input
(S603)
277-281: Starting a process with a partial executable path
(S607)
290-290: Consider moving this statement to an else block
(TRY300)
292-292: Do not catch blind exception: Exception
(BLE001)
316-316: subprocess call: check for execution of untrusted input
(S603)
317-324: Starting a process with a partial executable path
(S607)
347-347: Consider moving this statement to an else block
(TRY300)
355-355: Do not catch blind exception: Exception
(BLE001)
482-482: Do not catch blind exception: Exception
(BLE001)
513-513: Do not catch blind exception: Exception
(BLE001)
603-603: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze-placement.py
47-47: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
245-245: Do not catch blind exception: Exception
(BLE001)
290-290: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
🪛 Shellcheck (0.11.0)
plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh
[warning] 107-107: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 112-112: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 117-117: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[error] 157-157: 'local' is only valid in functions.
(SC2168)
🔇 Additional comments (1)
docs/data.json (1)
509-527: Registry entries properly formatted.The new command and skill entries are correctly structured and follow existing conventions. Command metadata aligns with command documentation, and skill registration is complete.
plugins/openshift/skills/generating-ovn-topology/scripts/check-permissions.py
Show resolved
Hide resolved
plugins/openshift/skills/generating-ovn-topology/scripts/collect-ovn-data.py
Outdated
Show resolved
Hide resolved
plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh
Show resolved
Hide resolved
plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh
Outdated
Show resolved
Hide resolved
- Remove false load balancer claims from documentation - Fix environment handling in Python scripts to preserve PATH - Fix KUBECONFIG colon-delimited handling in detect-cluster.sh - Fix 'local' keyword usage outside function in detect-cluster.sh - Fix markdown formatting (language specifiers, capitalization, headings) Addresses all CodeRabbit review comments on PR openshift-eng#128
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
plugins/openshift/skills/generating-ovn-topology/README.md (2)
29-31: Consider adding language identifiers to code blocks.Several code blocks lack language identifiers, which can improve syntax highlighting and readability in various markdown renderers.
Apply these changes:
-``` +```bash /openshift:visualize-ovn-topology```diff -``` +```text ✅ Successfully generated OVN-Kubernetes topology diagram ...```diff -``` +```text Found 2 OVN-Kubernetes clusters: ...```diff -``` +```text /tmp/ovn_switch_placement.txt # Component placement analysis ...Also applies to: 43-56, 260-270, 333-341 --- `354-355`: **Consider formatting bare URLs as markdown links.** Bare URLs reduce accessibility for screen readers and make link management harder. Apply this diff: ```diff -- **Repository**: https://github.com/openshift-eng/ai-helpers -- **Issues**: https://github.com/openshift-eng/ai-helpers/issues +- **Repository**: [openshift-eng/ai-helpers](https://github.com/openshift-eng/ai-helpers) +- **Issues**: [Report an issue](https://github.com/openshift-eng/ai-helpers/issues)plugins/openshift/commands/visualize-ovn-topology.md (1)
12-14: Add language identifier to code block.The Synopsis code block would benefit from a language identifier for better rendering.
Apply this diff:
-``` +```bash /openshift:visualize-ovn-topology</blockquote></details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/check-permissions.py (1)</summary><blockquote> `24-35`: **Consider annotating mutable class attributes with `ClassVar`.** The `DANGEROUS_PERMS` list is a class-level constant that should be annotated with `typing.ClassVar` to help type checkers distinguish it from instance attributes. Apply this diff: ```diff +from typing import ClassVar, Dict, List, Optional -from typing import List, Optional class PermissionChecker: """Check Kubernetes RBAC permissions for the OVN topology skill.""" # Configuration constants PERMISSION_CHECK_TIMEOUT = 5 # seconds OVN_NAMESPACE = "ovn-kubernetes" # Dangerous write permissions to check - DANGEROUS_PERMS = [ + DANGEROUS_PERMS: ClassVar[List[Tuple[str, str, str]]] = [ ("delete", "pods", "Delete pods"), ... ]
📜 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 (7)
ovn-topology-diagram-20251104-173133-compact.md(1 hunks)plugins/openshift/commands/visualize-ovn-topology.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/README.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/SKILL.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/check-permissions.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/collect-ovn-data.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/openshift/skills/generating-ovn-topology/SKILL.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/commands/visualize-ovn-topology.md
12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/openshift/skills/generating-ovn-topology/README.md
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
260-260: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
333-333: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
354-354: Bare URL used
(MD034, no-bare-urls)
355-355: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.3)
plugins/openshift/skills/generating-ovn-topology/scripts/collect-ovn-data.py
72-72: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
180-180: subprocess call: check for execution of untrusted input
(S603)
181-184: Starting a process with a partial executable path
(S607)
205-205: Consider moving this statement to an else block
(TRY300)
220-220: Do not catch blind exception: Exception
(BLE001)
232-232: subprocess call: check for execution of untrusted input
(S603)
233-236: Starting a process with a partial executable path
(S607)
271-271: Consider moving this statement to an else block
(TRY300)
273-273: Do not catch blind exception: Exception
(BLE001)
282-282: subprocess call: check for execution of untrusted input
(S603)
283-287: Starting a process with a partial executable path
(S607)
296-296: Consider moving this statement to an else block
(TRY300)
298-298: Do not catch blind exception: Exception
(BLE001)
324-324: subprocess call: check for execution of untrusted input
(S603)
325-332: Starting a process with a partial executable path
(S607)
355-355: Consider moving this statement to an else block
(TRY300)
363-363: Do not catch blind exception: Exception
(BLE001)
490-490: Do not catch blind exception: Exception
(BLE001)
521-521: Do not catch blind exception: Exception
(BLE001)
611-611: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
plugins/openshift/skills/generating-ovn-topology/scripts/check-permissions.py
24-35: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
47-47: subprocess call: check for execution of untrusted input
(S603)
48-55: Starting a process with a partial executable path
(S607)
60-60: Consider moving this statement to an else block
(TRY300)
102-102: subprocess call: check for execution of untrusted input
(S603)
108-108: Consider moving this statement to an else block
(TRY300)
116-116: Do not catch blind exception: Exception
(BLE001)
🪛 Shellcheck (0.11.0)
plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh
[warning] 111-111: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 116-116: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 121-121: Tilde does not expand in quotes. Use $HOME.
(SC2088)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (5)
49-63: Consider using a more secure temporary directory.The default
/tmpdirectory can be a security concern in multi-user environments. Consider usingtempfile.mkdtemp()or allowing users to specify a workspace directory explicitly.However, given that this is an administrative tool for cluster analysis with read-only operations, and users can override the directory via command-line argument, this is a lower-priority concern.
Example refactor using
tempfile:+import tempfile + class PlacementAnalyzer: """Analyze OVN component placement from collected data.""" - def __init__(self, tmpdir: str = "/tmp"): + def __init__(self, tmpdir: str = None): + if tmpdir is None: + tmpdir = os.environ.get('TMPDIR') or tempfile.gettempdir() self.tmpdir = tmpdir
94-127: Add error handling for file I/O operations.The file reading operations lack try/except blocks. If there's an I/O error or permission issue, the script will crash with an unhandled exception.
Consider wrapping the file operations:
def load_data(self): """Load switch and router data from files into memory.""" print("=" * 50) print("LOADING COLLECTED DATA") print("=" * 50) # Load switches: node|uuid|name|other_config switch_count = 0 + try: - with open(self.switches_file) as f: + with open(self.switches_file) as f: - for line in f: + for line in f: - if not line.strip(): + if not line.strip(): - continue + continue - parts = line.strip().split('|', 3) + parts = line.strip().split('|', 3) - if len(parts) >= 3: + if len(parts) >= 3: - node, uuid, name = parts[0], parts[1], parts[2] + node, uuid, name = parts[0], parts[1], parts[2] - self.switch_data[name].append((node, uuid)) + self.switch_data[name].append((node, uuid)) - switch_count += 1 + switch_count += 1 + except IOError as e: + print(f"❌ Error reading {self.switches_file}: {e}", file=sys.stderr) + raise print(f" ✓ Loaded {switch_count} switch entries")Apply similar pattern for routers.
211-217: Add error handling for file write operations.The file write operations lack error handling. If there's a permission issue or disk full condition, the script will crash with an unhandled exception.
Wrap the file writes with try/except:
# Write placement files + try: - with open(self.switch_placement_file, 'w') as f: + with open(self.switch_placement_file, 'w') as f: - for name in sorted(switch_placement.keys()): + for name in sorted(switch_placement.keys()): - f.write(f"{name}|{switch_placement[name]}\n") + f.write(f"{name}|{switch_placement[name]}\n") - with open(self.router_placement_file, 'w') as f: + with open(self.router_placement_file, 'w') as f: - for name in sorted(router_placement.keys()): + for name in sorted(router_placement.keys()): - f.write(f"{name}|{router_placement[name]}\n") + f.write(f"{name}|{router_placement[name]}\n") + except IOError as e: + print(f"❌ Error writing placement files: {e}", file=sys.stderr) + raise
243-248: Refine exception handling in count_lines helper.The bare
except Exceptioncatch is too broad. Consider catching specific exceptions to avoid hiding unexpected errors.Suggested improvement:
def count_lines(filepath): try: with open(filepath) as f: return sum(1 for _ in f) - except Exception: + except (FileNotFoundError, IOError, PermissionError): return 0
290-299: Consider adding tmpdir validation.The script accepts a tmpdir argument but doesn't validate that it's a valid, accessible directory. This could lead to confusing errors later in the execution.
Example validation:
def main(): """Main entry point.""" tmpdir = sys.argv[1] if len(sys.argv) > 1 else os.environ.get('TMPDIR', '/tmp') + + if not os.path.isdir(tmpdir): + print(f"❌ Error: {tmpdir} is not a valid directory", file=sys.stderr) + return 1 analyzer = PlacementAnalyzer(tmpdir) return analyzer.run()
📜 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/openshift/skills/generating-ovn-topology/SKILL.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/skills/generating-ovn-topology/SKILL.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
178-178: Table column count
Expected: 4; Actual: 8; Too many cells, extra data will be missing
(MD056, table-column-count)
🪛 Ruff (0.14.3)
plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py
30-41: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
57-57: subprocess call: check for execution of untrusted input
(S603)
58-65: Starting a process with a partial executable path
(S607)
70-70: Consider moving this statement to an else block
(TRY300)
112-112: subprocess call: check for execution of untrusted input
(S603)
118-118: Consider moving this statement to an else block
(TRY300)
126-126: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py
1-1: Shebang is present but file is not executable
(EXE001)
42-42: subprocess call: check for execution of untrusted input
(S603)
43-50: Starting a process with a partial executable path
(S607)
58-58: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
65-65: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
73-88: Avoid specifying long messages outside the exception class
(TRY003)
102-102: subprocess call: check for execution of untrusted input
(S603)
103-110: Starting a process with a partial executable path
(S607)
120-120: subprocess call: check for execution of untrusted input
(S603)
121-128: Starting a process with a partial executable path
(S607)
176-176: subprocess call: check for execution of untrusted input
(S603)
177-182: Starting a process with a partial executable path
(S607)
189-192: Avoid specifying long messages outside the exception class
(TRY003)
204-204: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
220-239: Avoid specifying long messages outside the exception class
(TRY003)
242-245: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
242-245: Avoid specifying long messages outside the exception class
(TRY003)
247-249: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
247-249: Avoid specifying long messages outside the exception class
(TRY003)
265-265: subprocess call: check for execution of untrusted input
(S603)
266-273: Starting a process with a partial executable path
(S607)
277-277: Consider moving this statement to an else block
(TRY300)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py
49-49: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
247-247: Do not catch blind exception: Exception
(BLE001)
292-292: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py
78-78: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
189-189: subprocess call: check for execution of untrusted input
(S603)
190-193: Starting a process with a partial executable path
(S607)
214-214: Consider moving this statement to an else block
(TRY300)
229-229: Do not catch blind exception: Exception
(BLE001)
241-241: subprocess call: check for execution of untrusted input
(S603)
242-245: Starting a process with a partial executable path
(S607)
280-280: Consider moving this statement to an else block
(TRY300)
282-282: Do not catch blind exception: Exception
(BLE001)
291-291: subprocess call: check for execution of untrusted input
(S603)
292-296: Starting a process with a partial executable path
(S607)
305-305: Consider moving this statement to an else block
(TRY300)
307-307: Do not catch blind exception: Exception
(BLE001)
333-333: subprocess call: check for execution of untrusted input
(S603)
334-341: Starting a process with a partial executable path
(S607)
364-364: Consider moving this statement to an else block
(TRY300)
372-372: Do not catch blind exception: Exception
(BLE001)
520-520: Do not catch blind exception: Exception
(BLE001)
551-551: Do not catch blind exception: Exception
(BLE001)
641-641: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
🪛 Shellcheck (0.11.0)
plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh
[warning] 124-124: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 129-129: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 134-134: Tilde does not expand in quotes. Use $HOME.
(SC2088)
🔇 Additional comments (4)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (4)
1-48: LGTM: Well-documented and organized structure.The module docstring is comprehensive, imports are minimal, and constants are clearly organized. Good use of class-level constants for magic strings.
64-92: LGTM: Clear input validation with helpful error messages.The verification logic is straightforward, and the error messages appropriately direct users to the data collection script.
128-167: LGTM: Sound placement analysis logic.The UUID-based heuristic correctly distinguishes between cluster-wide components (same UUID across nodes) and per-node components (different UUIDs or single node). The logic is clear and well-documented.
264-287: LGTM: Clear orchestration of analysis steps.The
runmethod provides a clean, sequential workflow with appropriate exit codes. Error handling concerns have been noted in previous comments for the individual methods.
plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py
Show resolved
Hide resolved
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py
Outdated
Show resolved
Hide resolved
|
Addressed CodeRabbit comments about deferring namespace detection: Fixed in
Fixed in
Both scripts now handle namespace detection failures gracefully and return exit code 1 with user-friendly error messages, rather than crashing with uncaught exceptions during object construction. AI-assisted response via Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (1)
78-103: Create the tmpdir before writing filesIf a caller sets
TMPDIRto a path that hasn’t been created yet, everyopen(..., 'w')here will raiseFileNotFoundError, aborting the collection run. It's safer to ensure the directory exists up front.(geeksforgeeks.org)You can handle it where
self.tmpdiris assigned:def __init__(self, kubeconfig: str, tmpdir: str = "/tmp"): self.kubeconfig = kubeconfig - self.tmpdir = tmpdir + self.tmpdir = tmpdir + os.makedirs(self.tmpdir, exist_ok=True) self.stats = CollectionStats()That way the script keeps working even if users point
TMPDIRat an empty location.
📜 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 (2)
plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.3)
plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py
30-41: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
55-55: subprocess call: check for execution of untrusted input
(S603)
56-63: Starting a process with a partial executable path
(S607)
68-68: Consider moving this statement to an else block
(TRY300)
110-110: subprocess call: check for execution of untrusted input
(S603)
116-116: Consider moving this statement to an else block
(TRY300)
124-124: Do not catch blind exception: Exception
(BLE001)
206-206: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py
78-78: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
188-188: subprocess call: check for execution of untrusted input
(S603)
189-192: Starting a process with a partial executable path
(S607)
213-213: Consider moving this statement to an else block
(TRY300)
228-228: Do not catch blind exception: Exception
(BLE001)
240-240: subprocess call: check for execution of untrusted input
(S603)
241-244: Starting a process with a partial executable path
(S607)
279-279: Consider moving this statement to an else block
(TRY300)
281-281: Do not catch blind exception: Exception
(BLE001)
290-290: subprocess call: check for execution of untrusted input
(S603)
291-295: Starting a process with a partial executable path
(S607)
304-304: Consider moving this statement to an else block
(TRY300)
306-306: Do not catch blind exception: Exception
(BLE001)
332-332: subprocess call: check for execution of untrusted input
(S603)
333-340: Starting a process with a partial executable path
(S607)
363-363: Consider moving this statement to an else block
(TRY300)
371-371: Do not catch blind exception: Exception
(BLE001)
519-519: Do not catch blind exception: Exception
(BLE001)
550-550: Do not catch blind exception: Exception
(BLE001)
600-600: Do not catch blind exception: Exception
(BLE001)
647-647: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
🔇 Additional comments (1)
plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py (1)
203-208: Nice fix deferring namespace detectionMoving the OVN namespace probe into
run()after the kubectl presence check keeps object construction side-effect free and lets you surface the new friendly error instead of a traceback. Thanks for tightening up the flow here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugins/openshift/skills/generating-ovn-topology/ovn-topology-diagram.md (2)
1-11: Add introductory context and legend documentation.This file contains only a Mermaid diagram without explaining what it represents, how to interpret it, or when it was generated. For users discovering this file, add:
- A brief description of what the diagram visualizes (cluster topology, node structure, OVN components)
- A legend explaining the color coding and component types
- A note that this is an example/template output
For example, add a section before the diagram explaining the topology structure and color scheme.
1-395: Consider accessibility and rendering performance for large Mermaid diagrams.This diagram is complex (~395 lines, 100+ nodes) and relies heavily on color coding for component distinction. Two concerns:
Accessibility: No alt text or text-based legend for users relying on screen readers or unable to distinguish colors. The node labels alone may not be sufficient for full context.
Rendering: Some tools/viewers may struggle with diagrams of this size or complexity. Consider documenting known limitations or recommended viewers.
📜 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 (1)
plugins/openshift/skills/generating-ovn-topology/ovn-topology-diagram.md(1 hunks)
🔇 Additional comments (1)
plugins/openshift/skills/generating-ovn-topology/ovn-topology-diagram.md (1)
1-395: Verify that cluster-specific metadata should remain in the example.The diagram contains real cluster identifiers (node names like
ci-ln-8ck7ygb-72292-ww95f-*, specific IP ranges, pod information). Confirm whether this is intentional for an example or if this should be anonymized/templated for reusability.
mattedallo
force-pushed
the
add-visualize-ovn-topology
branch
from
e529bb8 to
e8ea97e
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
|
could the commits be squashed a bit? |
mattedallo
force-pushed
the
add-visualize-ovn-topology
branch
from
e8ea97e to
f0feb60
Compare
openshift-merge-robot
added
the
needs-rebase
mattedallo
force-pushed
the
add-visualize-ovn-topology
branch
from
f0feb60 to
ac0adbc
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugins/openshift/skills/generating-ovn-topology/README.md (1)
29-31: Consider adding language specifiers to code blocks.Adding language identifiers (e.g., ```bash) to the code blocks improves syntax highlighting and readability in rendered markdown.
Example:
-``` +```bash /openshift:visualize-ovn-topology</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro **Cache: Disabled due to data retention organization setting** **Knowledge base: Disabled due to `Reviews -> Disable Knowledge Base` setting** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between e529bb8cbfa0419af46a0a5669b092ee890d6fba and ac0adbcfa4952e1b5db40c581e0a11ca2fe92f53. </details> <details> <summary>📒 Files selected for processing (10)</summary> * `PLUGINS.md` (1 hunks) * `docs/data.json` (1 hunks) * `plugins/openshift/commands/visualize-ovn-topology.md` (1 hunks) * `plugins/openshift/skills/generating-ovn-topology/README.md` (1 hunks) * `plugins/openshift/skills/generating-ovn-topology/SKILL.md` (1 hunks) * `plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py` (1 hunks) * `plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py` (1 hunks) * `plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py` (1 hunks) * `plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh` (1 hunks) * `plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * PLUGINS.md </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>plugins/openshift/skills/generating-ovn-topology/README.md</summary> 29-29: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 43-43: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 260-260: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 333-333: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 354-354: Bare URL used (MD034, no-bare-urls) --- 355-355: Bare URL used (MD034, no-bare-urls) </details> <details> <summary>plugins/openshift/commands/visualize-ovn-topology.md</summary> 21-21: Heading levels should only increment by one level at a time Expected: h2; Actual: h3 (MD001, heading-increment) </details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/SKILL.md</summary> 25-25: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 178-178: Table column count Expected: 4; Actual: 8; Too many cells, extra data will be missing (MD056, table-column-count) </details> </details> <details> <summary>🪛 Ruff (0.14.4)</summary> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py</summary> 30-41: Mutable class attributes should be annotated with `typing.ClassVar` (RUF012) --- 55-55: `subprocess` call: check for execution of untrusted input (S603) --- 56-63: Starting a process with a partial executable path (S607) --- 68-68: Consider moving this statement to an `else` block (TRY300) --- 110-110: `subprocess` call: check for execution of untrusted input (S603) --- 116-116: Consider moving this statement to an `else` block (TRY300) --- 124-124: Do not catch blind exception: `Exception` (BLE001) --- 206-206: Do not catch blind exception: `Exception` (BLE001) </details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py</summary> 49-49: Probable insecure usage of temporary file or directory: "/tmp" (S108) --- 247-247: Do not catch blind exception: `Exception` (BLE001) --- 292-292: Probable insecure usage of temporary file or directory: "/tmp" (S108) </details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py</summary> 78-78: Probable insecure usage of temporary file or directory: "/tmp" (S108) --- 188-188: `subprocess` call: check for execution of untrusted input (S603) --- 189-192: Starting a process with a partial executable path (S607) --- 213-213: Consider moving this statement to an `else` block (TRY300) --- 228-228: Do not catch blind exception: `Exception` (BLE001) --- 240-240: `subprocess` call: check for execution of untrusted input (S603) --- 241-244: Starting a process with a partial executable path (S607) --- 279-279: Consider moving this statement to an `else` block (TRY300) --- 281-281: Do not catch blind exception: `Exception` (BLE001) --- 290-290: `subprocess` call: check for execution of untrusted input (S603) --- 291-295: Starting a process with a partial executable path (S607) --- 304-304: Consider moving this statement to an `else` block (TRY300) --- 306-306: Do not catch blind exception: `Exception` (BLE001) --- 332-332: `subprocess` call: check for execution of untrusted input (S603) --- 333-340: Starting a process with a partial executable path (S607) --- 363-363: Consider moving this statement to an `else` block (TRY300) --- 371-371: Do not catch blind exception: `Exception` (BLE001) --- 519-519: Do not catch blind exception: `Exception` (BLE001) --- 550-550: Do not catch blind exception: `Exception` (BLE001) --- 600-600: Do not catch blind exception: `Exception` (BLE001) --- 647-647: Probable insecure usage of temporary file or directory: "/tmp" (S108) </details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py</summary> 1-1: Shebang is present but file is not executable (EXE001) --- 42-42: `subprocess` call: check for execution of untrusted input (S603) --- 43-50: Starting a process with a partial executable path (S607) --- 58-58: String contains ambiguous `ℹ` (INFORMATION SOURCE). Did you mean `i` (LATIN SMALL LETTER I)? (RUF001) --- 65-65: String contains ambiguous `ℹ` (INFORMATION SOURCE). Did you mean `i` (LATIN SMALL LETTER I)? (RUF001) --- 73-88: Avoid specifying long messages outside the exception class (TRY003) --- 102-102: `subprocess` call: check for execution of untrusted input (S603) --- 103-110: Starting a process with a partial executable path (S607) --- 120-120: `subprocess` call: check for execution of untrusted input (S603) --- 121-128: Starting a process with a partial executable path (S607) --- 176-176: `subprocess` call: check for execution of untrusted input (S603) --- 177-182: Starting a process with a partial executable path (S607) --- 189-192: Avoid specifying long messages outside the exception class (TRY003) --- 204-204: String contains ambiguous `ℹ` (INFORMATION SOURCE). Did you mean `i` (LATIN SMALL LETTER I)? (RUF001) --- 220-239: Avoid specifying long messages outside the exception class (TRY003) --- 242-245: Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling (B904) --- 242-245: Avoid specifying long messages outside the exception class (TRY003) --- 247-249: Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling (B904) --- 247-249: Avoid specifying long messages outside the exception class (TRY003) --- 265-265: `subprocess` call: check for execution of untrusted input (S603) --- 266-273: Starting a process with a partial executable path (S607) --- 277-277: Consider moving this statement to an `else` block (TRY300) </details> </details> <details> <summary>🪛 Shellcheck (0.11.0)</summary> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh</summary> [warning] 124-124: Tilde does not expand in quotes. Use $HOME. (SC2088) --- [warning] 129-129: Tilde does not expand in quotes. Use $HOME. (SC2088) --- [warning] 134-134: Tilde does not expand in quotes. Use $HOME. (SC2088) </details> </details> </details> <details> <summary>🔇 Additional comments (19)</summary><blockquote> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh (1)</summary><blockquote> `1-182`: **LGTM!** The cluster detection logic is well-structured with clear separation of stdout (parseable data) and stderr (diagnostics). The deduplication mechanism using resolved paths and the multi-source scanning approach provide robust cluster discovery. </blockquote></details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (1)</summary><blockquote> `1-299`: **LGTM!** The placement analysis logic correctly determines component distribution by analyzing UUID patterns. The visualization override for transit_switch is well-documented and the two-phase analysis (per-component analysis + overrides) is clear and maintainable. </blockquote></details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/SKILL.md (1)</summary><blockquote> `1-399`: **LGTM!** The skill documentation provides comprehensive guidance for AI agents, including clear security notes, step-by-step instructions, and detailed diagram generation rules. The read-only operation guarantees and permission checking workflow are well-documented. </blockquote></details> <details> <summary>plugins/openshift/commands/visualize-ovn-topology.md (1)</summary><blockquote> `1-121`: **LGTM!** The command documentation accurately describes the implementation, security guarantees, and usage patterns. The explicit note about read-only operations and permission checking aligns well with the skill's security-first design. </blockquote></details> <details> <summary>docs/data.json (1)</summary><blockquote> `527-539`: **LGTM!** The command and skill registration follows the established pattern and correctly integrates the new visualize-ovn-topology functionality into the OpenShift plugin. </blockquote></details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py (1)</summary><blockquote> `198-212`: **LGTM!** The refactored namespace detection correctly defers OVN namespace discovery until after kubectl availability is verified, preventing uncaught exceptions during object construction. The explicit error handling ensures user-friendly error messages. </blockquote></details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py (2)</summary><blockquote> `15-89`: **LGTM!** The two-phase detection strategy (common namespaces first, then exhaustive search) provides both performance and robustness. The comprehensive error messages with troubleshooting steps are excellent for user experience. --- `147-250`: **LGTM!** The hybrid container detection approach correctly identifies NBDB containers across different OVN distributions. The fallback to tool-based verification ensures future compatibility with new container naming conventions. </blockquote></details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (11)</summary><blockquote> `1-38`: **LGTM! Clear documentation and clean imports.** The module docstring provides comprehensive usage information, and the import structure is well-organized. The note about PYTHONPATH requirements is helpful for users. --- `40-55`: **LGTM! Well-structured metrics tracking.** The `CollectionStats` dataclass provides a clean way to track collection metrics with appropriate defaults and helper methods. --- `78-92`: **LGTM! Past review feedback correctly implemented.** The namespace detection has been properly deferred to the `run()` method (lines 597-602), addressing the previous review concern. The `ovn_namespace` is initialized to `None` and set after validation, ensuring graceful error handling. The `/tmp` default is acceptable here since `main()` (line 647) respects the `TMPDIR` environment variable, allowing users to override the temporary directory location. --- `93-170`: **LGTM! Robust pod detection and parsing.** The `_is_ovnkube_node_pod` method uses multiple detection strategies (app label, name label, name pattern) for compatibility across different deployments. The JSON parsing logic correctly filters for running pods with IPs and auto-discovers ovnkube-node pods. --- `171-284`: **LGTM! Solid fallback strategy for pod collection.** The primary JSON-based collection with a fallback to wide-output parsing provides resilience against different cluster configurations. The timeout handling and error recovery are appropriate for a data collection tool. --- `285-308`: **LGTM! Clean node name resolution.** The method uses jsonpath extraction with appropriate timeout and error handling. Returning `None` on failures allows the caller to handle missing node names gracefully. --- `378-463`: **LGTM! Well-structured component data collection.** The method systematically queries all OVN components with appropriate columns and handles partial failures gracefully. The approach of accepting partial success (any component succeeds) is reasonable for resilience. --- `464-528`: **LGTM! Comprehensive node data collection orchestration.** The method handles container detection, provides informative architecture summaries, and gracefully tracks per-node failures. The error handling allows partial data collection to succeed, which is appropriate for this use case. --- `529-589`: **LGTM! Excellent user feedback and diagnostics.** The summary provides comprehensive collection statistics, file locations, failure details, and helpful debugging tips. The success criteria (any successful node) appropriately defines partial success. --- `590-627`: **LGTM! Well-orchestrated collection workflow.** The `run()` method properly implements the deferred namespace detection (addressing the past review), includes appropriate error handling, and allows graceful degradation. The exit code logic clearly distinguishes total failure from partial success. --- `629-654`: **LGTM! Clean CLI entry point.** The `main()` function includes proper argument validation, kubeconfig existence checks, and respects the `TMPDIR` environment variable. The entry point structure is standard and correct. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py
Outdated
Show resolved
Hide resolved
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py
Show resolved
Hide resolved
mattedallo
force-pushed
the
add-visualize-ovn-topology
branch
from
ac0adbc to
e713ea3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
plugins/openshift/skills/generating-ovn-topology/README.md (2)
29-29: Add language specifiers to code blocks for better IDE rendering.Fenced code blocks should specify a language for syntax highlighting and proper formatting in IDE previews.
Apply this diff to add language specifiers:
-``` +```shell /openshift:visualize-ovn-topology -``` +```Apply similar changes to lines 43, 260, and 333 (use
shell,text, ormermaidas appropriate for each block).Also applies to: 43-43, 260-260, 333-333
354-355: Format bare URLs as markdown links.Bare URLs should be wrapped in markdown link syntax for proper rendering and accessibility.
Apply this diff:
-- **Repository**: https://github.com/openshift-eng/ai-helpers -- **Issues**: https://github.com/openshift-eng/ai-helpers/issues +- **Repository**: [https://github.com/openshift-eng/ai-helpers](https://github.com/openshift-eng/ai-helpers) +- **Issues**: [https://github.com/openshift-eng/ai-helpers/issues](https://github.com/openshift-eng/ai-helpers/issues)plugins/openshift/skills/generating-ovn-topology/SKILL.md (2)
260-260: Add language specifiers to code blocks.Lines 260 and 333 contain fenced code blocks without language specifiers. Add
shell,text, ormermaidtags as appropriate for better rendering.Also applies to: 333-333
185-320: Diagram generation rules are comprehensive but complex.The requirements are well-structured (graph direction, component placement, node subgraphs, pod representation, physical network layer, colors, connections) and include concrete examples. The bottom-to-top layer ordering and color rotation scheme (lines 302-306) are explicitly defined, which aids implementation.
However, the complexity of these rules may benefit from a worked example showing all layers for a single node—the existing example at lines 357-384 is helpful but shows only pod-to-LSP mapping.
plugins/openshift/commands/visualize-ovn-topology.md (1)
12-12: Add language specifier to code block.Line 12 contains a fenced code block without a language tag. Add
shellfor consistency:-``` +```shell /openshift:visualize-ovn-topology -``` +```
📜 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 (8)
plugins/openshift/commands/visualize-ovn-topology.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/README.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/SKILL.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/skills/generating-ovn-topology/SKILL.md
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
260-260: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
333-333: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
354-354: Bare URL used
(MD034, no-bare-urls)
355-355: Bare URL used
(MD034, no-bare-urls)
plugins/openshift/commands/visualize-ovn-topology.md
12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.4)
plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py
55-55: subprocess call: check for execution of untrusted input
(S603)
56-63: Starting a process with a partial executable path
(S607)
68-68: Consider moving this statement to an else block
(TRY300)
110-110: subprocess call: check for execution of untrusted input
(S603)
116-116: Consider moving this statement to an else block
(TRY300)
124-124: Do not catch blind exception: Exception
(BLE001)
206-206: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py
80-80: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
190-190: subprocess call: check for execution of untrusted input
(S603)
191-194: Starting a process with a partial executable path
(S607)
215-215: Consider moving this statement to an else block
(TRY300)
230-230: Do not catch blind exception: Exception
(BLE001)
242-242: subprocess call: check for execution of untrusted input
(S603)
243-246: Starting a process with a partial executable path
(S607)
281-281: Consider moving this statement to an else block
(TRY300)
283-283: Do not catch blind exception: Exception
(BLE001)
292-292: subprocess call: check for execution of untrusted input
(S603)
293-297: Starting a process with a partial executable path
(S607)
306-306: Consider moving this statement to an else block
(TRY300)
308-308: Do not catch blind exception: Exception
(BLE001)
334-334: subprocess call: check for execution of untrusted input
(S603)
335-342: Starting a process with a partial executable path
(S607)
366-366: Consider moving this statement to an else block
(TRY300)
374-374: Do not catch blind exception: Exception
(BLE001)
522-522: Do not catch blind exception: Exception
(BLE001)
553-553: Do not catch blind exception: Exception
(BLE001)
603-603: Do not catch blind exception: Exception
(BLE001)
650-650: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py
49-49: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
247-247: Do not catch blind exception: Exception
(BLE001)
292-292: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py
1-1: Shebang is present but file is not executable
(EXE001)
42-42: subprocess call: check for execution of untrusted input
(S603)
43-50: Starting a process with a partial executable path
(S607)
58-58: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
65-65: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
73-88: Avoid specifying long messages outside the exception class
(TRY003)
102-102: subprocess call: check for execution of untrusted input
(S603)
103-110: Starting a process with a partial executable path
(S607)
120-120: subprocess call: check for execution of untrusted input
(S603)
121-128: Starting a process with a partial executable path
(S607)
176-176: subprocess call: check for execution of untrusted input
(S603)
177-182: Starting a process with a partial executable path
(S607)
189-192: Avoid specifying long messages outside the exception class
(TRY003)
204-204: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
220-239: Avoid specifying long messages outside the exception class
(TRY003)
242-245: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
242-245: Avoid specifying long messages outside the exception class
(TRY003)
247-249: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
247-249: Avoid specifying long messages outside the exception class
(TRY003)
265-265: subprocess call: check for execution of untrusted input
(S603)
266-273: Starting a process with a partial executable path
(S607)
277-277: Consider moving this statement to an else block
(TRY300)
🪛 Shellcheck (0.11.0)
plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh
[warning] 124-124: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 129-129: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 134-134: Tilde does not expand in quotes. Use $HOME.
(SC2088)
🔇 Additional comments (12)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (1)
355-364: Great call switching to real CSV parsingParsing the
ovn-nbctloutput withcsv.readeravoids the comma-in-field corruption we saw before; this keepsother_config,options, etc., intact without resorting to brittle string hacks. Nice improvement.plugins/openshift/skills/generating-ovn-topology/README.md (1)
1-359: Documentation is comprehensive and well-structured.The README provides excellent user guidance with clear quick-start instructions, security considerations, troubleshooting, and FAQ. The layout follows markdown conventions and the examples are concrete. With the markdown formatting fixes above, this documentation will be production-ready.
plugins/openshift/skills/generating-ovn-topology/SKILL.md (5)
45-90: Permission check exit code handling is clear and well-documented.The exit code semantics (0=proceed, 1=stop, 2=needs confirmation) and the provided user communication template appropriately address the admin credential warning requirement. The transparency about read-only operations and explicit consent request follow security best practices.
156-171: Architecture concept documentation is accurate.The distinction between interconnect and default modes is clearly explained with concrete examples of which components are per-node vs cluster-wide. The visualization override note for
transit_switchis important and properly flagged.
172-182: Helper scripts table provides essential reference information.The script table concisely documents inputs, outputs, exit codes, and graceful degradation for
collect_ovn_data.py. The distinction between data collection and placement analysis scripts is clear.
321-349: Pod discovery rules are detailed and address host-network pod handling.The distinction between host-network pods (sharing management port LSP) and pod-network pods (individual LSPs) is critical and well-documented. The CRITICAL flag on line 323 appropriately emphasizes that pods must be discovered from
ovn_pods_detail.txt, not derived from LSPs. The example at lines 356–384 effectively illustrates the control-plane multi-pod scenario.
1-399: SKILL.md is a comprehensive technical guide for implementing topology generation.The document provides clear step-by-step instructions, explicit exit code semantics, and detailed rules for diagram generation. The security section appropriately emphasizes read-only operations and informed consent. With the markdown formatting fixes noted above, this guidance will effectively direct AI implementation.
plugins/openshift/commands/visualize-ovn-topology.md (5)
18-26: Load balancer support has been properly removed from feature list.The previous review flagged that the command documentation incorrectly claimed load balancer support while the implementation does not collect load balancer data. This has been corrected—the current description omits load balancers and accurately lists only switches, routers, ports, and connectivity. Documentation is now consistent with actual implementation.
28-50: Command implementation section properly delegates to skill.The section clearly outlines the six-step workflow (detection, permissions, architecture discovery, data collection, diagram generation, output) and references the SKILL.md and helper scripts for detailed implementation. The "Key Features" bullet list accurately describes the command's capabilities.
101-114: Security & Safety section appropriately emphasizes read-only operations and permission checks.The distinction between allowed operations (kubectl get, kubectl exec for ovn-nbctl list, local file writes) and forbidden operations (no create/delete/patch/apply, no ovn-nbctl modifications) is clear. The permission check warning correctly informs users they will be prompted if admin permissions are detected, with the assurance that only read-only operations will execute.
58-94: Examples section is clear and realistic.Example 1 shows basic usage with expected output format. Example 2 demonstrates the file conflict handling with the menu options, which helps users understand interactive behavior. Both examples are concrete and actionable.
1-121: Command documentation is accurate, complete, and well-aligned with implementation.The file clearly describes the command's purpose, workflow, security guarantees, and usage patterns. The load balancer support issue from the previous review has been properly addressed (removed from feature claims). Documentation is consistent with the README and SKILL.md guidance. With the minor markdown formatting fix above, this documentation is production-ready.
plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh
Show resolved
Hide resolved
mattedallo
force-pushed
the
add-visualize-ovn-topology
branch
from
e713ea3 to
fdcb319
Compare
@bryan-cox |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (8)
plugins/openshift/commands/visualize-ovn-topology.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/README.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/SKILL.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/skills/generating-ovn-topology/SKILL.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
178-178: Table column count
Expected: 4; Actual: 8; Too many cells, extra data will be missing
(MD056, table-column-count)
plugins/openshift/commands/visualize-ovn-topology.md
12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.4)
plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py
1-1: Shebang is present but file is not executable
(EXE001)
42-42: subprocess call: check for execution of untrusted input
(S603)
43-50: Starting a process with a partial executable path
(S607)
58-58: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
65-65: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
73-88: Avoid specifying long messages outside the exception class
(TRY003)
102-102: subprocess call: check for execution of untrusted input
(S603)
103-110: Starting a process with a partial executable path
(S607)
120-120: subprocess call: check for execution of untrusted input
(S603)
121-128: Starting a process with a partial executable path
(S607)
176-176: subprocess call: check for execution of untrusted input
(S603)
177-182: Starting a process with a partial executable path
(S607)
189-192: Avoid specifying long messages outside the exception class
(TRY003)
204-204: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
220-239: Avoid specifying long messages outside the exception class
(TRY003)
242-245: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
242-245: Avoid specifying long messages outside the exception class
(TRY003)
247-249: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
247-249: Avoid specifying long messages outside the exception class
(TRY003)
265-265: subprocess call: check for execution of untrusted input
(S603)
266-273: Starting a process with a partial executable path
(S607)
277-277: Consider moving this statement to an else block
(TRY300)
plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py
55-55: subprocess call: check for execution of untrusted input
(S603)
56-63: Starting a process with a partial executable path
(S607)
68-68: Consider moving this statement to an else block
(TRY300)
110-110: subprocess call: check for execution of untrusted input
(S603)
116-116: Consider moving this statement to an else block
(TRY300)
124-124: Do not catch blind exception: Exception
(BLE001)
206-206: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py
80-80: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
190-190: subprocess call: check for execution of untrusted input
(S603)
191-194: Starting a process with a partial executable path
(S607)
215-215: Consider moving this statement to an else block
(TRY300)
230-230: Do not catch blind exception: Exception
(BLE001)
242-242: subprocess call: check for execution of untrusted input
(S603)
243-246: Starting a process with a partial executable path
(S607)
281-281: Consider moving this statement to an else block
(TRY300)
283-283: Do not catch blind exception: Exception
(BLE001)
292-292: subprocess call: check for execution of untrusted input
(S603)
293-297: Starting a process with a partial executable path
(S607)
306-306: Consider moving this statement to an else block
(TRY300)
308-308: Do not catch blind exception: Exception
(BLE001)
334-334: subprocess call: check for execution of untrusted input
(S603)
335-342: Starting a process with a partial executable path
(S607)
366-366: Consider moving this statement to an else block
(TRY300)
374-374: Do not catch blind exception: Exception
(BLE001)
522-522: Do not catch blind exception: Exception
(BLE001)
553-553: Do not catch blind exception: Exception
(BLE001)
603-603: Do not catch blind exception: Exception
(BLE001)
650-650: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py
49-49: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
247-247: Do not catch blind exception: Exception
(BLE001)
292-292: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
🪛 Shellcheck (0.11.0)
plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh
[warning] 131-131: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 136-136: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 141-141: Tilde does not expand in quotes. Use $HOME.
(SC2088)
🔇 Additional comments (11)
plugins/openshift/commands/visualize-ovn-topology.md (1)
1-121: Excellent documentation structure and clarity.The command documentation is comprehensive, well-organized, and accurately describes the implementation. The security section effectively communicates the read-only nature of operations, prerequisites are clear, and the examples provide helpful context for users. The previous issue regarding load balancer claims has been properly addressed.
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (4)
31-63: Well-structured class with clear constants and type hints.The class organization is excellent with descriptive constants for filenames, component names, and placement types. The use of type hints and defaultdict for data structures improves code clarity and maintainability.
64-92: Excellent error handling with actionable user guidance.The input validation provides clear, helpful error messages that guide users to run the data collection script first. This prevents confusion and makes the workflow explicit.
128-167: Sound placement detection logic.The UUID pattern analysis correctly distinguishes between cluster-wide components (same UUID across nodes) and per-node components (different UUIDs or single-node presence). The logic handles all cases appropriately and provides clear progress output.
169-299: Well-designed workflow with appropriate visualization overrides.The placement analysis workflow is clear and maintainable. The transit_switch override for visualization purposes is well-documented and intentional. The join switch correctly respects discovered data, demonstrating a good balance between data-driven analysis and visualization requirements. The main entry point properly handles tmpdir configuration from multiple sources.
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (6)
80-94: Previous critical issue properly resolved: namespace detection deferred.The OVN namespace detection has been correctly moved out of
__init__(line 86 initializes to None) and into therun()method (lines 600-605) where it's wrapped in proper error handling. This prevents uncaught exceptions during object construction and provides clear user-facing error messages.
107-285: Robust pod collection with excellent fallback strategy.The pod collection implementation demonstrates strong defensive programming:
- Multi-method ovnkube-node pod identification ensures compatibility across deployment types
- Primary JSON parsing with automatic fallback to
kubectl -o widehandles various failure modes- Timeout handling prevents operations from hanging
- Auto-discovery of ovnkube-node pods keeps the workflow simple
311-379: Previous critical issue properly resolved: CSV parsing fixed.The CSV parsing has been correctly implemented using the
csvmodule (lines 35-36, 359-364). This properly handles OVN fields that contain commas (common inoptions,external_ids, andother_config), preventing data corruption that would have occurred with naive string replacement.
381-465: Excellent graceful degradation in data collection.The node data collection correctly implements partial success semantics—returning
Trueif ANY component is successfully collected. This is appropriate for distributed systems where individual queries may fail. Independent component queries with per-component error handling ensure maximum data collection even in degraded conditions.
467-531: Well-designed collection orchestration with helpful diagnostics.The architecture detection and collection loop demonstrate excellent design:
- Automatic OVSDB container detection reduces configuration burden
- Clear architecture summary aids debugging (platform, namespace, container, node count)
- Per-node error tracking without halting overall collection maximizes data gathering
- Error context (pod, node, error details) facilitates troubleshooting
532-657: Comprehensive summary output and clean workflow orchestration.The collection workflow is well-orchestrated with appropriate success criteria:
- Success defined as "any nodes succeeded" aligns with the graceful degradation design
- Detailed summary includes counts, file paths, and actionable troubleshooting tips for failures
- The
run()method has a clear sequence: namespace detection → initialization → pod collection → node collection → verification → summary- Proper argument validation and conventional exit codes (0 for success/partial success, 1 for total failure)
mattedallo
force-pushed
the
add-visualize-ovn-topology
branch
from
fdcb319 to
008b2e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/openshift/skills/generating-ovn-topology/README.md (1)
29-31: Add language identifier to fenced code block.The fenced code block is missing a language identifier, which affects syntax highlighting and accessibility.
Apply this diff:
-``` +```bash /openshift:visualize-ovn-topology</blockquote></details> <details> <summary>plugins/openshift/commands/visualize-ovn-topology.md (1)</summary><blockquote> `12-14`: **Add language identifier to fenced code block.** The code block is missing a language identifier for proper syntax highlighting. Apply this diff: ```diff -``` +```bash /openshift:visualize-ovn-topology</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **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** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between fdcb31974d2d1460eb24cf3728cf721a735c3516 and 008b2e1b934c0147eab08f48a63c203b75687a5d. </details> <details> <summary>📒 Files selected for processing (8)</summary> * `plugins/openshift/commands/visualize-ovn-topology.md` (1 hunks) * `plugins/openshift/skills/generating-ovn-topology/README.md` (1 hunks) * `plugins/openshift/skills/generating-ovn-topology/SKILL.md` (1 hunks) * `plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py` (1 hunks) * `plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py` (1 hunks) * `plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py` (1 hunks) * `plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh` (1 hunks) * `plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>plugins/openshift/commands/visualize-ovn-topology.md</summary> 12-12: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/README.md</summary> 25-25: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 178-178: Table column count Expected: 4; Actual: 8; Too many cells, extra data will be missing (MD056, table-column-count) </details> </details> <details> <summary>🪛 Ruff (0.14.4)</summary> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py</summary> 1-1: Shebang is present but file is not executable (EXE001) --- 43-43: `subprocess` call: check for execution of untrusted input (S603) --- 44-51: Starting a process with a partial executable path (S607) --- 59-59: String contains ambiguous `ℹ` (INFORMATION SOURCE). Did you mean `i` (LATIN SMALL LETTER I)? (RUF001) --- 66-66: String contains ambiguous `ℹ` (INFORMATION SOURCE). Did you mean `i` (LATIN SMALL LETTER I)? (RUF001) --- 77-92: Avoid specifying long messages outside the exception class (TRY003) --- 106-106: `subprocess` call: check for execution of untrusted input (S603) --- 107-114: Starting a process with a partial executable path (S607) --- 124-124: `subprocess` call: check for execution of untrusted input (S603) --- 125-132: Starting a process with a partial executable path (S607) --- 180-180: `subprocess` call: check for execution of untrusted input (S603) --- 181-186: Starting a process with a partial executable path (S607) --- 193-196: Avoid specifying long messages outside the exception class (TRY003) --- 208-208: String contains ambiguous `ℹ` (INFORMATION SOURCE). Did you mean `i` (LATIN SMALL LETTER I)? (RUF001) --- 224-243: Avoid specifying long messages outside the exception class (TRY003) --- 246-249: Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling (B904) --- 246-249: Avoid specifying long messages outside the exception class (TRY003) --- 251-253: Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling (B904) --- 251-253: Avoid specifying long messages outside the exception class (TRY003) --- 269-269: `subprocess` call: check for execution of untrusted input (S603) --- 270-277: Starting a process with a partial executable path (S607) --- 281-281: Consider moving this statement to an `else` block (TRY300) </details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py</summary> 80-80: Probable insecure usage of temporary file or directory: "/tmp" (S108) --- 190-190: `subprocess` call: check for execution of untrusted input (S603) --- 191-194: Starting a process with a partial executable path (S607) --- 215-215: Consider moving this statement to an `else` block (TRY300) --- 230-230: Do not catch blind exception: `Exception` (BLE001) --- 242-242: `subprocess` call: check for execution of untrusted input (S603) --- 243-246: Starting a process with a partial executable path (S607) --- 281-281: Consider moving this statement to an `else` block (TRY300) --- 283-283: Do not catch blind exception: `Exception` (BLE001) --- 292-292: `subprocess` call: check for execution of untrusted input (S603) --- 293-297: Starting a process with a partial executable path (S607) --- 306-306: Consider moving this statement to an `else` block (TRY300) --- 308-308: Do not catch blind exception: `Exception` (BLE001) --- 334-334: `subprocess` call: check for execution of untrusted input (S603) --- 335-342: Starting a process with a partial executable path (S607) --- 366-366: Consider moving this statement to an `else` block (TRY300) --- 374-374: Do not catch blind exception: `Exception` (BLE001) --- 522-522: Do not catch blind exception: `Exception` (BLE001) --- 553-553: Do not catch blind exception: `Exception` (BLE001) --- 603-603: Do not catch blind exception: `Exception` (BLE001) --- 650-650: Probable insecure usage of temporary file or directory: "/tmp" (S108) </details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py</summary> 55-55: `subprocess` call: check for execution of untrusted input (S603) --- 56-63: Starting a process with a partial executable path (S607) --- 68-68: Consider moving this statement to an `else` block (TRY300) --- 110-110: `subprocess` call: check for execution of untrusted input (S603) --- 116-116: Consider moving this statement to an `else` block (TRY300) --- 124-124: Do not catch blind exception: `Exception` (BLE001) --- 206-206: Do not catch blind exception: `Exception` (BLE001) </details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py</summary> 49-49: Probable insecure usage of temporary file or directory: "/tmp" (S108) --- 247-247: Do not catch blind exception: `Exception` (BLE001) --- 292-292: Probable insecure usage of temporary file or directory: "/tmp" (S108) </details> </details> <details> <summary>🪛 Shellcheck (0.11.0)</summary> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh</summary> [warning] 131-131: Tilde does not expand in quotes. Use $HOME. (SC2088) --- [warning] 136-136: Tilde does not expand in quotes. Use $HOME. (SC2088) --- [warning] 141-141: Tilde does not expand in quotes. Use $HOME. (SC2088) </details> </details> </details> <details> <summary>🔇 Additional comments (5)</summary><blockquote> <details> <summary>plugins/openshift/skills/generating-ovn-topology/SKILL.md (1)</summary><blockquote> `1-399`: **LGTM!** The skill definition is comprehensive and well-structured. It provides clear instructions for AI agents, including cluster detection, permission checks, data collection, placement analysis, and diagram generation rules. The security guarantees and read-only operation emphasis are appropriate. </blockquote></details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh (1)</summary><blockquote> `1-190`: **LGTM!** The cluster detection logic is well-implemented with proper handling of colon-delimited `KUBECONFIG` entries, tilde expansion in paths, and deduplication of kubeconfig+context combinations. The script correctly scans multiple kubeconfig sources and outputs parseable results to stdout while keeping diagnostics on stderr. **Note:** The shellcheck warnings about tilde expansion in quotes (lines 131, 136, 141) are false positives—those are intentional literal strings for user-facing diagnostic messages, not file paths. </blockquote></details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py (1)</summary><blockquote> `1-231`: **LGTM!** The permission checker is well-implemented with proper separation of concerns. The namespace detection is correctly deferred to `run()` after kubectl availability checks, and error handling is graceful with clear user-facing messages. The structured output (WRITE_PERMISSIONS_DETECTED markers) correctly goes to stdout for parsing while diagnostics go to stderr. </blockquote></details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (1)</summary><blockquote> `1-299`: **LGTM!** The placement analyzer correctly implements UUID-based analysis to distinguish per-node from cluster-wide components. The visualization override for `transit_switch` is well-documented and appropriate. Error handling is defensive with clear user guidance. </blockquote></details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (1)</summary><blockquote> `1-657`: **LGTM!** The data collector is well-architected with graceful degradation, proper CSV parsing to handle fields containing commas, and deferred namespace detection. The implementation correctly addresses previous review feedback: - CSV parsing uses `csv.reader` instead of naive string replacement (lines 359-364) - Namespace detection moved to `run()` method with proper error handling (lines 601-605) - Comprehensive error handling with fallback strategies throughout The graceful degradation pattern (continuing on node failures, fallback pod collection) ensures robustness in production environments. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py
Outdated
Show resolved
Hide resolved
mattedallo
force-pushed
the
add-visualize-ovn-topology
branch
from
008b2e1 to
0b317ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/openshift/skills/generating-ovn-topology/README.md (2)
29-56: Add language IDs to fenced blocks for lint compliance.markdownlint is flagging these anonymous fences (MD040). Please annotate each with an appropriate language, e.g.
(orshell</code> for commands and <code>textmermaid) for plain/log output, so previews and tooling behave correctly.
354-355: Convert bare URLs to Markdown links.The linter (MD034) prefers link syntax like
Project repo. Wrapping these URLs keeps the docs lint-clean and renders consistently.
📜 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 (8)
plugins/openshift/commands/visualize-ovn-topology.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/README.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/SKILL.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/commands/visualize-ovn-topology.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/openshift/skills/generating-ovn-topology/README.md
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
260-260: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
333-333: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
354-354: Bare URL used
(MD034, no-bare-urls)
355-355: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.4)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py
49-49: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
247-247: Do not catch blind exception: Exception
(BLE001)
292-292: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py
55-55: subprocess call: check for execution of untrusted input
(S603)
56-63: Starting a process with a partial executable path
(S607)
68-68: Consider moving this statement to an else block
(TRY300)
110-110: subprocess call: check for execution of untrusted input
(S603)
116-116: Consider moving this statement to an else block
(TRY300)
124-124: Do not catch blind exception: Exception
(BLE001)
206-206: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py
1-1: Shebang is present but file is not executable
(EXE001)
43-43: subprocess call: check for execution of untrusted input
(S603)
44-51: Starting a process with a partial executable path
(S607)
59-59: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
66-66: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
77-92: Avoid specifying long messages outside the exception class
(TRY003)
106-106: subprocess call: check for execution of untrusted input
(S603)
107-114: Starting a process with a partial executable path
(S607)
124-124: subprocess call: check for execution of untrusted input
(S603)
125-132: Starting a process with a partial executable path
(S607)
180-180: subprocess call: check for execution of untrusted input
(S603)
181-186: Starting a process with a partial executable path
(S607)
193-196: Avoid specifying long messages outside the exception class
(TRY003)
208-208: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
227-246: Avoid specifying long messages outside the exception class
(TRY003)
249-252: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
249-252: Avoid specifying long messages outside the exception class
(TRY003)
254-256: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
254-256: Avoid specifying long messages outside the exception class
(TRY003)
272-272: subprocess call: check for execution of untrusted input
(S603)
273-280: Starting a process with a partial executable path
(S607)
284-284: Consider moving this statement to an else block
(TRY300)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py
80-80: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
190-190: subprocess call: check for execution of untrusted input
(S603)
191-194: Starting a process with a partial executable path
(S607)
215-215: Consider moving this statement to an else block
(TRY300)
230-230: Do not catch blind exception: Exception
(BLE001)
242-242: subprocess call: check for execution of untrusted input
(S603)
243-246: Starting a process with a partial executable path
(S607)
281-281: Consider moving this statement to an else block
(TRY300)
283-283: Do not catch blind exception: Exception
(BLE001)
292-292: subprocess call: check for execution of untrusted input
(S603)
293-297: Starting a process with a partial executable path
(S607)
306-306: Consider moving this statement to an else block
(TRY300)
308-308: Do not catch blind exception: Exception
(BLE001)
334-334: subprocess call: check for execution of untrusted input
(S603)
335-342: Starting a process with a partial executable path
(S607)
366-366: Consider moving this statement to an else block
(TRY300)
374-374: Do not catch blind exception: Exception
(BLE001)
522-522: Do not catch blind exception: Exception
(BLE001)
553-553: Do not catch blind exception: Exception
(BLE001)
603-603: Do not catch blind exception: Exception
(BLE001)
650-650: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
🪛 Shellcheck (0.11.0)
plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh
[warning] 131-131: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 136-136: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 141-141: Tilde does not expand in quotes. Use $HOME.
(SC2088)
🔇 Additional comments (2)
plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py (1)
23-211: Change looks solid.Namespace detection and risky-permission scanning now run in the right order with clean failure handling. No issues spotted.
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (1)
80-629: Nice improvements.Moving namespace detection into
run()and using proper CSV parsing tightens reliability. Everything else checks out.
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py
Outdated
Show resolved
Hide resolved
openshift-merge-robot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
plugins/openshift/skills/generating-ovn-topology/SKILL.md (1)
25-28: Align TMPDIR usage and fix markdownlint issues.Two small but useful cleanups here:
/tmp vs TMPDIR (security / correctness)
- Line 264 currently says placement is determined by
/tmp/ovn_*_placement.txt, but the scripts write into the private$TMPDIRcreated viamkdtemp, and your own “CRITICAL RULES” forbid using/tmpdirectly.- Recommend updating that bullet to reference
$TMPDIR/ovn_*_placement.txtinstead, to match the implementation and avoid suggesting/tmp.Fenced code language for examples (MD040)
- The small example block around the detect‑cluster output (lines 25–28) lacks a language identifier.
- Consider marking it as
text(orbashwhere applicable), e.g. ```text to satisfy markdownlint and improve rendering.Table pipes / column count (MD056)
- The helper scripts table (lines 183–188) includes
index|kubeconfig|cluster|nodes|namespaceinside a table cell. The raw|characters cause markdownlint to see more than 4 columns.- Escaping the pipes (e.g.
index\|kubeconfig\|cluster\|nodes\|namespace) or using HTML entities (|) inside the inline code will fix the column count without changing the rendered content.Also applies to: 183-188, 262-267
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (1)
255-260: Narrow the broadExceptioncatch incount_lines(optional).
count_lines()currently swallows all exceptions and returns0. That’s fine for a best‑effort summary, but it also hides real issues (e.g., permission errors or unexpected IO problems).If you want slightly clearer failure modes without changing behavior much, consider narrowing this to
OSError(or at least logging unexpected exceptions to stderr) so logical errors don’t get silently converted to0:- def count_lines(filepath): - try: - with open(filepath) as f: - return sum(1 for _ in f) - except Exception: - return 0 + def count_lines(filepath): + try: + with open(filepath) as f: + return sum(1 for _ in f) + except OSError: + # Missing or unreadable file → treat as zero entries + return 0plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py (1)
18-76: Utilities and safe file handling look solid; only minor optional polish.Overall this module looks good:
- Namespace/container detection uses argument‑vector
subprocess.runand sensible timeouts.- All informational/status output now goes to stderr, keeping stdout machine‑readable for callers.
safe_write_file/safe_append_filecorrectly guard against symlink attacks withos.O_NOFOLLOW, explicitlexists/islinkchecks, and0o600permissions, which is a substantial improvement over direct/tmpusage.If you want a bit more polish (optional), two small tweaks:
- When re‑raising as
RuntimeErrorin thedetect_ovsdb_containerexception handlers, considerraise RuntimeError(...) from eto preserve the original traceback.- For very long error messages, you could factor the help text into constants to keep the functions a bit leaner, but this is stylistic.
Also applies to: 176-248, 292-367
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (1)
569-575: Validate TMPDIR input and optionally narrow a few broad exception handlers.The collector’s overall flow and error handling look good, and moving OVN namespace detection into
run()with a friendly error path is a nice improvement. Two small robustness tweaks to consider:
- TMPDIR validation in
main()
Right now you only check that the kubeconfig exists. If<TMPDIR>is missing or not a directory, the firstsafe_write_filewill raise and surface as a traceback. You can fail fast with a clearer message:kubeconfig = sys.argv[1] tmpdir = sys.argv[2] if not os.path.exists(kubeconfig): print( f"❌ Error: Kubeconfig not found: {kubeconfig}", file=sys.stderr, ) return 1 + + if not os.path.isdir(tmpdir): + print( + f"❌ Error: TMPDIR is not a directory: {tmpdir}", + file=sys.stderr, + ) + return 1
- Broad
except Exception(optional)
In helpers likecollect_pods_info(),_collect_pods_fallback(), andcount_lines(), broadExceptioncatches are used to keep going, which makes sense for a best‑effort collector. If you want clearer separation between expected IO/subprocess issues and programming errors, you could narrow those toOSError/subprocess.SubprocessErrorand let truly unexpected exceptions surface, but that’s optional given the CLI nature of this script.Also applies to: 652-673
📜 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 (4)
plugins/openshift/skills/generating-ovn-topology/SKILL.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/skills/generating-ovn-topology/SKILL.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
185-185: Table column count
Expected: 4; Actual: 8; Too many cells, extra data will be missing
(MD056, table-column-count)
🪛 Ruff (0.14.4)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py
259-259: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py
1-1: Shebang is present but file is not executable
(EXE001)
45-45: subprocess call: check for execution of untrusted input
(S603)
46-53: Starting a process with a partial executable path
(S607)
61-61: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
68-68: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
79-94: Avoid specifying long messages outside the exception class
(TRY003)
108-108: subprocess call: check for execution of untrusted input
(S603)
109-116: Starting a process with a partial executable path
(S607)
126-126: subprocess call: check for execution of untrusted input
(S603)
127-134: Starting a process with a partial executable path
(S607)
182-182: subprocess call: check for execution of untrusted input
(S603)
183-188: Starting a process with a partial executable path
(S607)
195-198: Avoid specifying long messages outside the exception class
(TRY003)
210-210: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
229-248: Avoid specifying long messages outside the exception class
(TRY003)
251-254: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
251-254: Avoid specifying long messages outside the exception class
(TRY003)
256-258: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
256-258: Avoid specifying long messages outside the exception class
(TRY003)
275-275: subprocess call: check for execution of untrusted input
(S603)
276-283: Starting a process with a partial executable path
(S607)
287-287: Consider moving this statement to an else block
(TRY300)
305-307: Avoid specifying long messages outside the exception class
(TRY003)
325-327: Avoid specifying long messages outside the exception class
(TRY003)
344-346: Avoid specifying long messages outside the exception class
(TRY003)
363-365: Avoid specifying long messages outside the exception class
(TRY003)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py
204-204: subprocess call: check for execution of untrusted input
(S603)
205-208: Starting a process with a partial executable path
(S607)
229-229: Consider moving this statement to an else block
(TRY300)
244-244: Do not catch blind exception: Exception
(BLE001)
256-256: subprocess call: check for execution of untrusted input
(S603)
257-260: Starting a process with a partial executable path
(S607)
298-298: Consider moving this statement to an else block
(TRY300)
300-300: Do not catch blind exception: Exception
(BLE001)
309-309: subprocess call: check for execution of untrusted input
(S603)
310-314: Starting a process with a partial executable path
(S607)
323-323: Consider moving this statement to an else block
(TRY300)
325-325: Do not catch blind exception: Exception
(BLE001)
351-351: subprocess call: check for execution of untrusted input
(S603)
352-359: Starting a process with a partial executable path
(S607)
387-387: Consider moving this statement to an else block
(TRY300)
395-395: Do not catch blind exception: Exception
(BLE001)
543-543: Do not catch blind exception: Exception
(BLE001)
549-549: Use explicit conversion flag
Replace with conversion flag
(RUF010)
573-573: Do not catch blind exception: Exception
(BLE001)
623-623: Do not catch blind exception: Exception
(BLE001)
mattedallo
force-pushed
the
add-visualize-ovn-topology
branch
from
7361f19 to
7c65674
Compare
openshift-merge-robot
removed
the
needs-rebase
Signed-off-by: Matteo Dallaglio <[email protected]>
- Defer namespace detection to avoid preflight crashes - Fix support for OpenShift cluster and multiple cluster detection logic - Remove generated topology diagram from repository - Standardize Python script filenames according to Google style guide - Extract shared utilities to ovn_utils.py for better code reuse Signed-off-by: Matteo Dallaglio <[email protected]> Signed-off-by: Matteo Dallaglio <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/openshift/skills/generating-ovn-topology/SKILL.md (1)
183-189: Verify table structure for proper rendering.The markdown table at line 185 may have formatting issues. Ensure the separator line matches the number of columns in the header (Script | Purpose | Input | Output should have 4 column separators).
If the table isn't rendering correctly, verify the separator line:
| Script | Purpose | Input | Output | -|--------|---------|-------|--------| +| ------ | ------- | ----- | ------ |plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py (1)
1-1: Consider making the file executable.The shebang is present but the file lacks execute permissions. If this script is intended to be run directly (e.g.,
./ovn_utils.py), add execute permissions withchmod +x. If it's only imported as a module, the shebang can be removed.Apply this command if the script should be executable:
chmod +x plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.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 (10)
PLUGINS.md(1 hunks)docs/data.json(1 hunks)plugins/openshift/commands/visualize-ovn-topology.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/README.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/SKILL.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/skills/generating-ovn-topology/README.md
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
260-260: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
333-333: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
354-354: Bare URL used
(MD034, no-bare-urls)
355-355: Bare URL used
(MD034, no-bare-urls)
plugins/openshift/commands/visualize-ovn-topology.md
12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/openshift/skills/generating-ovn-topology/SKILL.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
185-185: Table column count
Expected: 4; Actual: 8; Too many cells, extra data will be missing
(MD056, table-column-count)
🪛 Ruff (0.14.4)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py
204-204: subprocess call: check for execution of untrusted input
(S603)
205-208: Starting a process with a partial executable path
(S607)
229-229: Consider moving this statement to an else block
(TRY300)
244-244: Do not catch blind exception: Exception
(BLE001)
256-256: subprocess call: check for execution of untrusted input
(S603)
257-260: Starting a process with a partial executable path
(S607)
298-298: Consider moving this statement to an else block
(TRY300)
300-300: Do not catch blind exception: Exception
(BLE001)
309-309: subprocess call: check for execution of untrusted input
(S603)
310-314: Starting a process with a partial executable path
(S607)
323-323: Consider moving this statement to an else block
(TRY300)
325-325: Do not catch blind exception: Exception
(BLE001)
351-351: subprocess call: check for execution of untrusted input
(S603)
352-359: Starting a process with a partial executable path
(S607)
387-387: Consider moving this statement to an else block
(TRY300)
395-395: Do not catch blind exception: Exception
(BLE001)
543-543: Do not catch blind exception: Exception
(BLE001)
549-549: Use explicit conversion flag
Replace with conversion flag
(RUF010)
573-573: Do not catch blind exception: Exception
(BLE001)
623-623: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py
55-55: subprocess call: check for execution of untrusted input
(S603)
56-63: Starting a process with a partial executable path
(S607)
68-68: Consider moving this statement to an else block
(TRY300)
110-110: subprocess call: check for execution of untrusted input
(S603)
116-116: Consider moving this statement to an else block
(TRY300)
124-124: Do not catch blind exception: Exception
(BLE001)
206-206: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py
259-259: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py
1-1: Shebang is present but file is not executable
(EXE001)
45-45: subprocess call: check for execution of untrusted input
(S603)
46-53: Starting a process with a partial executable path
(S607)
61-61: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
68-68: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
79-94: Avoid specifying long messages outside the exception class
(TRY003)
108-108: subprocess call: check for execution of untrusted input
(S603)
109-116: Starting a process with a partial executable path
(S607)
126-126: subprocess call: check for execution of untrusted input
(S603)
127-134: Starting a process with a partial executable path
(S607)
182-182: subprocess call: check for execution of untrusted input
(S603)
183-188: Starting a process with a partial executable path
(S607)
195-198: Avoid specifying long messages outside the exception class
(TRY003)
210-210: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
229-248: Avoid specifying long messages outside the exception class
(TRY003)
251-254: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
251-254: Avoid specifying long messages outside the exception class
(TRY003)
256-258: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
256-258: Avoid specifying long messages outside the exception class
(TRY003)
275-275: subprocess call: check for execution of untrusted input
(S603)
276-283: Starting a process with a partial executable path
(S607)
287-287: Consider moving this statement to an else block
(TRY300)
305-307: Avoid specifying long messages outside the exception class
(TRY003)
325-327: Avoid specifying long messages outside the exception class
(TRY003)
344-346: Avoid specifying long messages outside the exception class
(TRY003)
363-365: Avoid specifying long messages outside the exception class
(TRY003)
🪛 Shellcheck (0.11.0)
plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh
[warning] 131-131: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 136-136: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 141-141: Tilde does not expand in quotes. Use $HOME.
(SC2088)
🔇 Additional comments (21)
plugins/openshift/skills/generating-ovn-topology/README.md (1)
1-359: Excellent comprehensive documentation.The README provides thorough coverage of the OVN topology visualization feature including prerequisites, usage examples, security guarantees, troubleshooting guidance, and technical details. The documentation clearly emphasizes the read-only nature of operations and includes helpful examples and FAQ entries.
plugins/openshift/skills/generating-ovn-topology/SKILL.md (1)
1-404: Well-structured AI agent workflow documentation.The SKILL.md provides clear step-by-step instructions for AI agents, comprehensive security notes, and detailed diagram generation rules. The emphasis on read-only operations, private temporary directories, and informed consent for admin credentials demonstrates strong security awareness.
PLUGINS.md (1)
164-164: Command entry correctly integrated.The new
/openshift:visualize-ovn-topologycommand is properly added to the OpenShift plugin section with consistent formatting and accurate description.plugins/openshift/commands/visualize-ovn-topology.md (1)
1-121: Clear and accurate command documentation.The command documentation correctly describes the functionality, emphasizes read-only operations, and provides helpful examples. The past review concern about load balancer claims has been properly addressed—the documentation now accurately reflects the current implementation scope.
plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh (1)
1-190: Robust cluster detection implementation.The script properly handles multiple kubeconfig sources, deduplicates contexts, and provides clear diagnostic output. Past review concerns have been addressed including KUBECONFIG colon-delimited splitting and proper variable scoping. The shellcheck warnings about tilde expansion are false positives—the tildes in display labels (lines 131, 136, 141) are intentionally preserved for user-friendly output.
docs/data.json (1)
580-586: Command metadata correctly structured.The new command entry is properly formatted with accurate description and metadata consistent with other OpenShift commands in the data structure.
plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py (3)
43-48: Namespace detection properly deferred.The initialization correctly defers OVN namespace detection by setting
self.ovn_namespace: Optional[str] = Nonein__init__. Detection is performed later inrun()after kubectl availability is verified (lines 203-208), addressing the critical issue from past reviews.
23-34: Permissions constant properly scoped and immutable.The
_DANGEROUS_PERMSconstant is now correctly implemented as a module-scoped, private (underscore prefix), immutable tuple of tuples, addressing past review feedback about preventing accidental mutation.
198-212: Robust error handling in run method.The
run()method properly orchestrates the permission check workflow with kubectl availability verification, namespace detection with exception handling, and graceful error reporting. The exception handling at lines 204-208 ensures users receive clear error messages rather than Python tracebacks.plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (3)
52-66: Secure temporary directory handling.The analyzer correctly uses a parameterized
tmpdirargument instead of hardcoded/tmppaths, addressing past security concerns about symlink attacks. All input and output file paths are properly constructed within the provided private temporary directory.
135-174: Sound UUID-based placement analysis.The
_analyze_component_placementmethod uses a clear and logical approach to determine component placement by analyzing UUID patterns across nodes. The logic correctly distinguishes between cluster-wide components (same UUID across nodes), per-node replicated components (different UUIDs), and node-specific components (single node only).
218-230: Safe file write operations for placement output.The placement results are written using the
safe_write_fileutility function, which provides protection against symlink attacks and ensures atomic file creation in the private temporary directory.plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py (4)
18-94: LGTM! Well-structured namespace detection with helpful error messages.The two-phase detection strategy (fast path for common namespaces, slow path for comprehensive search) is efficient and future-proof. Error messages properly go to stderr, and the RuntimeError provides actionable troubleshooting steps.
153-258: LGTM! Container detection follows the same robust pattern as namespace detection.The two-phase detection with tool verification is well-designed. All informational messages correctly go to stderr, and error messages provide helpful troubleshooting guidance.
292-328: Excellent security implementation for file operations.The use of
O_NOFOLLOWwith explicit symlink checks prevents symlink-based attacks (CWE-377/CWE-59). Setting0o600permissions follows the principle of least privilege. The error handling with proper cleanup is well-implemented.
331-366: LGTM! Consistent security approach for append operations.The function correctly handles both existing and new files while maintaining the same security guarantees as
safe_write_file. The conditional flag selection (O_APPEND vs O_CREAT|O_APPEND) is appropriate.plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (5)
1-63: LGTM! Well-organized imports and constants.The comprehensive docstring clearly explains usage, outputs, and exit codes. Module-scoped constants with
_prefix follow Python conventions for private module-level variables. The addition ofcsvandStringIOimports addresses the CSV parsing issue flagged in past reviews.
65-79: LGTM! Clean dataclass design for tracking collection statistics.The use of
field(default_factory=...)for mutable defaults (list,dict) is the correct pattern to avoid sharing mutable objects between instances.
85-105: LGTM! Constructor properly defers detection to run() method.The initialization correctly sets
ovn_namespaceandovsdb_containertoNone, deferring their detection to therun()method where errors can be handled gracefully. This addresses the critical issue raised in past reviews.
613-649: LGTM! Well-structured execution flow with proper error handling.The
run()method correctly wraps namespace detection (lines 621-625) in a try/except block, addressing the critical issue from past reviews. The graceful degradation strategy (continuing after pod collection failures) allows partial success, which is appropriate for a data collection tool.
328-400: LGTM! CSV parsing correctly handles quoted fields with commas.The implementation at lines 378-382 uses
csv.readerto properly parse CSV output, addressing the critical issue flagged in past reviews. This prevents data corruption when OVN fields contain commas (e.g., inoptionsorexternal_ids).
mattedallo
force-pushed
the
add-visualize-ovn-topology
branch
from
7c65674 to
9c82183
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/openshift/skills/generating-ovn-topology/SKILL.md (1)
25-28: Add language identifier to fenced code block.The fenced code block should specify a language identifier for proper rendering and accessibility.
Apply this diff:
- ``` + ```text 1|/home/user/.kube/kind-config|kind-ovn|3|ovn-kubernetes 2|/home/user/.kube/config|prod-cluster|12|openshift-ovn-kubernetes ```plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py (1)
1-1: Consider making the file executable or document the intention.The file has a shebang but isn't marked executable. If this module is intended to be run directly, add execute permissions. Otherwise, consider if the shebang is necessary.
To make it executable:
chmod +x plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.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 (5)
docs/data.json(1 hunks)plugins/openshift/skills/generating-ovn-topology/SKILL.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/data.json
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/skills/generating-ovn-topology/SKILL.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
185-185: Table column count
Expected: 4; Actual: 8; Too many cells, extra data will be missing
(MD056, table-column-count)
🪛 Ruff (0.14.4)
plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py
1-1: Shebang is present but file is not executable
(EXE001)
45-45: subprocess call: check for execution of untrusted input
(S603)
46-53: Starting a process with a partial executable path
(S607)
61-61: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
68-68: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
79-94: Avoid specifying long messages outside the exception class
(TRY003)
108-108: subprocess call: check for execution of untrusted input
(S603)
109-116: Starting a process with a partial executable path
(S607)
126-126: subprocess call: check for execution of untrusted input
(S603)
127-134: Starting a process with a partial executable path
(S607)
182-182: subprocess call: check for execution of untrusted input
(S603)
183-188: Starting a process with a partial executable path
(S607)
195-198: Avoid specifying long messages outside the exception class
(TRY003)
210-210: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
229-248: Avoid specifying long messages outside the exception class
(TRY003)
251-254: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
251-254: Avoid specifying long messages outside the exception class
(TRY003)
256-258: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
256-258: Avoid specifying long messages outside the exception class
(TRY003)
275-275: subprocess call: check for execution of untrusted input
(S603)
276-283: Starting a process with a partial executable path
(S607)
287-287: Consider moving this statement to an else block
(TRY300)
305-307: Avoid specifying long messages outside the exception class
(TRY003)
325-327: Avoid specifying long messages outside the exception class
(TRY003)
344-346: Avoid specifying long messages outside the exception class
(TRY003)
363-365: Avoid specifying long messages outside the exception class
(TRY003)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py
204-204: subprocess call: check for execution of untrusted input
(S603)
205-208: Starting a process with a partial executable path
(S607)
229-229: Consider moving this statement to an else block
(TRY300)
244-244: Do not catch blind exception: Exception
(BLE001)
256-256: subprocess call: check for execution of untrusted input
(S603)
257-260: Starting a process with a partial executable path
(S607)
298-298: Consider moving this statement to an else block
(TRY300)
300-300: Do not catch blind exception: Exception
(BLE001)
309-309: subprocess call: check for execution of untrusted input
(S603)
310-314: Starting a process with a partial executable path
(S607)
323-323: Consider moving this statement to an else block
(TRY300)
325-325: Do not catch blind exception: Exception
(BLE001)
351-351: subprocess call: check for execution of untrusted input
(S603)
352-359: Starting a process with a partial executable path
(S607)
387-387: Consider moving this statement to an else block
(TRY300)
395-395: Do not catch blind exception: Exception
(BLE001)
543-543: Do not catch blind exception: Exception
(BLE001)
549-549: Use explicit conversion flag
Replace with conversion flag
(RUF010)
573-573: Do not catch blind exception: Exception
(BLE001)
623-623: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py
259-259: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (10)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (1)
218-229: LGTM! Secure file operations.The use of
safe_write_fileprevents symlink attacks (CWE-377/CWE-59) and the content is properly assembled before writing.plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py (4)
292-329: LGTM! Robust symlink protection.The implementation properly prevents symlink attacks using
O_NOFOLLOW, explicit symlink checks, and handlesELOOPerrors. The file descriptor cleanup in the exception handler is correct.
331-367: LGTM! Consistent security implementation.The append function maintains the same symlink protections as
safe_write_fileand properly handles both existing and new files.
18-95: LGTM! Well-designed two-phase detection.The fast-path/slow-path strategy is efficient, the error messages are comprehensive with troubleshooting steps, and informational output correctly goes to stderr.
153-259: LGTM! Robust container detection.The implementation follows the same efficient two-phase pattern as namespace detection, provides detailed troubleshooting information, and properly routes diagnostic messages to stderr.
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (5)
85-105: LGTM! Proper separation of initialization and detection.Deferring namespace detection to
run()allows for better error handling and clearer error messages to users, as noted in past reviews.
620-626: LGTM! Graceful namespace detection error handling.The try/except wrapper provides clear error messages and proper exit codes when namespace detection fails, successfully addressing the past review concern.
374-382: LGTM! Proper CSV parsing prevents data corruption.Using
csv.readercorrectly handles OVN fields that contain commas (e.g., inoptions,external_ids), addressing the critical issue from past review.
106-116: LGTM! Secure file initialization.Using
safe_write_fileto initialize output files prevents symlink attacks and ensures clean state.
552-559: LGTM! Graceful degradation strategy.Accepting partial success (any node data collected) is appropriate for a read-only diagnostic tool where some data is better than none.
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py
Outdated
Show resolved
Hide resolved
mattedallo
force-pushed
the
add-visualize-ovn-topology
branch
from
9c82183 to
5510977
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (3)
101-134: Consider logging or validating malformed input lines
load_data()silently skips lines with fewer than 3 pipe‑separated fields. That’s fine for resiliency, but if upstream data ever gets truncated, it will be hard to notice.If this is meant to be strictly machine‑generated data, consider at least logging a warning with the raw line when
len(parts) < 3, so unexpected format drift is visible during troubleshooting.
236-275: Tightencount_linesexception handling for better diagnostics
print_summary()’scount_lines()helper catches a bareExceptionand returns0on any failure. That makes it impossible to distinguish “file legitimately empty” from “couldn’t read file (permissions, IO error, etc.)”.Consider narrowing the exception type and logging unexpected errors:
- def count_lines(filepath): - try: - with open(filepath) as f: - return sum(1 for _ in f) - except Exception: - return 0 + def count_lines(filepath: str) -> int: + try: + with open(filepath) as f: + return sum(1 for _ in f) + except OSError as exc: + print(f"⚠️ Failed to read {filepath}: {exc}", file=sys.stderr) + return 0(You’d need
import sysat the top if not already present.)
302-318: ValidateTMPDIRpath early for clearer failures
main()accepts any string astmpdirand you only discover problems later when file IO fails insidesafe_write_file/open. For users running this by hand, it would be nicer to fail fast if the directory doesn’t exist or isn’t a directory.For example:
tmpdir = sys.argv[1] - analyzer = PlacementAnalyzer(tmpdir) - return analyzer.run() + if not os.path.isdir(tmpdir): + print(f"❌ Error: TMPDIR is not a directory: {tmpdir}", file=sys.stderr) + return 1 + + analyzer = PlacementAnalyzer(tmpdir) + return analyzer.run()(Requires
import osif not already present in this module.)plugins/openshift/skills/generating-ovn-topology/SKILL.md (2)
1-4: Align load‑balancer mentions with current implementationThe skill description and “Required Info” list load balancers as a first‑class part of the diagram, but the current data‑collection script only pulls switches, routers, LSPs, LRPs, and pods.
Either:
- implement LB collection/rendering in this PR, or
- adjust the description and required‑info bullets to mark load balancers as future/optional, or remove them for now so expectations match reality.
Also applies to: 315-323
25-28: Fix markdownlint issues: add fenced languages and escape pipes in table cellsmarkdownlint’s findings look valid here:
- The example output block at lines 25–28 uses plain ``` fences; add a language (e.g.,
text) to satisfy MD040:- ``` + ```text 1|/home/user/.kube/kind-config|kind-ovn|3|ovn-kubernetes 2|/home/user/.kube/config|prod-cluster|12|openshift-ovn-kubernetes ```
- In the helper‑scripts table, unescaped
|characters inside the “Output” column (e.g.,index|kubeconfig|cluster|nodes|namespace) are parsed as extra columns, triggering MD056. Escape them or wrap with code that escapes the pipes:-| [detect-cluster.sh](scripts/detect-cluster.sh) | ... | Parseable list to stdout: `index|kubeconfig|cluster|nodes|namespace`. Exit: 0=success, 1=none found | +| [detect-cluster.sh](scripts/detect-cluster.sh) | ... | Parseable list to stdout: `index\|kubeconfig\|cluster\|nodes\|namespace`. Exit: 0=success, 1=none found |(Apply the same pattern to any other table cells that include
|.)Also applies to: 183-188
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (4)
187-303: JSON‑first pod discovery with structured fallback is good; consider narrowing broadExceptionThe JSON‑first approach with a
-o widefallback is solid, and all error paths log to stderr and preserve partial progress.You do, however, catch bare
Exceptionin bothcollect_pods_info()and_collect_pods_fallback(), which can mask programmer errors (e.g., KeyError) as generic “trying fallback” messages.If you want to keep behavior but improve debuggability, consider:
- Narrowing the catches to
subprocess.TimeoutExpired,subprocess.SubprocessError,json.JSONDecodeError, andOSError; or- Logging the exception type when you hit the generic handler, e.g.:
- except Exception as e: - print( - f" ⚠️ Error: {e}, trying fallback...", - file=sys.stderr, - ) + except Exception as e: + print( + f" ⚠️ Unexpected error in collect_pods_info: {type(e).__name__}: {e}, trying fallback...", + file=sys.stderr, + )Same idea applies to the generic
except Exceptionin_collect_pods_fallback().
488-552: Per‑node collection and architecture summary behavior is reasonable
collect_all_nodes():
- Fails early if no ovnkube‑node pods were discovered.
- Detects the NBDB container once, then iterates all nodes with per‑node error isolation and statistics.
- Prints a concise architecture summary (platform, namespace, container, node count).
One small nit:
platformclassifies anything other thanopenshift-ovn-kubernetesas “Upstream OVN-Kubernetes”, which may lump kube‑ovn or other future variants into that bucket. If you expect kube‑ovn deployments, you might consider a dedicated label there, but it’s not critical.
561-612: Summary output is helpful; same comment about broadExceptionincount_lines
print_summary()gives a good high‑level view of pods, nodes, and file counts, and it correctly distinguishes partial success from complete failure.Similar to
analyze_placement.py,count_lines()here catches a bareExceptionand returns 0, which hides the difference between “file empty” and “file unreadable”. If you touch this again, consider tightening it toOSErrorand logging the failure to stderr as suggested earlier, so IO problems don’t silently look like empty data.
613-673: run()/main() flow matches documented exit codes; add tmpdir validationThe
run()method’s behavior (exit 0 for any successful node, 1 when no nodes succeed or namespace detection fails) matches the header’s exit‑code documentation, andmain()already validates the kubeconfig path.You don’t currently verify that
tmpdirexists and is a directory before constructingOVNDataCollector, so a typo in the path will surface later as an unhandled IO error fromsafe_write_file. Consider adding a quick check:kubeconfig = sys.argv[1] tmpdir = sys.argv[2] if not os.path.exists(kubeconfig): ... return 1 + if not os.path.isdir(tmpdir): + print( + f"❌ Error: TMPDIR is not a directory: {tmpdir}", + file=sys.stderr, + ) + return 1 + collector = OVNDataCollector(kubeconfig, tmpdir) return collector.run()This would make CLI usage failures clearer without affecting the orchestrated mkdtemp flow.
📜 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 (5)
docs/data.json(1 hunks)plugins/openshift/skills/generating-ovn-topology/SKILL.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/skills/generating-ovn-topology/SKILL.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
185-185: Table column count
Expected: 4; Actual: 8; Too many cells, extra data will be missing
(MD056, table-column-count)
🪛 Ruff (0.14.4)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py
259-259: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py
204-204: subprocess call: check for execution of untrusted input
(S603)
205-208: Starting a process with a partial executable path
(S607)
229-229: Consider moving this statement to an else block
(TRY300)
244-244: Do not catch blind exception: Exception
(BLE001)
256-256: subprocess call: check for execution of untrusted input
(S603)
257-260: Starting a process with a partial executable path
(S607)
298-298: Consider moving this statement to an else block
(TRY300)
300-300: Do not catch blind exception: Exception
(BLE001)
309-309: subprocess call: check for execution of untrusted input
(S603)
310-314: Starting a process with a partial executable path
(S607)
323-323: Consider moving this statement to an else block
(TRY300)
325-325: Do not catch blind exception: Exception
(BLE001)
351-351: subprocess call: check for execution of untrusted input
(S603)
352-359: Starting a process with a partial executable path
(S607)
387-387: Consider moving this statement to an else block
(TRY300)
395-395: Do not catch blind exception: Exception
(BLE001)
543-543: Do not catch blind exception: Exception
(BLE001)
549-549: Use explicit conversion flag
Replace with conversion flag
(RUF010)
573-573: Do not catch blind exception: Exception
(BLE001)
623-623: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py
1-1: Shebang is present but file is not executable
(EXE001)
45-45: subprocess call: check for execution of untrusted input
(S603)
46-53: Starting a process with a partial executable path
(S607)
61-61: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
68-68: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
79-94: Avoid specifying long messages outside the exception class
(TRY003)
108-108: subprocess call: check for execution of untrusted input
(S603)
109-116: Starting a process with a partial executable path
(S607)
126-126: subprocess call: check for execution of untrusted input
(S603)
127-134: Starting a process with a partial executable path
(S607)
182-182: subprocess call: check for execution of untrusted input
(S603)
183-188: Starting a process with a partial executable path
(S607)
195-198: Avoid specifying long messages outside the exception class
(TRY003)
210-210: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
229-248: Avoid specifying long messages outside the exception class
(TRY003)
251-254: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
251-254: Avoid specifying long messages outside the exception class
(TRY003)
256-258: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
256-258: Avoid specifying long messages outside the exception class
(TRY003)
275-275: subprocess call: check for execution of untrusted input
(S603)
276-283: Starting a process with a partial executable path
(S607)
287-287: Consider moving this statement to an else block
(TRY300)
305-307: Avoid specifying long messages outside the exception class
(TRY003)
325-327: Avoid specifying long messages outside the exception class
(TRY003)
344-346: Avoid specifying long messages outside the exception class
(TRY003)
363-365: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (10)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (3)
52-70: Tmpdir handling and in‑memory structures look solidUsing a caller‑provided tmpdir and keeping only per‑run paths plus in‑memory
defaultdictstate is clean and avoids the previous/tmpissues. No changes needed here.
135-175: UUID‑based placement classification is consistent with the documented modelThe
_analyze_component_placement()logic (single UUID across >1 entries → cluster‑wide; >1 UUIDs → per‑node; single entry → per‑node) matches the interconnect model described in SKILL.md and should behave predictably as long as the detail files are complete. I don’t see any correctness issues here.
177-235: Visualization overrides and safe output writing look goodThe transit/join overrides match the documented behavior (only
transit_switchforced tocluster-wide-visual) and you’re writing placement files viasafe_write_file, which addresses the prior predictable/tmp/symlink concern.No changes needed for this section.
docs/data.json (1)
587-592: Skill metadata matches docs, aside from load‑balancer wordingThe
generating-ovn-topologyskill entry here matches the SKILL.md description (minus load balancers), and the ID/name wiring is correct. Once SKILL.md’s wording is updated as discussed, this manifest entry won’t need further changes.plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py (3)
18-75: Namespace detection logic and stderr logging are appropriateThe two‑phase namespace detection (
common_namespaces→_search_for_ovn_namespace) with timeouts and JSON parsing looks robust, and routing all informational prints to stderr keeps stdout clean for machine‑readable use by other scripts.No changes needed here.
Also applies to: 97-150
153-259: OVSDB container detection andovn-nbctlprobing look correct
detect_ovsdb_container()’s fast path overcommon_namesand slow path using_has_ovn_nbctl()is a good balance of speed and future‑proofing. Error messages on failure are detailed and will be useful for operators debugging unusual deployments.
_has_ovn_nbctl()correctly treats non‑zero exit codes and subprocess errors as “tool not present”.I don’t see any functional or security issues in this block.
Also applies to: 261-290
292-329: Secure file write/append helpers address the previous /tmp symlink riskBoth
safe_write_file()andsafe_append_file():
- Reject pre‑existing symlinks via
os.path.lexists/os.path.islink.- Use
os.open(..., os.O_NOFOLLOW, 0o600)to prevent following symlinks on the final path component.- Wrap the file descriptor safely and ensure it’s closed on all paths.
This is a solid mitigation for CWE‑377/CWE‑59 in conjunction with per‑run private tmpdirs; no further changes needed.
Also applies to: 331-367
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (3)
85-105: Constructor and output file handling are consistent with TMPDIR designInitializing only per‑run paths (pods/switches/routers/LSPs/LRPs) under the caller‑provided
tmpdir, and deferring namespace/container detection torun(), matches the SKILL guidance about a private mkdtemp directory and avoids the earlier constructor‑time failures.No issues here.
118-185: ovnkube‑node detection and pod JSON parsing look correctThe
_is_ovnkube_node_pod()helper:
- Filters by
namespace == self.ovn_namespace.- Checks both
app=ovnkube-nodeandname=ovnkube-nodelabels plus theovnkube-node-prefix.Combined with
_parse_pod_json()writingnamespace|name|podIP|nodeand auto‑populatingself.ovnkube_node_pods, this should behave well across older and newer OVN deployments as long asself.ovn_namespaceis set viarun()first.
328-401: OVN component querying and CSV handling look correctThe
query_ovn_component()wrapper:
- Uses
kubectl exec ... ovn-nbctl --format=csv --data=bare --no-headingswith explicit columns.- Parses each CSV line via
csv.reader(StringIO(line))so commas inoptions/external_ids/other_configare handled correctly.- Prepends the node name and appends via
safe_append_file().This cleanly resolves the earlier CSV/commas issue; I don’t see any remaining correctness problems here.
mattedallo
force-pushed
the
add-visualize-ovn-topology
branch
from
5510977 to
4ad0f88
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (1)
255-260: Consider narrowing the broad exception incount_linesCatching bare
Exceptionhere is safe but hides the root cause (e.g., permission or encoding issues). Consider narrowing this toOSError/IOErroror at least logging the exception detail before returning 0 so unexpected failures are easier to debug.plugins/openshift/skills/generating-ovn-topology/SKILL.md (2)
25-29: Add language identifiers to fenced code blocksThe early kubeconfig detection example uses a bare triple-backtick fence. Adding a language (e.g.,
```textor```bash) will satisfy markdownlint (MD040) and improve rendering in many viewers.
183-189: Escape|characters inside table cellsThe helper-scripts table row that shows
index|kubeconfig|cluster|nodes|namespaceuses unescaped|inside a cell, so markdownlint flags it as having too many columns (MD056). Escape the pipes (index\|kubeconfig\|cluster\|nodes\|namespace) or wrap that part differently to keep the table structurally valid.plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py (1)
18-94: Optional: provide clearer error whenkubectlis missingRight now a missing
kubectlbinary surfaces as a generic failure that gets wrapped and printed by callers. You might consider catchingFileNotFoundErrorhere and raising aRuntimeErrorwith an explicit “kubectl not found in PATH” message to make setup issues easier to diagnose.plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (1)
652-672: Optional: validate TMPDIR before constructing output paths
main()checks that the kubeconfig file exists but assumes the provided TMPDIR is valid. Given the SKILL docs already require callers to usemkdtemp, this is usually fine, but you might optionally add a check thattmpdirexists and is a directory, and emit a clearer error if not, instead of failing later during file writes.
📜 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 (5)
docs/data.json(1 hunks)plugins/openshift/skills/generating-ovn-topology/SKILL.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/skills/generating-ovn-topology/SKILL.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
185-185: Table column count
Expected: 4; Actual: 8; Too many cells, extra data will be missing
(MD056, table-column-count)
🪛 Ruff (0.14.4)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py
204-204: subprocess call: check for execution of untrusted input
(S603)
205-208: Starting a process with a partial executable path
(S607)
229-229: Consider moving this statement to an else block
(TRY300)
244-244: Do not catch blind exception: Exception
(BLE001)
256-256: subprocess call: check for execution of untrusted input
(S603)
257-260: Starting a process with a partial executable path
(S607)
298-298: Consider moving this statement to an else block
(TRY300)
300-300: Do not catch blind exception: Exception
(BLE001)
309-309: subprocess call: check for execution of untrusted input
(S603)
310-314: Starting a process with a partial executable path
(S607)
323-323: Consider moving this statement to an else block
(TRY300)
325-325: Do not catch blind exception: Exception
(BLE001)
351-351: subprocess call: check for execution of untrusted input
(S603)
352-359: Starting a process with a partial executable path
(S607)
387-387: Consider moving this statement to an else block
(TRY300)
395-395: Do not catch blind exception: Exception
(BLE001)
543-543: Do not catch blind exception: Exception
(BLE001)
549-549: Use explicit conversion flag
Replace with conversion flag
(RUF010)
573-573: Do not catch blind exception: Exception
(BLE001)
623-623: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py
259-259: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py
1-1: Shebang is present but file is not executable
(EXE001)
45-45: subprocess call: check for execution of untrusted input
(S603)
46-53: Starting a process with a partial executable path
(S607)
61-61: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
68-68: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
79-94: Avoid specifying long messages outside the exception class
(TRY003)
108-108: subprocess call: check for execution of untrusted input
(S603)
109-116: Starting a process with a partial executable path
(S607)
126-126: subprocess call: check for execution of untrusted input
(S603)
127-134: Starting a process with a partial executable path
(S607)
182-182: subprocess call: check for execution of untrusted input
(S603)
183-188: Starting a process with a partial executable path
(S607)
195-198: Avoid specifying long messages outside the exception class
(TRY003)
210-210: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
229-248: Avoid specifying long messages outside the exception class
(TRY003)
251-254: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
251-254: Avoid specifying long messages outside the exception class
(TRY003)
256-258: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
256-258: Avoid specifying long messages outside the exception class
(TRY003)
275-275: subprocess call: check for execution of untrusted input
(S603)
276-283: Starting a process with a partial executable path
(S607)
287-287: Consider moving this statement to an else block
(TRY300)
305-307: Avoid specifying long messages outside the exception class
(TRY003)
325-327: Avoid specifying long messages outside the exception class
(TRY003)
344-346: Avoid specifying long messages outside the exception class
(TRY003)
363-365: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (8)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (1)
34-235: Placement analysis and override logic looks solidThe core flow (verify inputs → load detail files → classify UUID patterns → apply transit/join overrides → write placement files) is coherent and matches the documented behavior (per-node vs cluster-wide / cluster-wide-visual). Use of
safe_write_fileand clear stdout/stderr separation is good for both safety and UX.docs/data.json (1)
581-592: New command and skill registration look consistentThe
/openshift:visualize-ovn-topologycommand andgenerating-ovn-topologyskill entries are well-formed, argument_hint is correctly empty for a no-arg command, and the skill description here matches the currently implemented feature set.plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py (2)
18-151: Namespace discovery flow is robust and stderr-only for logsThe hybrid detection for the OVN namespace (common namespaces first, then label/pod-name search) plus clear RuntimeError messaging and stderr logging is solid and aligns with how the collectors consume it. Returning
Nonefrom_search_for_ovn_namespaceon failures keeps the behavior predictable for callers.
292-367: Symlink-safe file writes/appends look goodThe
safe_write_fileandsafe_append_filehelpers correctly defend against final-path symlink attacks usingos.path.lexists/os.path.islinkplusO_NOFOLLOW, and they enforce0o600permissions. This is a good fit with the “private TMPDIR” model described in the SKILL docs.plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (4)
82-117: Initialization now avoids work in__init__and uses safe TMPDIR outputsDeferring OVN namespace detection out of
__init__and constructing all output paths under the provided TMPDIR (then initializing viasafe_write_file) is a good pattern. It avoids constructor-time side effects and keeps all temporary artifacts in the caller-provided private directory.
187-303: Pod collection with JSON-first and wide-output fallback is well-handledThe JSON-first approach for
kubectl get podswith a structured fallback to-o widecovers most environments, and you correctly:
- Restrict to running pods with IPs.
- Populate
ovn_pods_detail.txtin the documentednamespace|pod|ip|nodeformat.- Auto-discover
ovnkube-nodepods with both label- and name-based checks (JSON path) and a simpler pattern in the fallback.
Errors are logged to stderr and you degrade gracefully to the fallback where possible.
328-401: OVN component querying and CSV parsing are correct and resilientThe
query_ovn_componenthelper builds thekubectl exec ... ovn-nbctl --format=csvcommand correctly, parses each CSV line withcsv.reader(fixing the earlier comma-in-field issue), prepends the node name, and appends viasafe_append_file. Timeouts and other errors are caught and reported to stderr, and the(success, count)contract is cleanly honored.
488-552: Node collection/architecture summary flow is coherent
collect_all_nodescorrectly:
- Fails fast when no
ovnkube-nodepods are discovered.- Detects the NBDB container once via
detect_ovsdb_container.- Prints an architecture summary including namespace, container, and node count.
- Iterates discovered pods, resolving node names and collecting per-node data with error tracking.
The partial-success semantics (tracking
nodes_attempted,nodes_successful,nodes_failed) match the later summary andis_collection_successful()decision.
mattedallo
force-pushed
the
add-visualize-ovn-topology
branch
from
4ad0f88 to
ae17477
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/openshift/skills/generating-ovn-topology/SKILL.md (1)
300-322: Align load balancer mentions with current implementation scope.The "Colors", "Required Info", and "Final Steps" sections still talk about load balancers and "LBs" in the summary, but the data collectors and placement analyzer only handle switches, routers, ports, and pods. Either wire in LB collection/rendering to match these docs, or (simpler) drop/soften the LB-specific requirements and summary text until support exists.
Also applies to: 397-399
🧹 Nitpick comments (1)
plugins/openshift/skills/generating-ovn-topology/SKILL.md (1)
24-28: Fix minor markdown issues (code fence language + helper table).Two small markdown nits:
- Line 25 sample output block uses a bare
) to satisfy MD040 and match other fenced blocks.</code>; add a language (e.g. <code>text- The helper scripts table row for
detect-cluster.shincludes unescaped|characters in the exampleindex|kubeconfig|cluster|nodes|namespace, which breaks the column count (MD056). Escape those pipes (e.g.index|kubeconfig|cluster|nodes|namespace) or rephrase the cell to avoid raw|so the table renders with four columns.Also applies to: 183-189
📜 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 (5)
docs/data.json(1 hunks)plugins/openshift/skills/generating-ovn-topology/SKILL.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/skills/generating-ovn-topology/SKILL.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
185-185: Table column count
Expected: 4; Actual: 8; Too many cells, extra data will be missing
(MD056, table-column-count)
🪛 Ruff (0.14.4)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py
259-259: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py
1-1: Shebang is present but file is not executable
(EXE001)
45-45: subprocess call: check for execution of untrusted input
(S603)
46-53: Starting a process with a partial executable path
(S607)
61-61: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
68-68: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
79-94: Avoid specifying long messages outside the exception class
(TRY003)
108-108: subprocess call: check for execution of untrusted input
(S603)
109-116: Starting a process with a partial executable path
(S607)
126-126: subprocess call: check for execution of untrusted input
(S603)
127-134: Starting a process with a partial executable path
(S607)
182-182: subprocess call: check for execution of untrusted input
(S603)
183-188: Starting a process with a partial executable path
(S607)
195-198: Avoid specifying long messages outside the exception class
(TRY003)
210-210: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
229-248: Avoid specifying long messages outside the exception class
(TRY003)
251-254: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
251-254: Avoid specifying long messages outside the exception class
(TRY003)
256-258: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
256-258: Avoid specifying long messages outside the exception class
(TRY003)
275-275: subprocess call: check for execution of untrusted input
(S603)
276-283: Starting a process with a partial executable path
(S607)
287-287: Consider moving this statement to an else block
(TRY300)
305-307: Avoid specifying long messages outside the exception class
(TRY003)
325-327: Avoid specifying long messages outside the exception class
(TRY003)
344-346: Avoid specifying long messages outside the exception class
(TRY003)
363-365: Avoid specifying long messages outside the exception class
(TRY003)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py
204-204: subprocess call: check for execution of untrusted input
(S603)
205-208: Starting a process with a partial executable path
(S607)
229-229: Consider moving this statement to an else block
(TRY300)
244-244: Do not catch blind exception: Exception
(BLE001)
256-256: subprocess call: check for execution of untrusted input
(S603)
257-260: Starting a process with a partial executable path
(S607)
298-298: Consider moving this statement to an else block
(TRY300)
300-300: Do not catch blind exception: Exception
(BLE001)
309-309: subprocess call: check for execution of untrusted input
(S603)
310-314: Starting a process with a partial executable path
(S607)
323-323: Consider moving this statement to an else block
(TRY300)
325-325: Do not catch blind exception: Exception
(BLE001)
351-351: subprocess call: check for execution of untrusted input
(S603)
352-359: Starting a process with a partial executable path
(S607)
387-387: Consider moving this statement to an else block
(TRY300)
395-395: Do not catch blind exception: Exception
(BLE001)
543-543: Do not catch blind exception: Exception
(BLE001)
549-549: Use explicit conversion flag
Replace with conversion flag
(RUF010)
573-573: Do not catch blind exception: Exception
(BLE001)
623-623: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (4)
docs/data.json (1)
575-592: Newvisualize-ovn-topologycommand and skill wiring looks consistent.The command definition and skill entry for
generating-ovn-topologyare well-formed, argument_hint is correctly empty for a no-arg command, and the skill description matches SKILL.md without over-claiming load balancer support.plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (1)
34-235: Placement analysis and safe output handling look solid.The UUID-based per-node vs cluster-wide classification, the explicit transit_switch visualization override, and the use of
safe_write_fileunder a per-run TMPDIR give a clear, deterministic placement model without the earlier/tmprisks. Input validation viaverify_input_files()and the summary reporting are also in good shape.plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py (1)
18-367: Shared OVN utilities are robust and security-conscious.Namespace/container detection and the safe file helpers look well thought out: two-phase discovery with clear RuntimeError messages for callers, all informational logging routed to stderr, and
safe_write_file/safe_append_fileusingO_NOFOLLOWplus explicit symlink checks to avoid CWE‑377/CWE‑59 style issues. This aligns nicely with the TMPDIR and read‑only rules in SKILL.md.plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (1)
65-649: OVN data collection pipeline and error handling look well-structured.The refactored
OVNDataCollectorcleanly sequences namespace detection, pod discovery, and per-node OVN queries, with proper use of TMPDIR plussafe_write_file/safe_append_file, robust CSV parsing, and per-node “graceful degradation” semantics. Broad exception handling is consistently logged to stderr and used only to decide between JSON vs fallback or node-level success vs failure, which is appropriate for this CLI-style tool.
mattedallo
force-pushed
the
add-visualize-ovn-topology
branch
from
ae17477 to
08280d4
Compare
- Update `collect_ovn_data.py` and `analyze_placement.py` to require a private temporary directory for file operations, improving security. - Implement safe file writing and appending methods in `ovn_utils.py` to prevent symlink attacks. - Refactor file handling to ensure all output is directed to the specified temporary directory. Signed-off-by: Matteo Dallaglio <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py (2)
1-8: Shebang on an imported utility module is likely unnecessaryThis file is primarily used as an importable helper module, not an executable script. You can either remove the shebang or mark the file executable (if you intend to run it directly) to silence EXE001 and clarify its role.
229-258: Preserve original exception context when wrapping subprocess errorsIn
detect_ovsdb_container, theTimeoutExpiredandSubprocessErrorbranches wrap the error in a newRuntimeErrorbut drop the original traceback. Using exception chaining would make failures easier to debug without changing behavior.Example:
- except subprocess.TimeoutExpired: - raise RuntimeError( - f"Timeout querying pod {sample_pod}. " - f"Check cluster connectivity." - ) - except subprocess.SubprocessError as e: - raise RuntimeError( - f"Error querying pod {sample_pod}: {e}" - ) + except subprocess.TimeoutExpired as e: + raise RuntimeError( + f"Timeout querying pod {sample_pod}. Check cluster connectivity." + ) from e + except subprocess.SubprocessError as e: + raise RuntimeError( + f"Error querying pod {sample_pod}: {e}" + ) from e
📜 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 (5)
docs/data.json(1 hunks)plugins/openshift/skills/generating-ovn-topology/SKILL.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/data.json
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/skills/generating-ovn-topology/SKILL.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
185-185: Table column count
Expected: 4; Actual: 8; Too many cells, extra data will be missing
(MD056, table-column-count)
🪛 Ruff (0.14.4)
plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py
1-1: Shebang is present but file is not executable
(EXE001)
45-45: subprocess call: check for execution of untrusted input
(S603)
46-53: Starting a process with a partial executable path
(S607)
61-61: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
68-68: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
79-94: Avoid specifying long messages outside the exception class
(TRY003)
108-108: subprocess call: check for execution of untrusted input
(S603)
109-116: Starting a process with a partial executable path
(S607)
126-126: subprocess call: check for execution of untrusted input
(S603)
127-134: Starting a process with a partial executable path
(S607)
182-182: subprocess call: check for execution of untrusted input
(S603)
183-188: Starting a process with a partial executable path
(S607)
195-198: Avoid specifying long messages outside the exception class
(TRY003)
210-210: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
229-248: Avoid specifying long messages outside the exception class
(TRY003)
251-254: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
251-254: Avoid specifying long messages outside the exception class
(TRY003)
256-258: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
256-258: Avoid specifying long messages outside the exception class
(TRY003)
275-275: subprocess call: check for execution of untrusted input
(S603)
276-283: Starting a process with a partial executable path
(S607)
287-287: Consider moving this statement to an else block
(TRY300)
305-307: Avoid specifying long messages outside the exception class
(TRY003)
325-327: Avoid specifying long messages outside the exception class
(TRY003)
344-346: Avoid specifying long messages outside the exception class
(TRY003)
363-365: Avoid specifying long messages outside the exception class
(TRY003)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py
204-204: subprocess call: check for execution of untrusted input
(S603)
205-208: Starting a process with a partial executable path
(S607)
229-229: Consider moving this statement to an else block
(TRY300)
244-244: Do not catch blind exception: Exception
(BLE001)
256-256: subprocess call: check for execution of untrusted input
(S603)
257-260: Starting a process with a partial executable path
(S607)
298-298: Consider moving this statement to an else block
(TRY300)
300-300: Do not catch blind exception: Exception
(BLE001)
309-309: subprocess call: check for execution of untrusted input
(S603)
310-314: Starting a process with a partial executable path
(S607)
323-323: Consider moving this statement to an else block
(TRY300)
325-325: Do not catch blind exception: Exception
(BLE001)
351-351: subprocess call: check for execution of untrusted input
(S603)
352-359: Starting a process with a partial executable path
(S607)
387-387: Consider moving this statement to an else block
(TRY300)
395-395: Do not catch blind exception: Exception
(BLE001)
543-543: Do not catch blind exception: Exception
(BLE001)
549-549: Use explicit conversion flag
Replace with conversion flag
(RUF010)
573-573: Do not catch blind exception: Exception
(BLE001)
623-623: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py
259-259: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (1)
328-401: OVN component querying and CSV handling look solidThe
query_ovn_componentimplementation correctly:
- Uses
env+--kubeconfig(no shell),- Parses CSV via
csv.readerto handle commas safely, and- Appends via
safe_append_fileto benefit from the symlink protections inovn_utils.This is a good balance of robustness and safety for the data collectors.
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (1)
135-175: Placement classification and transit_switch override logic are well-structuredThe UUID-based placement rules (
unique_count == 1 && total_count > 1 → cluster-wide, otherwise per-node) plus the explicittransit_switch → cluster-wide-visualoverride match the documented behavior and are easy to reason about. Logging per-component decisions is also helpful for debugging.No changes needed here.
Also applies to: 177-217
| def __init__(self, tmpdir: str): | ||
| """Initialize PlacementAnalyzer with a private temporary directory. | ||
| Args: | ||
| tmpdir: Private temporary directory path. | ||
| """ | ||
|
|
||
| # Input files (with UUIDs) | ||
| self.switches_file = os.path.join(tmpdir, self.SWITCHES_DETAIL_FILE) | ||
| self.routers_file = os.path.join(tmpdir, self.ROUTERS_DETAIL_FILE) | ||
|
|
||
| # Output files (placement analysis only) | ||
| self.switch_placement_file = os.path.join(tmpdir, self.SWITCH_PLACEMENT_FILE) | ||
| self.router_placement_file = os.path.join(tmpdir, self.ROUTER_PLACEMENT_FILE) | ||
|
|
There was a problem hiding this comment.
Guard against invalid TMPDIR to avoid uncaught file I/O errors
PlacementAnalyzer assumes tmpdir exists and is writable. If the path is wrong, safe_write_file (or the initial open calls in load_data) will raise OSError, yielding a traceback instead of a clean status code.
Mirror the collector’s behavior by validating tmpdir in main():
def main():
@@
- if len(sys.argv) < 2:
+ if len(sys.argv) < 2:
print(
"Usage: analyze_placement.py TMPDIR",
file=sys.stderr,
)
return 1
- tmpdir = sys.argv[1]
-
- analyzer = PlacementAnalyzer(tmpdir)
- return analyzer.run()
+ tmpdir = sys.argv[1]
+
+ if not os.path.isdir(tmpdir):
+ print(
+ f"❌ Error: TMPDIR is not a directory or does not exist: {tmpdir}",
+ file=sys.stderr,
+ )
+ return 1
+
+ analyzer = PlacementAnalyzer(tmpdir)
+ try:
+ return analyzer.run()
+ except OSError as exc:
+ print(f"❌ Error reading/writing placement files: {exc}", file=sys.stderr)
+ return 1This keeps the analysis step failure modes predictable and user-friendly.
Also applies to: 302-315
🤖 Prompt for AI Agents
In plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py
around lines 52-66 (and also apply same guards at 302-315), the constructor
assumes tmpdir exists and is writable causing uncaught OSError on file I/O;
update main() to validate tmpdir before instantiating PlacementAnalyzer by
checking that the path exists and is a directory and that it is writable (or
attempt to create it if the collector allows), and if the check fails log/print
a clear error and exit with a non-zero status code so downstream callers get a
predictable failure mode rather than a traceback.
| def main(): | ||
| """Main entry point.""" | ||
| if len(sys.argv) != 3: | ||
| print( | ||
| f"Usage: {sys.argv[0]} <KUBECONFIG> <TMPDIR>", | ||
| file=sys.stderr, | ||
| ) | ||
| return 1 | ||
|
|
||
| kubeconfig = sys.argv[1] | ||
| tmpdir = sys.argv[2] | ||
|
|
||
| if not os.path.exists(kubeconfig): | ||
| print( | ||
| f"❌ Error: Kubeconfig not found: {kubeconfig}", | ||
| file=sys.stderr, | ||
| ) | ||
| return 1 | ||
|
|
||
| collector = OVNDataCollector(kubeconfig, tmpdir) | ||
| return collector.run() | ||
|
|
There was a problem hiding this comment.
Validate TMPDIR and handle directory errors before initializing output files
main() verifies the kubeconfig path but not TMPDIR. If a caller passes a non-existent or non-directory path, initialize_output_files() → safe_write_file() will raise OSError, causing an uncaught traceback instead of a clean, user-friendly error and exit code.
Consider validating tmpdir up front and failing gracefully:
def main():
@@
- kubeconfig = sys.argv[1]
- tmpdir = sys.argv[2]
-
- if not os.path.exists(kubeconfig):
+ kubeconfig = sys.argv[1]
+ tmpdir = sys.argv[2]
+
+ if not os.path.exists(kubeconfig):
print(
f"❌ Error: Kubeconfig not found: {kubeconfig}",
file=sys.stderr,
)
return 1
+
+ if not os.path.isdir(tmpdir):
+ print(
+ f"❌ Error: TMPDIR is not a directory or does not exist: {tmpdir}",
+ file=sys.stderr,
+ )
+ return 1
@@
- collector = OVNDataCollector(kubeconfig, tmpdir)
- return collector.run()
+ collector = OVNDataCollector(kubeconfig, tmpdir)
+ try:
+ return collector.run()
+ except OSError as exc:
+ print(f"❌ Error writing output files: {exc}", file=sys.stderr)
+ return 1This keeps the UX consistent with the improved namespace-detection error handling.
Also applies to: 106-117
| The script returns pipe-delimited lines to stdout, one per cluster found, e.g.: | ||
| ``` | ||
| 1|/home/user/.kube/kind-config|kind-ovn|3|ovn-kubernetes | ||
| 2|/home/user/.kube/config|prod-cluster|12|openshift-ovn-kubernetes | ||
| ``` |
There was a problem hiding this comment.
Tighten Markdown formatting for code fence and helper scripts table
Two small markdown issues that affect rendering:
- The example cluster-detection output block (Lines 24–28) lacks a language hint. Add one (e.g.,
text) to satisfy MD040 and improve readability. - In the helper scripts table, the inline code
index|kubeconfig|cluster|nodes|namespaceis parsed as extra columns. Escape the pipes (\|) or use|so the table remains 4 columns wide.
For example:
- ```
+ ```text
1|/home/user/.kube/kind-config|kind-ovn|3|ovn-kubernetes
2|/home/user/.kube/config|prod-cluster|12|openshift-ovn-kubernetes
```-| [detect-cluster.sh](scripts/detect-cluster.sh) | ... | None | Parseable list to stdout: `index|kubeconfig|cluster|nodes|namespace`. Exit: 0=success, 1=none found |
+| [detect-cluster.sh](scripts/detect-cluster.sh) | ... | None | Parseable list to stdout: `index\|kubeconfig\|cluster\|nodes\|namespace`. Exit: 0=success, 1=none found |Also applies to: 183-188
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In plugins/openshift/skills/generating-ovn-topology/SKILL.md around lines 24–28
and also apply the change to lines 183–188: the fenced example blocks are
missing a language hint and the inline code containing pipe-delimited columns is
being parsed as table columns; add a language hint (e.g., ```text) to the fenced
code blocks and escape pipes inside inline code (replace | with \| or use
|) in the helper scripts table so the code blocks and table render
correctly.
| 6. **Colors** (apply using classDef with **color:#000** for black text): | ||
| - Pods: `fill:#e8f5e9,stroke:#2e7d32,stroke-width:2px,color:#000` | ||
| - Switches: `fill:#fff3e0,stroke:#e65100,stroke-width:2px,color:#000` | ||
| - Routers: `fill:#fff9c4,stroke:#f57f17,stroke-width:2px,color:#000` | ||
| - LSPs (ALL types): `fill:#e3f2fd,stroke:#1565c0,stroke-width:2px,color:#000` | ||
| - LRPs: `fill:#f3e5f5,stroke:#6a1b9a,stroke-width:2px,color:#000` | ||
| - Load Balancers: `fill:#ffebee,stroke:#c62828,stroke-width:2px,color:#000` | ||
| - External Network: `fill:#e0e0e0,stroke:#424242,stroke-width:2px,color:#000` |
There was a problem hiding this comment.
Align load-balancer mentions with current implementation scope
The skill still references load balancers in the colors list, “Required Info”, and the final summary, but the described data collectors only emit switches, routers, ports, and pods. This can mislead users into expecting LB coverage that isn’t there yet.
Either:
- Drop/soften the LB bullets for now (and remove
LBsfrom the summary), or - Add explicit LB collection/rendering steps to match the text.
For example, to downscope:
- - Load Balancers: `fill:#ffebee,stroke:#c62828,stroke-width:2px,color:#000`
+ <!-- Load balancer styling reserved for future extension -->- - Load Balancers: Name, VIP:port, Backends
+ <!-- Load balancer details can be added when LB collection is implemented -->-3. Show summary: nodes, switches, routers, ports, LBs, mode
+3. Show summary: nodes, switches, routers, ports, modeAlso applies to: 315-323, 397-400
🤖 Prompt for AI Agents
In plugins/openshift/skills/generating-ovn-topology/SKILL.md around lines
300-307 (and similarly 315-323, 397-400), the doc mentions Load Balancers
(colors, required info, final summary) even though current collectors only emit
switches, routers, ports, and pods; update the file to align scope by either
removing or softening LB references: remove LB entries from the colors list and
summary and any "Required Info" LB items, or explicitly add the LB
collection/rendering steps to the code/text so the documentation matches
implementation; ensure all three affected ranges are updated consistently.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (1)
302-314: Validatetmpdirbefore instantiatingPlacementAnalyzer.The
main()function should verify thattmpdirexists and is a directory before passing it toPlacementAnalyzer, to provide clear error messages if the path is invalid. Without this check, the script will fail later with confusing file-not-found errors.Apply this diff:
def main(): """Main entry point.""" if len(sys.argv) < 2: print( "Usage: analyze_placement.py TMPDIR", file=sys.stderr, ) return 1 tmpdir = sys.argv[1] + + if not os.path.isdir(tmpdir): + print( + f"❌ Error: TMPDIR does not exist or is not a directory: {tmpdir}", + file=sys.stderr, + ) + return 1 analyzer = PlacementAnalyzer(tmpdir) return analyzer.run()plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (1)
652-673: Validate TMPDIR and handle file operation errors.The past review comment on lines 652-673 requested tmpdir validation and OSError handling, but this was not addressed. If
tmpdirdoesn't exist or isn't a directory,initialize_output_files()→safe_write_file()will raise an uncaughtOSError, resulting in a traceback instead of a user-friendly error message.Apply this diff to add the requested validation and error handling:
if not os.path.exists(kubeconfig): print( f"❌ Error: Kubeconfig not found: {kubeconfig}", file=sys.stderr, ) return 1 + if not os.path.isdir(tmpdir): + print( + f"❌ Error: TMPDIR is not a directory or does not exist: {tmpdir}", + file=sys.stderr, + ) + return 1 + collector = OVNDataCollector(kubeconfig, tmpdir) - return collector.run() + try: + return collector.run() + except OSError as exc: + print(f"❌ Error writing output files: {exc}", file=sys.stderr) + return 1This maintains consistency with the kubeconfig validation and ensures graceful error handling throughout.
🧹 Nitpick comments (3)
plugins/openshift/commands/visualize-ovn-topology.md (1)
12-14: Add language hint to synopsis code block.The fenced code block at line 12 should specify a language (e.g.,
shellorbash) for proper rendering and to satisfy markdown linting rules.Apply this diff:
## Synopsis -``` +```shell /openshift:visualize-ovn-topology</blockquote></details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/README.md (1)</summary><blockquote> `25-31`: **Add language hint to code block.** The fenced code block should specify a language hint (e.g., `shell` or `text`) for proper rendering. Apply this diff: ```diff Simply run the command - it will automatically detect your cluster: -``` +```shell /openshift:visualize-ovn-topology</blockquote></details> <details> <summary>plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py (1)</summary><blockquote> `1-1`: **Consider making the file executable.** The file has a shebang (`#!/usr/bin/env python3`) but is not marked as executable. While this module is primarily imported rather than executed directly, adding the executable bit would be consistent with the shebang. You can fix this with: ```shell chmod +x plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.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 (10)
PLUGINS.md(1 hunks)docs/data.json(1 hunks)plugins/openshift/commands/visualize-ovn-topology.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/README.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/SKILL.md(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh(1 hunks)plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/commands/visualize-ovn-topology.md
12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/openshift/skills/generating-ovn-topology/README.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
185-185: Table column count
Expected: 4; Actual: 8; Too many cells, extra data will be missing
(MD056, table-column-count)
🪛 Ruff (0.14.4)
plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py
55-55: subprocess call: check for execution of untrusted input
(S603)
56-63: Starting a process with a partial executable path
(S607)
68-68: Consider moving this statement to an else block
(TRY300)
110-110: subprocess call: check for execution of untrusted input
(S603)
116-116: Consider moving this statement to an else block
(TRY300)
124-124: Do not catch blind exception: Exception
(BLE001)
206-206: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py
259-259: Do not catch blind exception: Exception
(BLE001)
plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py
1-1: Shebang is present but file is not executable
(EXE001)
45-45: subprocess call: check for execution of untrusted input
(S603)
46-53: Starting a process with a partial executable path
(S607)
61-61: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
68-68: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
79-94: Avoid specifying long messages outside the exception class
(TRY003)
108-108: subprocess call: check for execution of untrusted input
(S603)
109-116: Starting a process with a partial executable path
(S607)
126-126: subprocess call: check for execution of untrusted input
(S603)
127-134: Starting a process with a partial executable path
(S607)
182-182: subprocess call: check for execution of untrusted input
(S603)
183-188: Starting a process with a partial executable path
(S607)
195-198: Avoid specifying long messages outside the exception class
(TRY003)
210-210: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
229-248: Avoid specifying long messages outside the exception class
(TRY003)
251-254: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
251-254: Avoid specifying long messages outside the exception class
(TRY003)
256-258: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
256-258: Avoid specifying long messages outside the exception class
(TRY003)
275-275: subprocess call: check for execution of untrusted input
(S603)
276-283: Starting a process with a partial executable path
(S607)
287-287: Consider moving this statement to an else block
(TRY300)
305-307: Avoid specifying long messages outside the exception class
(TRY003)
325-327: Avoid specifying long messages outside the exception class
(TRY003)
344-346: Avoid specifying long messages outside the exception class
(TRY003)
363-365: Avoid specifying long messages outside the exception class
(TRY003)
plugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py
204-204: subprocess call: check for execution of untrusted input
(S603)
205-208: Starting a process with a partial executable path
(S607)
229-229: Consider moving this statement to an else block
(TRY300)
244-244: Do not catch blind exception: Exception
(BLE001)
256-256: subprocess call: check for execution of untrusted input
(S603)
257-260: Starting a process with a partial executable path
(S607)
298-298: Consider moving this statement to an else block
(TRY300)
300-300: Do not catch blind exception: Exception
(BLE001)
309-309: subprocess call: check for execution of untrusted input
(S603)
310-314: Starting a process with a partial executable path
(S607)
323-323: Consider moving this statement to an else block
(TRY300)
325-325: Do not catch blind exception: Exception
(BLE001)
351-351: subprocess call: check for execution of untrusted input
(S603)
352-359: Starting a process with a partial executable path
(S607)
387-387: Consider moving this statement to an else block
(TRY300)
395-395: Do not catch blind exception: Exception
(BLE001)
543-543: Do not catch blind exception: Exception
(BLE001)
549-549: Use explicit conversion flag
Replace with conversion flag
(RUF010)
573-573: Do not catch blind exception: Exception
(BLE001)
623-623: Do not catch blind exception: Exception
(BLE001)
🪛 Shellcheck (0.11.0)
plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh
[warning] 131-131: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 136-136: Tilde does not expand in quotes. Use $HOME.
(SC2088)
[warning] 141-141: Tilde does not expand in quotes. Use $HOME.
(SC2088)
🔇 Additional comments (19)
plugins/openshift/skills/generating-ovn-topology/scripts/detect-cluster.sh (1)
1-189: LGTM! Cluster discovery implementation is solid.The script correctly implements multi-kubeconfig and multi-context discovery with proper deduplication, handles the colon-delimited
$KUBECONFIGvariable with tilde expansion, and emits parseable output. All previous review comments have been addressed.Note: The Shellcheck warnings about tildes in quotes (lines 131, 136, 141) are false positives—those strings appear only in diagnostic echo statements showing what was searched, not in actual file operations where
$HOMEis correctly used.plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py (1)
52-300: Placement analysis logic is well-structured.The
PlacementAnalyzerclass correctly loads data, analyzes UUID patterns, applies visualization overrides fortransit_switch, and writes placement files usingsafe_write_file. The use of a bareExceptioncatch at line 259 in thecount_lineshelper is acceptable for a non-critical count operation.PLUGINS.md (1)
164-164: LGTM! Command entry correctly added.The new
/openshift:visualize-ovn-topologyentry is properly formatted and consistent with other commands in the OpenShift plugin section.plugins/openshift/commands/visualize-ovn-topology.md (1)
1-121: Comprehensive and accurate command documentation.The documentation clearly describes the command's purpose, implementation, security guarantees, and usage. The read-only operation emphasis and permission check workflow are well explained. Previous concerns about load balancer mentions have been addressed.
plugins/openshift/skills/generating-ovn-topology/README.md (1)
1-359: Excellent skill documentation.The README provides comprehensive coverage of the OVN topology visualization feature, including prerequisites, usage, security considerations, troubleshooting, and FAQs. The structure is clear and user-friendly.
docs/data.json (1)
579-592: LGTM! Command and skill entries are correctly defined.The new
visualize-ovn-topologycommand andgenerating-ovn-topologyskill entries are properly formatted with correct field values. Previous concerns aboutargument_hintand load balancer mentions have been addressed.plugins/openshift/skills/generating-ovn-topology/SKILL.md (1)
1-404: Comprehensive skill documentation for AI agents.The SKILL.md provides detailed step-by-step instructions for generating OVN topology diagrams, including cluster detection, permission checks, data collection, placement analysis, and diagram generation rules. All previous concerns about
/tmpreferences and load balancer mentions have been addressed. The architecture concepts and pod discovery logic are well explained.Note: The markdown formatting issues flagged by static analysis (missing language hint at line 25, pipe characters in table at line 185) appear to have been addressed in previous commits based on the past review comments.
plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py (1)
1-231: LGTM! Permission checking implementation is solid.The script correctly defers namespace detection until after kubectl availability check (addressing the previous review comment), uses an immutable module-level constant for dangerous permissions, and handles errors gracefully with appropriate exit codes. The structure supports both interactive and non-interactive modes.
Static analysis warnings about subprocess security and exception handling are acceptable in this context:
subprocesscalls are for the expectedkubectltool, not untrusted input- Broad exception catches at lines 124 and 206 provide graceful degradation for permission checks and namespace detection
- The
TRY300warnings about else blocks are false positives—the code correctly returnsTrueon the success pathplugins/openshift/skills/generating-ovn-topology/scripts/collect_ovn_data.py (6)
1-63: LGTM - Well-structured imports and constants.The file header is clear, imports are appropriate, and module-scoped constants follow good naming conventions. The addition of
csvandStringIOimports supports the CSV parsing fix from previous reviews.
65-105: LGTM - Proper initialization with deferred namespace detection.The dataclass for stats tracking is well-structured, and the
__init__method correctly defers OVN namespace detection torun()(Line 97:self.ovn_namespace: Optional[str] = None), as addressed from previous reviews.
328-401: LGTM - Proper CSV parsing handles commas in fields.The CSV parsing fix (lines 377-382) correctly uses
csv.reader(StringIO(line))to handle quoted fields containing commas, addressing the critical issue from previous reviews. Error handling with timeouts and exceptions is appropriate.
613-650: LGTM - Proper namespace detection with graceful error handling.The namespace detection is correctly deferred to
run()(lines 620-625) with appropriate exception handling and user-friendly error messages, as addressed from previous reviews. The workflow orchestration is well-structured.
187-303: LGTM - Robust pod collection with fallback.The pod collection logic demonstrates good error handling with a JSON-first approach and fallback method. Timeout handling and graceful degradation are well-implemented.
402-612: LGTM - Well-orchestrated data collection with helpful feedback.The node data collection workflow is well-structured with proper error handling, graceful degradation for partial failures, and helpful summary output including troubleshooting tips. The architecture detection summary (lines 511-521) is particularly useful.
plugins/openshift/skills/generating-ovn-topology/scripts/ovn_utils.py (5)
1-16: LGTM - Clear module documentation and complete imports.The module header clearly describes the shared utilities purpose, and all necessary imports are present, including
sysfor stderr redirection to support machine-readable output.
18-95: LGTM - Namespace detection with proper stderr redirection.The namespace detection correctly redirects informational messages to stderr (lines 61, 68, 71-74), addressing the previous review concern about machine-readable output compatibility. The two-phase detection strategy and comprehensive error messages with troubleshooting steps provide excellent user experience.
97-151: LGTM - Robust namespace search with fallback strategies.The helper function implements a thorough search with label-based queries and fallback to name prefix matching. Error handling appropriately catches timeouts and JSON decode errors for graceful degradation.
153-290: LGTM - Container detection with proper stderr redirection.The OVSDB container detection correctly redirects status messages to stderr (lines 205, 209-212, 216-225), addressing the previous review concern. The two-phase detection strategy with tool availability verification and comprehensive error messages is well-implemented.
292-367: LGTM - Security-focused file operations prevent symlink attacks.Both
safe_write_fileandsafe_append_fileimplement robust security measures usingO_NOFOLLOW, explicit symlink checks, and appropriate file permissions (0o600). This prevents CWE-377/CWE-59 vulnerabilities and handles ELOOP errors gracefully.
4 participants
What this PR does / why we need it:
This PR adds a new command
/openshift:visualize-ovn-topologyto theopenshiftplugin that automatically generates comprehensive Mermaid diagrams of OVN-Kubernetes OVN network topology for running clusters.Key Features:
Implementation Highlights:
plugins/openshift/commands/visualize-ovn-topology.mdplugins/openshift/skills/generating-ovn-topology/with:collect-ovn-data.py: Comprehensive data collection from OVN northbound databasesanalyze-placement.py: Architecture detection and component placement analysischeck-permissions.py: Kubernetes permission verification with safety warningsdetect-cluster.sh: Cluster detection across kubeconfig contextsThis command addresses the need for visualizing complex OVN-Kubernetes network topologies, which is essential for debugging network issues, understanding cluster architecture. The data-driven approach ensures accuracy by querying real cluster state rather than generating synthetic diagrams.
Special notes for your reviewer:
Commit Organization:
The PR is organized into logical commits:
Future Improvements
Potential enhancements for future iterations:
Checklist:
Assisted-by: Claude [email protected]
Summary by CodeRabbit
New Features
Documentation