machine: Add provider detection API #22479
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jakecorrenti The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
Ephemeral COPR build failed. @containers/packit-build please check. |
1 similar comment
|
Ephemeral COPR build failed. @containers/packit-build please check. |
jakecorrenti
force-pushed
the
provider-detection
branch
from
f81f467 to
7fc5ae0
Compare
|
Code LGTM |
| } | ||
|
|
||
| var outBuf bytes.Buffer | ||
| cmd := exec.Command("powershell", "Get-WindowsOptionalFeature", "-FeatureName", "Microsoft-Hyper-V-All", "-Online") |
There was a problem hiding this comment.
Is there a way to query this without executing powershell?
Output parsing is fragile in my experience given windows likes to prints output in different locales which caused problems for us before.
There was a problem hiding this comment.
When researching it seemed that running some sort of command in the terminal and parsing the output was the most common solution. I'll try and find a better way to do this, I didn't think of locale issues coming up
There was a problem hiding this comment.
Well I am not sure if Get-WindowsOptionalFeature has locale issues, I know we has similar check for WSL that caused did wsl --version that caused problems #20240
If the powershell command does not has the problems then this might be fine, but I wonder do we not have something like this in libhvee already?
@baude @n1hility can we call something like https://github.com/containers/libhvee/blob/0698042e8c753a95c349a2c45f8a6b04a5aa2e3a/pkg/hypervctl/vmm.go#L28 to know if hyperV is supported or not?
There was a problem hiding this comment.
Installer side I avoided parsing the PowerShell command output. I used the Win32 API QueryServiceStatusEx to query for the status of the vmcompute service. But that's in C.
There was a problem hiding this comment.
Using the libhvee snippet that @Luap99 sent seems to work
jakecorrenti
force-pushed
the
provider-detection
branch
from
7fc5ae0 to
6960685
Compare
Extends the `pkg/machine/provider` package to add an API which includes provider detection based on the host operating system. Signed-off-by: Jake Correnti <[email protected]>
jakecorrenti
force-pushed
the
provider-detection
branch
from
6960685 to
431cbff
Compare
| func InstalledProviders() ([]define.VMType, error) { | ||
| var outBuf bytes.Buffer | ||
| // Apple's Virtualization.Framework is only supported on MacOS 11.0+ | ||
| const SupportedMacOSVersion = 11 |
There was a problem hiding this comment.
We might need to bump this to version 13 instead.
#22121 (comment)
There was a problem hiding this comment.
Other than this though, LGTM.
There was a problem hiding this comment.
Yes, podman machine's use of vfkit+uefi requires macos 13
| return nil, fmt.Errorf("unable to check current macOS version using `sw_vers --productVersion`: %s", err) | ||
| } | ||
|
|
||
| // the output will be in the format of MAJOR.MINOR.PATCH |
There was a problem hiding this comment.
This code is similar to what crc is doing https://github.com/crc-org/crc/blob/main/pkg/os/darwin/release_info.go
Maybe using a semver package would make the version parsing/checking simpler.
| func InstalledProviders() ([]define.VMType, error) { | ||
| var outBuf bytes.Buffer | ||
| // Apple's Virtualization.Framework is only supported on MacOS 11.0+ | ||
| const SupportedMacOSVersion = 11 |
There was a problem hiding this comment.
Yes, podman machine's use of vfkit+uefi requires macos 13
|
im going to merge but @jakecorrenti , perhaps you can consider a semver update |
|
/lgtm |
|
Regarding macos version checks, you could also add a check to the installer, something like cfergeau@f5c5f4a which I haven't tested. |
stale-locking-app
bot
added
the
locked - please file new issue/PR
Extends the
pkg/machine/providerpackage to add an API which includes provider detection based on the host operating system.Does this PR introduce a user-facing change?