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

Qemu windows machine #16872

Closed
wants to merge 2 commits into from
Closed

Conversation

arixmkii
Copy link
Contributor

@arixmkii arixmkii commented Dec 17, 2022

Adds QEMU podman machine provider on Windows hosts.

How to test it:

  1. Build Windows installer from this branch and install it as normally
  2. Install latest QEMU from https://qemu.weilnetz.de/w64/
  3. Run commands in console to test it:
    3.1. Set environment variable to select provider QEMU
    3.2. Set PATH to include qemu binaries (their default installer seems doesn't update path)
    3.3. Initialize new machine overriding username and image-path, because defaults are different from WSL and there is no way to have defaults per provider as of now
    3.4. Start the machine
    3.5. Run some network enabled workload to verify machine and networking

Example for bullet point 3:

set CONTAINERS_MACHINE_PROVIDER=QEMU
set PATH=%PROGRAMFILES%\qemu;%PATH%
podman machine init --cpus 4 --memory 8192 --username core --image-path testing
podman machine start
podman run -d -p 8080:80 nginx
curl http://localhost:8080/

Alternatively to setting environment variables in console session, one could opt for setting them globally for the host (restart is recommended to make sure that they would be propagated everywhere).

The PR will require rebase later as it includes commit from #16807 If needed the former one may be closed and squashed with this one, but for me it looks like #16807 has the value on its own and is pretty complete.

Note worthy technical details:

Missing features:

  • 9pfs support on Windows hosts is not yet merged into QEMU. So, folder mounts are unsupported.

Known issues:

  • sending Ctrl-C to the console or closing console session, where the machine was started will terminate QEMU and gvproxy. (fixed)

Does this PR introduce a user-facing change?

QEMU podman machine provider on Windows hosts

Signed-off-by: Arthur Sengileyev <[email protected]>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 17, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: arixmkii
Once this PR has been reviewed and has the lgtm label, please assign baude for approval by writing /assign @baude in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@arixmkii arixmkii marked this pull request as draft December 17, 2022 11:26
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 17, 2022
Copy link
Contributor Author

@arixmkii arixmkii left a comment

Choose a reason for hiding this comment

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

Some notes about specific code changes.

@@ -1749,6 +1798,10 @@ func (p *Provider) VMType() string {
return vmtype
}

func toVlanSockPath(path string) string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will not be needed, when #15852 is implemented. Now there is nowhere to save this, so, we rely on a different saved value and derive from it.

@@ -1289,6 +1330,14 @@ func (v *MachineVM) setupAPIForwarding(cmd []string) ([]string, string, apiForwa
cmd = append(cmd, []string{"-forward-user", forwardUser}...)
cmd = append(cmd, []string{"-forward-identity", v.IdentityPath}...)

// Workaround for Podman Desktop compatibility
if runtime.GOOS == "windows" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Workaround to make Podman Desktop happy. Will not be needed when #16860 is implemented and Podman Desktop will be updated to use the CLI instead of building the address internally.

@@ -1229,6 +1261,10 @@ func (v *MachineVM) startHostNetworking() (string, apiForwardingState, error) {
return "", noForwarding, err
}
binary, err := cfg.FindHelperBinary(machine.ForwarderBinaryName, false)
// workaround for lookup on Windows
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will not be needed, when containers/common#1123 is fixed.

cmd = append(cmd, []string{"-listen-qemu", fmt.Sprintf("unix://%s", v.QMPMonitor.Address.GetPath()), "-pid-file", v.PidFilePath.GetPath()}...)
} else {
vlanSocket := machine.VMFile{Path: toVlanSockPath(v.QMPMonitor.Address.GetPath())}
cmd = append(cmd, []string{"-listen-qemu", fmt.Sprintf("unix://%s", strings.Replace(vlanSocket.GetPath(), "\\", "/", -1)), "-pid-file", v.PidFilePath.GetPath()}...)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only place, which is tricky and OS specific, because GO URL parser seems to have troubles handling urls like unix://C:\temp\some.sock.

return err
}
defer qemuSocketConn.Close()
if runtime.GOOS != "windows" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't remember what didn't work well on Windows here. Will need to re-check.

@@ -128,7 +129,12 @@ func (p *Provider) NewMachine(opts machine.InitOptions) (machine.VM, error) {
// Add network
// Right now the mac address is hardcoded so that the host networking gives it a specific IP address. This is
// why we can only run one vm at a time right now
cmd = append(cmd, []string{"-netdev", "socket,id=vlan,fd=3", "-device", "virtio-net-pci,netdev=vlan,mac=5a:94:ef:e4:0c:ee"}...)
if runtime.GOOS != "windows" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically this could be not OS specific, but QEMU version specific switch - it should be possible to use podman machine w/o passing FDs on QEMU versions equal or newer than 7.2.0 on all platforms (no switch necessary at all). But I guess it is a no go yet as there are some older RHEL linuxes, which doesn't have latest QEMU, but still need to be supported, so, it is easier to keep it this way than replace it with a switch based on QEMU version.

@arixmkii
Copy link
Contributor Author

Also, I believe this should require some new tests, but I have no idea how to write e2e tests for this sort of changes (and if it is possible to get accelerated QEMU running in the CI machines).

@arixmkii
Copy link
Contributor Author

arixmkii commented Dec 17, 2022

  • sending Ctrl-C to the console or closing console session, where the machine was started will terminate QEMU and gvproxy.

The fix for this will be to use "w" versions of QEMU binaries ("qemu-system-x86_64w.exe" etc.) and build gvproxy on Windows with -ldflags -H=windowsgui

Fix for QEMU side could be applied by just modifying machine config json. Will push the code fix after gvproxy part is ready.

Update: gvproxy changes requested in containers/gvisor-tap-vsock#167

Signed-off-by: Arthur Sengileyev <[email protected]>
@arixmkii arixmkii force-pushed the qemu-windows-machine branch from c307283 to 5063d1a Compare December 22, 2022 19:47
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2023
@openshift-merge-robot
Copy link
Collaborator

@arixmkii: PR needs rebase.

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/test-infra repository.

@arixmkii
Copy link
Contributor Author

Closing this as prototype has evolved since this PR and potential feedback on this prototype will not be as useful as it could be.

@arixmkii arixmkii closed this Jan 22, 2023
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 5, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2023
@arixmkii arixmkii deleted the qemu-windows-machine branch March 1, 2024 13:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants