-
Notifications
You must be signed in to change notification settings - Fork 168
WIP feat(kubevirt): Add VM management toolset #386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
9aa2732 to
4eac816
Compare
| } | ||
|
|
||
| // extractDataSourceInfo extracts source information from a DataSource object | ||
| func extractDataSourceInfo(obj *unstructured.Unstructured) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to import here the CDI package and use structures from there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only if we really need it tbh, I don't think anything requires it just yet.
Introduces a new KubeVirt toolset providing virtual machine management capabilities through MCP tools. The vm_create tool generates comprehensive creation plans with pre-creation validation of instance types, preferences, and container disk images, enabling AI assistants to help users create VirtualMachines with appropriate resource configurations. The tool supports: - Workload specification via OS names or container disk URLs - Auto-selection of instance types based on size/performance hints - DataSource integration for common OS images - Comprehensive validation and planning before resource creation Assisted-By: Claude <[email protected]> Signed-off-by: Lee Yarwood <[email protected]>
…late Implements a new troubleshoot tool for VirtualMachines in the kubevirt toolset. The tool provides automated diagnostic plans based on VM status, conditions, and common issues, helping users identify and resolve VM problems efficiently. Adds: - Troubleshoot tool implementation with structured diagnostic output - Template-based diagnostic plan generation - Comprehensive test coverage for the troubleshoot functionality Assisted-By: Claude <[email protected]> Signed-off-by: Lee Yarwood <[email protected]>
The 'latest' tag for several containerdisk images was either missing or not the most appropriate tag to use. This change updates the image references to specific, valid tags to ensure reliability. - 'ubuntu' is updated from 'latest' to '24.04'. - 'centos-stream' is updated from 'latest' to '9-latest'. - 'opensuse-tumbleweed' is updated from 'latest' to '1.0.0'. - 'opensuse-leap' is updated from 'latest' to '15.6'. Assisted-By: Claude <[email protected]> Signed-off-by: Lee Yarwood <[email protected]>
Signed-off-by: Lee Yarwood <[email protected]>
Signed-off-by: Lee Yarwood <[email protected]>
Previously, the vm_create tool only searched for DataSources in the OpenShift-specific 'openshift-virtualization-os-images' namespace. This change extends the search to also include the 'kubevirt-os-images' namespace, which is commonly used in upstream KubeVirt deployments. The logic for querying a namespace has been refactored into a helper function to accommodate searching multiple well-known locations for boot images. Assisted-By: Claude <[email protected]> Signed-off-by: Lee Yarwood <[email protected]>
Signed-off-by: Lee Yarwood <[email protected]>
Signed-off-by: Lee Yarwood <[email protected]>
Break down the searchDataSources function into smaller, focused helper functions: - getDynamicClient: Handles dynamic client initialization - collectDataSources: Collects DataSources from well-known and all namespaces - deduplicateAndMergeDataSources: Merges and deduplicates DataSource results This improves code readability and maintainability by following the single responsibility principle. Assisted-By: Claude <[email protected]> Signed-off-by: Lee Yarwood <[email protected]>
Decompose the create function into smaller, focused helper functions: - parseCreateParameters: Parses and validates input parameters into a struct - matchDataSource: Matches workload input against available DataSources - resolvePreference: Resolves preference from DataSource defaults or cluster resources - resolveInstancetype: Resolves instancetype from DataSource defaults or size/performance hints - matchInstancetypeBySize: Matches instancetype based on size and performance criteria - filterInstancetypesBySize: Filters instancetypes by size hint - buildTemplateParams: Constructs vmParams from resolved resources - renderTemplate: Renders the VM creation plan template This refactoring improves code organization and readability by: - Following the single responsibility principle - Making the main create function a clear orchestrator - Reducing complexity and improving testability - Enhancing maintainability through focused, reusable functions Assisted-By: Claude <[email protected]> Signed-off-by: Lee Yarwood <[email protected]>
Extend the VM creation tool to search for both cluster-wide and namespaced resources: - Update searchPreferences to query: - VirtualMachineClusterPreference (cluster-wide) - VirtualMachinePreference (namespaced) - Update searchInstancetypes to query: - VirtualMachineClusterInstancetype (cluster-wide) - VirtualMachineInstancetype (namespaced) This allows the tool to discover and use namespaced preferences and instancetypes in addition to cluster-wide resources, providing more flexibility for VM creation in multi-tenant environments where resources may be scoped to specific namespaces. The namespace context is passed from the VM creation request, enabling the search to include resources from the target namespace where the VM will be created. Assisted-By: Claude <[email protected]> Signed-off-by: Lee Yarwood <[email protected]>
…uide Extend the DataVolume troubleshooting section (Step 3) to include: - Check underlying PersistentVolumeClaim (PVC) status - Verify PVC phase is "Bound" - Check storage class name and capacity - Common PVC issues and their meanings - Check StorageClass when PVC is pending - Verify storage class exists - Ensure provisioner is healthy This provides more comprehensive guidance for diagnosing storage-related issues, as PVC problems are often the root cause of DataVolume failures. The enhanced troubleshooting steps help users identify issues with: - Storage class availability - Provisioner health - Storage capacity - PVC binding problems Assisted-By: Claude <[email protected]> Signed-off-by: Lee Yarwood <[email protected]>
Introduce a comprehensive gevals testing framework to validate VM lifecycle operations including creation with various configurations (basic, Ubuntu, instancetypes, performance, sizing) and troubleshooting scenarios. This enables automated verification of the KubeVirt toolset's functionality and regression prevention. Assisted-By: Claude <[email protected]> Signed-off-by: Lee Yarwood <[email protected]>
…ed helpers Extract common verification patterns from VM test tasks into a shared helper library to improve code reuse, maintainability, and consistency. Changes: - Create shared verification helper library in tests/helpers/ - verify-vm.sh: 8 reusable verification functions with flexible matching - README.md: Comprehensive documentation with usage examples - Refactor all VM test tasks to use shared helpers: - create-vm-basic: 43 → 33 lines (~23% reduction) - create-vm-ubuntu: 43 → 33 lines (~23% reduction) - create-vm-with-size: 62 → 39 lines (~37% reduction) - create-vm-with-instancetype: 59 → 39 lines (~34% reduction) - create-vm-with-performance: 72 → 45 lines (~38% reduction) Helper Functions: - verify_vm_exists: Wait for VM creation - verify_container_disk: Verify OS container disk (checks all volumes) - verify_run_strategy: Check runStrategy (accepts spec or status) - verify_instancetype: Verify instancetype with optional exact match - verify_instancetype_contains: Check instancetype contains substring - verify_instancetype_prefix: Check instancetype starts with prefix - verify_no_direct_resources: Verify VM uses instancetype - verify_has_resources_or_instancetype: Verify either instancetype or resources Bug Fixes: - Fix volume name verification to check all volumes instead of specific names (handles different VM creation approaches) - Fix runStrategy verification to accept both spec.runStrategy and status.runStrategy (handles deprecated 'running' field) Benefits: - Eliminated ~150 lines of duplicated verification code - Single source of truth for verification logic - More maintainable and easier to extend - Consistent error messages and output formatting - Tests are more concise and readable Assisted-By: Claude <[email protected]> Signed-off-by: Lee Yarwood <[email protected]>
…eprecated running field Strengthen the VM creation guidance and add explicit test assertions to ensure the deprecated 'running' field is not used in VirtualMachine specs. Changes to VM Creation Template (plan.tmpl): - Add prominent warning section about deprecated 'running' field - Include side-by-side comparison showing incorrect vs. correct usage - Clear visual indicators (❌ INCORRECT vs. ✅ CORRECT) - Emphasize that 'runStrategy' must be used instead Changes to Test Helper Functions (verify-vm.sh): - Add new `verify_no_deprecated_running_field()` function - Explicitly checks that spec.running is not set - Returns failure if deprecated field is found - Provides clear error message directing users to use runStrategy Changes to Test Tasks (all 5 VM creation tests): - Add assertion in all test tasks to verify running field is not used - Tests now explicitly call `verify_no_deprecated_running_field()` - Ensures compliance with KubeVirt best practices - Tests fail if VMs are created with deprecated field Benefits: - Prevents use of deprecated KubeVirt API fields - Explicit test coverage for API field deprecation - Clear guidance for AI assistants creating VMs - Ensures forward compatibility with future KubeVirt versions - Enforces best practices through automated testing Assisted-By: Claude <[email protected]> Signed-off-by: Lee Yarwood <[email protected]>
|
Just dumping some test output using genmcp/gevals#24 to help visualise the results and |
@lyarwood +1 from my side on that being nice, the reason we haven't been able to add it there is that we use claude code in the non-interactive setup in |
| } | ||
| } | ||
|
|
||
| type vmParams struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd vote to name it as VirtualMachineParams.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually thinking of going the other way and reducing this to params as the package already highlights that this is for VMs.
| } | ||
|
|
||
| // createParameters holds parsed input parameters for VM creation | ||
| type createParameters struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CreateVirtualMachineParams? Keep it consistent with Params or Parameters.
| } | ||
|
|
||
| // parseCreateParameters parses and validates input parameters | ||
| func parseCreateParameters(params api.ToolHandlerParams) (*createParameters, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest naming it as parseCreateVirtualMachineParams. Even if it's a private function and not exposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, I think parseCreateParameters is enough given the package name context.
|
|
||
| workload := getOptionalString(params, "workload") | ||
| if workload == "" { | ||
| workload = "fedora" // Default to fedora if not specified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not returning an error as well? This is parameters validation, not a VM specification that can contain defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's optional, if anything getOptionalString should be extended to provide a default.
A very draft PR to start discussions about including a targeted KubeVirt toolset into the project focused on creating and troubleshooting VMs.