Add log analysis, comprehensive reporting, and CAMGI integration to must-gather plugin #133
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Prashanth684 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 |
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Plugin Summary & Metadataplugins/must-gather/PLUGIN-SUMMARY.md, PLUGINS.md, docs/data.json |
Adds plugin summary and metadata; registers camgi and comprehensive-analysis commands in docs/data.json and documents the analyzer suite and command metadata. |
CAMGI Docs & Launcherplugins/must-gather/README-CAMGI.md, plugins/must-gather/commands/camgi.md, plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh |
New CAMGI documentation and launcher script implementing start/stop flows, must-gather path resolution, permission checks/fixes, multiple launch strategies (local/module/container), examples, and troubleshooting. |
Comprehensive Analysis Command & Orchestratorplugins/must-gather/commands/comprehensive-analysis.md, plugins/must-gather/skills/must-gather-analyzer/scripts/run-comprehensive-analysis.sh |
New command doc and orchestrator script implementing a five-phase analysis pipeline that invokes multiple analysis scripts, aggregates output into a timestamped consolidated report, and tolerates missing scripts/partial failures. |
Command Docs & README Updatesplugins/must-gather/README.md, plugins/must-gather/commands/analyze.md, plugins/must-gather/skills/must-gather-analyzer/SKILL.md, plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md |
Expanded README, analyze command docs, skill metadata, and quick-reference to cover new analysis targets (Ingress/Routes, Service logs, MachineConfigPools, Pod/Node logs, OVN DBs, CAMGI), usage examples, slash commands, and workflows. |
Analysis Scripts — Ingress & MachineConfigPoolsplugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py, plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_machineconfigpools.py |
New scripts to parse/analyze IngressControllers/Routes and MachineConfigPools: YAML parsing, condition/status extraction, age/duration utilities, formatted tables and detailed views, CLI flags and problems-only filtering. |
Analysis Scripts — Pod/Node/Service Logsplugins/must-gather/skills/must-gather-analyzer/scripts/analyze_pod_logs.py, plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_node_logs.py, plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_servicelogs.py |
New log-analysis scripts with multi-heuristic pattern extraction, per-file aggregation (errors/warnings), gzipped kubelet handling (node logs), filtering (namespace/node/service/container), and formatted summary/details output. |
Analysis Scripts — OVN DBs (referenced)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ovn_dbs.py* |
OVN DB analysis referenced in documentation and included in comprehensive phases where present to parse and report OVN DB diagnostics. |
Automation & Examplesplugins/must-gather/README.md, plugins/must-gather/PLUGIN-SUMMARY.md, plugins/must-gather/commands/*.md |
Adds automation scripts, usage examples, sample outputs, slash-command references, and guidance for integrated workflows and reporting across documentation. |
* File referenced in docs; include where present in repository.
Sequence Diagram(s)
sequenceDiagram
actor User
participant CLI as /must-gather:comprehensive-analysis
participant Orchestrator as run-comprehensive-analysis.sh
participant Script as analyze_*.py
participant FS as Must-Gather FS
participant Report as Consolidated Report
User->>CLI: invoke with must-gather path
CLI->>Orchestrator: pass path & options
Orchestrator->>Orchestrator: validate & locate must-gather
loop Phases 1..5
Orchestrator->>Script: run phase scripts (e.g., analyze_clusterversion.py, analyze_nodes.py, analyze_pod_logs.py)
Script->>FS: read YAML/log/gz files
FS-->>Script: data
Script->>Script: parse → extract patterns → summarize
Script-->>Orchestrator: stdout appended to report
end
Orchestrator->>Report: write consolidated report
Report-->>User: final analysis report (file/STDOUT)
sequenceDiagram
actor User
participant CLI as /must-gather:camgi
participant Launcher as run-camgi.sh
participant Runtime as Local/Module/Container
participant Browser as Browser (optional)
User->>CLI: invoke camgi [path|stop]
CLI->>Launcher: start/stop request
Launcher->>Launcher: validate path & permissions
Launcher->>Runtime: try local okd-camgi -> python module -> container
Runtime-->>Launcher: started (or error)
Launcher->>Browser: open http://127.0.0.1:8080 (if started)
Browser-->>User: CAMGI UI
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
- Areas needing extra attention:
- Pattern-extraction heuristics and correctness in
analyze_pod_logs.py,analyze_node_logs.py, andanalyze_servicelogs.py. - Gzipped kubelet parsing, memory/stream handling, and performance in
analyze_node_logs.py. - YAML parsing, redaction handling, condition/status and duration calculations in
analyze_ingress.pyandanalyze_machineconfigpools.py. - Orchestrator sequencing, error handling, continuation-on-failure behavior, and report assembly in
run-comprehensive-analysis.sh. - Container/local/module fallback logic, file-permission handling, and start/stop robustness in
run-camgi.sh.
- Pattern-extraction heuristics and correctness in
Pre-merge checks and finishing touches
✅ Passed checks (7 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately describes the main changes: adding log analysis capabilities, comprehensive reporting, and CAMGI integration to the must-gather plugin, matching the PR's core objectives. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 89.13% which is sufficient. The required threshold is 80.00%. |
| No Real People Names In Style References | ✅ Passed | Comprehensive search across all documentation, commands, scripts, and configuration files found no references to real people's names in style references, example prompts, or instructions. |
| No Assumed Git Remote Names | ✅ Passed | Comprehensive search of PR changes reveals no hardcoded git remote names or git remote operations in any added files. |
| Git Push Safety Rules | ✅ Passed | Comprehensive search found no git push commands, force push operations, or automated git workflows in any PR files. |
| No Untrusted Mcp Servers | ✅ Passed | The pull request does not introduce any MCP server installations from untrusted or any sources. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (11)
plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md (1)
166-173: Add language identifiers to fenced code blocks for better rendering.Several fenced code blocks are missing language identifiers, which can affect syntax highlighting and rendering in different markdown viewers.
Apply these changes:
-``` +```text /must-gather:analyze <path> # Quick comprehensive analysis /must-gather:comprehensive-analysis <path> # Detailed report generation-``` +```text [176x] Error message here-``` +```text SUMMARY: X/Y resources healthy ⚠️ N resources with issues-``` +```text .claude-plugin/skills/must-gather-analyzer/scripts/Also applies to: 184-189, 191-196, 210-212
plugins/must-gather/commands/camgi.md (1)
33-43: Add language identifiers to fenced code blocks.Multiple fenced code blocks are missing language identifiers. Adding them improves rendering and syntax highlighting.
Examples of recommended changes:
-``` +```text must-gather/ └── registry-ci-openshift-org-origin-...-sha256-<hash>/-``` +```text Fix permissions now? (Y/n)-``` +```text CAMGI is now running!Also applies to: 54-63, 75-77, 93-93, 116-133, 169-185, 204-207
plugins/must-gather/PLUGIN-SUMMARY.md (1)
44-47: Add language identifier to fenced code block.The code block showing example output would benefit from a language identifier for consistent rendering.
-``` +```text NAME VERSION AVAILABLE PROGRESSING DEGRADED authentication 4.18.26 True False Falseplugins/must-gather/commands/analyze.md (1)
200-202: Add language identifier to fenced code block.The output format example should have a language identifier for better rendering.
-``` +```text INGRESS CONTROLLERS: [output from analyze_ingress.py --ingresscontrollers]plugins/must-gather/skills/must-gather-analyzer/scripts/run-comprehensive-analysis.sh (1)
64-87: Remove or utilize unused parameter.The
phaseparameter (line 67) is passed torun_scriptthroughout the script but never used within the function. Either remove the parameter or use it for enhanced logging/reporting.Option 1 - Remove the parameter:
run_script() { local script_name=$1 local script_args=${2:-} - local phase=$3 echo -e "${BLUE}Running: $script_name $script_args${NC}"And update all calls to remove the third argument (e.g., change
run_script "analyze_clusterversion.py" "" "Phase 1"torun_script "analyze_clusterversion.py" "").Option 2 - Use it for logging:
run_script() { local script_name=$1 local script_args=${2:-} local phase=$3 - echo -e "${BLUE}Running: $script_name $script_args${NC}" + echo -e "${BLUE}[$phase] Running: $script_name $script_args${NC}"plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_servicelogs.py (3)
74-76: Catch specific exceptions instead of broad Exception.Catching bare
Exceptionis too broad and can mask unexpected errors. Catch specific exceptions that might occur during file reading.- except Exception as e: + except (IOError, OSError, UnicodeDecodeError) as e: print(f"Warning: Failed to read {file_path}: {e}", file=sys.stderr)
160-161: Use explicit Optional type hint.PEP 484 discourages implicit
Optional. Use explicitstr | NoneorOptional[str]for better type clarity.-def analyze_service_logs(must_gather_path: str, service_filter: str = None, +def analyze_service_logs(must_gather_path: str, service_filter: str | None = None, show_warnings: bool = False, errors_only: bool = False):Or if supporting older Python versions:
+from typing import Optional + -def analyze_service_logs(must_gather_path: str, service_filter: str = None, +def analyze_service_logs(must_gather_path: str, service_filter: Optional[str] = None, show_warnings: bool = False, errors_only: bool = False):
231-231: Remove unnecessary f-string prefix.The f-string doesn't contain any placeholders, so the
fprefix is unnecessary.- print(f" ✅ No issues found!") + print(" ✅ No issues found!")plugins/must-gather/commands/comprehensive-analysis.md (2)
70-70: Use heading markup instead of emphasis for "CRITICAL: Script-Only Analysis".Line 70 should use a heading (e.g.,
### CRITICAL: Script-Only Analysis) for proper document structure, rather than bold emphasis.-**CRITICAL: Script-Only Analysis** +### CRITICAL: Script-Only Analysis
10-10: Add language specifiers to fenced code blocks for syntax highlighting.Multiple code blocks lack language identifiers. Specify the language (bash, python, yaml, text, etc.) after the opening backticks for proper rendering and tool compliance (MD040).
Example fixes:
-``` +```bash ls .claude-plugin/skills/must-gather-analyzer/scripts/analyze_clusteroperators.py -``` +```bashApply this pattern to all fenced code blocks in the file. Most are
bashortext; adjust based on content.Also applies to: 35-35, 50-50, 88-88, 310-310, 477-477, 483-483, 489-489, 591-591, 602-602, 613-613, 624-624, 636-636, 647-647, 664-664, 674-674, 717-717, 734-734
plugins/must-gather/skills/must-gather-analyzer/SKILL.md (1)
201-201: Add language specifiers to fenced code blocks for syntax highlighting.Multiple code blocks lack language identifiers (MD040). Specify the language (bash, python, etc.) after the opening backticks.
Example fixes:
-``` +```bash ./scripts/analyze_ingress.py <must-gather-path> --ingresscontrollers -``` +```bashApply this pattern to all fenced code blocks: lines 201, 221, 249, 278, 302, 326, 425. Most are
bash; adjust based on content.Also applies to: 221-221, 249-249, 278-278, 302-302, 326-326, 425-425
📜 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 (15)
plugins/must-gather/PLUGIN-SUMMARY.md(1 hunks)plugins/must-gather/README-CAMGI.md(1 hunks)plugins/must-gather/README.md(4 hunks)plugins/must-gather/commands/analyze.md(9 hunks)plugins/must-gather/commands/camgi.md(1 hunks)plugins/must-gather/commands/comprehensive-analysis.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/SKILL.md(5 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_machineconfigpools.py(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_node_logs.py(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_pod_logs.py(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_servicelogs.py(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/run-comprehensive-analysis.sh(1 hunks)
🧰 Additional context used
🪛 LanguageTool
plugins/must-gather/commands/comprehensive-analysis.md
[style] ~770-~770: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ive data removed 3. Log Truncation: Very large log files may be truncated in collectio...
(EN_WEAK_ADJECTIVE)
plugins/must-gather/commands/camgi.md
[style] ~74-~74: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:** If the web interface is not accessible: - The script opens http://127.0.0.1:80...
(NOT_ABLE_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/README-CAMGI.md
16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
24-24: Bare URL used
(MD034, no-bare-urls)
56-56: Bare URL used
(MD034, no-bare-urls)
128-128: Bare URL used
(MD034, no-bare-urls)
142-142: Bare URL used
(MD034, no-bare-urls)
164-164: Bare URL used
(MD034, no-bare-urls)
165-165: Bare URL used
(MD034, no-bare-urls)
166-166: Bare URL used
(MD034, no-bare-urls)
plugins/must-gather/PLUGIN-SUMMARY.md
16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
24-24: Bare URL used
(MD034, no-bare-urls)
56-56: Bare URL used
(MD034, no-bare-urls)
128-128: Bare URL used
(MD034, no-bare-urls)
142-142: Bare URL used
(MD034, no-bare-urls)
164-164: Bare URL used
(MD034, no-bare-urls)
165-165: Bare URL used
(MD034, no-bare-urls)
166-166: Bare URL used
(MD034, no-bare-urls)
plugins/must-gather/commands/comprehensive-analysis.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
70-70: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
310-310: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
477-477: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
483-483: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
489-489: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
591-591: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
602-602: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
613-613: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
624-624: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
636-636: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
647-647: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
664-664: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
674-674: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
700-700: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
717-717: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
734-734: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/skills/must-gather-analyzer/SKILL.md
201-201: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
221-221: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
249-249: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
278-278: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
302-302: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
326-326: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
425-425: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/commands/camgi.md
33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
75-75: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
201-201: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/commands/analyze.md
201-201: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md
166-166: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
184-184: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
191-191: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
210-210: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.3)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_servicelogs.py
74-74: Do not catch blind exception: Exception
(BLE001)
160-160: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
231-231: f-string without any placeholders
Remove extraneous f prefix
(F541)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_pod_logs.py
83-83: Do not catch blind exception: Exception
(BLE001)
95-95: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
95-95: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
96-96: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
201-201: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
201-201: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
202-202: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
254-254: f-string without any placeholders
Remove extraneous f prefix
(F541)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_machineconfigpools.py
32-32: Do not catch blind exception: Exception
(BLE001)
76-76: Do not catch blind exception: Exception
(BLE001)
172-172: f-string without any placeholders
Remove extraneous f prefix
(F541)
272-272: f-string without any placeholders
Remove extraneous f prefix
(F541)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_node_logs.py
94-94: Do not catch blind exception: Exception
(BLE001)
106-106: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
208-208: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
208-208: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
281-281: f-string without any placeholders
Remove extraneous f prefix
(F541)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py
24-24: Do not catch blind exception: Exception
(BLE001)
41-41: Do not catch blind exception: Exception
(BLE001)
73-73: Do not catch blind exception: Exception
(BLE001)
181-181: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
235-235: f-string without any placeholders
Remove extraneous f prefix
(F541)
291-291: f-string without any placeholders
Remove extraneous f prefix
(F541)
297-297: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
346-346: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 Shellcheck (0.11.0)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-comprehensive-analysis.sh
[warning] 67-67: phase appears unused. Verify use (or export if used externally).
(SC2034)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh
[warning] 37-37: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 53-53: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 102-102: ORIGINAL_PATH appears unused. Verify use (or export if used externally).
(SC2034)
| with open(file_path, 'r') as f: | ||
| doc = yaml.safe_load(f) | ||
| if doc and doc.get('kind') == 'RouteList': | ||
| items = doc.get('items') | ||
| if items is not None: | ||
| routes.extend(items) | ||
| elif doc and doc.get('kind') == 'Route': | ||
| routes.append(doc) | ||
| except Exception as e: | ||
| print(f"Warning: Failed to parse {file_path}: {e}", file=sys.stderr) | ||
| return routes |
There was a problem hiding this comment.
Accept kind: List from oc get route
oc get route -o yaml produces kind: List, not RouteList, so this code drops every route object and the analyzer always prints “No Routes found.” That breaks the command entirely. Please treat both List and RouteList as aggregates so the items array is processed.
- doc = yaml.safe_load(f)
- if doc and doc.get('kind') == 'RouteList':
- items = doc.get('items')
- if items is not None:
- routes.extend(items)
- elif doc and doc.get('kind') == 'Route':
- routes.append(doc)
+ doc = yaml.safe_load(f)
+ if not doc:
+ return routes
+
+ kind = doc.get('kind')
+ if kind in ('RouteList', 'List'):
+ items = doc.get('items') or []
+ routes.extend(items)
+ elif kind == 'Route':
+ routes.append(doc)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with open(file_path, 'r') as f: | |
| doc = yaml.safe_load(f) | |
| if doc and doc.get('kind') == 'RouteList': | |
| items = doc.get('items') | |
| if items is not None: | |
| routes.extend(items) | |
| elif doc and doc.get('kind') == 'Route': | |
| routes.append(doc) | |
| except Exception as e: | |
| print(f"Warning: Failed to parse {file_path}: {e}", file=sys.stderr) | |
| return routes | |
| with open(file_path, 'r') as f: | |
| doc = yaml.safe_load(f) | |
| if not doc: | |
| return routes | |
| kind = doc.get('kind') | |
| if kind in ('RouteList', 'List'): | |
| items = doc.get('items') or [] | |
| routes.extend(items) | |
| elif kind == 'Route': | |
| routes.append(doc) | |
| except Exception as e: | |
| print(f"Warning: Failed to parse {file_path}: {e}", file=sys.stderr) | |
| return routes |
🧰 Tools
🪛 Ruff (0.14.3)
41-41: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py
around lines 33 to 43, the parser only treats 'RouteList' as an aggregate and
ignores YAML where kind == 'List' (the output of `oc get route -o yaml`), so
route objects are dropped; update the conditional to treat both 'List' and
'RouteList' as aggregates (e.g., if doc.get('kind') in ('List', 'RouteList'))
and then process/extend the items array when present, falling back to appending
a single 'Route' doc when kind == 'Route' and keeping the existing exception
handling.
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py
Show resolved
Hide resolved
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_pod_logs.py
Outdated
Show resolved
Hide resolved
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh
Outdated
Show resolved
Hide resolved
…ust-gather plugin This PR adds log analysis capabilities, automated comprehensive reporting, and CAMGI web UI integration to the must-gather plugin. New Analysis Scripts: Log Analysis: - analyze_servicelogs.py - Systemd service logs (kubelet, crio) with pattern extraction - analyze_pod_logs.py - Container application logs with error pattern deduplication - analyze_node_logs.py - Node logs (kubelet, sysinfo, dmesg) with gzip support Infrastructure: - analyze_ingress.py - IngressControllers and Routes status - analyze_machineconfigpools.py - Node configuration rollout and update status Other: - run-comprehensive-analysis.sh - Runs all analysis scripts in systematic order, generates timestamped reports - run-camgi.sh - Automated CAMGI container management with permission fixing and browser opening All scripts feature pattern-based error extraction, occurrence counting, and oc-like output format. New Commands: - /must-gather:comprehensive-analysis - Systematic 5-phase analysis with detailed report generation - /must-gather:camgi - Launch CAMGI (Cluster Autoscaler Must-Gather Inspector) web interface Documentation: - PLUGIN-SUMMARY.md - Complete technical summary of plugin capabilities - QUICK-REFERENCE.md - Command cheat sheet - README-CAMGI.md - CAMGI integration guide and troubleshooting - README.md - Added documentation for all new scripts and commands
Prashanth684
force-pushed
the
mg-ai-helpers
branch
from
0f8e740 to
56ed740
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)
193-202: Match Docker SELinux handling with Podman.We still mount
$MG_PATHwithout:Z, so Docker users on SELinux can’t read the must-gather and CAMGI fails. Please add the relabel flag, same as Podman, to prevent permission errors. (Follow-up to earlier feedback.)- docker run --rm -it -p 8080:8080 -v "$MG_PATH:/must-gather" quay.io/elmiko/okd-camgi + docker run --rm -it -p 8080:8080 -v "$MG_PATH:/must-gather:Z" quay.io/elmiko/okd-camgiplugins/must-gather/skills/must-gather-analyzer/scripts/analyze_pod_logs.py (1)
74-82: Restore kube-style severity detection.The parser still ignores the canonical
E0818…/W0818…prefixes, so it misses most Kubernetes errors and warnings—breaking the analyzer’s core purpose. Please reinstate the prefix checks alongside the keyword search. (Reiterating prior feedback.)- if re.search(r'\b(ERROR|Error|error|FATAL|Fatal|fatal|CRIT|Critical|PANIC|Panic|panic)\b', line): + if ( + re.search(r'\b(ERROR|Error|error|FATAL|Fatal|fatal|CRIT|Critical|PANIC|Panic|panic)\b', line) + or re.match(r'^[EF]\d{4}', line) + ): total_errors += 1 pattern = extract_error_pattern(line) error_patterns[pattern] += 1 - elif re.search(r'\b(WARN|Warning|warning)\b', line): + elif ( + re.search(r'\b(WARN|Warning|warning)\b', line) + or re.match(r'^W\d{4}', line) + ): total_warnings += 1 pattern = extract_error_pattern(line) warning_patterns[pattern] += 1plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py (2)
33-43: Acceptkind: Listfromoc get route -o yaml.
oc get route -o yamlproduceskind: List, notRouteList, causing all route objects to be dropped. This breaks the route analysis entirely.Apply this diff to accept both
ListandRouteList:with open(file_path, 'r') as f: doc = yaml.safe_load(f) - if doc and doc.get('kind') == 'RouteList': + kind = doc.get('kind') + if kind in ('RouteList', 'List'): items = doc.get('items') if items is not None: routes.extend(items)
334-347: Align summary counts with namespace filtering.When
--namespaceis provided, the table shows only that namespace but the summary counts all routes in the cluster because it uses the unfilteredroute_info_list. This produces misleading totals.Apply this diff to filter before computing the summary:
+ # Filter by namespace if specified + display_routes = route_info_list + if namespace: + display_routes = [r for r in route_info_list if r['namespace'] == namespace] + # Print table - print_route_table(route_info_list, namespace) + print_route_table(display_routes) # Summary - total = len(route_info_list) - admitted = sum(1 for r in route_info_list if r['admitted'] == 'True') + total = len(display_routes) + admitted = sum(1 for r in display_routes if r['admitted'] == 'True')plugins/must-gather/README.md (1)
444-444: Remove hardcoded user-specific path.Line 444 contains an absolute path specific to your local machine, which will not work for other users.
Apply this fix:
-See `/home/psundara/ws/src/github.com/openshift/must-gather/.claude-plugin/commands/comprehensive-analysis.md` for full details. +See `commands/comprehensive-analysis.md` for full details.
🧹 Nitpick comments (14)
plugins/must-gather/README-CAMGI.md (1)
16-33: Label the slash-command blocks for markdown lint compliance.Add
bashto these fences so markdownlint stops flagging MD040.-``` +```bash /must-gather:camgi <must-gather-path>@@
-+bash
/must-gather:camgi stopplugins/must-gather/commands/analyze.md (1)
258-268: Add language hints to fenced command examples.These examples trigger MD040; mark them as bash like the surrounding snippets.
-``` +```bash /must-gather:analyze analyze service logs@@
-+bash
/must-gather:analyze check ingressplugins/must-gather/skills/must-gather-analyzer/scripts/analyze_machineconfigpools.py (2)
172-172: Remove unnecessary f-string prefix.The f-string on line 172 does not contain any placeholders.
Apply this diff:
- print(f" Conditions:") + print(" Conditions:")
272-272: Remove unnecessary f-string prefix.The f-string on line 272 does not contain any placeholders.
Apply this diff:
- print(f" ✅ All pools healthy and updated") + print(" ✅ All pools healthy and updated")plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py (4)
181-195: Namespace filtering happens twice.The
print_route_tablefunction already filters by namespace (lines 191-195), but it's called with a pre-filtered list. This creates redundant filtering logic.Consider removing the filtering from
print_route_tableand doing it once at the call site for clarity.
235-235: Remove unnecessary f-string prefix.The f-string on line 235 does not contain any placeholders.
Apply this diff:
- print(f" Conditions:") + print(" Conditions:")
291-291: Remove unnecessary f-string prefix.The f-string on line 291 does not contain any placeholders.
Apply this diff:
- print(f" ✅ No issues found") + print(" ✅ No issues found")
346-346: Remove unnecessary f-string prefix.The f-string on line 346 does not contain any placeholders.
Apply this diff:
- print(f" ✅ All routes admitted") + print(" ✅ All routes admitted")plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md (1)
166-166: Consider adding language specifiers to fenced code blocks.Several fenced code blocks are missing language identifiers. Adding them improves syntax highlighting and readability.
For example, line 166:
-``` +```text /must-gather:analyze <path> # Quick comprehensive analysisApply similar changes to lines 184, 191, and 210.
Also applies to: 184-184, 191-191, 210-210
plugins/must-gather/skills/must-gather-analyzer/scripts/run-comprehensive-analysis.sh (1)
64-87: Remove or use the unusedphaseparameter.The
phaseparameter is passed torun_scriptbut never used within the function. Either remove it or prefix with underscore to indicate intentional non-use.Option 1 - Remove the parameter:
run_script() { local script_name=$1 local script_args=${2:-} - local phase=$3And update all call sites to remove the third argument.
Option 2 - Mark as intentionally unused:
- local phase=$3 + local _phase=$3 # Reserved for future usedocs/data.json (1)
545-550: Clarify thecamgicommand's argument options in description.The argument_hint indicates
[must-gather-path|stop], which suggests users can pass "stop" to halt CAMGI. However, the description only mentions launching the interface and doesn't explain the "stop" functionality. Add a note to the description or synopsis clarifying what the "stop" argument does.Consider updating the description to:
- "Launch CAMGI (Cluster Autoscaler Must-Gather Inspector) web interface to analyze cluster autoscaler behavior" + "Launch CAMGI (Cluster Autoscaler Must-Gather Inspector) web interface to analyze cluster autoscaler behavior, or stop running CAMGI containers"Alternatively, update the argument_hint if "stop" is not a supported option:
- "argument_hint": "[must-gather-path|stop]" + "argument_hint": "[must-gather-path]"plugins/must-gather/commands/comprehensive-analysis.md (2)
283-287: Minor: Event validation guidance is emphasized twice (redundant but acceptable).The critical event validation guidance appears at lines 280-287 and is repeated at lines 700-704. While this redundancy could be seen as emphasizing important content, it slightly dilutes the impact of the CRITICAL section. Consider consolidating this guidance or removing the duplication if space is a concern.
Alternatively, if emphasis is intentional (appearing both in the implementation section and in the tips section), this is acceptable for a comprehensive guide.
770-770: Minor: Replace weak intensifier "very" with more specific description.Line 770 uses "Very large log files" which LanguageTool flags as over-using the intensifier "very". Consider being more specific:
- 3. **Log Truncation**: Very large log files may be truncated in collection + 3. **Log Truncation**: Large log files may be truncated in collectionOr, if you want to be more specific about size:
- 3. **Log Truncation**: Very large log files may be truncated in collection + 3. **Log Truncation**: Log files exceeding collector limits may be truncated in collectionplugins/must-gather/skills/must-gather-analyzer/SKILL.md (1)
414-445: LGTM - Helper scripts reference section is comprehensive.The new script references for analyze_ingress.py, analyze_servicelogs.py, analyze_machineconfigpools.py, analyze_pod_logs.py, analyze_node_logs.py, analyze_ovn_dbs.py, and run-camgi.sh are well-documented with parsing paths, output descriptions, and prerequisites clearly stated.
Note:
run-comprehensive-analysis.shis referenced earlier in the "Analysis Modes" section but doesn't have an entry in the "Helper Scripts Reference" section. This may be intentional (since it's orchestration rather than a standalone analysis script), but consider adding an entry if it should be discoverable here.
📜 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 (17)
PLUGINS.md(1 hunks)docs/data.json(1 hunks)plugins/must-gather/PLUGIN-SUMMARY.md(1 hunks)plugins/must-gather/README-CAMGI.md(1 hunks)plugins/must-gather/README.md(4 hunks)plugins/must-gather/commands/analyze.md(9 hunks)plugins/must-gather/commands/camgi.md(1 hunks)plugins/must-gather/commands/comprehensive-analysis.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/SKILL.md(5 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_machineconfigpools.py(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_node_logs.py(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_pod_logs.py(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_servicelogs.py(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/run-comprehensive-analysis.sh(1 hunks)
🧰 Additional context used
🪛 LanguageTool
plugins/must-gather/commands/comprehensive-analysis.md
[style] ~770-~770: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ive data removed 3. Log Truncation: Very large log files may be truncated in collectio...
(EN_WEAK_ADJECTIVE)
plugins/must-gather/commands/camgi.md
[style] ~74-~74: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:** If the web interface is not accessible: - The script opens http://127.0.0.1:80...
(NOT_ABLE_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/commands/analyze.md
259-259: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
265-265: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/skills/must-gather-analyzer/SKILL.md
351-351: Bare URL used
(MD034, no-bare-urls)
443-443: Bare URL used
(MD034, no-bare-urls)
plugins/must-gather/README-CAMGI.md
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
103-103: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/README.md
249-249: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
278-278: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
302-302: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
326-326: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
365-365: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
425-425: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
568-568: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
573-573: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/commands/camgi.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
76-76: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
94-94: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
169-169: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md
166-166: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
184-184: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
191-191: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
210-210: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.3)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_servicelogs.py
74-74: Do not catch blind exception: Exception
(BLE001)
160-160: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
231-231: f-string without any placeholders
Remove extraneous f prefix
(F541)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_pod_logs.py
83-83: Do not catch blind exception: Exception
(BLE001)
95-95: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
95-95: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
96-96: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
201-201: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
201-201: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
202-202: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
254-254: f-string without any placeholders
Remove extraneous f prefix
(F541)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_node_logs.py
94-94: Do not catch blind exception: Exception
(BLE001)
106-106: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
208-208: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
208-208: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
281-281: f-string without any placeholders
Remove extraneous f prefix
(F541)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_machineconfigpools.py
32-32: Do not catch blind exception: Exception
(BLE001)
76-76: Do not catch blind exception: Exception
(BLE001)
172-172: f-string without any placeholders
Remove extraneous f prefix
(F541)
272-272: f-string without any placeholders
Remove extraneous f prefix
(F541)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py
24-24: Do not catch blind exception: Exception
(BLE001)
41-41: Do not catch blind exception: Exception
(BLE001)
73-73: Do not catch blind exception: Exception
(BLE001)
181-181: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
235-235: f-string without any placeholders
Remove extraneous f prefix
(F541)
291-291: f-string without any placeholders
Remove extraneous f prefix
(F541)
297-297: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
346-346: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 Shellcheck (0.11.0)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-comprehensive-analysis.sh
[warning] 67-67: phase appears unused. Verify use (or export if used externally).
(SC2034)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh
[warning] 37-37: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 53-53: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 102-102: ORIGINAL_PATH appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (15)
PLUGINS.md (1)
116-117: LGTM! New command documentation is clear and consistent.The new command entries for
camgiandcomprehensive-analysisfollow the established format and provide clear descriptions.plugins/must-gather/PLUGIN-SUMMARY.md (1)
1-340: Excellent comprehensive documentation!This summary provides a clear, well-structured overview of the entire plugin ecosystem including scripts, features, workflows, and design principles.
docs/data.json (1)
551-556: LGTM!The
comprehensive-analysiscommand metadata is well-structured. The description clearly conveys the command's purpose, and the synopsis and argument_hint are appropriate and consistent.plugins/must-gather/commands/comprehensive-analysis.md (6)
20-29: Verify script count consistency: 13 vs 14 scripts.Line 25 states the command "Runs all 13 analysis scripts in systematic 5-phase order", but lines 52-65 (Prerequisites section) list 14 scripts. The Phase 1-5 implementation (lines 106-240) also shows 14 scripts (1. ClusterVersion, 2. Operators, 3. Nodes, 4. Network, 5. OVN, 6. Ingress, 7. Pods, 8. Storage, 9. MCPs, 10. etcd, 11. Events, 12. Service Logs, 13. Pod Logs, 14. Node Logs).
Correct the discrepancy:
- - Runs all 13 analysis scripts in systematic 5-phase order + - Runs all 14 analysis scripts in systematic 5-phase order
68-90: Audit error handling guidance against implementation.The documentation (lines 72-90) provides strict prescriptive guidance: "NEVER attempt to analyze must-gather data directly using bash commands" and "ONLY use the provided Python scripts." This is excellent guidance, but verify that:
- The run-comprehensive-analysis.sh implementation follows this guidance strictly
- The documented script availability check at line 84 is actually executable and meaningful
- The error message format (lines 88-90) matches what will be produced
Please verify that the run-comprehensive-analysis.sh orchestration script validates script availability before executing analysis, as documented here. If the implementation deviates from this guidance, update the documentation to reflect reality or update the implementation to match the guidance.
144-152: LGTM - OVN Database prerequisites are clear.The OVN database section properly documents prerequisites, availability conditions, and execution examples. The note about ovsdb-tool and the must-gather file requirement is helpful for users. The Phase 2 analysis workflow correctly places OVN analysis after network operator checks.
280-287: Excellent event validation guidance for FailedScheduling patterns.Lines 280-287 provide critical guidance about cross-referencing events with current pod state. The distinction between "FailedScheduling events + Pod Running" (resolved) versus "FailedScheduling events + Pod Pending" (active issue) is exactly the kind of nuance that prevents false positives in analysis. This guidance will help users avoid misdiagnosing transient Kubernetes retry behavior.
741-765: LGTM - Report customization guidance is practical and well-targeted.The customization guidance for different audiences (SRE/Operations, Development, Management) shows good understanding of real-world usage. Tailoring the report focus helps users present findings appropriately for each stakeholder group.
1-4: Naming inconsistency:argument-hint(hyphen) vsargument_hint(underscore).The frontmatter uses
argument-hint(line 3), but docs/data.json usesargument_hint(underscore). Verify if this is intentional or should be consistent across both files. Check if the markdown frontmatter parser expects hyphens or underscores.Suggested fix (if underscores are correct):
- argument-hint: [must-gather-path] + argument_hint: [must-gather-path]plugins/must-gather/skills/must-gather-analyzer/SKILL.md (6)
10-11: LGTM - Trigger keywords appropriately expanded.The additions of "comprehensive analysis" and "cluster report" to the trigger keywords are appropriate and will help users discover the comprehensive-analysis command through natural language queries.
18-29: LGTM - Analysis Modes section clearly positions new capabilities.The new "Analysis Modes" section (Quick vs Comprehensive) effectively explains the distinction. The reference to
.claude-plugin/commands/comprehensive-analysis.mdis helpful for detailed guidance.
25-25: Verify script count: Line 25 also states "13 analysis scripts" (should be 14).This matches the same discrepancy flagged in comprehensive-analysis.md line 25. Correct to:
- - Runs all 13 analysis scripts in systematic 5-phase order + - Runs all 14 analysis scripts in systematic 5-phase order
194-250: LGTM - New analysis script sections are well-documented.All five new analysis sections (Ingress, Service Logs, MachineConfigPools, Pod Logs, Node Logs) follow the established documentation pattern with clear command examples, parameter descriptions, and expected output. The formatting is consistent with existing sections.
332-352: LGTM - CAMGI documentation is clear and complete.The CAMGI section effectively explains its purpose (cluster autoscaler analysis), usage examples (including the "stop" command for stopping containers), and prerequisites. The reference to
scripts/README-CAMGI.mdfor detailed documentation is appropriate.
351-351: Fix bare URLs - wrap in markdown link syntax.MarkdownLint flags two bare URLs at lines 351 and 443. Wrap them in markdown link syntax:
Line 351:
- - Runs on http://127.0.0.1:8080 + - Runs on [http://127.0.0.1:8080](http://127.0.0.1:8080)Line 443:
- - Runs on http://127.0.0.1:8080 + - Runs on [http://127.0.0.1:8080](http://127.0.0.1:8080)Also applies to: 443-443
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugins/must-gather/commands/camgi.md (3)
10-10: Add language specifiers to fenced code blocks for proper rendering.Markdown fenced code blocks should include language identifiers for syntax highlighting. Apply these fixes:
-``` +```bash /must-gather:camgi [must-gather-path]-
+
must-gather/-
+bash
Fix permissions now? (Y/n)-
+
CAMGI is now running!-
+
Stopping all CAMGI containers...-
+bash
/must-gather:camgi /home/user/must-gather-
+bash
/must-gather:camgi-
+bash
/must-gather:camgi stopAlso applies to: 37-37, 54-54, 116-116, 136-136, 170-170, 176-176, 182-182 --- `75-76`: **Wrap bare URLs in markdown link format for better accessibility.** Bare URLs should be wrapped in markdown links for proper rendering and accessibility. Update: ```diff - The script opens http://127.0.0.1:8080 automatically - If it doesn't open, manually navigate to http://127.0.0.1:8080 + The script opens [http://127.0.0.1:8080](http://127.0.0.1:8080) automatically + If it doesn't open, manually navigate to [http://127.0.0.1:8080](http://127.0.0.1:8080) - Accessible at http://127.0.0.1:8080 + Accessible at [http://127.0.0.1:8080](http://127.0.0.1:8080) - CAMGI GitHub: https://github.com/elmiko/okd-camgi - CAMGI PyPI: https://pypi.org/project/okd-camgi/ + CAMGI GitHub: [https://github.com/elmiko/okd-camgi](https://github.com/elmiko/okd-camgi) + CAMGI PyPI: [https://pypi.org/project/okd-camgi/](https://pypi.org/project/okd-camgi/)Also applies to: 106-106, 160-160, 198-199
72-77: Replace "is not accessible" with "inaccessible" for conciseness.- If the web interface is not accessible: + If the web interface is inaccessible:
📜 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/must-gather/commands/camgi.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
plugins/must-gather/commands/camgi.md
[style] ~74-~74: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:** If the web interface is not accessible: - The script opens http://127.0.0.1:80...
(NOT_ABLE_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/commands/camgi.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
75-75: Bare URL used
(MD034, no-bare-urls)
76-76: Bare URL used
(MD034, no-bare-urls)
106-106: Bare URL used
(MD034, no-bare-urls)
116-116: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
136-136: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
160-160: Bare URL used
(MD034, no-bare-urls)
170-170: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
176-176: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
182-182: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
198-198: Bare URL used
(MD034, no-bare-urls)
199-199: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (1)
plugins/must-gather/commands/camgi.md (1)
1-207: Documentation is comprehensive and well-structured.The file provides clear instructions, prerequisites, error handling, technical details, and usage examples. The content is well-organized and covers the CAMGI integration effectively. Once the path inconsistency and markdown formatting issues are resolved, this will be a solid addition to the must-gather command documentation.
Prashanth684
force-pushed
the
mg-ai-helpers
branch
from
56488ed to
e1de04a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)
102-114: Remove unused variable.The
ORIGINAL_PATHvariable is assigned on line 102 but never referenced. Remove this unused assignment:-# Check if we need to find the subdirectory with hash -ORIGINAL_PATH="$MG_PATH" -if [ ! -d "$MG_PATH/cluster-scoped-resources" ]; then +# Check if we need to find the subdirectory with hash +if [ ! -d "$MG_PATH/cluster-scoped-resources" ]; then
🧹 Nitpick comments (7)
plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md (1)
166-173: Add language specifiers to fenced code blocks.Several code blocks are missing language identifiers, which violates markdown style guidelines. Add
bashspecifiers to code blocks containing shell commands.- ``` + ```bash /must-gather:analyze <path>This applies to:
- Line 166: code block with slash commands
- Line 184: code block with pattern format
- Line 191: code block with summary format
- Line 210: code block with script locations
Also applies to: 184-195, 191-196, 210-213
plugins/must-gather/commands/camgi.md (1)
72-77: Consider minor wording improvement.Line 74 could be more concise: replace "is not accessible" with "inaccessible".
- If the web interface is not accessible: + If the web interface is inaccessible:plugins/must-gather/PLUGIN-SUMMARY.md (1)
33-54: Add language specifiers to fenced code blocks.Multiple code blocks throughout the document are missing language identifiers. Add
bashspecifiers to shell command blocks andyamlfor YAML/output examples to comply with markdown style guidelines.Examples of code blocks needing language specifiers:
- Line 33: Pattern extraction examples
- Line 54: oc-like output format
- Line 75-93: Usage modes
- Line 128-156: Example output
- Line 181-214: Common investigation patterns
- Line 249-276: Files created/modified
- Line 278-302: Example output patterns
Also applies to: 75-93, 128-156, 181-214, 249-276, 278-302, 326-330
plugins/must-gather/commands/comprehensive-analysis.md (3)
70-79: Use proper heading syntax instead of emphasis for section titles.Line 70 uses emphasis (
**CRITICAL: ...**) formatted like a heading. Use proper markdown heading syntax:-**CRITICAL: Script-Only Analysis** +## CRITICAL: Script-Only AnalysisThis also applies to line 700 with "CRITICAL: Always validate events against current state".
10-12: Add language specifiers to fenced code blocks.Multiple code blocks throughout are missing language identifiers. Add
bashspecifiers to shell command blocks and appropriate language identifiers for other code examples to comply with markdown style guidelines (MD040).Also applies to: 35-42, 50-66, 70-90, 88-110, 310-472, 477-495, 591-610, 624-644, 647-660, 664-710, 717-740, 734-791
770-770: Minor wording improvement.Line 770 uses "very large" which is a weak intensifier. Consider: "Log files may be truncated in collection" (omit "very") or use "Large log files may be truncated in collection" for clarity.
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)
37-49: Separate variable declaration and assignment to avoid masking return values.Lines 37 and 53 declare and assign variables in a single statement. To avoid masking return values from command substitution, separate the declaration from the assignment:
Line 37:
- local containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi) + local containers + containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi)Line 53:
- local containers=$(docker ps -q --filter ancestor=quay.io/elmiko/okd-camgi) + local containers + containers=$(docker ps -q --filter ancestor=quay.io/elmiko/okd-camgi)Also applies to: 53-65
📜 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)
plugins/must-gather/PLUGIN-SUMMARY.md(1 hunks)plugins/must-gather/README.md(4 hunks)plugins/must-gather/commands/camgi.md(1 hunks)plugins/must-gather/commands/comprehensive-analysis.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/SKILL.md(5 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh(1 hunks)
🧰 Additional context used
🪛 LanguageTool
plugins/must-gather/commands/camgi.md
[style] ~74-~74: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:** If the web interface is not accessible: - The script opens http://127.0.0.1:80...
(NOT_ABLE_PREMIUM)
plugins/must-gather/commands/comprehensive-analysis.md
[style] ~770-~770: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ive data removed 3. Log Truncation: Very large log files may be truncated in collectio...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/skills/must-gather-analyzer/SKILL.md
351-351: Bare URL used
(MD034, no-bare-urls)
443-443: Bare URL used
(MD034, no-bare-urls)
plugins/must-gather/PLUGIN-SUMMARY.md
33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
75-75: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
201-201: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
221-221: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
249-249: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
278-278: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
302-302: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
326-326: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/commands/comprehensive-analysis.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
70-70: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
310-310: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
477-477: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
483-483: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
489-489: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
591-591: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
602-602: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
613-613: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
624-624: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
636-636: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
647-647: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
664-664: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
674-674: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
700-700: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
717-717: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
734-734: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md
166-166: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
184-184: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
191-191: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
210-210: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh
[warning] 37-37: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 53-53: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 102-102: ORIGINAL_PATH appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (3)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)
1-245: Overall script structure looks solid.The script implements a robust multi-strategy approach to launching CAMGI with good error handling, user feedback, and fallback options. Error messages are helpful, and permission handling is user-friendly. The SELinux configuration is consistent across container runtimes.
plugins/must-gather/skills/must-gather-analyzer/SKILL.md (1)
1-490: Documentation additions are comprehensive and well-structured.The new sections documenting additional analysis scripts (ingress, servicelogs, machineconfigpools, pod_logs, node_logs, ovn_dbs, camgi) follow the existing documentation patterns consistently. Examples are clear, and usage instructions align with the overall skill design. The new "Analysis Modes" section at the top provides good context for users.
plugins/must-gather/README.md (1)
1-582: Documentation expansion is comprehensive and well-organized.The additions for new analysis scripts (ingress, servicelogs, machineconfigpools, pod_logs, node_logs) and new commands (comprehensive-analysis, automation script) are well-structured and provide clear usage examples and output format descriptions. The quick reference link at line 9 helps users get started quickly. Previous issues with hardcoded user-specific paths have been resolved—all paths now use repository-relative references.
Prashanth684
force-pushed
the
mg-ai-helpers
branch
from
e1de04a to
54a872f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md (1)
47-47: Add language specifier to fenced code block.Line 47 has a fenced code block without a language identifier. Add a language specifier (e.g.,
```bashor```sh) for consistency with markdown standards.-``` +```bash [176x] Error message here</blockquote></details> <details> <summary>plugins/must-gather/commands/camgi.md (1)</summary><blockquote> `74-78`: **Minor wording improvement: Consider "inaccessible" for conciseness.** Line 74 uses "is not accessible" which could be streamlined to "inaccessible" for better readability, though this is purely stylistic. ```diff - If the web interface is not accessible: + If the web interface is inaccessible:plugins/must-gather/PLUGIN-SUMMARY.md (1)
44-44: Add language specifiers to fenced code blocks.Multiple fenced code blocks throughout the document lack language identifiers. While functional, these should include appropriate language specifiers (bash, yaml, etc.) for consistency with markdown linting standards:
-``` +```bash script examples hereConsider using `bash`, `yaml`, or `text` as appropriate for each code block to match the content type. Also applies to: 103-103, 200-200, 206-206, 212-212, 218-218, 226-226, 234-234, 253-253, 259-259, 268-268, 280-280, 294-294 </blockquote></details> <details> <summary>plugins/must-gather/commands/comprehensive-analysis.md (1)</summary><blockquote> `10-10`: **Add language specifiers to fenced code blocks.** Multiple fenced code blocks throughout the document lack language identifiers. Add appropriate language specifiers (bash, python, text, etc.) to all code blocks for consistency with markdown linting standards and improved syntax highlighting. ```diff -``` +```bash command exampleAlso applies to: 35-35, 50-50, 70-70, 88-88, 310-310, 477-477, 483-483, 489-489, 591-591, 602-602, 613-613, 624-624, 636-636, 647-647, 664-664, 674-674, 700-700, 717-717, 734-734 </blockquote></details> <details> <summary>plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)</summary><blockquote> `37-37`: **Declare and assign shell variables separately to avoid masking return values.** Lines 37 and 53 use `local containers=$(command)` syntax, which masks the return value of the command and makes error handling unreliable. Separate declaration from assignment. ```diff - local containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi) + local containers + containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi)Apply the same fix to line 53 for the docker command.
Also applies to: 53-53
📜 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)
plugins/must-gather/PLUGIN-SUMMARY.md(1 hunks)plugins/must-gather/README.md(4 hunks)plugins/must-gather/commands/camgi.md(1 hunks)plugins/must-gather/commands/comprehensive-analysis.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/SKILL.md(5 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh(1 hunks)
🧰 Additional context used
🪛 LanguageTool
plugins/must-gather/commands/comprehensive-analysis.md
[style] ~770-~770: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ive data removed 3. Log Truncation: Very large log files may be truncated in collectio...
(EN_WEAK_ADJECTIVE)
plugins/must-gather/commands/camgi.md
[style] ~74-~74: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:** If the web interface is not accessible: - The script opens http://127.0.0.1:80...
(NOT_ABLE_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/PLUGIN-SUMMARY.md
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
103-103: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
200-200: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
206-206: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
212-212: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
218-218: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
226-226: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
234-234: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
253-253: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
259-259: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
268-268: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
280-280: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
294-294: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/commands/comprehensive-analysis.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
70-70: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
310-310: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
477-477: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
483-483: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
489-489: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
591-591: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
602-602: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
613-613: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
624-624: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
636-636: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
647-647: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
664-664: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
674-674: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
700-700: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
717-717: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
734-734: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/README.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md
47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh
[warning] 37-37: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 53-53: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (16)
plugins/must-gather/skills/must-gather-analyzer/SKILL.md (2)
10-11: Good addition of comprehensive analysis triggers.The new triggers align well with the expanded plugin capabilities and help users discover the comprehensive analysis workflow.
194-352: Excellent documentation of new analysis scripts.All five new analysis scripts (ingress, servicelogs, machineconfigpools, pod_logs, node_logs), OVN databases, and CAMGI are well-documented with clear examples and output formats. Documentation follows consistent style and provides adequate context.
plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md (1)
1-217: Excellent quick-reference guide for must-gather analysis workflows.The document provides well-organized command cheatsheet, investigation workflows, and output format guidance. All new scripts and commands are properly referenced. Structure is intuitive for users seeking quick answers.
plugins/must-gather/commands/camgi.md (1)
1-206: Command documentation is complete and accurate.The camgi command documentation is well-structured with clear descriptions, prerequisites, error handling, technical details, examples, and additional resources. Past issues with path consistency have been resolved. Documentation correctly references script locations and provides comprehensive usage guidance.
plugins/must-gather/PLUGIN-SUMMARY.md (1)
1-100: Comprehensive plugin overview is well-structured and informative.The opening sections provide clear organization of the 14-script analysis suite with categories, key features, and automation tooling. The document effectively communicates the plugin's scope and design philosophy.
plugins/must-gather/commands/comprehensive-analysis.md (3)
1-100: Excellent command documentation with thorough prerequisites and error handling.The comprehensive-analysis command documentation provides clear structure, prerequisites, error handling, and implementation flow. Prerequisites section accurately describes the must-gather directory structure and required scripts. Error handling section provides practical guidance for common failure scenarios.
102-289: 5-phase analysis workflow is well-documented with clear cross-referencing.All five phases are thoroughly documented with explicit script invocations, expected outputs, and cross-referencing guidance. The analysis guidelines (lines 254-289) provide essential context for proper interpretation of results, including important guidance on validating event status against current component state.
588-710: Common patterns and tips provide valuable investigation guidance.The document includes six well-documented investigation patterns (degraded operator, node NotReady, pod CrashLoopBackOff, network issues, etcd problems, FailedScheduling events) with clear symptom-to-investigation pathways. Critical emphasis on event validation (lines 700-704) correctly instructs users to cross-reference events against current component status before concluding failure—excellent attention to Kubernetes operational context.
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (4)
1-91: Script structure and argument handling are well-organized.The script uses clear color definitions, proper error handling, and appropriate argument parsing. Help text is informative and the stop_camgi() function is comprehensive, checking both podman and docker for running CAMGI containers.
93-146: Path resolution and permission handling logic is solid.The must-gather path validation correctly detects the subdirectory structure and attempts auto-discovery if needed. Permission checking with user prompt and optional fix is user-friendly and appropriate for must-gather data (which should not contain secrets).
155-219: Multi-method fallback strategy for CAMGI execution is well-designed.The run_camgi() function provides three execution paths in sensible priority order: local installation, Python module, then containerized. Each method includes appropriate user feedback and browser launch attempts. The error message when all methods fail (lines 205-218) provides helpful guidance with installation options and links to documentation.
187-187: SELinux handling is consistent across container runtimes.Both podman (line 187) and docker (line 200) use the
:Zflag for SELinux relabeling, ensuring consistent permission handling across different container runtime options.Also applies to: 200-200
plugins/must-gather/README.md (4)
1-50: README introduction and quick-start guidance are clear and helpful.The opening sections clearly communicate the plugin's purpose, direct users to the QUICK-REFERENCE guide, and provide an overview of skills and analysis scripts. The quick-start approach improves discoverability.
231-420: Comprehensive documentation of new analysis scripts is thorough and consistent.All new analysis scripts (ingress, servicelogs, machineconfigpools, pod_logs, node_logs, ovn_dbs) are well-documented with usage examples, output formats, and use cases. Each section follows a consistent pattern and provides practical guidance. The OVN database section (lines 290-335) includes important information about requirements, features, and use cases.
422-454: New comprehensive-analysis command and automation script are well-documented.The
/comprehensive-analysiscommand documentation clearly describes the 5-phase workflow and report generation. References toplugins/must-gather/commands/comprehensive-analysis.mdprovide users with access to full details. The automation script section shows both direct usage and via slash command, with clear output expectations.
523-564: Usage examples for new analysis capabilities are practical and discoverable.The usage examples section now includes proper coverage of ingress/routes, service logs, MachineConfigPools, pod logs, and node logs. Each example shows realistic user queries that Claude will understand, improving discoverability of new features.
Prashanth684
force-pushed
the
mg-ai-helpers
branch
from
54a872f to
24c1682
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py (1)
29-43: Critical: Acceptkind: Listfrom route YAML files.The past review comment about supporting
kind: Listdoes not appear to be resolved. The current code only acceptsRouteList(line 35) andRoute(line 39), butoc get route -o yamlproduceskind: List. This will cause the analyzer to drop all routes and report "No Routes found."Apply this fix:
def parse_routes(file_path: Path) -> List[Dict[str, Any]]: """Parse routes from a RouteList YAML file.""" routes = [] try: with open(file_path, 'r') as f: doc = yaml.safe_load(f) - if doc and doc.get('kind') == 'RouteList': + if doc and doc.get('kind') in ('RouteList', 'List'): items = doc.get('items') if items is not None: routes.extend(items) elif doc and doc.get('kind') == 'Route': routes.append(doc) except Exception as e: print(f"Warning: Failed to parse {file_path}: {e}", file=sys.stderr) return routes
🧹 Nitpick comments (4)
plugins/must-gather/skills/must-gather-analyzer/SKILL.md (1)
351-351: Consider wrapping bare URLs in angle brackets for better markdown compliance.Static analysis flagged bare URLs on lines 351 and 443. While functional, wrapping them in angle brackets improves markdown compliance.
Apply these fixes:
- Runs on http://127.0.0.1:8080 + Runs on <http://127.0.0.1:8080>Also applies to line 443.
Also applies to: 443-443
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)
37-37: Consider separating variable declaration and assignment to avoid masking return values.Lines 37 and 53 combine
localdeclaration with command substitution, which can mask the command's return value. While not critical in this context (since you check if the variable is non-empty), separating them improves error handling visibility.Apply this pattern:
- local containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi) + local containers + containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi)Based on static analysis.
Also applies to: 53-53
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py (2)
181-181: Consider using explicitOptionaltype hints for better PEP 484 compliance.Lines 181 and 290 use implicit
Optional(e.g.,namespace_filter: str = None). PEP 484 recommends explicitOptional[str]orstr | None.Apply these fixes:
+from typing import Dict, List, Any, Optional -def print_route_table(route_list: List[Dict[str, str]], namespace_filter: str = None): +def print_route_table(route_list: List[Dict[str, str]], namespace_filter: Optional[str] = None):-def analyze_routes(must_gather_path: str, namespace: str = None, problems_only: bool = False): +def analyze_routes(must_gather_path: str, namespace: Optional[str] = None, problems_only: bool = False):Based on static analysis.
Also applies to: 290-290
228-228: Remove extraneousfprefixes from strings without placeholders.Lines 228, 284, and 343 use
fprefix on strings that contain no formatting placeholders.Apply these fixes:
- print(f" Conditions:") + print(" Conditions:")- print(f" ✅ No issues found") + print(" ✅ No issues found")- print(f" ✅ All routes admitted") + print(" ✅ All routes admitted")Based on static analysis.
Also applies to: 284-284, 343-343
📜 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/must-gather/PLUGIN-SUMMARY.md(1 hunks)plugins/must-gather/README.md(4 hunks)plugins/must-gather/commands/camgi.md(1 hunks)plugins/must-gather/commands/comprehensive-analysis.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/SKILL.md(5 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh(1 hunks)
🧰 Additional context used
🪛 LanguageTool
plugins/must-gather/PLUGIN-SUMMARY.md
[grammar] ~139-~139: Did you mean “because”?
Context: ...ailed Diagnostics Log analysis for root cause - Service logs (systemd) - Pod logs (co...
(CAUSE_BECAUSE)
[uncategorized] ~172-~172: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...## 5. Familiar Format Use oc-like output so OpenShift users immediately understand ...
(COMMA_COMPOUND_SENTENCE_2)
plugins/must-gather/commands/camgi.md
[grammar] ~47-~47: The verb “read” needs to be in the to-infinitive form.
Context: ...File Permissions:* Must-gather files need read permissions for the container user. The...
(MISSING_TO_BEFORE_A_VERB)
[uncategorized] ~58-~58: Possible missing comma found.
Context: ... -R a+r`). This is safe for must-gather data which should not contain secrets. **Ma...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~74-~74: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:** If the web interface is not accessible: - The script opens http://127.0.0.1:80...
(NOT_ABLE_PREMIUM)
[uncategorized] ~93-~93: Possible missing preposition found.
Context: ...provided as argument, ask the user - Accept either root must-gather directory or ex...
(AI_HYDRA_LEO_MISSING_TO)
[grammar] ~105-~105: The word ‘install’ is not a noun.
Context: ...erized version (podman/docker) or local install - Open browser automatically at http...
(A_INSTALL)
[grammar] ~190-~190: The verb “read” needs to be in the to-infinitive form.
Context: ...File Permissions: Must-gather files need read permissions for the container user - **...
(MISSING_TO_BEFORE_A_VERB)
plugins/must-gather/commands/comprehensive-analysis.md
[grammar] ~276-~276: It seems that “to” is missing before the verb.
Context: ...onsistent - some errors are transient - Look for persistent error patterns, not isol...
(MISSING_TO_BETWEEN_BE_AND_VB)
[uncategorized] ~308-~308: Possible missing comma found.
Context: ... ## Return Value The command outputs a comprehensive structured report to stdout: ``` =====...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~480-~480: Possible missing article found.
Context: ...analysis scripts in order and generates comprehensive report with executive summary, issues, ...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~686-~686: Possible missing preposition found.
Context: ...ne - Check when issues first appeared - Correlate with cluster updates or configuration c...
(AI_HYDRA_LEO_MISSING_TO)
[duplication] ~707-~707: Possible typo: you repeated a word.
Context: ...lure Focus on: - Persistent high-count errors - Errors correlated with degraded state - Errors...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~770-~770: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ive data removed 3. Log Truncation: Very large log files may be truncated in collectio...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/skills/must-gather-analyzer/SKILL.md
351-351: Bare URL used
(MD034, no-bare-urls)
443-443: Bare URL used
(MD034, no-bare-urls)
plugins/must-gather/commands/camgi.md
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
103-103: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
200-200: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
206-206: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/commands/comprehensive-analysis.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
70-70: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
310-310: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
477-477: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
483-483: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
489-489: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
591-591: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
602-602: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
613-613: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
624-624: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
636-636: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
647-647: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
664-664: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
674-674: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
700-700: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
717-717: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
734-734: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
70-70: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.3)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py
24-24: Do not catch blind exception: Exception
(BLE001)
41-41: Do not catch blind exception: Exception
(BLE001)
73-73: Do not catch blind exception: Exception
(BLE001)
181-181: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
228-228: f-string without any placeholders
Remove extraneous f prefix
(F541)
284-284: f-string without any placeholders
Remove extraneous f prefix
(F541)
290-290: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
343-343: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 Shellcheck (0.11.0)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh
[warning] 37-37: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 53-53: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (6)
plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md (1)
1-219: LGTM! Comprehensive quick reference documentation.This quick reference provides excellent coverage of all analysis scripts with clear examples and workflows. The organization by category makes it easy to find relevant commands.
plugins/must-gather/commands/camgi.md (1)
1-207: LGTM! Comprehensive CAMGI command documentation.This documentation provides thorough coverage of the CAMGI command including prerequisites, error handling, implementation details, and examples. The structure is clear and user-friendly.
plugins/must-gather/commands/comprehensive-analysis.md (1)
1-795: LGTM! Excellent comprehensive analysis documentation.This documentation provides outstanding detail on the 5-phase analysis workflow, including specific script invocations, cross-referencing guidelines, and common issue patterns. The structure effectively guides users from high-level cluster health down to detailed diagnostics.
plugins/must-gather/PLUGIN-SUMMARY.md (1)
1-340: LGTM! Excellent plugin summary.This summary provides an outstanding high-level overview of the entire must-gather analyzer plugin, effectively communicating the scope, features, and design principles. The breakdown by script categories and analysis phases is particularly helpful.
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)
1-245: LGTM! Well-structured CAMGI launcher script.The script provides excellent user experience with:
- Multiple execution strategies (local install, Python module, containerized)
- Permission checking and automatic fixing
- Browser auto-launch
- Clear error messages and guidance
- Proper cleanup with --rm flag
The implementation is robust and user-friendly.
plugins/must-gather/README.md (1)
9-9: LGTM! Comprehensive README updates.The README updates effectively document all new analysis scripts (ingress, service logs, MachineConfigPools, pod logs, node logs) with consistent formatting, clear examples, and helpful output samples. The additions for comprehensive analysis and automation scripts are well-integrated.
Also applies to: 231-375, 422-590
Prashanth684
force-pushed
the
mg-ai-helpers
branch
from
24c1682 to
e7a6406
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py (1)
35-40: Handlekind: Listroute dumps
oc get route -o yamlemitskind: List, so this logic still throws away every route in that common case and the analyzer reports nothing. Please treat bothListandRouteListas aggregates before falling back to singleRoutedocs.Apply this diff:
- doc = yaml.safe_load(f) - if doc and doc.get('kind') == 'RouteList': - items = doc.get('items') - if items is not None: - routes.extend(items) - elif doc and doc.get('kind') == 'Route': - routes.append(doc) + doc = yaml.safe_load(f) + if not doc: + return routes + + kind = doc.get('kind') + if kind in ('RouteList', 'List'): + items = doc.get('items') or [] + routes.extend(items) + elif kind == 'Route': + routes.append(doc)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_pod_logs.py (1)
78-85: Include kube Fatal prefix in severity detectionKubernetes fatal lines start with
F####, but this branch only recognizes theE####prefix. Any fatal entry without a literal “fatal” word will slip through and the analyzer will miss the most critical failures. Please treat bothE####andF####as errors.Apply this diff:
- if re.search(r'\b(ERROR|Error|error|FATAL|Fatal|fatal|CRIT|Critical|PANIC|Panic|panic)\b', line) or re.match(r'^E\d{4}', line): + if ( + re.search(r'\b(ERROR|Error|error|FATAL|Fatal|fatal|CRIT|Critical|PANIC|Panic|panic)\b', line) + or re.match(r'^[EF]\d{4}', line) + ):
🧹 Nitpick comments (6)
plugins/must-gather/commands/comprehensive-analysis.md (2)
10-12: Add language identifiers to fenced code blocks for better rendering.Multiple fenced code blocks lack language specifications (MD040 violations), which reduces syntax highlighting clarity. Examples include:
- Lines 10-12: should be
```bash- Lines 35-43: should be
```yamlor```text- Lines 50-66: should be
```textApply this pattern to all fenced code blocks:
-``` +```bash code hereAlso applies to: 35-43, 50-66, 88-90, 310-312, 477-479, 483-485 --- `276-278`: **Minor: Improve sentence structure for clarity.** Line 276 reads better with improved punctuation: "Kubernetes is eventually consistent — some errors are transient. Look for persistent error patterns..." Apply this fix: ```diff -Context Matters: -- Kubernetes is eventually consistent - some errors are transient -- Look for persistent error patterns, not isolated occurrences +Context Matters: +- Kubernetes is eventually consistent — some errors are transient. +- Look for persistent error patterns, not isolated occurrences.plugins/must-gather/skills/must-gather-analyzer/SKILL.md (1)
10-10: Add language identifiers to fenced code blocks (MD040).Several code blocks lack language specifications. Apply language tags for better rendering consistency.
Also applies to: 310-310, 477-477, 483-483
plugins/must-gather/README.md (1)
249-249: Add language identifiers to fenced code blocks (MD040).Multiple code blocks lack language specifications. For example, lines 249, 278, 302 should specify the language (e.g.,
```bashor```text).Also applies to: 278-278, 302-302, 326-326, 365-365, 425-425, 568-568, 573-573
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (2)
37-37: Optional: Separate variable declaration from assignment (SC2155).Shellcheck recommends declaring variables separately from assignment to avoid masking command failures:
- local containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi) + local containers + containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi)This is less critical for
podman ps(which returns exit 0 with empty results) but follows safer bash practices.Also applies to: 53-53
184-184: Optional: Add portability for browser opening across operating systems.The script uses
xdg-openwhich is Linux-specific. On macOS and other systems, this command will not exist. While the error is suppressed (making the script non-blocking), it silently fails to open the browser on non-Linux systems.Consider improving portability:
# Function to open browser portably open_browser() { local url="$1" if command_exists xdg-open; then xdg-open "$url" 2>/dev/null & elif command_exists open; then open "$url" 2>/dev/null & fi } # Then use in containerized blocks: (sleep 2 && open_browser http://127.0.0.1:8080) &This allows the script to work on Linux (xdg-open), macOS (open), and other systems gracefully.
Also applies to: 198-198
📜 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/must-gather/PLUGIN-SUMMARY.md(1 hunks)plugins/must-gather/README.md(4 hunks)plugins/must-gather/commands/camgi.md(1 hunks)plugins/must-gather/commands/comprehensive-analysis.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/SKILL.md(5 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_pod_logs.py(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh(1 hunks)
🧰 Additional context used
🪛 LanguageTool
plugins/must-gather/PLUGIN-SUMMARY.md
[grammar] ~139-~139: Did you mean “because”?
Context: ...ailed Diagnostics Log analysis for root cause - Service logs (systemd) - Pod logs (co...
(CAUSE_BECAUSE)
[uncategorized] ~172-~172: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...## 5. Familiar Format Use oc-like output so OpenShift users immediately understand ...
(COMMA_COMPOUND_SENTENCE_2)
plugins/must-gather/commands/camgi.md
[grammar] ~47-~47: The verb “read” needs to be in the to-infinitive form.
Context: ...File Permissions:* Must-gather files need read permissions for the container user. The...
(MISSING_TO_BEFORE_A_VERB)
[uncategorized] ~58-~58: Possible missing comma found.
Context: ... -R a+r`). This is safe for must-gather data which should not contain secrets. **Ma...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~74-~74: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:** If the web interface is not accessible: - The script opens http://127.0.0.1:80...
(NOT_ABLE_PREMIUM)
[uncategorized] ~93-~93: Possible missing preposition found.
Context: ...provided as argument, ask the user - Accept either root must-gather directory or ex...
(AI_HYDRA_LEO_MISSING_TO)
[grammar] ~105-~105: The word ‘install’ is not a noun.
Context: ...erized version (podman/docker) or local install - Open browser automatically at http...
(A_INSTALL)
[grammar] ~190-~190: The verb “read” needs to be in the to-infinitive form.
Context: ...File Permissions: Must-gather files need read permissions for the container user - **...
(MISSING_TO_BEFORE_A_VERB)
plugins/must-gather/commands/comprehensive-analysis.md
[grammar] ~276-~276: It seems that “to” is missing before the verb.
Context: ...onsistent - some errors are transient - Look for persistent error patterns, not isol...
(MISSING_TO_BETWEEN_BE_AND_VB)
[uncategorized] ~308-~308: Possible missing comma found.
Context: ... ## Return Value The command outputs a comprehensive structured report to stdout: ``` =====...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~480-~480: Possible missing article found.
Context: ...analysis scripts in order and generates comprehensive report with executive summary, issues, ...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~686-~686: Possible missing preposition found.
Context: ...ne - Check when issues first appeared - Correlate with cluster updates or configuration c...
(AI_HYDRA_LEO_MISSING_TO)
[duplication] ~707-~707: Possible typo: you repeated a word.
Context: ...lure Focus on: - Persistent high-count errors - Errors correlated with degraded state - Errors...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~770-~770: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ive data removed 3. Log Truncation: Very large log files may be truncated in collectio...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/PLUGIN-SUMMARY.md
33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
75-75: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
201-201: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
221-221: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
249-249: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
278-278: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
302-302: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
326-326: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/commands/camgi.md
33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
75-75: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
201-201: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/README.md
249-249: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
278-278: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
302-302: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
326-326: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
365-365: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
425-425: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
568-568: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
573-573: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/skills/must-gather-analyzer/SKILL.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
310-310: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
477-477: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
483-483: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/commands/comprehensive-analysis.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
70-70: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
310-310: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
477-477: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
483-483: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
489-489: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
591-591: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
602-602: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
613-613: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
624-624: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
636-636: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
647-647: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
664-664: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
674-674: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
700-700: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
717-717: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
734-734: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md
33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
75-75: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
201-201: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.3)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py
24-24: Do not catch blind exception: Exception
(BLE001)
41-41: Do not catch blind exception: Exception
(BLE001)
73-73: Do not catch blind exception: Exception
(BLE001)
181-181: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
228-228: f-string without any placeholders
Remove extraneous f prefix
(F541)
284-284: f-string without any placeholders
Remove extraneous f prefix
(F541)
290-290: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
343-343: f-string without any placeholders
Remove extraneous f prefix
(F541)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_pod_logs.py
86-86: Do not catch blind exception: Exception
(BLE001)
98-98: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
98-98: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
99-99: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
204-204: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
204-204: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
205-205: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
257-257: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 Shellcheck (0.11.0)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh
[warning] 37-37: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 53-53: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (12)
plugins/must-gather/commands/comprehensive-analysis.md (2)
1-12: Command documentation structure is clear and properly formatted.The frontmatter, command name, and synopsis follow established conventions. The description concisely conveys the command's purpose and value.
254-287: Excellent guidance on event validation and Kubernetes behavior.The section on cross-referencing events with current state (lines 280-287) provides crucial context that prevents misinterpretation of transient failures. The concrete examples clarify when "FailedScheduling events" indicate resolved vs. ongoing issues.
plugins/must-gather/skills/must-gather-analyzer/SKILL.md (3)
18-29: New "Analysis Modes" section adds valuable context.The distinction between "Quick Analysis" and "Comprehensive Analysis" modes clarifies the two workflows available to users. References to the comprehensive-analysis.md documentation are helpful.
194-352: New analysis script sections are comprehensive and well-structured.Each script documentation includes:
- Clear command syntax with multiple usage patterns
- Output format examples
- Important prerequisites and limitations
- Use cases and scenarios
The inclusion of notes about master-only collection (line 234) and gzip extraction (line 300) helps users understand data availability constraints.
473-482: New scenarios provide practical investigation workflows.The autoscaler and OVN network scenarios address real operational challenges with clear investigation paths using the appropriate analysis scripts.
plugins/must-gather/README.md (4)
9-9: Quick Start link improves discoverability.Adding the QUICK-REFERENCE.md link at the top of the README provides immediate access to command cheat sheets for users in a hurry.
23-23: Path references are properly relative and repository-agnostic.All file paths use relative repository paths (e.g.,
plugins/must-gather/...) rather than hardcoded user-specific paths, making the documentation portable and addressing the feedback from the previous review.Also applies to: 444-444, 462-462, 472-472, 579-579
231-374: New analysis script documentation is comprehensive and well-formatted.Each section clearly documents:
- Command syntax with multiple usage patterns
- Output format with concrete examples
- Important operational notes (master-only collection, gzip extraction)
- Multiple practical use cases
523-589: Usage examples thoroughly cover new analysis capabilities.Examples clearly demonstrate how to request analysis of each component type, with both command syntax and natural language prompts. The distinction between quick analysis and comprehensive analysis is well-illustrated.
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (3)
115-146: Permission checking and fixing logic is well-implemented.The script appropriately:
- Checks for restrictive permissions using
find ... ! -perm -004- Provides user choice to fix or skip
- Uses
chmod -R a+rcorrectly- Includes error handling and helpful messages
154-219: Robust multi-method execution strategy with good fallback handling.The script intelligently tries multiple execution approaches:
- Local
okd-camgiinstallation (preferred)- Python module via
python3 -m(fallback)- Containerized version (last resort)
With clear error messages and installation guidance when all methods fail, users have a clear path to resolution.
1-15: Script framework and setup are well-structured.The use of
set -uo pipefail, color definitions, and thecommand_exists()helper function demonstrate good bash scripting practices. These foundations support the robust error handling throughout the script.
| **CRITICAL: Script-Only Analysis** | ||
|
|
||
| - **NEVER** attempt to analyze must-gather data directly using bash commands, grep, or manual file reading | ||
| - **ONLY** use the provided Python scripts in `plugins/must-gather/skills/must-gather-analyzer/scripts/` | ||
| - If scripts are missing or not found: | ||
| 1. Stop immediately | ||
| 2. Inform the user that the analysis scripts are not available | ||
| 3. Ask the user to ensure the scripts are installed at the correct path | ||
| 4. Do NOT attempt alternative approaches |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Convert emphasis to proper Markdown heading.
Line 70 uses emphasis (CRITICAL) instead of a Markdown heading, which violates MD036 conventions and reduces navigability of the documentation.
Apply this fix:
-**CRITICAL: Script-Only Analysis**
+### CRITICAL: Script-Only Analysis📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **CRITICAL: Script-Only Analysis** | |
| - **NEVER** attempt to analyze must-gather data directly using bash commands, grep, or manual file reading | |
| - **ONLY** use the provided Python scripts in `plugins/must-gather/skills/must-gather-analyzer/scripts/` | |
| - If scripts are missing or not found: | |
| 1. Stop immediately | |
| 2. Inform the user that the analysis scripts are not available | |
| 3. Ask the user to ensure the scripts are installed at the correct path | |
| 4. Do NOT attempt alternative approaches | |
| ### CRITICAL: Script-Only Analysis | |
| - **NEVER** attempt to analyze must-gather data directly using bash commands, grep, or manual file reading | |
| - **ONLY** use the provided Python scripts in `plugins/must-gather/skills/must-gather-analyzer/scripts/` | |
| - If scripts are missing or not found: | |
| 1. Stop immediately | |
| 2. Inform the user that the analysis scripts are not available | |
| 3. Ask the user to ensure the scripts are installed at the correct path | |
| 4. Do NOT attempt alternative approaches |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
70-70: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
plugins/must-gather/commands/comprehensive-analysis.md around lines 70 to 78:
the section title is currently formatted as bold emphasis (**CRITICAL:
Script-Only Analysis**) which violates MD036 and should be a proper Markdown
heading; replace the bold text with an appropriate heading (e.g., "## CRITICAL:
Script-Only Analysis" or another consistent heading level used in the document),
keeping the same text and following content unchanged so the lists and
instructions remain intact.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Prashanth684
force-pushed
the
mg-ai-helpers
branch
from
e7a6406 to
20030f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
plugins/must-gather/commands/comprehensive-analysis.md (1)
70-70: Convert emphasis to proper Markdown headingLine 70 uses emphasis (
**CRITICAL: Script-Only Analysis**) instead of a Markdown heading, which reduces document navigability and violates MD036 conventions.Apply this fix:
-**CRITICAL: Script-Only Analysis** +### CRITICAL: Script-Only Analysisplugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py (1)
29-43: Critical:kind: Liststill not handled, breaking route parsingThis issue was flagged in a previous review but remains unfixed.
oc get route -o yamlproduceskind: List, notRouteList, so line 35 drops every route object and the analyzer prints "No Routes found" even when routes exist. This breaks the command entirely.Apply this diff to accept both
ListandRouteList:-def parse_routes(file_path: Path) -> List[Dict[str, Any]]: - """Parse routes from a RouteList YAML file.""" - routes = [] - try: - with open(file_path, 'r') as f: - doc = yaml.safe_load(f) - if doc and doc.get('kind') == 'RouteList': - items = doc.get('items') - if items is not None: - routes.extend(items) - elif doc and doc.get('kind') == 'Route': - routes.append(doc) - except Exception as e: - print(f"Warning: Failed to parse {file_path}: {e}", file=sys.stderr) - return routes +def parse_routes(file_path: Path) -> List[Dict[str, Any]]: + """Parse routes from a RouteList or List YAML file.""" + routes = [] + try: + with open(file_path, 'r') as f: + doc = yaml.safe_load(f) + if not doc: + return routes + + kind = doc.get('kind') + if kind in ('RouteList', 'List'): + items = doc.get('items') or [] + routes.extend(items) + elif kind == 'Route': + routes.append(doc) + except Exception as e: + print(f"Warning: Failed to parse {file_path}: {e}", file=sys.stderr) + return routes
🧹 Nitpick comments (9)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)
35-35: Split declaration and assignment to avoid masking return valuesCombining
localdeclaration with command substitution masks the return value of the command. Ifpodman psordocker psfails, the script won't detect the error becauselocalalways returns 0.Apply this fix:
- local containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi) + local containers + containers=$(podman ps -q --filter ancestor=quay.io/elmiko/okd-camgi)And similarly for the docker section:
- local containers=$(docker ps -q --filter ancestor=quay.io/elmiko/okd-camgi) + local containers + containers=$(docker ps -q --filter ancestor=quay.io/elmiko/okd-camgi)As per coding guidelines.
Also applies to: 50-50
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_pod_logs.py (1)
257-257: Remove unnecessary f-string prefixLine 257 uses an f-string without any placeholders, which is unnecessary.
Apply this fix:
- print(f" ✅ No issues found!") + print(" ✅ No issues found!")plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py (7)
17-26: Consider more specific exception typesThe broad
Exceptioncatch ensures robustness when parsing potentially malformed must-gather files, but catchingyaml.YAMLErrorandIOErrorspecifically would make error handling clearer and avoid masking unexpected bugs.Apply this diff for more precise error handling:
- except Exception as e: - print(f"Warning: Failed to parse {file_path}: {e}", file=sys.stderr) + except (yaml.YAMLError, IOError, OSError) as e: + print(f"Warning: Failed to parse {file_path}: {e}", file=sys.stderr) + except Exception as e: + print(f"Warning: Unexpected error parsing {file_path}: {e}", file=sys.stderr)
54-74: Narrow exception handling to datetime errorsThe bare
Exceptioncatch suppresses all errors, including potential bugs in the duration calculation logic. Catching specific exceptions likeValueErrorandAttributeErrorwould be more appropriate.Apply this diff:
- except Exception: - return "" + except (ValueError, AttributeError, TypeError): + return ""
181-181: Use explicitOptionaltype hintPEP 484 requires explicit
Optional[str]orstr | Nonefor parameters that default toNone. This improves type checker accuracy.Apply this diff:
-def print_route_table(route_list: List[Dict[str, str]], namespace_filter: str = None): +def print_route_table(route_list: List[Dict[str, str]], namespace_filter: Optional[str] = None):
290-290: Use explicitOptionaltype hintThe
namespaceparameter should be explicitly typed asOptional[str]orstr | Noneper PEP 484.Apply this diff:
-def analyze_routes(must_gather_path: str, namespace: str = None, problems_only: bool = False): +def analyze_routes(must_gather_path: str, namespace: Optional[str] = None, problems_only: bool = False):
228-228: Remove unnecessary f-string prefixLine 228 has no placeholders, so the
fprefix is unnecessary.- print(f" Conditions:") + print(" Conditions:")
284-284: Remove unnecessary f-string prefixLine 284 has no placeholders, so the
fprefix is unnecessary.- print(f" ✅ No issues found") + print(" ✅ No issues found")
343-343: Remove unnecessary f-string prefixLine 343 has no placeholders, so the
fprefix is unnecessary.- print(f" ✅ All routes admitted") + print(" ✅ All routes admitted")
📜 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/must-gather/PLUGIN-SUMMARY.md(1 hunks)plugins/must-gather/README.md(4 hunks)plugins/must-gather/commands/camgi.md(1 hunks)plugins/must-gather/commands/comprehensive-analysis.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/SKILL.md(5 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_pod_logs.py(1 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh(1 hunks)
🧰 Additional context used
🪛 LanguageTool
plugins/must-gather/PLUGIN-SUMMARY.md
[grammar] ~139-~139: Did you mean “because”?
Context: ...ailed Diagnostics Log analysis for root cause - Service logs (systemd) - Pod logs (co...
(CAUSE_BECAUSE)
[uncategorized] ~172-~172: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...## 5. Familiar Format Use oc-like output so OpenShift users immediately understand ...
(COMMA_COMPOUND_SENTENCE_2)
plugins/must-gather/commands/camgi.md
[grammar] ~47-~47: The verb “read” needs to be in the to-infinitive form.
Context: ...File Permissions:* Must-gather files need read permissions for the container user. The...
(MISSING_TO_BEFORE_A_VERB)
[uncategorized] ~58-~58: Possible missing comma found.
Context: ... -R a+r`). This is safe for must-gather data which should not contain secrets. **Ma...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~74-~74: Consider using “inaccessible” to avoid wordiness.
Context: ...oesn't Open:** If the web interface is not accessible: - The script opens http://127.0.0.1:80...
(NOT_ABLE_PREMIUM)
[uncategorized] ~93-~93: Possible missing preposition found.
Context: ...provided as argument, ask the user - Accept either root must-gather directory or ex...
(AI_HYDRA_LEO_MISSING_TO)
[grammar] ~105-~105: The word ‘install’ is not a noun.
Context: ...erized version (podman/docker) or local install - Open browser automatically at http...
(A_INSTALL)
[grammar] ~190-~190: The verb “read” needs to be in the to-infinitive form.
Context: ...File Permissions: Must-gather files need read permissions for the container user - **...
(MISSING_TO_BEFORE_A_VERB)
plugins/must-gather/commands/comprehensive-analysis.md
[grammar] ~276-~276: It seems that “to” is missing before the verb.
Context: ...onsistent - some errors are transient - Look for persistent error patterns, not isol...
(MISSING_TO_BETWEEN_BE_AND_VB)
[uncategorized] ~308-~308: Possible missing comma found.
Context: ... ## Return Value The command outputs a comprehensive structured report to stdout: ``` =====...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~480-~480: Possible missing article found.
Context: ...analysis scripts in order and generates comprehensive report with executive summary, issues, ...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~686-~686: Possible missing preposition found.
Context: ...ne - Check when issues first appeared - Correlate with cluster updates or configuration c...
(AI_HYDRA_LEO_MISSING_TO)
[duplication] ~707-~707: Possible typo: you repeated a word.
Context: ...lure Focus on: - Persistent high-count errors - Errors correlated with degraded state - Errors...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~770-~770: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ive data removed 3. Log Truncation: Very large log files may be truncated in collectio...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/skills/must-gather-analyzer/SKILL.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
310-310: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
477-477: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
483-483: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/PLUGIN-SUMMARY.md
33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
75-75: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
201-201: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
221-221: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
249-249: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
278-278: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
302-302: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
326-326: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/commands/camgi.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
70-70: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/must-gather/commands/comprehensive-analysis.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
70-70: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
310-310: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
477-477: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
483-483: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
489-489: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
591-591: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
602-602: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
613-613: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
624-624: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
636-636: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
647-647: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
664-664: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
674-674: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
700-700: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
717-717: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
734-734: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.3)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_pod_logs.py
86-86: Do not catch blind exception: Exception
(BLE001)
98-98: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
98-98: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
99-99: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
204-204: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
204-204: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
205-205: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
257-257: f-string without any placeholders
Remove extraneous f prefix
(F541)
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py
24-24: Do not catch blind exception: Exception
(BLE001)
41-41: Do not catch blind exception: Exception
(BLE001)
73-73: Do not catch blind exception: Exception
(BLE001)
181-181: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
228-228: f-string without any placeholders
Remove extraneous f prefix
(F541)
284-284: f-string without any placeholders
Remove extraneous f prefix
(F541)
290-290: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
343-343: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 Shellcheck (0.11.0)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh
[warning] 35-35: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 50-50: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (11)
plugins/must-gather/PLUGIN-SUMMARY.md (1)
1-340: LGTM - Comprehensive documentationThe plugin summary is well-structured and comprehensive. The script count is correctly stated as 14 throughout the document, and the documentation provides clear organization of analysis scripts, automation tools, and usage patterns.
plugins/must-gather/skills/must-gather-analyzer/QUICK-REFERENCE.md (1)
1-219: LGTM - Clear quick reference guideThe quick reference provides a well-organized command cheat sheet with examples for all analysis scripts and common investigation workflows. The structure makes it easy to find relevant commands quickly.
plugins/must-gather/skills/must-gather-analyzer/SKILL.md (1)
1-491: LGTM - Comprehensive skill documentationThe skill definition properly documents all the new analysis capabilities, including CAMGI integration, expanded analysis scripts, and new usage scenarios. The documentation is thorough and well-organized.
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)
66-241: Well-structured launcher with multiple fallback pathsThe script provides robust CAMGI launching with appropriate fallbacks (local install → Python module → container). The permission checking and auto-fix workflow, SELinux handling with
:Zflag, and user guidance are well-implemented.plugins/must-gather/commands/camgi.md (1)
1-207: LGTM - Clear CAMGI command documentationThe CAMGI command documentation is comprehensive and well-structured. It clearly explains prerequisites, error handling, implementation flow, and provides helpful usage examples. The technical details section is particularly useful.
plugins/must-gather/README.md (1)
1-653: LGTM - Comprehensive README updateThe README properly documents all new analysis capabilities with correct script counts (14 scripts), detailed usage examples for each new module, and clear explanations of the comprehensive analysis workflow. The documentation is thorough and well-organized.
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_pod_logs.py (3)
19-60: Well-designed error pattern extractionThe
extract_error_patternfunction uses multiple heuristics to intelligently extract meaningful error patterns from various log formats, including quoted errors, key-value patterns, kubelet-style errors, and panic messages. The fallback logic ensures robust pattern extraction.
62-96: Robust log file parsing with appropriate error handlingThe
parse_log_filefunction correctly identifies both kube-style severity prefixes (E\d{4},W\d{4}) and keyword-based patterns. The broad exception handling at line 86 is appropriate for log parsing where files may contain unexpected content.
263-315: Well-structured CLI interfaceThe main function provides a comprehensive argument parser with helpful examples, proper validation, and clear error messages. The interface design is consistent with other analysis scripts.
plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py (2)
320-331: Namespace filtering correctly implementedThe namespace filtering logic is now correctly applied before both the table display and summary calculation, ensuring the counts match the displayed data. This addresses the concern from the previous review.
385-399: CLI orchestration logic is well-structuredThe default behavior of showing both IngressControllers and Routes when no flags are specified is intuitive, and the exit code handling correctly propagates errors from either analysis function.
| # Extract namespace from path | ||
| parts = route_file.parts | ||
| ns_index = parts.index('namespaces') + 1 | ||
| if ns_index < len(parts): | ||
| ns = parts[ns_index] | ||
| route_list = parse_routes(route_file) | ||
| for route in route_list: | ||
| routes.append((ns, route)) |
There was a problem hiding this comment.
Handle missing 'namespaces' in path gracefully
Line 305 calls parts.index('namespaces') which raises ValueError if 'namespaces' is not in the path. If a route file is found that doesn't match the expected path structure, the entire analysis will crash.
Apply this diff to handle the error:
for route_file in base_path.glob(pattern):
# Extract namespace from path
parts = route_file.parts
- ns_index = parts.index('namespaces') + 1
- if ns_index < len(parts):
+ try:
+ ns_index = parts.index('namespaces') + 1
+ except ValueError:
+ print(f"Warning: Could not extract namespace from {route_file}", file=sys.stderr)
+ continue
+
+ if ns_index < len(parts):
ns = parts[ns_index]
route_list = parse_routes(route_file)
for route in route_list:
routes.append((ns, route))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Extract namespace from path | |
| parts = route_file.parts | |
| ns_index = parts.index('namespaces') + 1 | |
| if ns_index < len(parts): | |
| ns = parts[ns_index] | |
| route_list = parse_routes(route_file) | |
| for route in route_list: | |
| routes.append((ns, route)) | |
| # Extract namespace from path | |
| parts = route_file.parts | |
| try: | |
| ns_index = parts.index('namespaces') + 1 | |
| except ValueError: | |
| print(f"Warning: Could not extract namespace from {route_file}", file=sys.stderr) | |
| continue | |
| if ns_index < len(parts): | |
| ns = parts[ns_index] | |
| route_list = parse_routes(route_file) | |
| for route in route_list: | |
| routes.append((ns, route)) |
🤖 Prompt for AI Agents
In plugins/must-gather/skills/must-gather-analyzer/scripts/analyze_ingress.py
around lines 303 to 310, calling parts.index('namespaces') can raise ValueError
if the path does not contain 'namespaces'; change the logic to handle that case
by checking for 'namespaces' in parts (or wrapping the index call in a
try/except ValueError), and if it's missing skip that file (optionally log a
warning) so the analyzer does not crash, otherwise proceed to extract ns and
parse routes as before.
| If the browser didn't open automatically, click the URL above. | ||
| Use CAMGI to: |
There was a problem hiding this comment.
CAMGI has significantly more uses than these listed, are they worth enumerating?
There was a problem hiding this comment.
i highlighted the high level functionality. Are there docs on what else it can do?
|
Just looking at the size of this PR, and thinking about ease of review - do we think that splitting this up (e.g per command, so to 3 PRs?) could make sense?? |
| @@ -0,0 +1,219 @@ | |||
| # Must-Gather Analyzer - Quick Reference | |||
There was a problem hiding this comment.
I'm not sure we typically ship quick references with skills - is this an existing or external pattern? Would this make more sense in the README? Or being condensed and added there?
There was a problem hiding this comment.
given that we have a readme this is redundant in a way - but i thought it was neat that claude generated it. i'll remove it. too much noise around documenting is also not good.
| @@ -0,0 +1,340 @@ | |||
| # Must-Gather Analyzer Plugin - Complete Summary | |||
There was a problem hiding this comment.
Is this leftover from building out the additional skills? Typically we don't ship these :)
There was a problem hiding this comment.
Claude generated this and i thought it would be useful to have - not for the tool itself but for documentation and posterity - but i guess with the readme it is redundant. i will remove it.
makes sense. I will split it up and send reviews again. |
split the camgi part out of openshift-eng#133 to make it easier to review and more readable
split the camgi part out of openshift-eng#133 to make it easier to review and more readable
2 participants
This PR adds log analysis capabilities, automated comprehensive reporting, and CAMGI web UI integration to the must-gather plugin.
New Analysis Scripts:
Log Analysis:
Infrastructure:
Other:
All scripts feature pattern-based error extraction, occurrence counting, and oc-like output format.
New Commands:
Documentation:
Summary by CodeRabbit
New Features
Documentation
Commands