Skip to content

hvf: Add API to verify Nested Virt is supported #320

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jakecorrenti
Copy link
Member

Add an API to check if the current system supports Nested Virt on macOS.

@jakecorrenti
Copy link
Member Author

Related to comments in containers/podman#25922

@jakecorrenti
Copy link
Member Author

jakecorrenti commented Apr 23, 2025

On the krunkit side, my thought was we could allow the user to still do --nested, but if krun_check_nested_virt returns <1 we would just ignore the argument and print something to stdout or log it describing as such

@jakecorrenti jakecorrenti force-pushed the macos-nested-check branch 4 times, most recently from 0c582d5 to cc35fce Compare April 23, 2025 20:21
@tylerfanelli
Copy link
Member

If it's not supported, couldn't we just have krun_set_nested_virt return an error code?

@jakecorrenti
Copy link
Member Author

jakecorrenti commented Apr 24, 2025

If it's not supported, couldn't we just have krun_set_nested_virt return an error code?

That was the original behavior. At this moment, if the user wants to enable nested virt, they either have to already know the requirements for the feature and do some system queries, or they have to try and call krun_set_nested_virt then check if it fails. This provides an easy way for the consumer of the API to programmatically check if they meet all necessary requirements before hand. Think of it like calling KVM_CHECK_EXTENSION on a specific capability before using the associated ioctl.

For consumers of krunkit, like Podman, having the command fail is not a graceful way to handle the issue. It's also not ideal UX to have each consumer do the same manual checks when we can provide it for them.

@tylerfanelli
Copy link
Member

tylerfanelli commented Apr 24, 2025

Your stated use-case in krunkit (the main consumer of this API):

On the krunkit side, my thought was we could allow the user to still do --nested, but if krun_check_nested_virt returns <1 we would just ignore the argument and print something to stdout or log it describing as such

What is the difference between krun_check_nested_virt returning <1 as opposed to krun_set_nested_virt returning <1. The outcome would be the same IMO.

@tylerfanelli
Copy link
Member

For consumers of krunkit, like Podman, having the command fail is not a graceful way to handle the issue. It's also not ideal UX to have each consumer do the same manual checks when we can provide it for them.

We can define this failure as its own error code and have krunkit wrap/handle it accordingly for podman.

Add an API to check if the current system supports Nested Virt on macOS.

Signed-off-by: Jake Correnti <[email protected]>
@jakecorrenti
Copy link
Member Author

jakecorrenti commented Apr 24, 2025

Your stated use-case in krunkit (the main consumer of this API):

On the krunkit side, my thought was we could allow the user to still do --nested, but if krun_check_nested_virt returns <1 we would just ignore the argument and print something to stdout or log it describing as such

What is the difference between krun_check_nested_virt returning <1 as opposed to krun_set_nested_virt returning <1. The outcome would be the same IMO.

In this scenario, you're right and the outcome would be the same. In the grand scheme of things, in my opinion, checking if nested virt is supported and enabling nested virt should be two separate operations.

I don't want this to be a controversial change, so if it's best to go about this a different way so everyone's happy I can do that.

@tylerfanelli
Copy link
Member

I think the check itself is fine. I'm just not convinced it warrants its own krun API, when krun_set_nested_virt could do the check itself and return EOPNOTSUP if it fails. krunkit could then ignore the arg and print some log message saying its not supported.

I try to avoid adding new krun APIs unless absolutely necessary though. Perhaps @slp has different thoughts?

@tylerfanelli
Copy link
Member

In this scenario, you're right and the outcome would be the same. In the grand scheme of things, in my opinion, checking if nested virt is supported and enabling nested virt should be two separate operations.

But every time you check for nested virt and it is supported, you would then enable, correct? This suggests to me it can be one operation.

@mtjhrc
Copy link
Collaborator

mtjhrc commented Apr 28, 2025

I don't have a strong opinion either way, but for me having this API seems fine (and doesn't seem hard to maintain). But yes, you can get by without having this API.

This would perhaps be useful if you are creating a VM configuration to save to a file using some sort of CLI, but not starting it the VM yet - you can tell the user nested virt is not supported on their system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants