cmd: add support for nested virtualization#327
cmd: add support for nested virtualization#327openshift-merge-bot[bot] merged 1 commit intocrc-org:mainfrom
Conversation
Reviewer's GuideAdds nested virtualization support by introducing a CLI flag, extending the VM configuration, propagating the flag through the VM creation path, and implementing a helper that configures vz.GenericPlatformConfiguration with nested virtualization enabled. Sequence diagram for VM creation with nested virtualization flagsequenceDiagram
actor User
participant CLI as vfkit CLI
participant Options
participant Config as VirtualMachine Config
participant VM as VirtualMachine
participant VZ as vz.GenericPlatformConfiguration
User->>CLI: Run vfkit with --nested-virtualization
CLI->>Options: Parse flags
Options->>Config: Set NestedVirtualization = true
Config->>VM: NewVirtualMachine(config)
VM->>VZ: setNestedVirtualization(vfConfig)
VZ-->>VM: Platform config with nested virtualization enabled
VM-->>Config: VirtualMachine instance created
Class diagram for updated VirtualMachine configurationclassDiagram
class VirtualMachine {
uint Vcpus
strongunits.B Memory
Bootloader Bootloader
VirtioDevice[] Devices
TimeSync* Timesync
Ignition* Ignition
bool NestedVirtualization
}
Class diagram for Options struct with NestedVirtualization flagclassDiagram
class Options {
string IgnitionPath
stringSliceValue CloudInitFiles
bool NestedVirtualization
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThis update introduces support for nested virtualization throughout the codebase. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Config
participant VM
participant Platform
User->>CLI: Run vfkit with --nested flag
CLI->>Config: Parse flags, set Options.Nested
Config->>VM: Pass VirtualMachine config with Nested field
VM->>Platform: Call NewGenericPlatformConfiguration (with Nested)
Platform-->>VM: Return platform config (with nested enabled if supported)
VM-->>CLI: Return VM instance or error if unsupported
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/config/config.go (1)
35-35: Consider adding support for the new field in theToCmdLine()method.The new
NestedVirtualizationfield is properly added to the struct, but theToCmdLine()method (lines 84-117) doesn't handle serialization of this field back to command-line arguments. For consistency and completeness, consider adding support for the--nested-virtualizationflag in theToCmdLine()method.if vm.Ignition != nil { args = append(args, "--ignition", vm.Ignition.ConfigPath) } + + if vm.NestedVirtualization { + args = append(args, "--nested-virtualization") + } return args, nilpkg/vf/vm.go (1)
55-73: Consider adding host capability validation.The
setNestedVirtualizationfunction is well-implemented with comprehensive error handling and clear error messages. However, consider adding validation to check if the host system actually supports nested virtualization before attempting to enable it.#!/bin/bash # Check if there are any existing validation patterns for virtualization capabilities in the codebase rg -A 5 -B 5 "Support|Capabilit|Available" --type go
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/vfkit/main.go(1 hunks)pkg/cmdline/cmdline.go(2 hunks)pkg/config/config.go(1 hunks)pkg/config/json_test.go(1 hunks)pkg/vf/vm.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (macOS-14)
- GitHub Check: build (macOS-13)
🔇 Additional comments (5)
cmd/vfkit/main.go (1)
84-84: LGTM! Clean integration of the nested virtualization option.The assignment correctly propagates the command-line option to the VM configuration and follows the established pattern used for other configuration fields.
pkg/config/json_test.go (1)
234-234: Correct test adjustment for the new optional field.Adding
NestedVirtualizationtoskipFieldsis appropriate since the field has anomitemptyJSON tag and should maintain its defaultfalsevalue in stability tests, rather than being automatically filled withtrueby thefillStructfunction.pkg/cmdline/cmdline.go (2)
31-31: LGTM! Clean addition of the nested virtualization option.The new boolean field integrates well with the existing Options struct.
61-61: Well-designed command-line flag.The flag implementation follows established patterns with a descriptive name, reasonable short form (
-n), appropriate default value (false), and clear description.pkg/vf/vm.go (1)
42-47: Well-integrated nested virtualization support for Linux VMs.The conditional logic correctly applies nested virtualization only to Linux platform VMs and includes proper error handling. The placement within the platform configuration section is logical.
gbraad
left a comment
gbraad
left a comment
There was a problem hiding this comment.
Not a big one, but the enable flag says this is 'use' instead.
vyasgun
force-pushed
the
pr/nested-virt
branch
2 times, most recently
from
95488cc to
c47b50f
Compare
cfergeau
left a comment
cfergeau
left a comment
There was a problem hiding this comment.
Can you add a
Fixes: https://github.com/crc-org/vfkit/issues/279
to the commit log?
I’ll test this on a machine without nested support to see if we need to use https://github.com/Code-Hex/vz/blob/5bbaec6daefb962461fdf7fb33b27aff8e939cff/platform.go#L44-L51
pkg/vf/vm.go
Outdated
| } else { | ||
| PlatformType = "linux" | ||
| if vmConfig.NestedVirtualization { | ||
| err := setNestedVirtualization(vfConfig) |
There was a problem hiding this comment.
Can we take a similar approach to the macosBootloader branch and introduce a NewGenericPlatformConfiguration function? At the moment, nested virtualization won’t be enabled with macOS guests.
There was a problem hiding this comment.
do you mean replace NewMacPlatformConfiguration with NewGenericPlatformConfiguration?
There was a problem hiding this comment.
The macOSBootloader branch has to keep using NewMacPlatformConfiguration, but the linux codepath could make use of NewGenericPlatformConfiguration to have some kind of symmetry between the 2.
There was a problem hiding this comment.
ah, okay got it. I'll add the function
This fails with this message which should be ok |
|
Lot of duplication in that error ( |
|
This is merely based on the macOS version, so not a hardware limitation? Does this mean 15 on an M1 would support this? |
It’s not merely based on the macOS version. If you have macOS < 15, then nested virt is not supported. Otherwise, on a M1, this would return false: |
|
I added a check for availability of nested virtualization like this: |
|
When requested, but not available, would you error out or only inform the user (but continue)? If have an opinion, but WDYT? |
|
@gbraad would it be correct to continue with the |
|
@cfergeau WDYS? |
If someone requested nested virtualization when it’s not available, this should be an error. |
|
Agree, fail early... error out hard. You can not continue when something you requested is not available. |
pkg/vf/vm_amd64.go
Outdated
| } | ||
|
|
||
| func NewGenericPlatformConfiguration(vmConfig config.VirtualMachine) (vz.PlatformConfiguration, error) { | ||
| return nil, fmt.Errorf("running generic platform configuration is only supported on ARM devices") |
There was a problem hiding this comment.
Won’t this prevent us from starting VMs on intel? The "platform" API seems available on intel, was there a problem with having NewGenericPlatformConfiguration in vm.go?
There was a problem hiding this comment.
I tried on an Intel machine and virtualization was not available on it so I thought it's only there on arm64 machines. I will put the function in vm.go as that's better
pkg/vf/vm_arm64.go
Outdated
|
|
||
| if vmConfig.Nested { | ||
| err = platformConfig.SetNestedVirtualizationEnabled(true) | ||
| if err != nil { |
There was a problem hiding this comment.
Can this part be moved out of NewGenericPlatformConfiguration so that we can enable nested virt both for mac and linux VMs?
There was a problem hiding this comment.
I'm not sure how to do this since the method SetNestedVirtualizationEnabled is only available in GenericPlatformConfiguration
There was a problem hiding this comment.
Oh indeed, I missed that! We probably should return an error in the macos code path if nested virt was requested.
Apparently no nested virt with macos guests? utmapp/UTM#6700 (comment) this is unexpected!
There was a problem hiding this comment.
Returning an error for macOS when nested virt is requested sounds like the right thing to do. Made the change.
Also, I didn’t expect nested virt to be unsupported for macOS guests either. That UTM issue was helpful, thanks for linking it!
There was a problem hiding this comment.
maybe implied, always possible... or not? would be good to verify/look into further. at least something to describe as I am sure this will come up as a question
Edit: read the issue, and.seems clear; no virt inside nested macos. documentation issue in that case and I would say; either error out when requested on mac or, in this case an INFO as there is nothing you can do about this.
There was a problem hiding this comment.
I tried with tart and it gives an error that nested virtualization is unsupported on macOS guests.
There was a problem hiding this comment.
This is important to also note on https://github.com/crc-org/crc-internal/issues/123, as that would conclude it; the idea was to use macOS to run nested virtualization to test vfkit in a 'clean host'. Done
This commit adds a --nested (-n) flag to enable nested virtualization in vfkit. When enabled on macOS, it sets up a vz.GenericPlatformConfiguration with SetNestedVirtualizationEnabled(true). Fixes: crc-org#279
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfergeau The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
This commit adds a
--nested(-n) flag to enable nested virtualization in vfkit. When enabled on macOS, it sets up avz.GenericPlatformConfigurationwithSetNestedVirtualizationEnabled(true).Fixes: #279
Summary by Sourcery
Add support for nested virtualization by introducing a CLI flag, extending the VM config, and applying a generic platform configuration with nested virtualization enabled on macOS
New Features:
Enhancements:
Tests:
Summary by CodeRabbit