-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add Hyper-V option in windows installer #22506
Conversation
Ephemeral COPR build failed. @containers/packit-build please check. |
LGTM but I dont really have a strong understanding for most of this to accurately review. you need to rebase to pick up the BUD fixes from main. |
ce9f204
to
6a30e11
Compare
Thanks. Rebased. |
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.
Should we fix #22411 before 5.1? If we change the location later we might have to consider backwards compatibility problems
Also should there be a third option, not create the config file at all? So that the user can use whatever the default is without having the config file. I think that is better on updates so a users does not accidental overwrite the previous default and then wonders where the machine went.
Absolutely. If we fix #22411 before 5.1 things will be easier to handle. I am working on that.
Adding a third option ( |
Well I mean selecting none as default seems to best option to me. We still default to WSL if nothing is set in containers.conf so it has the same effect. |
A I can update this PR so that when selecting |
|
||
BOOL isHyperVEnabled() { | ||
/* | ||
* Checks if the Windows service `vmcompute` is running to |
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.
Looking for service names is a good idea around the dism perm issues, but vmcompute is also used by WSL, so we need another way to detect it
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.
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.
yes that sounds like it should work
It's usually acceptable practice to comment out the default, and only when that differs specify a value. From that perspective you could have two options a default of wsl (no config) and hyperv (config file is placed down). Then you could follow up with a custom action that just selects hyperv as the default if it's specified in the config file. |
The reason I am not comfortable with this is that we are going to be in trouble if in the future we decide to make hyper-v the podman machine default (when there is not config file) and we forget to update the installer to create a config file when wsl is selected. Also we are discussing about a rare case: a user that run the installer with wsl and run it again selecting hyper-v without understanding that he will lose its machine. And in my opinion the main problem here, is not that we are allowing a config change during installation (that's fine), but that a user cannot see both wsl and hyper machines when running |
That won't work well, if you select hyperV the first time then WSL the seond time it would need to remove the file which complicates the installer I asumme??
This direction maybe but the other way around first selecting hyperV then the next time leave the default and then all of the sudden things no longer work for the user. I think it is important to keep in mind that most users will have no idea that a containers.conf was created or even it exists which influences this. The support burden is extremely high on us to figure out what happen and why x no longer works in issues. Also has anyone looked at how this works via podman-desktop? AFAIK they allow users to update podman and if they just force the default there it might very well overwrite hyperV breaking things unexpectedly.
That certainly is one problem but I don't see how/when this is going to change. |
The PR to support system-wide containers config containers/common#1973 |
6a30e11
to
14a6024
Compare
A quick update: I have updated this PR to verify if HyperV is installed using the windows service I am testing some changes to create the configuration file only if those conditions are met:
|
c648f6b
to
20085b0
Compare
The PR is updated but we should wait for containers/common#1973 to get vendored in podman in order to merge it. @Luap99 now the creation of
Also if one of the conditions above is met the provider choice is not presented to the user when running the @n1hility the criteria to check for Hyper-V installation is now to look at the service
Also all the logic is now in the |
be25c0e
to
4a3afe5
Compare
Sounds good to me. |
Changes to |
That sounds ok as long as we aren't distributing the msi. The wsl installation aspect requires it. Also a code signed msi on its own looks like a rogue component (no podman icon). I assume you tested manually both options just to verify no strange msi states? From a code review perspective it looks good just want to double check you verified it. |
You mean if I've tested the standalone msi? That was always the first thing I validated as that's fastest to build and test compared to the bundle. For every wsl/hyperv permutation I have tested the msi with GUI and the setup.exe via CLI (as this PR doesn't introduces changes on the bundle template side). |
Agreed. That's a proposal (for a next PR) related to the observation that the standalone msi isn't published on GitHub release page and podman.io. |
podman.msi GUI has a radio-button to select WSL or Hyper-V The checkbox in podman.msi GUI allow the user to specify if the machine provider installation (WSL or Hyper-V) should be part of podman installation or not. podman-setup.exe supports 2 new variables: MachineProvider (valid values are `wsl` and `hyperv`) and HyperVCheckbox (valid values are `0` and `1`) Installation creates the configuration file `99-podman-machine-provider.conf` under folder `%APPDATA\containers\containers.conf.d` with the selected machine provider Cirrus CI `win_installer_task` tests the installation with both `hyperv` and `wsl` and verifies the configuration. Uninstallation is tested too. Note that podman-setup.exe GUI doesn't allow to choose the provider yet. See containers#22492 Signed-off-by: Mario Loriedo <[email protected]>
/lgtm |
/retest |
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.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: l0rd, Luap99 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 |
72db50e
into
containers:main
/cherrypick v5.1 |
@l0rd: only containers org members may request cherry picks. If you are already part of the org, make sure to change your membership to public. Otherwise you can still do the cherry-pick manually. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/cherrypick v5.1 |
@l0rd: new pull request created: #22836 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Here is a list of the changes included with this PR:
podman.msi
GUI has a radio button to select WSL or Hyper-Vpodman.msi
GUI checkbox (to install WSL) installs Hyper-V if that's the selected providerpodman.msi
GUI radio buttons and checkbox are hidden if one of the following conditions are met:%PROGRAMDATA%\containers\containers.conf.d\99-podman-machine-provider.conf
exists (no matter the content)podman.exe
file is found in%PROGRAMFILES%/RedHat/Podman
SkipConfigFileCreation=1
via command line%PROGRAMDATA%\containers\containers.conf.d\99-podman-machine-provider.conf
:99-podman-machine-provider.conf
is skipped if one of those conditions are met:podman.exe
file is found in%PROGRAMFILES%/RedHat/Podman
SkipConfigFileCreation=1
via command linewin_installer_task
has been updated to test installation withhyperv
and uninstallation too.podman-dialog.png
has been slightly changed to match the color of the MSI package checkbox and radio-button.podman-setup.exe
supports 3 new variables:MachineProvider
(valid values arewsl
andhyperv
)HyperVCheckbox
(valid values are0
and1
)SkipConfigFileCreation
(valid values are0
and1
)Note that
podman-setup.exe
GUI doesn't allow to choose the provider yet. See the screenshot below and #22492How to test it
Does this PR introduce a user-facing change?