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

Enable QEMU Podman machine on Windows #18488

Closed
wants to merge 1 commit into from

Conversation

arixmkii
Copy link
Contributor

@arixmkii arixmkii commented May 5, 2023

Fixes #13006

This simply enables new machine provider as all the required changes were implemented in separate PRs.

Technically this could be merged, but this will not be really functional before #17473 is completed and merged.

No new tests, because this doesn't introduce new unit testable functionality, but for e2e tests this would require Windows VM with nested virtualization or Windows bare metal machine, where QEMU will run.

Does this PR introduce a user-facing change?

Enable QEMU Podman machine on Windows amd64 hosts

[NO NEW TESTS NEEDED]

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels May 5, 2023
@TomSweeneyRedHat
Copy link
Member

@arixmkii is this ready to have the WIP label removed?

@cevich
Copy link
Member

cevich commented May 17, 2023

Windows bare metal machine, where QEMU will run.

We use bare-metal Windows instances for installer and WSL2 testing already. Possibly the only major part missing to enable addition of QEMU tests, is inclusion of it and any dependencies inside the CI VM images. I'm not a Windows expert, but I think the place to do that is https://github.com/containers/automation_images/blob/main/win_images/win_packaging.ps1

With updated images in hand, I can help with updating .cirrus.yml. But I'm afraid I wouldn't be useful in adding/updating the powershell scripts (under contrib/cirrus/). Any assistance there would be greatly appreciated.

@arixmkii
Copy link
Contributor Author

With updated images in hand, I can help with updating .cirrus.yml. But I'm afraid I wouldn't be useful in adding/updating the powershell scripts (under contrib/cirrus/). Any assistance there would be greatly appreciated.

Sounds good. I will try to move forward with #17473 and then come back here to do my best to help with this.

@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Jun 20, 2023

We now support hyperV, I don't think We should be supporting a third way.

@github-actions github-actions bot removed the stale-pr label Jun 21, 2023
@arixmkii
Copy link
Contributor Author

Last time I checked Hyper-V required super user for starting up the machine. This one didn't. But I have to check the current state for Hyper-V. So, there is some reasoning for this to exist still, but I can understand this being dropped - this is fine if this is easy to add and compile from sources (if #17473 is kept and finished).

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2023
@github-actions
Copy link

github-actions bot commented Aug 3, 2023

A friendly reminder that this PR had no activity for 30 days.

@cevich
Copy link
Member

cevich commented Aug 3, 2023

A friendly reminder that this PR had no activity for 30 days.

Still unlikely a problem. There's lots of thought/planning/work going on with this topic. It's part of a bigger multi-os podman-machine testing initiative and design-work. Not sure we're far enough along to decide what to do with this PR yet.

@arixmkii
Copy link
Contributor Author

arixmkii commented Aug 3, 2023

I have rebased version of this one, which I use to test my ongoing work for #17473 I just delay the re-submit until the other part is finalized.

@github-actions github-actions bot removed the stale-pr label Aug 4, 2023
@arixmkii arixmkii force-pushed the enable-qemu-windows branch from c444cc4 to 0f94374 Compare August 8, 2023 20:54
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 8, 2023

[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 umohnani8 for approval. 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

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2023
@arixmkii
Copy link
Contributor Author

arixmkii commented Aug 8, 2023

Build issues will be addressed in #17473 (as this one has no point going forward before the latter). The build issues are caused by recently added functionality, which had no Windows compatible alternatives and were added straight to machine.go instead of machine_windows.go.

@arixmkii arixmkii force-pushed the enable-qemu-windows branch 2 times, most recently from 1bfdfe1 to bd9d494 Compare September 22, 2023 13:51
@arixmkii
Copy link
Contributor Author

Rebased and updated for latest codebase changes.
Still dependent on #17473

@baude
Copy link
Member

baude commented Sep 25, 2023

hello @arixmkii We've been watching this PR with interest and frankly are struggling with it a bit. We already have two supported platforms on windows in WSL and soon to be HyperV. Could we have a broader conversation around why QEMU is more attractive that those two options?

One concern we have is that this broadens and deepens our provider support to three different providers on Windows and we are struggling to keep as it is.

@arixmkii
Copy link
Contributor Author

arixmkii commented Sep 25, 2023

Ran e2e tests with this provider. Only 3 failed tests, which are most probably some OS specifics in them, which has to be checked in details.

  [FAIL] podman machine rm [It] Remove running machine
  C:/msys64/home/username/gitrepos/podman/pkg/machine/e2e/rm_test.go:67
  [FAIL] podman machine start [It] start simple machine
  C:/msys64/home/username/gitrepos/podman/pkg/machine/e2e/start_test.go:40 
  [FAIL] podman machine stop [It] Stop running machine
  C:/msys64/home/username/gitrepos/podman/pkg/machine/e2e/stop_test.go:39 

Brief check is that in all cases successful (expected result) machine stop commands return non zero exit code (not able to reproduce manually outside of test)

@arixmkii
Copy link
Contributor Author

arixmkii commented Sep 25, 2023

@baude sure! First of all this was a thing way before Hyper-V full virt was even publicly debated. I started it in January 2022 #13006

What was beneficial for me:

  • it worked exactly like the one I'm using on macOS (it will soon become irrelevant, when applevf is finalized)
  • full virt - no shared resources with other WSL infra
  • networking - no issues publishing ports for entire network, not only local host
  • comparing to Hyper-V - no need to use elevated shell to start/stop machine
  • verified to work on bare metal and with nested virtualization (with access to published ports from host machine as well)
  • it exposes API via UNIX domain socket in addition to named pipe, so, it is possible to use same automation script written in bash/curl (running it with msys2 on Windows) written on macOS (very niche usecase, but still valid)

I can understand if this one never lands in main branch :) It was pretty interesting learning experience anyway.

@arixmkii
Copy link
Contributor Author

arixmkii commented Sep 25, 2023

Found a cross process file access/race condition like combination, which doesn't work pretty on Windows. Now it passes all e2e tests. Will update PRs after full machine e2e run passes (fixes were tested running tests isolated).

Test run results (time is irrelevant, it ran inside Hyper-V machine on power gapped laptop).

Ran 34 of 34 Specs in 1599.477 seconds
SUCCESS! -- 34 Passed | 0 Failed | 0 Pending | 0 Skipped
PASS

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

arixmkii commented Feb 9, 2024

Will be continued in #21594

@arixmkii arixmkii closed this Feb 9, 2024
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 10, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 10, 2024
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. machine release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Qemu accelerated podman machine on Windows hosts
6 participants