fix(openshift): address PR review comments and improve robustness

- 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]>

Author: mattedallo

plugins/openshift/commands/visualize-ovn-topology.md

@@ -19,7 +19,6 @@ The `openshift:visualize-ovn-topology` command generates a comprehensive Mermaid
 
 - Logical switches and routers
 - Switch and router ports with MAC/IP addresses
-- Load balancers and their VIPs
 - Pod connectivity
 - External network connections
 - Per-node component placement (interconnect mode) or centralized components (default mode)
@@ -43,7 +42,7 @@ This command invokes the `generating-ovn-topology` skill which implements a data
 **Key Features:**
 - **Data-driven**: Never generates synthetic data - always queries real cluster
 - **Architecture-aware**: Correctly handles both interconnect and default deployment modes
-- **Complete topology**: Shows all logical switches, routers, ports, and load balancers
+- **Complete topology**: Shows all logical switches, routers, and ports
 - **Visual clarity**: Uses color-coded components and node subgraphs for organization
 
 **Skill Reference:**
@@ -59,12 +58,12 @@ This command invokes the `generating-ovn-topology` skill which implements a data
 ## Examples
 
 1. **Basic usage** (generates topology for detected cluster):
-   ```
+   ```shell
    /openshift:visualize-ovn-topology
    ```
 
    Output:
-   ```
+   ```text
    ✓ Successfully generated OVN-Kubernetes topology diagram
 
    📄 Diagram saved to: ovn-topology-diagram.md
@@ -73,19 +72,19 @@ This command invokes the `generating-ovn-topology` skill which implements a data
    - 3 nodes (ovn-control-plane, ovn-worker, ovn-worker2)
    - 10 logical switches, 4 logical routers
    - 27 logical switch ports, 13 logical router ports
-   - 9 running pods, 4 load balancers
+   - 9 running pods
    - Mode: Interconnect (distributed control plane)
 
    💡 Open the file in your IDE to view the full rendered Mermaid diagram!
    ```
 
 2. **With existing file** (prompts for action):
-   ```
+   ```shell
    /openshift:visualize-ovn-topology
    ```
 
    You'll be asked:
-   ```
+   ```text
    File ovn-topology-diagram.md already exists. Would you like to:
    (1) Overwrite it
    (2) Save to a different location

plugins/openshift/skills/generating-ovn-topology/README.md

@@ -112,7 +112,7 @@ This ensures **informed consent** - you're always aware when admin credentials a
 
 ### Generated Diagram
 
-The command creates a Mermaid diagram in markdown format:
+The command creates a Mermaid diagram in Markdown format:
 
 ```markdown
 # OVN-Kubernetes Network Topology

plugins/openshift/skills/generating-ovn-topology/SKILL.md

@@ -10,20 +10,41 @@ tools: [Bash, Read, Write]
 
 1. **Detect Cluster**: Find the OVN-Kubernetes cluster kubeconfig
 
-   Run: `KC=$(scripts/detect-cluster.sh)`
+   Run: `scripts/detect-cluster.sh 2>/dev/null`
 
-   The script intelligently discovers OVN-Kubernetes clusters:
+   The script discovers OVN-Kubernetes clusters:
    - Scans all kubeconfig files: current KUBECONFIG env, ~/.kube/kind-config, ~/ovn.conf, ~/.kube/config
    - Tests ALL contexts in each kubeconfig (not just current-context)
