Skip to content
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

Merged
merged 1 commit into from
May 29, 2024

Conversation

l0rd
Copy link
Member

@l0rd l0rd commented Apr 25, 2024

image

Here is a list of the changes included with this PR:

  • podman.msi GUI has a radio button to select WSL or Hyper-V
  • podman.msi GUI checkbox (to install WSL) installs Hyper-V if that's the selected provider
  • podman.msi GUI radio buttons and checkbox are hidden if one of the following conditions are met:
    • The file %PROGRAMDATA%\containers\containers.conf.d\99-podman-machine-provider.conf exists (no matter the content)
    • A pre-existing podman.exe file is found in %PROGRAMFILES%/RedHat/Podman
    • The user sets the property SkipConfigFileCreation=1 via command line
  • Fresh installation creates file %PROGRAMDATA%\containers\containers.conf.d\99-podman-machine-provider.conf:
    [machine]
    provider="hyperv" # or "wsl"
  • Creation of the file 99-podman-machine-provider.conf is skipped if one of those conditions are met:
    • The file already exists (no matter the content)
    • A pre-existing podman.exe file is found in %PROGRAMFILES%/RedHat/Podman
    • The user sets the property SkipConfigFileCreation=1 via command line
  • Cirrus CI win_installer_task has been updated to test installation with hyperv and uninstallation too.
  • The background color of the 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 are wsl and hyperv)
    • HyperVCheckbox (valid values are 0 and 1)
    • SkipConfigFileCreation (valid values are 0 and 1)

Note that podman-setup.exe GUI doesn't allow to choose the provider yet. See the screenshot below and #22492

image

How to test it

# Build the installer 
cd .\contrib\win-installer\
.\build.ps1 5.0.2 dev

# Run the msi and use the GUI
msiexec /package podman.msi /l*v inst.log

# Or run the setup.exe installer selecting WSL
`.\podman-5.0.2-dev-setup.exe /install /quiet WSLCheckbox=1 /log install-wsl.log`

# Or run the setup.exe installer selecting Hyper-V
.\podman-5.0.2-dev-setup.exe /install /quiet MachineProvider=hyperv HyperVCheckbox=0 /log install-hyperv.log

Does this PR introduce a user-facing change?

Windows installer `podman-setup.exe` supports 3 new command line variables:
- `MachineProvider`: choose podman machine virtualization provider (can be `wsl` or `hyperv`, the default is `wsl`).
- `HyperVCheckbox`: if set to `1` the installation installs Hyper-V (the default is `0`)
- `SkipConfigFileCreation`: if set to `1` the installation skips the creation of  `%PROGRAMDATA%\containers\containers.conf.d\99-podman-machine-provider.conf` (the default is `0`)

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@baude
Copy link
Member

baude commented Apr 25, 2024

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.

@l0rd
Copy link
Member Author

l0rd commented Apr 25, 2024

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.

Thanks. Rebased.

Copy link
Member

@Luap99 Luap99 left a 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.

@l0rd
Copy link
Member Author

l0rd commented Apr 26, 2024

Should we fix #22411 before 5.1? If we change the location later we might have to consider backwards compatibility problems

Absolutely. If we fix #22411 before 5.1 things will be easier to handle. I am working on that.

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.

Adding a third option (wsl/hyperv/none) is clever because it's a simple solution. But if we keep wsl as the radio-button selected by default, most users will go with that, no matter if they are installing for the first time or if they are doing an update. So that won't be effective in my opinion. We should introduce a custom action, executed before the GUI is rendered, that looks for an existing machine provider config. If it finds one, the options in the GUI will be hidden. That's for a distinct PR though.

@Luap99
Copy link
Member

Luap99 commented Apr 26, 2024

Adding a third option (wsl/hyperv/none) is clever because it's a simple solution. But if we keep wsl as the radio-button selected by default, most users will go with that, no matter if they are installing for the first time or if they are doing an update. So that won't be effective in my opinion. We should introduce a custom action, executed before the GUI is rendered, that looks for an existing machine provider config. If it finds one, the options in the GUI will be hidden. That's for a distinct PR though.

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.

@l0rd
Copy link
Member Author

l0rd commented Apr 26, 2024

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 none that is actually wsl looks confusing.

I can update this PR so that when selecting wsl, then no config file is created. But I don't like that neither as it unnecessarily couples installer default with machine default. None of those options looks ideal to me and I would rather keep the PR as it is and work on a proper solution (dynamic options) in a distinct PR.


