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.

Sign up for GitHub

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

Jump to bottom

Add libhvee as alternative Hyper-V driver on Windows #3824

Merged
merged 3 commits into from
Sep 23, 2024
Merged

Add libhvee as alternative Hyper-V driver on Windows #3824

merged 3 commits into from
Sep 23, 2024

Conversation

gbraad
Copy link
Contributor

@gbraad gbraad commented Sep 6, 2023 •
edited
Loading

Fixes #3811

Basic functionality for:

  • create
  • start
  • stop
  • delete

@gbraad gbraad force-pushed the gbraad/spike-add-libhvee-as-alternative-3811 branch 6 times, most recently from 9c10def to 2e72ce2 Compare September 11, 2023 06:51
@gbraad gbraad changed the title [WIP] fixes #3811 add libhvee as alternative Hyper-V driver on Windows Add libhvee as alternative Hyper-V driver on Windows Sep 11, 2023
@gbraad gbraad force-pushed the gbraad/spike-add-libhvee-as-alternative-3811 branch from 0a919ec to 6013d5f Compare September 13, 2023 06:54
@gbraad

This comment was marked as outdated.

Sign in to view
@gbraad gbraad force-pushed the gbraad/spike-add-libhvee-as-alternative-3811 branch from 6013d5f to d1658c1 Compare September 13, 2023 08:13
@gbraad gbraad self-assigned this Sep 19, 2023
@adrianriobo
Copy link
Contributor

adrianriobo commented Oct 5, 2023 •
edited
Loading

These are the results for this PR on windows https://crcqe-asia.s3.amazonaws.com/nightly/ocp/4.13.12/20231005/11-pro/qe-results/e2e-non-ux.xml all green except the known issue with the latest version for the pipelines operator

@gbraad
Copy link
Contributor Author

gbraad commented Oct 6, 2023

Want to incorporate 9P, but the current codebase for this lives in the PR containers/gvisor-tap-vsock#280

@gbraad gbraad mentioned this pull request Oct 6, 2023
Closed
@cfergeau
Copy link
Contributor

cfergeau commented Oct 6, 2023

Want to incorporate 9P, but the current codebase for this lives in the PR containers/gvisor-tap-vsock#280

The code from this gvisor-tap-vsock PR is short, and thus easy to add to the daemon when we need it. But there's also some guest code involved, which might be trickier mheon/libpod@334e213

@gbraad
Copy link
Contributor Author

gbraad commented Oct 6, 2023

@cfergeau it basically is the same/similar code as the 9p-ufs that I tested with earlier, so I am not so concerned. I am more curious if this will therefore be part of a library, or the changes for libpod remain seperate?

@gbraad
Copy link
Contributor Author

gbraad commented Oct 17, 2023

ref: #3870

cfergeau
cfergeau reviewed Oct 31, 2023
View reviewed changes
Copy link
Contributor

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

I've rebased this and squashed the commits in https://github.com/cfergeau/crc/tree/libhvee
This looks like a fairly straightforward change, though I haven't tried running it on a Windows machine :)

pkg/crc/preflight/preflight_checks_windows.go
@@ -166,19 +165,6 @@ func fixUserPartOfCrcUsersAndHypervAdminsGroup() error {
return errReboot
}

func checkIfHyperVVirtualSwitchExists() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Fwiw, it would be good to split the system mode networking removal on Windows in its own preparatory commit, this would make it easier to focus on/review the config/preflight changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I do not even know if this functionality still works. I'll consider, but these checks are also performed when not even necessary (like the existence of the virtual switch) and therefore actually wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've done this in https://github.com/cfergeau/crc/tree/libhvee, this removes even more code

pkg/drivers/libhvee/libhvee_windows.go Show resolved Hide resolved
pkg/drivers/libhvee/libhvee_windows.go Show resolved Hide resolved
pkg/drivers/libhvee/libhvee_windows.go Outdated Show resolved Hide resolved
pkg/drivers/libhvee/libhvee_windows.go Show resolved Hide resolved
pkg/drivers/libhvee/libhvee_windows.go Outdated Show resolved Hide resolved
pkg/drivers/libhvee/libhvee_windows.go Outdated Show resolved Hide resolved
@gbraad gbraad mentioned this pull request Nov 2, 2023
Merged
@gbraad gbraad changed the title Add libhvee as alternative Hyper-V driver on Windows [WIP] Add libhvee as alternative Hyper-V driver on Windows Nov 29, 2023
@cfergeau
Copy link
Contributor

Everlasting rebasing