-   - If one cluster found: automatically selects it
-   - If multiple clusters found: prompts user to choose
-   - If selected context isn't current-context: automatically switches it
-   - Returns kubeconfig path on stdout, diagnostics on stderr
-   - Exit codes: 0=success, 1=no cluster found, 2=user cancelled
+   - Returns parseable list to stdout: `index|kubeconfig|cluster_name|node_count|namespace`
+   - Diagnostics go to stderr
+   - Exit code: 0=success, 1=no clusters found
+
+   **How to handle the output:**
+
+   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
+   ```
+
+   **Decision logic:**
+   - If **one cluster** found → automatically use it (extract kubeconfig path from column 2)
+   - If **multiple clusters** found → show the list to user and ask them to choose by number
+   - After selection, extract the kubeconfig path from column 2 of the chosen line
+   - Store the selected kubeconfig path in variable `KC` for use in subsequent steps
+
+   **Example output format parsing:**
+   - Column 1: Index number (for user selection)
+   - Column 2: Kubeconfig file path (this is what you need for `$KC`)
+   - Column 3: Cluster display name
+   - Column 4: Number of nodes
+   - Column 5: OVN namespace name
+
+   **Important**: Parse the output using standard text processing. The exact implementation is up to you - use whatever approach works best (awk, Python, inline parsing, etc.).
 
 2. **Check Permissions**: Verify user's Kubernetes access level and inform about write permissions
 
-   Run: `scripts/check-permissions.py "$KC"`
+   Run: `scripts/check_permissions.py "$KC"`
 
    The script returns:
    - **Exit 0**: Read-only access or user confirmed → proceed
@@ -43,7 +64,7 @@ tools: [Bash, Read, Write]
    5. If user says yes → continue, if no → stop
 
    **Example of proper user communication:**
-   ```
+   ```text
    ⚠️  WARNING: Write Permissions Detected
 
    Your kubeconfig has cluster admin permissions:
@@ -72,7 +93,7 @@ tools: [Bash, Read, Write]
 
 4. **Collect OVN Data**: Get full topology data from the cluster
 
-   Run: `scripts/collect-ovn-data.py "$KC"`
+   Run: `scripts/collect_ovn_data.py "$KC"`
 
    Detail files written to `$TMPDIR`:
    - `ovn_switches_detail.txt` - node|uuid|name|other_config
@@ -83,7 +104,7 @@ tools: [Bash, Read, Write]
 
 5. **Analyze Placement**: Determine per-node vs cluster-wide components
 
-   Run: `scripts/analyze-placement.py "$TMPDIR"`
+   Run: `scripts/analyze_placement.py "$TMPDIR"`
 
    Placement results written to `$TMPDIR`:
    - `ovn_switch_placement.txt` - name|placement (per-node|cluster-wide|cluster-wide-visual)
@@ -150,14 +171,14 @@ In **interconnect mode**, each node runs its own NBDB with local copies of compo
 
 ## Helper Scripts
 
-All helper scripts are in the `scripts/` directory. 
+All helper scripts are in the `scripts/` directory.
 
 | Script | Purpose | Input | Output |
 |--------|---------|-------|--------|
-| [detect-cluster.sh](scripts/detect-cluster.sh) | Find OVN cluster kubeconfig across all contexts. Scans multiple kubeconfig files and all their contexts. Prompts user if multiple clusters found. | None | Kubeconfig path to stdout, diagnostics to stderr. Exit: 0=success, 1=none found, 2=cancelled |
-| [check-permissions.py](scripts/check-permissions.py) | Check user permissions and warn if write access detected. | KUBECONFIG path | Exit: 0=proceed, 1=cancelled/error, 2=write perms (needs user confirmation) |
-| [collect-ovn-data.py](scripts/collect-ovn-data.py) | **Data collector**: Queries each node for all data, with **graceful degradation** (continues on node failures). Writes detail files. | KUBECONFIG path | Detail files: `ovn_switches_detail.txt`, `ovn_routers_detail.txt`, `ovn_lsps_detail.txt`, `ovn_lrps_detail.txt`, `ovn_pods_detail.txt` |
-| [analyze-placement.py](scripts/analyze-placement.py) | **Placement analyzer**: Analyzes UUID patterns from detail files to determine per-node vs cluster-wide placement. | TMPDIR (reads detail files) | Placement files: `ovn_switch_placement.txt`, `ovn_router_placement.txt` |
+| [detect-cluster.sh](scripts/detect-cluster.sh) | Find OVN cluster kubeconfig across all contexts. Scans multiple kubeconfig files and all their contexts. Returns parseable list. | None | Parseable list to stdout: `index|kubeconfig|cluster|nodes|namespace`. Exit: 0=success, 1=none found |
+| [check_permissions.py](scripts/check_permissions.py) | Check user permissions and warn if write access detected. | KUBECONFIG path | Exit: 0=proceed, 1=cancelled/error, 2=write perms (needs user confirmation) |
+| [collect_ovn_data.py](scripts/collect_ovn_data.py) | **Data collector**: Queries each node for all data, with **graceful degradation** (continues on node failures). Writes detail files. | KUBECONFIG path | Detail files: `ovn_switches_detail.txt`, `ovn_routers_detail.txt`, `ovn_lsps_detail.txt`, `ovn_lrps_detail.txt`, `ovn_pods_detail.txt` |
+| [analyze_placement.py](scripts/analyze_placement.py) | **Placement analyzer**: Analyzes UUID patterns from detail files to determine per-node vs cluster-wide placement. | TMPDIR (reads detail files) | Placement files: `ovn_switch_placement.txt`, `ovn_router_placement.txt` |
 
 ---
 
@@ -301,7 +322,7 @@ graph BT
 
 **CRITICAL: Discover pods from `$TMPDIR/ovn_pods_detail.txt`, NOT from LSPs**
 
-The file `$TMPDIR/ovn_pods_detail.txt` contains ALL running pods in the cluster (populated by collect-ovn-data.py).
+The file `$TMPDIR/ovn_pods_detail.txt` contains ALL running pods in the cluster (populated by collect_ovn_data.py).
 Format: `namespace|pod_name|pod_ip|node_name`
 
 **Pod-to-LSP Mapping Logic:**
@@ -332,7 +353,8 @@ Format: `namespace|pod_name|pod_ip|node_name`
 - `breth0_{node}`: LocalNet port (type="localnet")
 - `jtor-*`, `etor-*`, `tstor-*`: Router ports (type="router")
 
-**Example: Control Plane Node with Host-Network Pods**
+### Example: Control Plane Node with Host-Network Pods
+
 ```mermaid
 %% 8 host-network pods sharing management port
 POD_etcd["Pod: etcd-ovn-control-plane<br/>Namespace: kube-system<br/>IP: 10.89.0.10"]

plugins/openshift/skills/generating-ovn-topology/scripts/analyze-placement.py renamed to plugins/openshift/skills/generating-ovn-topology/scripts/analyze_placement.py

@@ -1,8 +1,8 @@
 #!/usr/bin/env python3
 """
-analyze-placement.py - Analyze OVN component placement from collected data
+analyze_placement.py - Analyze OVN component placement from collected data
 
-Usage: analyze-placement.py [TMPDIR]
+Usage: analyze_placement.py [TMPDIR]
 
 This script analyzes UUID patterns from previously collected OVN data to determine
 component placement (per-node vs cluster-wide).
@@ -18,6 +18,8 @@
 Exit codes:
   0 - Success
   1 - Missing input files or analysis failed
+
+Requirements: Python 3.6+
 """
 
 import os
@@ -71,7 +73,7 @@ def verify_input_files(self) -> bool:
                 file=sys.stderr,
             )
             print(
-                "   Run collect-ovn-data.py first to gather data.",
+                "   Run collect_ovn_data.py first to gather data.",
                 file=sys.stderr,
             )
             return False
@@ -82,7 +84,7 @@ def verify_input_files(self) -> bool:
                 file=sys.stderr,
             )
             print(
-                "   Run collect-ovn-data.py first to gather data.",
+                "   Run collect_ovn_data.py first to gather data.",
                 file=sys.stderr,
             )
             return False

plugins/openshift/skills/generating-ovn-topology/scripts/check-permissions.py renamed to plugins/openshift/skills/generating-ovn-topology/scripts/check_permissions.py

@@ -1,24 +1,30 @@
 #!/usr/bin/env python3
 """
-check-permissions.py - Check user permissions and warn if write access detected
+check_permissions.py - Check user permissions and warn if write access detected
 
-Usage: ./check-permissions.py KUBECONFIG
+Usage: ./check_permissions.py KUBECONFIG
 Returns: 0 if user confirms to proceed, 1 if user cancels or error, 2 if write perms detected
 
+Note: This script must be run from the scripts/ directory or have the scripts/
+      directory in PYTHONPATH for the ovn_utils import to work.
+
+Requirements: Python 3.6+, kubectl in PATH
 """
 
 import subprocess
 import sys
 import os
 from typing import List, Optional
 
+# Import shared utilities (must be in same directory or in PYTHONPATH)
+from ovn_utils import detect_ovn_namespace
+
 
 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 = [
@@ -38,10 +44,14 @@ def __init__(self, kubeconfig: str):
         self.kubeconfig = kubeconfig
         self.write_perms_found = False
         self.write_perms_list: List[str] = []
+        self.ovn_namespace: Optional[str] = None
+
 
     def check_kubectl_available(self) -> bool:
         """Check if kubectl is available in PATH."""
         try:
+            env = os.environ.copy()
+            env["KUBECONFIG"] = self.kubeconfig
             subprocess.run(
                 [
                     "kubectl",
@@ -53,7 +63,7 @@ def check_kubectl_available(self) -> bool:
                 ],
                 capture_output=True,
                 check=True,
-                env={"KUBECONFIG": self.kubeconfig},
+                env=env,
             )
             return True
         except FileNotFoundError:
@@ -95,11 +105,13 @@ def check_permission(
             cmd.extend(["-n", namespace])
 
         try:
+            env = os.environ.copy()
+            env["KUBECONFIG"] = self.kubeconfig
             result = subprocess.run(
                 cmd,
                 capture_output=True,
                 timeout=self.PERMISSION_CHECK_TIMEOUT,
-                env={"KUBECONFIG": self.kubeconfig}
+                env=env
             )
             return result.returncode == 0
         except subprocess.TimeoutExpired:
@@ -120,11 +132,11 @@ def check_permission(
     def check_all_permissions(self) -> None:
         """Check all dangerous write permissions."""
         print("🔐 Checking your Kubernetes permissions...\n", file=sys.stderr)
-        print(f"Checking permissions in '{self.OVN_NAMESPACE}' namespace...\n", file=sys.stderr)
+        print(f"Checking permissions in '{self.ovn_namespace}' namespace...\n", file=sys.stderr)
 
         # Check dangerous write permissions
         for verb, resource, description in self.DANGEROUS_PERMS:
-            if self.check_permission(resource, verb, self.OVN_NAMESPACE):
+            if self.check_permission(resource, verb, self.ovn_namespace):
                 self.write_perms_found = True
                 self.write_perms_list.append(f"  ⚠️  {description} ({verb} {resource})")
 
@@ -138,11 +150,11 @@ def check_all_permissions(self) -> None:
 
         # Check namespace admin (only add if not already cluster admin)
         if not any("CLUSTER ADMIN" in perm for perm in self.write_perms_list):
-            if self.check_permission("*", "*", self.OVN_NAMESPACE):
+            if self.check_permission("*", "*", self.ovn_namespace):
                 self.write_perms_found = True
                 self.write_perms_list.append(
                     f"  ⚠️  NAMESPACE ADMIN - Full access to "
-                    f"{self.OVN_NAMESPACE} namespace"
+                    f"{self.ovn_namespace} namespace"
                 )
 
     def display_warning(self) -> None:
@@ -188,6 +200,13 @@ def run(self) -> int:
         if not self.check_kubectl_available():
             return 1
 
+        # Detect OVN namespace after kubectl availability check
+        try:
+            self.ovn_namespace = detect_ovn_namespace(self.kubeconfig)
+        except Exception as exc:
+            print(f"❌ Error detecting OVN namespace: {exc}", file=sys.stderr)
+            return 1
+
         self.check_all_permissions()
         return self.handle_confirmation()