Add skill to launch CAMGI for must-gather #144
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 documentation PLUGINS.md, plugins/must-gather/README-CAMGI.md, plugins/must-gather/commands/camgi.md, plugins/must-gather/skills/must-gather-analyzer/SKILL.md |
Add comprehensive CAMGI docs: overview, quick start, usage examples, troubleshooting, integration into must-gather analyzer workflows, and references to the launcher script. |
Command metadata docs/data.json |
Register new camgi command in must-gather plugin (name, description, synopsis /must-gather:camgi [must-gather-path], argument hint `[must-gather-path |
Launcher script plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh |
New executable shell script implementing CAMGI orchestration: argument parsing, path discovery/validation, permission handling, stop command, execution fallback chain (local binary → python -m → container runtimes), container lifecycle handling, and optional browser auto-open. |
Sequence Diagram(s)
sequenceDiagram
participant User
participant Script as run-camgi.sh
participant Local as Local Binary / Python Module
participant Container as Podman/Docker
participant CAMGI as CAMGI Service
participant Browser
User->>Script: run-camgi.sh [must-gather-path|stop]
Script->>Script: parse args & validate path
alt stop
Script->>Container: find & stop CAMGI containers
Script-->>User: stopped / status
else start
Script->>Script: check permissions & must-gather layout
alt permission fix required
Script->>User: prompt to fix permissions
User-->>Script: approve/deny
end
rect rgb(230,245,255)
note right of Script: Execution fallback chain
Script->>Local: try okd-camgi binary
Local-->>Script: not available / success
Script->>Local: try python -m okd_camgi
Local-->>Script: not available / success
Script->>Container: run containerized CAMGI
end
Container->>CAMGI: start service (port 8080)
CAMGI-->>Script: ready
Script->>Browser: auto-open 127.0.0.1:8080 (if possible)
Script-->>User: report URL and status
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
- Pay special attention to
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh(argument parsing, fallback order, container run flags, SELinux:Zhandling, port and volume mounts). - Verify documentation files (
README-CAMGI.md,commands/camgi.md,SKILL.md,PLUGINS.md) accurately reflect script behavior (port, URL, permission prompts, stop semantics). - Check
docs/data.jsonentry correctness and formatting for tooling that consumes command metadata.
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 directly and clearly summarizes the main change: adding a skill to launch CAMGI for must-gather. It matches the changeset which adds CAMGI documentation, commands, and a launch script to the must-gather plugin. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
| No Real People Names In Style References | ✅ Passed | No references to real people's names found in plugin commands, skill documentation, example prompts, instructions, or style references. |
| No Assumed Git Remote Names | ✅ Passed | No hardcoded git remote names like 'origin' or 'upstream' found in PR files. Documentation and scripts properly avoid assuming specific git remote names. |
| Git Push Safety Rules | ✅ Passed | No git push commands, force push operations, or automated push workflows found in any modified files. |
| No Untrusted Mcp Servers | ✅ Passed | PR introduces CAMGI documentation and deployment scripts with no MCP server installations from untrusted 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.
| If the browser didn't open automatically, click the URL above. | ||
| Use CAMGI to: | ||
| - Examine cluster autoscaler configuration |
There was a problem hiding this comment.
@theobarberbany could you help me list out the other uses of camgi please?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
plugins/must-gather/README-CAMGI.md (2)
10-13: Add language identifiers to fenced code blocks.All code blocks should specify the language for proper syntax highlighting. Add
bashidentifiers to the bash code blocks.Examples:
- ``` + ```bash ./run-camgi.sh /path/to/must-gather - ``` + ```Apply this consistently to all bash blocks throughout the file (lines 10–13, 37–40, 54–56, 116–123, 136–140, 170–172).
Also applies to: 37-40, 54-56, 116-123, 136-140, 170-172
75-76: Wrap bare URLs in markdown link syntax.Several URLs are bare and should be wrapped in markdown link syntax for consistency and better rendering.
Examples:
- - CAMGI GitHub: https://github.com/elmiko/okd-camgi + - CAMGI GitHub: [https://github.com/elmiko/okd-camgi](https://github.com/elmiko/okd-camgi)Apply to lines 75–76, 106, and 160.
Also applies to: 106-106, 160-160
plugins/must-gather/commands/camgi.md (2)
47-47: Grammar: Fix verb form in file permissions description.Lines 47 and 190 should use the infinitive form "to read" instead of the noun "read".
Apply this diff:
-Must-gather files need read permissions for the container user. +Must-gather files need to read permissions for the container user.Actually, the better phrasing would be:
-Must-gather files need read permissions for the container user. +Must-gather files need read access for the container user.Or:
-Must-gather files need read permissions for the container user. +The container user needs read permissions for must-gather files.Also applies to: 190-190
58-58: Minor grammar improvements.
- Line 58: Add missing comma after closing parenthesis: "...which should not contain secrets**,**"
- Line 105: Change "local install" to "local installation" for proper noun form
Also applies to: 105-105
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (2)
35-35: Shellcheck SC2155: Separate variable declaration and assignment.Store the command output in a separate step to avoid masking return values and improve clarity.
Apply to lines 35 and 50:
- 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)Also applies to: 50-50
180-180: Browser opening viaxdg-openmay silently fail in non-graphical environments.Lines 180 and 194 attempt to open the browser via
xdg-open, but this tool is not available in headless systems, SSH sessions, or container environments. The error is silently suppressed (2>/dev/null), leaving the user uncertain whether the browser opened.Consider:
- Checking if
xdg-openis available before attempting to use it- Providing clearer messaging to the user that the browser opening was skipped
- Suggesting the user manually navigate to the URL if browser opening fails
This is a UX refinement; the functionality still works (server starts regardless), but users on headless systems won't know whether the browser opening succeeded.
Also applies to: 194-194
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
PLUGINS.md(1 hunks)docs/data.json(1 hunks)plugins/must-gather/README-CAMGI.md(1 hunks)plugins/must-gather/commands/camgi.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/SKILL.md(3 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh(1 hunks)
🧰 Additional context used
🪛 LanguageTool
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)
🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/README-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)
🪛 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 (2)
PLUGINS.md (1)
116-116: LGTM! Documentation addition is clear, well-placed, and follows the existing format.plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)
151-215: Strong implementation of multiple fallback methods with verified permission checking.The
run_camgi()function demonstrates good defensive programming with clear fallback paths (local binary → Python module → podman → docker) and helpful error messages guiding users through installation.Permission checking logic (lines 116–141) verified:
- The
find ... ! -perm -004command correctly identifies files without world-read permission- The
chmod -R a+rcommand safely uses the default behavior (does not follow symlinks by default; uses-Pimplicitly)- Error suppression via
2>/dev/nullis acceptable here since the script provides fallback messaging (lines 131–133) guiding users to manually fix permissions if needed- Edge cases with special files (sockets, FIFOs) are unlikely to block most must-gather archives
The logic is sound and handles missing dependencies gracefully.
| { | ||
| "name": "camgi", | ||
| "description": "Launch CAMGI (Cluster Autoscaler Must-Gather Inspector) web interface to analyze cluster autoscaler behavior", | ||
| "synopsis": "/must-gather:camgi [must-gather-path]", | ||
| "argument_hint": "[must-gather-path|stop]" | ||
| }, |
There was a problem hiding this comment.
Inconsistency between synopsis and argument_hint.
The synopsis shows only [must-gather-path] but argument_hint includes the stop option. These should be consistent. The synopsis should reflect both available options: [must-gather-path|stop].
Apply this diff to fix:
- "synopsis": "/must-gather:camgi [must-gather-path]",
+ "synopsis": "/must-gather:camgi [must-gather-path|stop]",📝 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.
| { | |
| "name": "camgi", | |
| "description": "Launch CAMGI (Cluster Autoscaler Must-Gather Inspector) web interface to analyze cluster autoscaler behavior", | |
| "synopsis": "/must-gather:camgi [must-gather-path]", | |
| "argument_hint": "[must-gather-path|stop]" | |
| }, | |
| { | |
| "name": "camgi", | |
| "description": "Launch CAMGI (Cluster Autoscaler Must-Gather Inspector) web interface to analyze cluster autoscaler behavior", | |
| "synopsis": "/must-gather:camgi [must-gather-path|stop]", | |
| "argument_hint": "[must-gather-path|stop]" | |
| }, |
🤖 Prompt for AI Agents
In docs/data.json around lines 545 to 550, the synopsis field for the "camgi"
entry is inconsistent with argument_hint; update the "synopsis" value to include
both options by changing it from "[must-gather-path]" to
"[must-gather-path|stop]" so it matches argument_hint.
split the camgi part out of openshift-eng#133 to make it easier to review and more readable
Prashanth684
force-pushed
the
ai-mg-camgi
branch
from
9e8da09 to
6ac0b31
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/data.json (1)
545-550: Inconsistency betweensynopsisandargument_hint(existing feedback).The
synopsisshows only[must-gather-path]butargument_hintincludes thestopoption. These should be consistent.Apply this diff to fix:
- "synopsis": "/must-gather:camgi [must-gather-path]", + "synopsis": "/must-gather:camgi [must-gather-path|stop]",
🧹 Nitpick comments (2)
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (2)
30-45: Fix Shellcheck SC2155 warning: declare and assign separately.Line 35 assigns to
containerswithin a pipeline, which masks the return value ofpodman ps. Split the declaration and assignment to properly capture the exit code.stop_camgi() { echo -e "${BLUE}Stopping camgi containers...${NC}" # Check for podman if command_exists podman; then - 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) if [ -n "$containers" ]; then
48-61: Fix Shellcheck SC2155 warning: declare and assign separately (docker check).Line 50 has the same issue as line 35. Split the declaration and assignment for docker.
# Check for docker if command_exists docker; then - 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) if [ -n "$containers" ]; then
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
PLUGINS.md(1 hunks)docs/data.json(1 hunks)plugins/must-gather/README-CAMGI.md(1 hunks)plugins/must-gather/commands/camgi.md(1 hunks)plugins/must-gather/skills/must-gather-analyzer/SKILL.md(3 hunks)plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- PLUGINS.md
🧰 Additional context used
🪛 LanguageTool
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)
🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/commands/camgi.md
21-21: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
plugins/must-gather/README-CAMGI.md
21-21: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🪛 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 (6)
plugins/must-gather/README-CAMGI.md (1)
1-173: Clear documentation for CAMGI integration and setup.The README provides comprehensive coverage of CAMGI features, prerequisites, troubleshooting, and technical details. The structure with Quick Start, Commands, Features, Installation, and Troubleshooting sections makes this easy to follow.
plugins/must-gather/skills/must-gather-analyzer/SKILL.md (3)
179-200: CAMGI section well-integrated into storage analysis.The new CAMGI section fits naturally after storage analysis and before the interpret-and-report phase. The documentation clearly explains when and how to use CAMGI for autoscaler investigation, with proper references to detailed docs.
261-265: CAMGI launcher script properly documented in helper scripts reference.The entry concisely documents the launcher script's purpose, output, prerequisites, and reference to detailed documentation.
293-296: New "Cluster autoscaler problems" workflow adds practical guidance.The new scenario flow (run CAMGI → inspect UI → review decisions) provides clear next steps for users troubleshooting autoscaler issues.
plugins/must-gather/commands/camgi.md (1)
1-202: Well-structured command documentation with clear implementation details.The documentation comprehensively covers prerequisites, error handling, implementation flow, and practical examples. The connection to the launcher script at
plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.shis clearly documented. Users have clear guidance on permissions, port conflicts, SELinux, and browser issues.plugins/must-gather/skills/must-gather-analyzer/scripts/run-camgi.sh (1)
150-215: Well-implemented CAMGI launcher with robust fallback chain.The
run_camgi()function methodically attempts multiple approaches (local binary → Python module → containerized) with clear error messaging at each stage. The use of--webbrowserflag for local execution and automatic browser opening for containers provides good UX. Permissions checking with interactive fixing is user-friendly.
1 participant
split the camgi part out of #133 to make it easier to review and more readable
Summary by CodeRabbit
New Features
Documentation