Rebase done.

@cfergeau cfergeau force-pushed the gbraad/spike-add-libhvee-as-alternative-3811 branch from 729ee25 to 3def960 Compare June 18, 2024 11:44
@cfergeau cfergeau force-pushed the gbraad/spike-add-libhvee-as-alternative-3811 branch 2 times, most recently from 17ce677 to ee4795d Compare September 11, 2024 03:49
@gbraad
Copy link
Contributor Author

gbraad commented Sep 19, 2024

@lilyLuLiu @adrianriobo For this change, the proxy tests are failing. Do we have more information aroudn this?

@adrianriobo
Copy link
Contributor

@gbraad those are mainly failing due to timeout on the full suite, let me check downstream running only that one I will ack if it is green (and will increase the timeout for the suite here)

@lilyLuLiu
Copy link
Contributor

Downstream windows test passed for this PR.

@adrianriobo
Copy link
Contributor

adrianriobo commented Sep 20, 2024 •
edited
Loading

🙏 hold till today mid day need to check something else

We are good to go, downstream proxy is green

cfergeau and others added 3 commits September 20, 2024 11:28
@cfergeau
windows: Remove system networking
4866c23 
Should return an error when trying to set the "system networking"
option. But I think this is already the case, or it's not possible to
choose this option in release builds?

Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
@gbraad @cfergeau
Use libhvee instead of the current PowerShell cmdlet implementation
9d5bab2 
This replaces the interaction of Hyper-V from our machine-drivers based
driver as used in Minikube/minishift to the libhvee library as used by
podman machine.
@cfergeau
libhvee: Use libhvee APIs for disk resize
3003fc5 
Disk handling API was added to libhvee since the first iteration of the
libhvee support, we can now make use of it.

Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
adrianriobo
adrianriobo approved these changes Sep 20, 2024
View reviewed changes
Copy link
Contributor

@adrianriobo adrianriobo left a comment

Choose a reason for hiding this comment

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

Full green on downstream windows env

@openshift-ci openshift-ci bot added the lgtm label Sep 20, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Sep 20, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianriobo

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

@cfergeau
Copy link
Contributor

Full green on downstream windows env

Didn't we wait too long though? We held this PR a while back as we did not want to land it at the same time as the switch to 4.16.
The next crc release could be 4.17-based, should we delay this again by one release?

@cfergeau cfergeau force-pushed the gbraad/spike-add-libhvee-as-alternative-3811 branch from ee4795d to 3003fc5 Compare September 20, 2024 11:07
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Sep 20, 2024

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot removed the lgtm label Sep 20, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Sep 20, 2024

@gbraad: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/security 3003fc5 link false /test security
ci/prow/integration-crc 3003fc5 link true /test integration-crc
ci/prow/e2e-crc 3003fc5 link true /test e2e-crc

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@praveenkumar
Copy link
Member

Full green on downstream windows env

Didn't we wait too long though? We held this PR a while back as we did not want to land it at the same time as the switch to 4.16. The next crc release could be 4.17-based, should we delay this again by one release?

Since it is pass the pipeline, I don't think we should hold it any longer. I am merging this now.

@praveenkumar praveenkumar merged commit d6c6e90 into main Sep 23, 2024
22 of 30 checks passed
@cfergeau
Copy link
Contributor

Since it is pass the pipeline, I don't think we should hold it any longer. I am merging this now.

We held it last time because QE had concerns about their capacity to deal with 2 large changes at once, would have been nice to wait for their input before merging this..

@adrianriobo
Copy link
Contributor

No worries, I am fine with it, as @praveenkumar this already passed (and actually has been passing for a while now). So I think we should be good.

@gbraad
Copy link
Contributor Author

gbraad commented Sep 26, 2024

Thank you all

@gbraad gbraad deleted the gbraad/spike-add-libhvee-as-alternative-3811 branch September 26, 2024 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfergeau cfergeau cfergeau left review comments

@adrianriobo adrianriobo adrianriobo approved these changes

@anjannath anjannath Awaiting requested review from anjannath

@evidolob evidolob Awaiting requested review from evidolob

@praveenkumar praveenkumar Awaiting requested review from praveenkumar

Assignees

@gbraad gbraad

@adrianriobo adrianriobo

Labels
approved os/windows points/3 priority/major
Projects
Project planning: crc
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Spike] Add libhvee as alternative driver for accessing Hyper-V
7 participants
@gbraad @adrianriobo @cfergeau @anjannath @lilyLuLiu @praveenkumar @openshift-merge-robot