proposal for must-gather agentic debugging via MCP - #2064
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe proposal defines opt-in agentic must-gather analysis through Lightspeed and a shared MCP server. It covers API validation, serialized PVC processing, deployment constraints, adapter behavior, lifecycle rules, testing, and operations. ChangesAgentic debugging proposal
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant MustGather
participant MCPServer
participant Proposal
participant Lightspeed
participant AnalysisResult
MustGather->>MCPServer: Serve gathered data
MustGather->>Proposal: Create analysis proposal
Proposal->>Lightspeed: Request High-priority analysis
Lightspeed->>MCPServer: Query must-gather toolset
Lightspeed->>AnalysisResult: Produce analysis result
Suggested labels: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
enhancements/support-log-gather/must-gather-agentic-debugging.md (1)
125-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language identifiers to fenced blocks.
The CEL block and the argument block at Lines 188-191 omit fence languages, triggering markdownlint MD040. Use identifiers such as
celandyaml(or the repository-approved equivalents).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` around lines 125 - 127, Add language identifiers to the fenced CEL and argument blocks near the referenced rule, using the repository-approved identifiers such as cel and yaml, while leaving the block contents unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md`:
- Line 92: Update the MCP connection instructions to construct the service
hostname using the actual operator namespace from r.OperatorNamespace instead of
hardcoding must-gather-operator. Revise the surrounding operational examples so
the default namespace is presented only as an example, not a requirement.
- Around line 98-103: Update the MCP setup and Proposal creation flow described
in the Error Cases section so a Proposal is not created when MCP server creation
fails. Create the Proposal only after the MCP Deployment/Service is confirmed
ready, or preserve the setup failure for retry while skipping Proposal creation.
- Around line 207-211: Enforce the TechPreview feature gate in the reconcile
path before any MCP server or Proposal processing when agenticDebuggingEnabled
is true, using the existing gate mechanism and ignoring the field when the gate
is disabled; otherwise remove the TechPreview gating claim from the graduation
documentation.
- Around line 175-184: The MCP_SERVER_IMAGE configuration must not use an
unverified or mutable :latest default. Update the documented default in the MCP
server image configuration and the corresponding deployment guidance to use a
verified immutable published tag or digest; if none is available, require
MCP_SERVER_IMAGE to be explicitly provided.
- Around line 197-205: The shared MCP server design must enforce its single-PVC
limitation instead of treating Deployment AlreadyExists as sufficient. Update
ensureMCPServer() and the MustGather reconciliation flow to reject any CR
referencing a PVC different from the one already mounted, or partition server
instances so each PVC gets its own MCP Deployment; ensure Proposal path
resolution always uses the correct PVC.
- Around line 233-235: Align the primary flow’s stated IntelliAide pipeline
scope with the Proposal produced by createIntelliAideProposal(): either update
the generated instructions to run analyze_data.py and perform_rca.py for high,
medium, and low priorities, or explicitly limit the proposal’s goals, tests, and
graduation criteria to High-priority analysis only. Ensure all scope claims
consistently match the selected behavior.
- Around line 88-94: Update the documented MCP flow around ensureMCPServer(),
resolveMustGatherDataPath(), and createIntelliAideProposal() to enforce
collection isolation at the server boundary rather than relying on the prompt or
mustgather_use() path. Specify a per-request collection root, per-collection
credentials, or an isolated MCP instance so agents cannot access other PVC
collections and concurrent analyses remain separated; apply the same correction
to the corresponding section referenced by the additional lines.
---
Nitpick comments:
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md`:
- Around line 125-127: Add language identifiers to the fenced CEL and argument
blocks near the referenced rule, using the repository-approved identifiers such
as cel and yaml, while leaving the block contents unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f3e8a32-63d7-4f8b-bfbf-ac8d0aa44f8d
📒 Files selected for processing (1)
enhancements/support-log-gather/must-gather-agentic-debugging.md
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
d61f8c7 to
628e9e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md`:
- Around line 218-221: The stale Proposal handling around ensureMCPDeployment()
must not allow a PVC swap based solely on analysis.timeoutMinutes and
grace-period age. Before swapping, cancel or terminate the existing analysis and
confirm it has completed; alternatively, isolate each analysis with its own MCP
instance, while preserving the errMCPServerBusy retry and cleanup deferral until
the shared server is safe to reuse.
- Around line 215-221: Make the PVC ownership transition in ensureMCPDeployment
atomic across concurrent reconcilers by using a Kubernetes Lease or
resource-version compare-and-swap on the Deployment annotation. Ensure only the
reconciler that successfully claims the ownership update rewrites the volume and
creates its Proposal, while losing reconcilers return errMCPServerBusy and
preserve their deferred cleanup/requeue behavior. Add tests covering concurrent
reconciles and verifying that only the successful owner creates a Proposal.
- Around line 203-204: Update the shared MCP server deployment documentation to
require a dedicated ServiceAccount rather than reusing the must-gather-operator
ServiceAccount. Configure that account with automountServiceAccountToken: false,
while retaining PVC-only access and the disabled cluster-provider behavior;
mention narrowly scoped permissions only if future API access is introduced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 82c12159-ee07-4fe2-966f-e6e99de3bc41
📒 Files selected for processing (1)
enhancements/support-log-gather/must-gather-agentic-debugging.md
| No dedicated ServiceAccount is created for the shared MCP server. It runs under the must-gather-operator's own ServiceAccount (`must-gather-operator`), not the ServiceAccount configured on the MustGather CR's `spec.serviceAccountName` (which is used only by the gather Job). Since the MCP server only reads a mounted PVC and runs with `--cluster-provider disabled` (no live cluster API access), it needs no additional RBAC beyond what the operator's own SA already has. | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not reuse the operator ServiceAccount for MCP.
The MCP pod only needs PVC filesystem access; running it as the operator’s ServiceAccount unnecessarily exposes operator credentials and RBAC to an HTTP-facing third-party image. Use a dedicated ServiceAccount with automountServiceAccountToken: false (or grant only the minimum required permissions if API access is later needed).
🔒 Proposed direction
- No dedicated ServiceAccount is created for the shared MCP server.
- It runs under the must-gather-operator's own ServiceAccount.
+ Create a dedicated ServiceAccount for the shared MCP server.
+ Disable automatic token mounting; add narrowly scoped permissions only if
+ the server later requires Kubernetes API access.📝 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.
| No dedicated ServiceAccount is created for the shared MCP server. It runs under the must-gather-operator's own ServiceAccount (`must-gather-operator`), not the ServiceAccount configured on the MustGather CR's `spec.serviceAccountName` (which is used only by the gather Job). Since the MCP server only reads a mounted PVC and runs with `--cluster-provider disabled` (no live cluster API access), it needs no additional RBAC beyond what the operator's own SA already has. | |
| Create a dedicated ServiceAccount for the shared MCP server. It should not reuse the must-gather-operator's own ServiceAccount, and automatic token mounting should be disabled unless the server later needs Kubernetes API access. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` around
lines 203 - 204, Update the shared MCP server deployment documentation to
require a dedicated ServiceAccount rather than reusing the must-gather-operator
ServiceAccount. Configure that account with automountServiceAccountToken: false,
while retaining PVC-only access and the disabled cluster-provider behavior;
mention narrowly scoped permissions only if future API access is introduced.
628e9e1 to
fa9cc72
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
enhancements/support-log-gather/must-gather-agentic-debugging.md (1)
197-203: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRestrict access to the MCP HTTP service.
--cluster-provider disabledprevents live API access, not unauthorized HTTP callers. The design specifies neither authentication nor a NetworkPolicy, so any pod able to reach the Service could query sensitive MustGather data. Require an authenticated, allowlisted connection from the Lightspeed sandbox.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` around lines 197 - 203, Update the Shared MCP Server Design to require authenticated access to the MCP HTTP Service, restricted through an allowlist or NetworkPolicy to only the Lightspeed sandbox. Specify the authentication mechanism and required credentials/configuration, and ensure unauthenticated or non-allowlisted callers cannot query mounted MustGather data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md`:
- Around line 205-209: Enforce the operator-namespace requirement before
creating the MCP resources in the agenticDebuggingEnabled flow. Reject or
conditionally disable agentic processing when the MustGather CR namespace
differs from the operator namespace, while preserving normal gathering behavior
as appropriate. Add a negative test covering the namespace mismatch and
confirming no MCP Deployment or Proposal flow is created.
- Around line 309-320: Expand the Unit and Integration tests sections to
explicitly cover gate-disabled behavior, namespace mismatch, MCP readiness
before Proposal creation, atomic concurrent ownership, an active stale agent
during PVC swap, and unauthorized MCP access. Associate each case with the
relevant symbols or flows such as MustGather reconciliation, MCP readiness
checks, proposal ownership, PVC-swap guards, and MCP authorization, while
preserving the existing test coverage.
---
Outside diff comments:
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md`:
- Around line 197-203: Update the Shared MCP Server Design to require
authenticated access to the MCP HTTP Service, restricted through an allowlist or
NetworkPolicy to only the Lightspeed sandbox. Specify the authentication
mechanism and required credentials/configuration, and ensure unauthenticated or
non-allowlisted callers cannot query mounted MustGather data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b5281a2f-ce22-4e38-826f-da636b382e07
📒 Files selected for processing (1)
enhancements/support-log-gather/must-gather-agentic-debugging.md
fa9cc72 to
a724236
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (7)
enhancements/support-log-gather/must-gather-agentic-debugging.md (7)
92-92: 🎯 Functional Correctness | 🟠 MajorBuild the MCP URL from the actual operator namespace.
Line 92 and the support commands use
must-gather-operatoras a fixed namespace. Build the Service DNS name fromr.OperatorNamespaceand use the same value in operational commands. Otherwise, installations in another namespace create an unreachable Proposal.Also applies to: 272-272, 392-393
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` at line 92, Replace the hard-coded must-gather-operator namespace in the MCP URL and the referenced support commands with the actual operator namespace from r.OperatorNamespace, preserving the existing Service DNS and command structure so installations in any namespace produce reachable proposals.
211-212: 🔒 Security & Privacy | 🟠 MajorSecurity Misconfiguration (CWE-250)
Reachability: External
Use a dedicated ServiceAccount for the MCP server.
The HTTP-facing third-party image runs as the operator ServiceAccount. Unless token automount is disabled, the pod receives operator credentials.
--cluster-provider disableddoes not remove or scope that token. Use a dedicated ServiceAccount withautomountServiceAccountToken: falseand no RBAC, or document the exact equivalent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` around lines 211 - 212, Update the shared MCP server deployment documentation to use a dedicated ServiceAccount with automountServiceAccountToken disabled and no RBAC permissions, rather than the must-gather-operator ServiceAccount. Ensure the documented configuration applies this account to the MCP server while keeping spec.serviceAccountName scoped to the gather Job.
215-217: 🎯 Functional Correctness | 🟠 MajorEnforce the operator-namespace precondition before MCP creation.
The document says that a CR in another namespace can gather successfully and then fail when the MCP Deployment mounts the PVC. Check the namespace before creating MCP or Proposal resources. Preserve normal gathering behavior as appropriate, and add the negative test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` around lines 215 - 217, Update the agentic-debugging reconciliation flow to validate that the MustGather CR namespace matches the operator namespace before creating MCP or Proposal resources. Return or skip those agentic resources on mismatch while preserving normal gathering behavior, and add a negative test covering a CR created outside the operator namespace.
207-209: 🔒 Security & Privacy | 🟠 MajorAuthorization Bypass (CWE-639): Authorization Bypass Through User-Controlled Key (IDOR)
Reachability: External
Enforce collection isolation at the MCP boundary.
The server mounts the entire PVC, and the agent selects
/data/{subPath}/{podName}through the prompt. The prompt is not an authorization boundary. An agent or reachable client can select another collection on the same PVC. Add server-side root scoping, per-collection credentials, or an isolated MCP instance before claiming independent collection isolation.Also applies to: 40-40, 47-47, 93-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` around lines 207 - 209, Update the shared must-gather MCP serving design to enforce collection isolation at the server boundary rather than relying on the Proposal prompt path. Implement server-side root scoping, per-collection credentials, or isolated MCP instances so clients cannot access other collections on the PVC, and revise the affected isolation claims to reflect the enforced mechanism.
221-226: 🗄️ Data Integrity & Integration | 🟠 MajorMake shared-server ownership transitions atomic.
AlreadyExistshandling makes Deployment creation idempotent only. Two reconcilers can both observe a free server, update the owner and volume, and create Proposals. Use a Lease or resource-version compare-and-swap. Only the successful owner may rewrite the volume and create its Proposal.Also applies to: 267-268
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` around lines 221 - 226, Make the shared MCP server ownership transition atomic in ensureMCPDeployment and the related Proposal-creation flow: use a Lease or resource-version compare-and-swap so concurrent reconcilers cannot both claim the Deployment, rewrite its PVC, and create Proposals. Ensure only the reconciler that successfully acquires ownership performs the volume/owner annotation update and Proposal creation, while preserving errMCPServerBusy for an active unAnalyzed owner.
88-101: 🩺 Stability & Availability | 🟠 MajorCreate the Proposal only after MCP readiness.
The error case permits Proposal creation after MCP setup fails. The primary flow also confirms resource creation, not Deployment readiness or MCP endpoint readiness. Create the Proposal only after the MCP server is ready and reachable. Otherwise, retain the MustGather for retry without creating a doomed Proposal.
Also applies to: 382-384
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` around lines 88 - 101, The documented flow must gate createIntelliAideProposal() on confirmed MCP Deployment readiness and reachable MCP endpoint availability, not merely ensureMCPServer() resource creation. If readiness or reachability fails, retain the MustGather for retry and skip Proposal creation; update the related error-case and corresponding flow text consistently.
209-209: 🗄️ Data Integrity & Integration | 🟠 MajorEnforce the single-PVC scope.
The proposal declares one shared Deployment per operator namespace, but it does not define a check that rejects a CR referencing a different PVC. A second CR can then use the wrong mounted claim or wait on an unrelated analysis. Reject mismatched PVC names or create one MCP instance per PVC.
Also applies to: 215-217, 308-308
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` at line 209, Update the shared Deployment and “PVC-swap guard” design to enforce the single-PVC scope: validate each MustGather CR’s referenced PVC against the configured/shared claim and reject mismatches before analysis or serving begins. Ensure the same constraint is stated in the related deployment and concurrency sections, rather than allowing a CR to use an unrelated mounted claim.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md`:
- Line 16: Update the document metadata and corresponding “Update” entry from
the future date 2026-08-06 to the actual update date, ensuring no future-dated
document updates remain.
- Around line 231-234: Update the agentic debugging workflow and graduation
criteria to require checking the TechPreview feature gate before processing
agenticDebuggingEnabled: true; when disabled, skip all MCP and Proposal
processing. Add a negative test covering the disabled-gate path and ensure the
documentation uses mandatory behavior rather than future-tense wording.
- Around line 223-229: The PVC-swap guard must not treat Proposal age as proof
that analysis has stopped. Update ensureMCPDeployment and the errMCPServerBusy
flow so a non-terminal prior Proposal cannot be swapped solely because its
timeout plus grace period elapsed; cancel or terminate the existing analysis and
confirm its terminal Analyzed condition before swapping, or isolate analyses
with separate PVC/server resources.
- Line 184: The documentation must define MCP_SERVER_IMAGE as an immutable
contract: specify one verified, pullable Red Hat catalog tag or digest as the
operator default, or explicitly require the variable and fail configuration when
it is unset. Update the related sections describing the operator Deployment and
startup behavior consistently, and remove any ambiguity around rolling :latest
or unverified openshift/mustgather images.
- Line 25: Align the documented pipeline scope with the generated Lightspeed
Proposal around the Proposal definition and referenced summary, goals, workflow,
and E2E test sections. Either configure the Proposal to run the High-, Medium-,
and Low-priority IntelliAide passes, or revise all claims and graduation
criteria to describe High-only analysis; keep the documentation and test
expectations consistent with the selected scope.
- Line 50: Extend Goal 6 to require idempotent createIntelliAideProposal()
behavior: use a deterministic Proposal name or ownership marker, detect an
existing Proposal, and handle AlreadyExists by reusing it rather than creating a
duplicate or starting duplicate analysis.
---
Duplicate comments:
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md`:
- Line 92: Replace the hard-coded must-gather-operator namespace in the MCP URL
and the referenced support commands with the actual operator namespace from
r.OperatorNamespace, preserving the existing Service DNS and command structure
so installations in any namespace produce reachable proposals.
- Around line 211-212: Update the shared MCP server deployment documentation to
use a dedicated ServiceAccount with automountServiceAccountToken disabled and no
RBAC permissions, rather than the must-gather-operator ServiceAccount. Ensure
the documented configuration applies this account to the MCP server while
keeping spec.serviceAccountName scoped to the gather Job.
- Around line 215-217: Update the agentic-debugging reconciliation flow to
validate that the MustGather CR namespace matches the operator namespace before
creating MCP or Proposal resources. Return or skip those agentic resources on
mismatch while preserving normal gathering behavior, and add a negative test
covering a CR created outside the operator namespace.
- Around line 207-209: Update the shared must-gather MCP serving design to
enforce collection isolation at the server boundary rather than relying on the
Proposal prompt path. Implement server-side root scoping, per-collection
credentials, or isolated MCP instances so clients cannot access other
collections on the PVC, and revise the affected isolation claims to reflect the
enforced mechanism.
- Around line 221-226: Make the shared MCP server ownership transition atomic in
ensureMCPDeployment and the related Proposal-creation flow: use a Lease or
resource-version compare-and-swap so concurrent reconcilers cannot both claim
the Deployment, rewrite its PVC, and create Proposals. Ensure only the
reconciler that successfully acquires ownership performs the volume/owner
annotation update and Proposal creation, while preserving errMCPServerBusy for
an active unAnalyzed owner.
- Around line 88-101: The documented flow must gate createIntelliAideProposal()
on confirmed MCP Deployment readiness and reachable MCP endpoint availability,
not merely ensureMCPServer() resource creation. If readiness or reachability
fails, retain the MustGather for retry and skip Proposal creation; update the
related error-case and corresponding flow text consistently.
- Line 209: Update the shared Deployment and “PVC-swap guard” design to enforce
the single-PVC scope: validate each MustGather CR’s referenced PVC against the
configured/shared claim and reject mismatches before analysis or serving begins.
Ensure the same constraint is stated in the related deployment and concurrency
sections, rather than allowing a CR to use an unrelated mounted claim.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 71c9d7ae-2f82-4552-b652-2a26f7155080
📒 Files selected for processing (1)
enhancements/support-log-gather/must-gather-agentic-debugging.md
| **PVC-swap guard** The Deployment carries an annotation (`must-gather-operator.openshift.io/current-mustgather`) naming the MustGather whose PVC is currently mounted. Before `ensureMCPDeployment()` rewrites the volume to a different MustGather's PVC, it looks up that recorded owner's Proposal and checks its `Analyzed` condition: | ||
|
|
||
| - No Proposal, or `Analyzed: True` → safe, swap proceeds and the annotation is updated to the new owner. | ||
| - Proposal exists and has not reported `Analyzed: True` → **not safe**. `ensureMCPDeployment()` returns a sentinel `errMCPServerBusy` instead of swapping. | ||
| - Proposal exists but is older than its own `analysis.timeoutMinutes` plus a 15-minute grace period → treated as abandoned/stale (e.g. the agent sandbox crashed without reporting a terminal condition) and the swap proceeds anyway, so a broken analysis can't block the shared server forever. | ||
|
|
||
| When `errMCPServerBusy` is returned, `handleJobCompletion()` defers *both* Proposal creation and post-completion resource cleanup for the newly-completed MustGather, and requeues the reconcile after 30 seconds. Deferring cleanup (rather than deleting the Job/Pod and only skipping the Proposal) matters because `resolveMustGatherDataPath()` needs the completed Job's Pod to still exist to compute the PVC subpath; deleting it early would make the retry fail once the server is free. This serializes agentic analyses on the shared server — the second MustGather's Proposal is created only once the first's Proposal reports a terminal `Analyzed` condition (or is judged stale) — trading latency for correctness. **v1 decision:** single shared MCP server, serialized PVC access with the guard above; true concurrent multi-PVC serving remains a post-v1 question (see Open Questions). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
Do not treat Proposal age as proof that the agent stopped.
The stale rule permits a PVC swap after the timeout and grace period even when the sandbox is still running or partitioned. That agent can continue MCP calls after another collection is mounted. Cancel or terminate the old analysis and confirm completion before swapping, or isolate each analysis.
Also applies to: 268-268
🧰 Tools
🪛 LanguageTool
[uncategorized] ~229-~229: Do not mix variants of the same word (‘analyse’ and ‘analyze’) within a single text.
Context: ...server is free. This serializes agentic analyses on the shared server — the second MustG...
(EN_WORD_COHERENCY)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` around
lines 223 - 229, The PVC-swap guard must not treat Proposal age as proof that
analysis has stopped. Update ensureMCPDeployment and the errMCPServerBusy flow
so a non-terminal prior Proposal cannot be swapped solely because its timeout
plus grace period elapsed; cancel or terminate the existing analysis and confirm
its terminal Analyzed condition before swapping, or isolate analyses with
separate PVC/server resources.
There was a problem hiding this comment.
This is an intentional v1 tradeoff rather than an implementation bug. The timeout is used as a heuristic to prevent a stalled or abandoned analysis from blocking the shared MCP server indefinitely. While a long-running agent could theoretically still be active when the timeout expires, introducing explicit cancellation or isolated MCP instances is a larger architectural change. I'll clarify this tradeoff in the proposal rather than changing the design.
There was a problem hiding this comment.
@sakshiep1, understood. This is an accepted v1 limitation, not an unintended defect.
Please document that timeout expiry does not confirm that the prior agent stopped. After the PVC swap, a still-running or partitioned prior agent can access the newly mounted collection through the shared MCP server. State that this can affect analysis isolation and that v1 accepts this risk to prevent indefinite blocking.
A follow-up design can add explicit Proposal cancellation with terminal-condition confirmation, or per-analysis isolation. The finding remains open until the proposal documents the tradeoff.
✏️ Learnings added
Learnt from: sakshiep1
Repo: openshift/enhancements PR: 2064
File: enhancements/support-log-gather/must-gather-agentic-debugging.md:223-229
Timestamp: 2026-08-06T08:47:57.653Z
Learning: In `enhancements/support-log-gather/must-gather-agentic-debugging.md`, the v1 PVC-swap guard intentionally treats a Proposal older than `analysis.timeoutMinutes` plus a 15-minute grace period as stale and permits a shared MCP server PVC swap without confirming that the prior agent stopped. This is an accepted tradeoff to prevent an abandoned analysis from blocking later analyses indefinitely. The proposal must document that a still-running or partitioned prior agent can then access the newly mounted collection, which weakens analysis isolation. Future designs may use Proposal cancellation with terminal-condition confirmation or per-analysis isolation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
| #### Feature Gating | ||
|
|
||
| For the initial release, this feature will be gated behind a TechPreview mechanism. The operator will check a feature gate (environment variable or cluster FeatureGate resource) before processing `agenticDebuggingEnabled: true`. If the gate is off, the field is ignored. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Enforce the TechPreview gate before agentic processing.
The document says the gate “will” be checked, while the workflow and graduation criteria already promise gated behavior. When the gate is disabled, skip MCP and Proposal processing for agenticDebuggingEnabled: true. Add a negative test for this path.
Also applies to: 341-342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` around
lines 231 - 234, Update the agentic debugging workflow and graduation criteria
to require checking the TechPreview feature gate before processing
agenticDebuggingEnabled: true; when disabled, skip all MCP and Proposal
processing. Add a negative test covering the disabled-gate path and ensure the
documentation uses mandatory behavior rather than future-tense wording.
a724236 to
63a6302
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
enhancements/support-log-gather/must-gather-agentic-debugging.md (1)
92-93: 🎯 Functional Correctness | 🟠 MajorBuild the MCP URL from the actual operator namespace.
The workflow and support commands hardcode
must-gather-operator, while the risk table requiresr.OperatorNamespace. ConstructmcpServerURLfrom the configured operator namespace and use a placeholder in operational examples. The current URL fails when the operator is installed elsewhere.Also applies to: 273-273, 395-396
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` around lines 92 - 93, Replace hardcoded must-gather-operator namespace values in the agent workflow, support commands, and operational examples with the configured r.OperatorNamespace when constructing mcpServerURL. Use a namespace placeholder in documentation examples, while preserving the existing MCP endpoint and path structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md`:
- Around line 88-90: Update the flow around ensureMCPServer() and
createIntelliAideProposal() to wait until the MCP Deployment is ready and the
Service has an endpoint before creating the Proposal. If MCP setup or readiness
fails, do not create the Proposal; instead requeue the MustGather and propagate
the failure through the existing error path.
- Around line 93-94: Update the must-gather MCP design around mustgather_use and
the IntelliAide pass to enforce collection isolation server-side rather than
relying on the agent-selected path. Scope each MCP instance or credential to its
assigned collection root, and add coverage verifying requests for another
collection are denied.
- Line 211: Update the MCP server design to use a dedicated ServiceAccount
instead of the must-gather-operator ServiceAccount. Configure the dedicated
account with automountServiceAccountToken: false, and state that no RBAC
permissions are added unless future MCP requirements need them; remove the open
design question about reusing the operator account.
- Around line 88-93: Update the must-gather MCP deployment documentation around
ensureMCPServer() and createIntelliAideProposal() to restrict Service ingress to
the Lightspeed sandbox through an explicit NetworkPolicy allow-path, and define
MCP authentication for cases network policy cannot enforce client identity.
Document the expected rejection behavior and add coverage for unauthorized MCP
clients and invalid authentication tokens when describing the flow.
---
Duplicate comments:
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md`:
- Around line 92-93: Replace hardcoded must-gather-operator namespace values in
the agent workflow, support commands, and operational examples with the
configured r.OperatorNamespace when constructing mcpServerURL. Use a namespace
placeholder in documentation examples, while preserving the existing MCP
endpoint and path structure.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bae62517-ef4e-468d-9f20-dc5bbd17626b
📒 Files selected for processing (1)
enhancements/support-log-gather/must-gather-agentic-debugging.md
| 8. `ensureMCPServer()` creates (or confirms existence of) the `must-gather-mcp` Deployment + Service. The Deployment mounts the PVC at `/data` as read-only. | ||
| 9. `resolveMustGatherDataPath()` determines the specific collection path: `{subPath}/{podName}`. | ||
| 10. `createIntelliAideProposal()` creates a Proposal CR in `openshift-lightspeed` with the MCP server URL and IntelliAide instructions. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
Wait for MCP readiness before creating the Proposal.
The flow creates or confirms the Deployment and then immediately creates the Proposal. The error case also says that a Proposal may be created after MCP setup fails. Create the Proposal only after the Deployment is ready and the Service has an endpoint. On setup failure, skip Proposal creation and requeue the MustGather.
Also applies to: 101-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` around
lines 88 - 90, Update the flow around ensureMCPServer() and
createIntelliAideProposal() to wait until the MCP Deployment is ready and the
Service has an endpoint before creating the Proposal. If MCP setup or readiness
fails, do not create the Proposal; instead requeue the MustGather and propagate
the failure through the existing error path.
| 8. `ensureMCPServer()` creates (or confirms existence of) the `must-gather-mcp` Deployment + Service. The Deployment mounts the PVC at `/data` as read-only. | ||
| 9. `resolveMustGatherDataPath()` determines the specific collection path: `{subPath}/{podName}`. | ||
| 10. `createIntelliAideProposal()` creates a Proposal CR in `openshift-lightspeed` with the MCP server URL and IntelliAide instructions. | ||
| 11. The Lightspeed agentic operator processes the Proposal, spawns a sandboxed agent pod. | ||
| 12. The agent connects to `http://must-gather-mcp.must-gather-operator.svc:8080/mcp`. | ||
| 13. The agent calls `mustgather_use("/data/{subPath}/{podName}")` to select the archive. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'must-gather-mcp|NetworkPolicy|ServiceAccount|automountServiceAccountToken|Authorization|Bearer|mcpServerURL' \
. -g '*.go' -g '*.yaml' -g '*.yml' -g '*.md'Repository: openshift/enhancements
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== listed target files =="
git ls-files 'enhancements/support-log-gather/must-gather-agentic-debugging.md'
echo "== target file line counts =="
wc -l enhancements/support-log-gather/must-gather-agentic-debugging.md
echo "== relevant sections =="
sed -n '70,115p' enhancements/support-log-gather/must-gather-agentic-debugging.md
echo "---"
sed -n '175,215p' enhancements/support-log-gather/must-gather-agentic-debugging.md
echo "== focused occurrences in target =="
rg -n -C 4 \
'must-gather-mcp|IntelliAide proposal|Proposal|HTTP|Service|NetworkPolicy|ServiceAccount|Authorization|Authentication|Bearer|8080|mcpServerURL|mcp endpoint|mustgather_use|Sandbox|sandbox' \
enhancements/support-log-gather/must-gather-agentic-debugging.md
echo "== focused occurrences in support-log-gather enhancement only =="
rg -n -C 4 \
'must-gather-mcp|IntelliAide|Proposal|HTTP|Service|NetworkPolicy|ServiceAccount|Authorization|Authentication|Bearer|8080|mcpServerURL|mcp endpoint|mustgather_use|Sandbox|sandbox' \
enhancements/support-log-gather -g '*.md' -g '*.go' -g '*.yaml' -g '*.yml'Repository: openshift/enhancements
Length of output: 50381
Restrict the MCP Service to the intended client.
The Proposal sends mcpServerURL as plain HTTP to a cluster-internal Service. The enhancement does not describe a NetworkPolicy or MCP authentication for must-gather-mcp, so any pod that can reach the Service can call mustgather_use with any path under /data. Add an allow-path for the Lightspeed sandbox only and add authentication when network policy alone cannot prevent access. Include coverage for unauthorized MCP clients and invalid tokens.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` around
lines 88 - 93, Update the must-gather MCP deployment documentation around
ensureMCPServer() and createIntelliAideProposal() to restrict Service ingress to
the Lightspeed sandbox through an explicit NetworkPolicy allow-path, and define
MCP authentication for cases network policy cannot enforce client identity.
Document the expected rejection behavior and add coverage for unauthorized MCP
clients and invalid authentication tokens when describing the flow.
| 13. The agent calls `mustgather_use("/data/{subPath}/{podName}")` to select the archive. | ||
| 14. The agent runs the IntelliAide High-priority pass (extract → select → fetch via MCP → analyze → RCA). |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Enforce collection isolation at the MCP boundary.
The server mounts the full PVC at /data, while the collection is selected through the agent prompt. The PVC-swap guard does not prevent a request for another collection on the mounted claim. Use server-side root scoping, per-collection credentials, or isolated MCP instances, and test cross-collection access denial.
Also applies to: 207-209
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` around
lines 93 - 94, Update the must-gather MCP design around mustgather_use and the
IntelliAide pass to enforce collection isolation server-side rather than relying
on the agent-selected path. Scope each MCP instance or credential to its
assigned collection root, and add coverage verifying requests for another
collection are denied.
|
|
||
| The initial implementation supports a single PVC with **serialized** access when multiple MustGather CRs complete while an analysis is in flight (see "PVC-swap guard" below). True concurrent multi-PVC serving is deferred (see Open Questions). The shared server approach avoids resource waste from per-CR server pods. | ||
|
|
||
| The PoC runs the shared MCP server under the must-gather-operator's own ServiceAccount (`must-gather-operator`), not the ServiceAccount on the MustGather CR's `spec.serviceAccountName` (which is used only by the gather Job). The MCP server only reads a mounted PVC and runs with `--cluster-provider disabled` (no live cluster API access). Whether to introduce a **dedicated MCP ServiceAccount** (e.g. with `automountServiceAccountToken: false`) instead of reusing the operator SA is an open design question (see Open Questions). |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
Use a dedicated ServiceAccount for MCP.
The design runs a third-party HTTP-facing image under the operator ServiceAccount. --cluster-provider disabled does not remove the mounted token or its RBAC permissions. Create a dedicated ServiceAccount with automountServiceAccountToken: false; add permissions only if a future requirement needs them.
Proposed change
- The PoC runs the shared MCP server under the must-gather-operator ServiceAccount.
+ Create a dedicated ServiceAccount for the shared MCP server.
+ Set automountServiceAccountToken: false.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@enhancements/support-log-gather/must-gather-agentic-debugging.md` at line
211, Update the MCP server design to use a dedicated ServiceAccount instead of
the must-gather-operator ServiceAccount. Configure the dedicated account with
automountServiceAccountToken: false, and state that no RBAC permissions are
added unless future MCP requirements need them; remove the open design question
about reusing the operator account.
| tracking-link: | ||
| - [https://redhat.atlassian.net/browse/OAPE-688](https://redhat.atlassian.net/browse/OAPE-688) | ||
|
|
||
| # Must-Gather Agentic Debugging via MCP Server |
There was a problem hiding this comment.
make the title more generic like - "Must-gather adapter for agentic OLS" or something like that
| ## title: must-gather-agentic-debugging | ||
|
|
||
| authors: | ||
|
|
|
|
||
| ## Summary | ||
|
|
||
| Integrate the must-gather-operator with OpenShift Lightspeed to enable automated root-cause analysis (RCA) on must-gather diagnostic bundles. When a user sets `agenticDebuggingEnabled: true` on a MustGather CR, the operator - after a successful gather Job - deploys a shared MCP server (using the `openshift-mcp-server` image with the `openshift/mustgather` toolset) that mounts the same PVC, and creates a Lightspeed Proposal CR. The Lightspeed agentic platform spawns a sandboxed agent that connects to the MCP server, fetches diagnostic data through structured MCP tool calls, and runs the IntelliAide RCA pipeline (**High-priority pass in v1**; see "Analysis Pass Coverage") - producing a structured AnalysisResult without requiring cluster-admin privileges for the analysis agent. |
There was a problem hiding this comment.
"Openshift Lightspeed" -> "agentic OLS"
|
|
||
| ## Summary | ||
|
|
||
| Integrate the must-gather-operator with OpenShift Lightspeed to enable automated root-cause analysis (RCA) on must-gather diagnostic bundles. When a user sets `agenticDebuggingEnabled: true` on a MustGather CR, the operator - after a successful gather Job - deploys a shared MCP server (using the `openshift-mcp-server` image with the `openshift/mustgather` toolset) that mounts the same PVC, and creates a Lightspeed Proposal CR. The Lightspeed agentic platform spawns a sandboxed agent that connects to the MCP server, fetches diagnostic data through structured MCP tool calls, and runs the IntelliAide RCA pipeline (**High-priority pass in v1**; see "Analysis Pass Coverage") - producing a structured AnalysisResult without requiring cluster-admin privileges for the analysis agent. |
There was a problem hiding this comment.
these are more of the steps or the "how" its done - not the what/why - which is what the summary should be about
2 participants
Summary
Enhancement proposal for must-gather agentic debugging adapter with MCP server and IntelliAide integration.
What this proposes
When a user sets
agenticDebuggingEnabled: trueon a MustGather CR, the operator - after a successful gather Job - will:openshift-mcp-serverimage with only theopenshift/mustgathertoolset enabled) that mounts the same PVCThe agent never requires cluster-admin privileges - it accesses must-gather data exclusively through MCP tool calls over HTTP.
Key design decisions
registry.redhat.io/openshift-mcp-beta/openshift-mcp-server-rhel9agenticDebuggingEnableddefaults tofalse; feature gated for initial releasesComponents involved
openshift/must-gather-operator- new fields, MCP server management, Proposal creationopenshift/openshift-mcp-server- consumed as container image (no code changes)openshift/lightspeed-agentic-sandbox- consumed via existing Proposal/AnalysisResult APIs (no code changes needed from us)Summary by CodeRabbit