feat(kubevirt): add vm_troubleshoot as an MCP Tool#365
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new ChangesVM troubleshooting tool
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sradco 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 |
920b3bb to
41bf6a8
Compare
The existing vm-troubleshoot functionality is registered only as an MCP Prompt, which requires explicit user invocation. LLMs connected via OLS cannot autonomously call Prompts during conversations. Add vm_troubleshoot as a proper MCP Tool so that LLMs can proactively invoke it when a user asks about VM issues. The tool collects the same diagnostic data (VM status, VMI status, volumes, virt-launcher pod, pod logs, and events) and returns a structured diagnostic report. The Prompt is preserved for backward compatibility with clients that use the MCP prompts interface. Signed-off-by: Shirly Radco <sradco@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
41bf6a8 to
1dece99
Compare
|
Hey @sradco any reasons this PR isn't going to the upstream repo? |
Cover tool registration, schema validation, and all helper functions: - fetchVMStatus: VM found, not found, no status field - fetchVMIStatus: VMI found, not found - fetchVolumes: from VM, fallback to VMI, nil/empty - fetchVirtLauncherPod: pod found, not found - fetchVirtLauncherPodLogs: nil/empty pod names Signed-off-by: Shirly Radco <sradco@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@pkg/toolsets/kubevirt/vm/troubleshoot/tool_test.go`:
- Around line 55-112: The suite is testing private helpers like fetchVMStatus
directly instead of the public MCP entrypoint. Rewrite these cases to invoke
Tools()[0].Handler and assert on the returned report/output so the tests cover
the real surface area and final assembly. Keep the same scenarios (VM exists,
not found, no status) but drive them through the tool handler and its public
inputs/outputs rather than unexported functions.
In `@pkg/toolsets/kubevirt/vm/troubleshoot/tool.go`:
- Around line 217-225: The log collection in the VM troubleshooting flow is
hardcoded to the compute container, so it misses the failing virt-launcher
container when compute never starts. Update the logic in the troubleshooting
tool’s pod log loop to target the actual failing virt-launcher containers
instead of only using containerName := "compute", and make sure the output still
labels each pod/container clearly when calling core.PodsLog and formatting the
result.
- Around line 252-265: The current fetchEvents path in troubleshoot/tool.go
scans all namespace events with core.EventsList and filters client-side, which
is too broad. Update fetchEvents to accept the known VM/VMI/virt-launcher object
names from the caller and use those names to query events directly instead of
iterating every event in the namespace. Keep the related event filtering logic
tied to the existing symbols fetchEvents, core.EventsList, vmName, and
relatedEvents, but narrow the lookup to the specific objects being troubleshot.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 443b67f7-5dcc-477c-8fd5-9de9d72a0c14
📒 Files selected for processing (4)
pkg/mcp/testdata/toolsets-kubevirt-tools.jsonpkg/toolsets/kubevirt/toolset.gopkg/toolsets/kubevirt/vm/troubleshoot/tool.gopkg/toolsets/kubevirt/vm/troubleshoot/tool_test.go
| func (s *TroubleshootToolSuite) TestFetchVMStatus() { | ||
| ctx := context.Background() | ||
| gvrToListKind := map[schema.GroupVersionResource]string{ | ||
| kubevirt.VirtualMachineGVR: "VirtualMachineList", | ||
| } | ||
|
|
||
| s.Run("returns status when VM exists", func() { | ||
| testVM := &unstructured.Unstructured{} | ||
| testVM.SetUnstructuredContent(map[string]interface{}{ | ||
| "apiVersion": "kubevirt.io/v1", | ||
| "kind": "VirtualMachine", | ||
| "metadata": map[string]interface{}{ | ||
| "name": "test-vm", | ||
| "namespace": "test-ns", | ||
| }, | ||
| "status": map[string]interface{}{ | ||
| "printableStatus": "Running", | ||
| "ready": true, | ||
| }, | ||
| }) | ||
|
|
||
| client := fake.NewSimpleDynamicClientWithCustomListKinds(runtime.NewScheme(), gvrToListKind, testVM) | ||
| result, vm := fetchVMStatus(ctx, client, "test-ns", "test-vm") | ||
|
|
||
| s.Contains(result, "## VirtualMachine Status: test-ns/test-vm") | ||
| s.Contains(result, "printableStatus") | ||
| s.Contains(result, "Running") | ||
| s.NotNil(vm) | ||
| }) | ||
|
|
||
| s.Run("returns error when VM not found", func() { | ||
| client := fake.NewSimpleDynamicClientWithCustomListKinds(runtime.NewScheme(), gvrToListKind) | ||
| result, vm := fetchVMStatus(ctx, client, "test-ns", "nonexistent") | ||
|
|
||
| s.Contains(result, "## VirtualMachine") | ||
| s.Contains(result, "Error") | ||
| s.Nil(vm) | ||
| }) | ||
|
|
||
| s.Run("handles VM with no status field", func() { | ||
| testVM := &unstructured.Unstructured{} | ||
| testVM.SetUnstructuredContent(map[string]interface{}{ | ||
| "apiVersion": "kubevirt.io/v1", | ||
| "kind": "VirtualMachine", | ||
| "metadata": map[string]interface{}{ | ||
| "name": "test-vm", | ||
| "namespace": "test-ns", | ||
| }, | ||
| "spec": map[string]interface{}{}, | ||
| }) | ||
|
|
||
| client := fake.NewSimpleDynamicClientWithCustomListKinds(runtime.NewScheme(), gvrToListKind, testVM) | ||
| result, vm := fetchVMStatus(ctx, client, "test-ns", "test-vm") | ||
|
|
||
| s.Contains(result, "No status found") | ||
| s.NotNil(vm) | ||
| }) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Drive this suite through Tools()[0].Handler, not the private helpers.
Most of this file is white-box testing fetchVMStatus, fetchVMIStatus, fetchVolumes, and the other unexported helpers directly. That couples the suite to implementation details while leaving the actual MCP surface and final report assembly uncovered.
As per coding guidelines, "**/*_test.go: Test the public API only - tests should be black-box and not access internal/private functions`."
Also applies to: 114-152, 154-261, 263-316
🤖 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 `@pkg/toolsets/kubevirt/vm/troubleshoot/tool_test.go` around lines 55 - 112,
The suite is testing private helpers like fetchVMStatus directly instead of the
public MCP entrypoint. Rewrite these cases to invoke Tools()[0].Handler and
assert on the returned report/output so the tests cover the real surface area
and final assembly. Keep the same scenarios (VM exists, not found, no status)
but drive them through the tool handler and its public inputs/outputs rather
than unexported functions.
Source: Coding guidelines
| containerName := "compute" | ||
| for _, podName := range podNames { | ||
| logs, err := core.PodsLog(ctx, namespace, podName, containerName, false, 50) | ||
| if err != nil { | ||
| fmt.Fprintf(&result, "### %s\n\n*Error fetching logs: %v*\n\n", podName, err) | ||
| continue | ||
| } | ||
| fmt.Fprintf(&result, "### %s (container: %s)\n\n```\n%s\n```\n\n", podName, containerName, logs) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Collect logs from the failing virt-launcher containers, not just compute.
When the VM never reaches the compute container, this path returns no actionable logs for the exact "not starting" cases the tool is supposed to diagnose. Init/sidecar containers are often where the failure is.
🤖 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 `@pkg/toolsets/kubevirt/vm/troubleshoot/tool.go` around lines 217 - 225, The
log collection in the VM troubleshooting flow is hardcoded to the compute
container, so it misses the failing virt-launcher container when compute never
starts. Update the logic in the troubleshooting tool’s pod log loop to target
the actual failing virt-launcher containers instead of only using containerName
:= "compute", and make sure the output still labels each pod/container clearly
when calling core.PodsLog and formatting the result.
… diagnostics Add two targeted diagnostic sections to the vm_troubleshoot report: - DataVolume/PVC status: shows binding state, StorageClass references, and CDI error conditions (e.g., ErrClaimNotValid for missing StorageClasses) - Cloud-init configuration: extracts and displays userData/networkData content from cloudInitNoCloud and cloudInitConfigDrive volumes These additions address gaps identified during OLS testing where gpt-4o-mini could not identify root causes (missing StorageClass, shutdown command in cloud-init) because the data wasn't in the diagnostic report. Also improves the tool description to better signal its value for tool filtering/ranking by smaller models. Signed-off-by: Shirly Radco <sradco@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
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 `@pkg/toolsets/kubevirt/vm/troubleshoot/tool.go`:
- Around line 391-397: Redact the inline cloud-init payloads in the report
generation path: in the code that reads ciData["userData"] and
ciData["networkData"] inside the troubleshooting report builder, stop embedding
the raw YAML contents in the result. Instead, replace the current fmt.Fprintf
output with a safe summary that reports presence and size (or a
redacted/filtered summary of sensitive keys) so userData and networkData are
never returned verbatim.
- Around line 310-316: Gate the PVC lookup in the DataVolume troubleshooting
path on apierrors.IsNotFound(err) inside the kubevirt VM troubleshoot flow,
rather than retrying on every Get failure. In the block around
dynamicClient.Resource(...).Get and the subsequent PVC fetch, only fall back to
PersistentVolumeClaimGVR when the DataVolume error is a true NotFound; for RBAC,
timeout, or transport errors, report the original error directly and skip the
PVC retry. Use the existing dv, err, pvcErr variables in troubleshoot handling
to keep the logic localized.
- Around line 291-293: In fetchDataVolumeStatus and extractCloudInit, do not
treat unstructured.NestedSlice extraction/type errors as empty state; handle the
err return from NestedSlice separately before the !found/len==0 fallback. If the
vm.Object or VMI volume data is malformed, return a schema/type error message
instead of “No dataVolumeTemplates defined…” or “No volumes found,” and keep the
existing empty-state text only for the true no-data case.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f9a71ad6-3688-47fd-9b97-1c68198bea52
📒 Files selected for processing (3)
pkg/mcp/testdata/toolsets-kubevirt-tools.jsonpkg/toolsets/kubevirt/vm/troubleshoot/tool.gopkg/toolsets/kubevirt/vm/troubleshoot/tool_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/mcp/testdata/toolsets-kubevirt-tools.json
| dvTemplates, found, err := unstructured.NestedSlice(vm.Object, "spec", "dataVolumeTemplates") | ||
| if err != nil || !found || len(dvTemplates) == 0 { | ||
| return "## DataVolume/PVC Status\n\n*No dataVolumeTemplates defined in VM spec*" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pkg/toolsets/kubevirt/vm/troubleshoot/tool.go"
echo "== line count =="
wc -l "$FILE"
echo "== around 260-320 =="
sed -n '260,320p' "$FILE" | cat -n
echo "== around 340-390 =="
sed -n '340,390p' "$FILE" | cat -n
echo "== NestedSlice occurrences =="
rg -n "NestedSlice|No dataVolumeTemplates defined|No volumes found|dataVolumeTemplates|volumes" "$FILE"Repository: openshift/openshift-mcp-server
Length of output: 6335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pkg/toolsets/kubevirt/vm/troubleshoot/tool.go"
echo "== around 140-180 =="
sed -n '140,180p' "$FILE" | cat -n
echo "== around 400-412 =="
sed -n '400,412p' "$FILE" | cat -nRepository: openshift/openshift-mcp-server
Length of output: 2196
🌐 Web query:
kubectl unstructured.NestedSlice error returns malformed type intermediate field documentation
💡 Result:
The error message related to unstructured.NestedSlice typically manifests as an accessor error when the data structure at the specified path does not match the expected type, specifically when it is not a []interface{} [1][2]. When using functions like NestedSlice from the k8s.io/apimachinery/pkg/apis/meta/v1/unstructured package, the function performs a type assertion on the retrieved value [1][3]. If the value exists but is not of type []interface{}, the function returns an error formatted as: %v accessor error: %v is of the type %T, expected []interface{} [1][2] This indicates that while the path was traversed, the leaf node (or intermediate node if incorrectly identified) contains a different Go type than what is required for a slice [1][2]. Common causes for this type mismatch include: 1. Data type inconsistency: The data may have been unmarshaled using a standard JSON library (which often defaults numbers to float64) instead of the Kubernetes-specific apimachinery JSON decoder, leading to type expectation failures [4]. 2. Incorrect pathing: The provided field path might be targeting a field that is not a slice, or the path structure in the map[string]interface{} object does not align with the expected schema [1][2]. 3. Intermediate field issues: The error specifically arises when the target field is reached but fails the final type assertion. While documentation refers to this as an accessor error, it confirms that the target location holds data incompatible with slice operations [1]. To resolve this, inspect the underlying map[string]interface{} structure to verify the actual type stored at the target path, or use NestedFieldNoCopy to retrieve the interface{} value directly and perform a manual type assertion [4].
Citations:
- 1: https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/unstructured/helpers.go
- 2: https://github.com/kubernetes/apimachinery/blob/v0.35.3/pkg/apis/meta/v1/unstructured/helpers.go
- 3: https://github.com/kubernetes/apimachinery/blob/5a348c53/pkg/apis/meta/v1/unstructured/helpers.go
- 4: https://stackoverflow.com/questions/79251926/unstructured-spec-replicas-is-float
Handle NestedSlice errors before empty-state fallbacks. fetchDataVolumeStatus currently turns extraction/type errors into “No dataVolumeTemplates defined…”, and extractCloudInit drops the error entirely before falling back to “No volumes found.” Malformed VM/VMI objects should surface the schema/type problem instead of looking empty.
🤖 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 `@pkg/toolsets/kubevirt/vm/troubleshoot/tool.go` around lines 291 - 293, In
fetchDataVolumeStatus and extractCloudInit, do not treat
unstructured.NestedSlice extraction/type errors as empty state; handle the err
return from NestedSlice separately before the !found/len==0 fallback. If the
vm.Object or VMI volume data is malformed, return a schema/type error message
instead of “No dataVolumeTemplates defined…” or “No volumes found,” and keep the
existing empty-state text only for the true no-data case.
Source: Coding guidelines
- Replace namespace-wide event scan with targeted field selectors - Gate PVC fallback on apierrors.IsNotFound only - Redact sensitive cloud-init fields (passwords, SSH keys, tokens) Signed-off-by: Sagi Radco <sradco@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Good point @Cali0707 — you're right that the kubevirt toolset isn't OpenShift-specific and should live upstream. I've moved this work to the upstream repo: containers#1239 That PR includes the same functionality plus additional fixes based on CodeRabbit feedback:
I'll close this PR once the upstream one is reviewed/merged and flows back downstream. Thanks for the guidance! |
|
Closing in favor of the upstream PR: containers#1239 |
2 participants
Summary
vm_troubleshootas a proper MCP Tool (in addition to the existingvm-troubleshootPrompt)Motivation
The existing
vm-troubleshootis registered only as an MCP Prompt. MCP Prompts are user-facing templates that must be explicitly selected — LLMs cannot autonomously call them during conversations. When a user asks OLS "why is my VM not starting?", the LLM has no way to invoke the troubleshooting logic unless the user manually selects the prompt.By exposing the same diagnostic functionality as a Tool, the LLM can proactively gather VM diagnostic data when it determines troubleshooting is needed, enabling a much better user experience.
Testing Results (OLS + Troubleshooting Mode)
Tested against 3 broken VM scenarios with the kubevirt toolset enabled:
premium-nvme-storagedoesn't existshutdown -h now)runcmdentryThe DataVolume/PVC and cloud-init enrichment directly addresses the gpt-4o-mini gap — the data is now in the report without needing separate tool calls.
Changes
pkg/toolsets/kubevirt/vm/troubleshoot/tool.gopkg/toolsets/kubevirt/vm/troubleshoot/tool_test.gopkg/toolsets/kubevirt/toolset.goGetTools()pkg/mcp/testdata/toolsets-kubevirt-tools.jsonDiagnostic Report Sections
The tool returns a structured markdown report with:
Backward Compatibility
The existing
vm-troubleshootPrompt is preserved unchanged for clients that use the MCP prompts interface.Test plan
mode: troubleshootingproactively invokes the tool when asked about VM issues