BOOL isHyperVEnabled() {
/*
* Checks if the Windows service `vmcompute` is running to
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, thank you.

I have done some tests and it looks like the service vmms (Hyper-V Virtual Machine Management service) is not running when wsl is installed and is actually running when Hyper-V is installed. Would that work in your opinion?

image

image

Copy link
Member

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

@n1hility
Copy link
Member

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 none that is actually wsl looks confusing.

I can update this PR so that when selecting wsl, then no config file is created. But I don't like that neither as it unnecessarily couples installer default with machine default. None of those options looks ideal to me and I would rather keep the PR as it is and work on a proper solution (dynamic options) in a distinct PR.

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.

@l0rd
Copy link
Member Author

l0rd commented Apr 29, 2024

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 machine ls, and I believe this is going to be addressed anyway.

@Luap99
Copy link
Member

Luap99 commented Apr 29, 2024

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.

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??

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.

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.

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 machine ls, and I believe this is going to be addressed anyway.

That certainly is one problem but I don't see how/when this is going to change.

@l0rd
Copy link
Member Author

l0rd commented Apr 29, 2024

The PR to support system-wide containers config containers/common#1973

@l0rd
Copy link
Member Author

l0rd commented May 6, 2024

A quick update:

I have updated this PR to verify if HyperV is installed using the windows service vmms (Hyper-V Virtual Machine Management service)

I am testing some changes to create the configuration file only if those conditions are met:

  • The file $ProgramFiles/Red Hat/Podman/podman.exe doesn't exist
  • The configuration file $ProgramData/containers/containers.conf.d/99-podman-machine-provider.conf doesn't exist

@l0rd l0rd force-pushed the win-install-hyperv branch 2 times, most recently from c648f6b to 20085b0 Compare May 7, 2024 20:44
@l0rd
Copy link
Member Author

l0rd commented May 7, 2024

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 %PROGRAMDATA%\containers\containers.conf.d\99-podman-machine-provider.conf is skipped if one of these conditions are met:

  • %PROGRAMDATA%\containers\containers.conf.d\99-podman-machine-provider.conf exists (no matter the content)
  • A pre-existing podman.exe file is found in %PROGRAMFILES%/RedHat/Podman
  • The user sets the property SkipConfigFileCreation=1 while running setup.exe via the command line

Also if one of the conditions above is met the provider choice is not presented to the user when running the podman.msi GUI:
image

@n1hility the criteria to check for Hyper-V installation is now to look at the service vmms. And the changes to the podman.wxs are now more important to support the dynamic options on the GUI. To validate that I have tested all the permutations of the variables that I think are affecting the installation:

  • selected provider: wsl/hyperv
  • install the provider: yes/no
  • wsl is pre-installed: yes/no
  • hyperv is pre-installed: yes/no
  • is there a previous podman installation: yes/no

Also all the logic is now in the podman.msi GUI and not in the setup.exe GUI. In the short term we should update the setup.exe GUI too and simultaneously simplify the msi GUI (e.g. no radio buttons / checkboxes but some debugging text showing the msi properties values) as that GUI is not exposed to the users.

@l0rd l0rd force-pushed the win-install-hyperv branch 2 times, most recently from be25c0e to 4a3afe5 Compare May 8, 2024 09:26
@Luap99
Copy link
Member

Luap99 commented May 8, 2024

@Luap99 now the creation of %PROGRAMDATA%\containers\containers.conf.d\99-podman-machine-provider.conf is skipped if one of these conditions are met:

* `%PROGRAMDATA%\containers\containers.conf.d\99-podman-machine-provider.conf` exists (no matter the content)

* A pre-existing `podman.exe` file is found in `%PROGRAMFILES%/RedHat/Podman`

* The user sets the property `SkipConfigFileCreation=1` while running setup.exe via the command line

Sounds good to me.

@l0rd
Copy link
Member Author

l0rd commented May 13, 2024

Changes to containers/common required by this PR are now vendored in containers/podman. So it's ok to merge when @n1hility approves it.

@n1hility
Copy link
Member

n1hility commented May 15, 2024

Also all the logic is now in the podman.msi GUI and not in the setup.exe GUI. In the short term we should update the setup.exe GUI too and simultaneously simplify the msi GUI (e.g. no radio buttons / checkboxes but some debugging text showing the msi properties values) as that GUI is not exposed to the users.

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.

@l0rd
Copy link
Member Author

l0rd commented May 15, 2024

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).

@l0rd
Copy link
Member Author

l0rd commented May 15, 2024

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).

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 <mario.loriedo@gmail.com>
@n1hility
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 29, 2024
@l0rd
Copy link
Member Author

l0rd commented May 29, 2024

/retest

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

openshift-ci bot commented May 29, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 29, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 72db50e into containers:main May 29, 2024
87 of 90 checks passed
@l0rd
Copy link
Member Author

l0rd commented May 29, 2024

/cherrypick v5.1

@openshift-cherrypick-robot
Copy link
Collaborator

@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:

/cherrypick v5.1

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.

@l0rd
Copy link
Member Author

l0rd commented May 29, 2024

/cherrypick v5.1

@openshift-cherrypick-robot
Copy link
Collaborator

@l0rd: new pull request created: #22836

In response to this:

/cherrypick v5.1

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants