feat(kubevirt): add vm_troubleshoot tool for automated VM diagnostics#1239
feat(kubevirt): add vm_troubleshoot tool for automated VM diagnostics#1239sradco wants to merge 2 commits into
Conversation
2ec5a01 to
79443db
Compare
|
👋 Heads up — this pull request changes files owned by @ksimon1 @lyarwood. You are listed as an owner of one or more of the changed areas in |
lyarwood
left a comment
lyarwood
left a comment
There was a problem hiding this comment.
Thanks @sradco, I'll review in more detail tomorrow but initially it looks promising. We do however mandate that each new tool is accompanied by one or preferably more evals under https://github.com/containers/kubernetes-mcp-server/tree/main/evals/tasks/kubevirt in seperate commits that both show the need for this tooling and exercise it in a way that can be asserted over time to ensure correctness. Would you mind having a look into these next week, happy to help get you up and running if you need help.
cf06c60 to
5aa46d0
Compare
Added 2 evals |
| } | ||
|
|
||
| var issues []issue | ||
| for _, vmim := range vmimList.Items { |
There was a problem hiding this comment.
Would it make sense to filter out old migrations (e.g. older than 2 days)?
There was a problem hiding this comment.
Instead of a hard time cutoff (which is somewhat arbitrary), I now report only the 3 most recent failed migrations sorted by creation time, with age included in the message (e.g., "failed (3h ago)").
If more exist, an INFO note says how many older ones were skipped. This keeps output focused without losing potentially relevant failures.
5aa46d0 to
7e0d49b
Compare
lyarwood
left a comment
lyarwood
left a comment
There was a problem hiding this comment.
Review of the vm_troubleshoot tool implementation. All prior review comments from @ksimon1 have been addressed in the current code. Two eval tasks added as requested. Overall the code is solid — main concern is maintainability around prompt/tool duplication.
Summary: The heuristic analysis architecture is well-designed, cloud-init redaction is thorough, and the float64-safe numeric handling shows good attention to real-cluster behavior. See inline comments for specific suggestions.
| @@ -0,0 +1,559 @@ | |||
| package troubleshoot | |||
There was a problem hiding this comment.
Suggestion: Extract shared data-fetching logic with the existing prompt handler
This file reimplements fetchVMStatus, fetchVMIStatus, fetchVolumes, fetchVirtLauncherPod, fetchVirtLauncherPodLogs, and fetchEvents — all of which have near-identical versions in pkg/toolsets/kubevirt/vm_troubleshoot.go (the prompt handler).
The tool versions are enhanced (DataVolume/PVC status, cloud-init extraction with redaction, pod diagnostics summary, targeted events), but the base fetching logic is the same. As these evolve independently, bug fixes in one will need manual replication in the other.
Consider extracting the shared data-fetching logic into a common internal package (e.g., pkg/toolsets/kubevirt/internal/diagnostics/) that both the prompt and tool import, with the tool adding its analysis layer on top. This could be a follow-up PR if you'd prefer to keep this one focused.
There was a problem hiding this comment.
+1. Agreed — the duplication is a maintenance concern. The tool versions are enhanced (DataVolume/PVC status, cloud-init extraction with redaction, pod diagnostics summary, targeted events per object), but the base fetching logic is shared.
I'll open a follow-up PR to extract the common data-fetching logic into pkg/toolsets/kubevirt/internal/diagnostics/ that both the prompt and tool import, with the tool adding its analysis layer on top. This keeps the current PR focused on the new tool functionality.
| Handler: troubleshoot, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Add an MCP-level integration test
pkg/mcp/kubevirt_test.go tests the vm-troubleshoot prompt at the MCP layer but doesn't have a CallTool("vm_troubleshoot", ...) test for the new tool. An MCP-level test would exercise the full handler path (parameter extraction via the MCP server, tool registration, report assembly) and match the testing pattern used by other kubevirt tools in that file.
The unit tests here are thorough for the internal logic, but an integration test would catch wiring issues (e.g., if the tool handler signature or ToolHandlerParams fields change upstream).
There was a problem hiding this comment.
+1. Added a TestVMTroubleshoot MCP-level integration test in pkg/mcp/kubevirt_test.go that exercises the full handler path through CallTool("vm_troubleshoot", ...). It covers:
- Missing required params — verifies
namespaceandnameare required (matches the pattern used by other kubevirt tools) - Existing VM — creates a VM with a containerDisk volume, calls the tool, verifies the diagnostic report header, section structure, and VM name/namespace presence
- Non-existent VM — verifies the tool handles gracefully (returns report with error info, no
IsError)
This catches wiring issues like ToolHandlerParams field changes or parameter extraction regressions.
| return nil | ||
| } | ||
|
|
||
| vmimList, err := dynamicClient.Resource(kubevirt.VirtualMachineInstanceMigrationGVR).Namespace(namespace).List(ctx, metav1.ListOptions{ |
There was a problem hiding this comment.
Suggestion: Consider label-based filtering for migrations
This lists up to 50 migrations namespace-wide and filters by vmiName client-side. In namespaces with heavy migration activity, the relevant migration could fall outside the first 50 results.
If KubeVirt's VMIM resources carry a label like kubevirt.io/migration-target-vmi=<name> (or similar), a LabelSelector would be more efficient and avoid the cap concern. Worth checking the KubeVirt API — if no such label exists, the current approach with Limit: 50 is a reasonable pragmatic bound.
There was a problem hiding this comment.
Correction to my earlier reply — I was wrong. The kubevirt.io/vmi-name label (MigrationSelectorLabel) is still present on VMIM resources. PR #17382 proposed removing it but was closed without merging.
I'll switch checkMigrationStatus to use a LabelSelector with kubevirt.io/vmi-name=<name> for server-side filtering. This removes the Limit: 50 cap concern entirely and is more efficient.
926062d to
41a66ee
Compare
| fi | ||
| echo "✓ VirtualMachine crashloop-vm still exists" | ||
| exit 0 | ||
| - llmJudge: |
There was a problem hiding this comment.
nit: contains: "shutdown" is a fairly weak check — the agent could pass by saying something generic like "the VM appears to shut down" without actually diagnosing the cloud-init cause.
Consider tightening to contains: "shutdown -h now" or adding a second contains for "cloud-init" to confirm the agent identified the specific command and its source.
For comparison, the missing-storageclass task's contains: "non-existent-sc-xyz" is much stronger since it requires the exact StorageClass name.
| - k8s.delete: | ||
| apiVersion: v1 | ||
| kind: Namespace | ||
| metadata: |
There was a problem hiding this comment.
The prompt explicitly names the tool to use (Use the vm_troubleshoot tool). This is an anti-pattern for evals — it tests whether the agent can follow an instruction rather than whether it can select the right tool for the job.
Consider rephrasing to present the problem and let the agent figure out which tools to use, e.g.:
A VirtualMachine named "crashloop-vm" in the vm-test-troubleshoot-ci namespace keeps restarting.
Diagnose why this VM is crashlooping. Report:
- The root cause of the crashloop
- Which cloud-init command is causing the problem
- How to fix it
| contains: "non-existent-sc-xyz" | ||
| cleanup: | ||
| - k8s.delete: | ||
| apiVersion: kubevirt.io/v1 |
There was a problem hiding this comment.
Same comment as the cloud-init task — the prompt explicitly names vm_troubleshoot which makes the eval test instruction-following rather than tool selection. Consider removing the tool name and letting the agent discover the right tool on its own, e.g.:
A VirtualMachine named "broken-sc-vm" in the vm-test-troubleshoot-sc namespace is not starting.
Diagnose why this VM is stuck. Report:
- The root cause of the issue
- What StorageClass is missing
- What alternative StorageClasses are available on the cluster
lyarwood
left a comment
lyarwood
left a comment
There was a problem hiding this comment.
Review: feat(kubevirt): add vm_troubleshoot tool for automated VM diagnostics
Summary
This PR adds a well-designed vm_troubleshoot MCP tool with heuristic analysis for automated VM diagnostics, plus two eval tasks. The core implementation (analysis engine, cloud-init redaction, diagnostic data collection) is solid. All prior review feedback from @ksimon1 has been addressed, the MCP-level integration test was added as requested, and the migration label selector fix is in place.
Positive Observations
- Heuristic analysis architecture is clean —
analyzeIssuesorchestrates individual checkers that each return typedissuestructs with severity + fix suggestions. Easy to extend with new checks. - Cloud-init redaction is thorough — handles inline values, block scalars, SSH key patterns, and token fields while preserving diagnostic content.
- float64-safe numeric conversion via
toInt64()shows good attention to real-cluster behavior where JSON numbers deserialize as float64. - Deduplication between StorageClass and DataVolume errors avoids reporting symptoms alongside root causes.
- Pod diagnostics extraction strips env vars, images, and volumeMounts to avoid leaking sensitive data.
- Test coverage is comprehensive — unit tests for all heuristic checks, redaction, false-positive resistance, and an MCP integration test.
Issues
See inline comments. Summary:
isConfigKeyLinefalse-negative (analysis.go:349) — YAML keys without underscores or hyphens (e.g.hostname: shutdown) are not recognized as config keys, which could cause a false positive in dangerous command detection.matchDangerousCommandover-broad prefix match (analysis.go:371) —shutdown -vorshutdown --helpwould match as dangerous when they're not power-off commands.- Eval prompts name the tool explicitly (already commented) — anti-pattern for evals.
- Eval llmJudge
contains: "shutdown"is weak (already commented) — should be tightened.
Existing Discussion Summary
- @ksimon1's feedback (error handling, array format parsing, multi-pod diagnostics, migration filtering) — all addressed in current code.
- @lyarwood's suggestions (shared data-fetching extraction, MCP integration test, label-based migration filtering) — integration test added, migration label selector fixed, data-fetching extraction deferred to follow-up PR.
Verdict
Close to ready. The two eval issues (already commented) and the isConfigKeyLine false-negative are the main items to address. The shutdown - prefix match is minor.
| if strings.Contains(lower, ":") && !strings.HasPrefix(lower, "-") && !strings.HasPrefix(lower, "[") { | ||
| parts := strings.SplitN(lower, ":", 2) | ||
| key := strings.TrimSpace(parts[0]) | ||
| if strings.Contains(key, "_") || strings.Contains(key, "-") { |
There was a problem hiding this comment.
Bug: isConfigKeyLine misses YAML keys without underscores or hyphens
This function only returns true if the key part of a YAML line contains _ or -. Keys like hostname, packages, or bootcmd would not be recognized, so a line like:
hostname: shutdownwould fall through to matchDangerousCommand and trigger a false positive (the stripped value shutdown matches exactly).
The intent is to skip lines that are YAML key-value pairs (not shell commands), but the current heuristic is too narrow. Consider using a broader check — e.g., if the line has the form word: value and doesn't start with - , it's likely a config key regardless of whether the key name contains _ or -.
There was a problem hiding this comment.
+1. Fixed by broadening isConfigKeyLine to recognize any single-word YAML key (not just those containing _ or -). The check now treats any line matching the pattern word: value (that does not start with - or [) as a config key. This handles hostname: shutdown, packages: [halt], bootcmd: [], etc. without false positives.
Added hostname: shutdown and packages: [halt] to the existing false-positive test to cover this case.
| switch { | ||
| case stripped == "shutdown" || stripped == "shutdown -h now" || | ||
| stripped == "/sbin/shutdown" || strings.HasPrefix(stripped, "/sbin/shutdown ") || | ||
| strings.HasPrefix(stripped, "shutdown -"): |
There was a problem hiding this comment.
nit: strings.HasPrefix(stripped, "shutdown -") is quite broad — it matches shutdown -v, shutdown --help, shutdown -c (cancel a pending shutdown), etc. Only -h, -P, and -r (and bare shutdown) are actually dangerous power-off/reboot variants.
Not a high-priority fix since false positives here are relatively harmless (they'd surface a warning about a non-dangerous shutdown invocation), but worth noting for future refinement.
There was a problem hiding this comment.
+1. Tightened the shutdown flag matching to only match actually dangerous flags: -h (halt), -P (poweroff), and -r (reboot). Non-dangerous flags like -v, --help, and -c (cancel) no longer trigger a warning.
Added a test confirming shutdown -c does not produce a false positive.
f7e1e76 to
c2ae268
Compare
|
/lgtm |
|
/run-mcpchecker kubevirt |
|
@sradco can you run |
c2ae268 to
329c386
Compare
lyarwood
left a comment
lyarwood
left a comment
There was a problem hiding this comment.
All review feedback has been addressed:
isConfigKeyLinebroadened to recognize any single-word YAML key — fixes thehostname: shutdownfalse positivematchDangerousCommandtightened to only match-h,-P,-rflags —shutdown -cno longer triggers- Eval
llmJudgetightened fromcontains: "shutdown"tocontains: "shutdown -h now" - Eval prompts no longer name
vm_troubleshoot— both present the problem and let the agent choose the tool - New test cases added for both fixes
LGTM — nice work on the heuristic analysis engine and the thorough test coverage.
|
/lgtm |
|
@Cali0707 Hi, can we merge this pr? |
|
/run-mcpchecker kubevirt |
329c386 to
254606c
Compare
|
/run-mcpchecker kubevirt |
|
The code needed a rebase. |
automated issue detection for small-model performance Adds a new vm_troubleshoot MCP Tool that collects comprehensive diagnostic data for KubeVirt VMs and runs heuristic checks to produce pre-digested "Detected Issues" and "Suggested Fixes" sections. This enables small LLMs (e.g. gpt-4o-mini) to relay actionable findings without synthesizing root causes from raw YAML. Diagnostic data collected: - VM/VMI status and conditions - DataVolume/PVC state with StorageClass info - Cloud-init configuration (sensitive fields redacted) - virt-launcher pod state and logs - Related events (field-selector targeted) Heuristic checks implemented: - Missing StorageClass with available SC list - DataVolume/PVC errors with SC deduplication - Dangerous cloud-init commands (shutdown, poweroff, halt, reboot, systemctl variants, init 0/6) - Hostname nodeSelector migration blockers - Pod crashloops and OOMKill detection - Failed VirtualMachineInstanceMigration detection - VM condition analysis (Ready=False reasons) Key design decisions: - float64-safe numeric conversion for real cluster data (K8s JSON deserializes numbers as float64) - False-positive prevention in cloud-init parsing - Deduplication between StorageClass and DV errors - API call limiting (Limit: 50) for migrations Signed-off-by: Shirly Radco <sradco@redhat.com> Co-authored-by: AI Assistant <noreply@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com>
missing StorageClass and cloud-init crashloop Adds two eval tasks that exercise the vm_troubleshoot tool's heuristic issue detection: 1. troubleshoot-vm-missing-storageclass: Creates a VM referencing a non-existent StorageClass. Verifies the agent calls vm_troubleshoot and reports the CRITICAL missing SC finding with alternatives. 2. troubleshoot-vm-cloudinit-shutdown: Creates a VM with "shutdown -h now" in cloud-init runcmd that causes CrashLoopBackOff. Verifies the agent identifies the dangerous command as root cause. Both scenarios are deterministic and non-flaky on any cluster with KubeVirt installed. Signed-off-by: Shirly Radco <sradco@redhat.com> Co-authored-by: AI Assistant <noreply@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com>
254606c to
36ea603
Compare
|
/run-mcpchecker kubevirt |
| if isConfigKeyLine(trimmed) { | ||
| continue | ||
| } | ||
| if cmd := matchDangerousCommand(trimmed); cmd != "" { |
There was a problem hiding this comment.
Do we want to only surface the first match, or is it worth accumulating all dangerous matches?
| stripped == "/sbin/shutdown" || strings.HasPrefix(stripped, "/sbin/shutdown ") || | ||
| strings.HasPrefix(stripped, "shutdown -h") || | ||
| strings.HasPrefix(stripped, "shutdown -P") || strings.HasPrefix(stripped, "shutdown -p") || | ||
| strings.HasPrefix(stripped, "shutdown -r"): |
There was a problem hiding this comment.
Isn't shutdown -r a reboot?
| ctx := params.Context | ||
| if ctx == nil { | ||
| ctx = context.Background() | ||
| } |
There was a problem hiding this comment.
no need for this nil check - this should be reliably populated by the core of the mcp server
| func (s *TroubleshootToolSuite) TestToolRegistration() { | ||
| s.Run("tool is registered", func() { | ||
| tools := Tools(&fakeProvider{}) | ||
| s.Require().Len(tools, 1, "Expected 1 troubleshoot tool") | ||
| s.Equal("vm_troubleshoot", tools[0].Tool.Name) | ||
| s.Equal("Virtual Machine: Troubleshoot", tools[0].Tool.Annotations.Title) | ||
| s.NotNil(tools[0].Tool.InputSchema) | ||
| s.NotNil(tools[0].Handler) | ||
| }) | ||
|
|
||
| s.Run("tool has correct annotations", func() { | ||
| tools := Tools(&fakeProvider{}) | ||
| tool := tools[0].Tool | ||
|
|
||
| s.True(*tool.Annotations.ReadOnlyHint, "troubleshoot should be read-only") | ||
| s.False(*tool.Annotations.DestructiveHint, "troubleshoot should not be destructive") | ||
| s.True(*tool.Annotations.IdempotentHint, "troubleshoot should be idempotent") | ||
| s.True(*tool.Annotations.OpenWorldHint, "troubleshoot should be open-world") | ||
| }) | ||
|
|
||
| s.Run("tool has correct schema", func() { | ||
| tools := Tools(&fakeProvider{}) | ||
| schema := tools[0].Tool.InputSchema | ||
|
|
||
| s.Require().NotNil(schema.Properties) | ||
| s.Contains(schema.Properties, "namespace") | ||
| s.Contains(schema.Properties, "name") | ||
| s.ElementsMatch([]string{"namespace", "name"}, schema.Required) | ||
| }) | ||
|
|
||
| s.Run("description mentions priority and scope", func() { | ||
| tools := Tools(&fakeProvider{}) | ||
| desc := tools[0].Tool.Description | ||
| s.Contains(desc, "FIRST") | ||
| s.Contains(desc, "root-cause") | ||
| s.Contains(desc, "StorageClasses") | ||
| s.Contains(desc, "cloud-init") | ||
| }) | ||
| } |
There was a problem hiding this comment.
Generally speaking we do this verification through the pkg/mcp/testdata/.json, and by checking once that that is what we expect. Any future changes would fail those tests.
| }) | ||
| } | ||
|
|
||
| func (s *TroubleshootToolSuite) TestFetchVMStatus() { |
There was a problem hiding this comment.
Maybe for these tests we can just test the public API of the tools in pkg/mcp/kubevirt_test.go ?
We generally try to prefer testing the public api of the tools
4 participants
Summary
vm_troubleshootMCP Tool that collects comprehensive diagnostic data for KubeVirt VirtualMachines (VM/VMI status, DataVolume/PVC state, cloud-init config, virt-launcher pod state, pod logs, and events)vm-troubleshootPrompt with a proper Tool that LLMs can invoke automatically based on user intentMotivation
The existing
vm-troubleshootis implemented as a Prompt, which requires explicit user selection and cannot be auto-invoked by LLMs during troubleshooting conversations. This PR adds a parallel Tool implementation that:Automated Issue Detection
The tool runs heuristic checks on collected diagnostic data and produces a structured report:
Heuristic checks implemented:
Prompt vs. Tool coexistence
The existing
vm-troubleshootPrompt and this newvm_troubleshootTool serve different purposes:Both are kept because they serve different interaction patterns. The Tool enables the agentic/autonomous troubleshooting use case that the Prompt cannot support.
Testing
Tested manually against 3 broken VM scenarios on OpenShift 4.22:
Tested with both gpt-4o-mini (troubleshooting mode) and gpt-5.2 (simple questions) — tool is auto-invoked and produces accurate root-cause identification. Small models correctly relay the pre-analyzed findings without needing to interpret raw YAML.
Key Design Decisions
float64; thetoInt64()helper ensures restartCount detection works on real clusters (not just in tests usingint64directly)Related
vm-troubleshootPrompt (different interaction patterns)openshift/openshift-mcp-serverPR refactor: move provider initialization into factory #365 to upstream per reviewer guidanceTest plan
go build ./...andgo testpass locallySigned-off-by: Shirly Radco sradco@redhat.com
Co-authored-by: AI Assistant noreply@cursor.com