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 API forwarding support for HyperV #20878

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

ashley-cui
Copy link
Member

@ashley-cui ashley-cui commented Dec 1, 2023

Provides Docker API client access, allowing compose to work by default for HyperV. Basically the HyperV equiv of the work done here by #12916.

[NO NEW TESTS NEEDED]

Closes #20761

Fixed a bug where Podman machine on HyperV did not allow API forwarding

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 1, 2023
@ashley-cui ashley-cui marked this pull request as draft December 1, 2023 22:16
@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 1, 2023
@ashley-cui ashley-cui marked this pull request as ready for review December 1, 2023 23:02
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2023
@ashley-cui
Copy link
Member Author

@n1hility @jeffmaury PTAL. Tested and it seems to work for me.

@ashley-cui ashley-cui added the 4.8 label Dec 1, 2023
@ashley-cui
Copy link
Member Author

@containers/podman-maintainers @mheon @baude PTAL, would like to get this into v4.8.1 if possible

Copy link

Cockpit tests failed for commit 13ade675756ecc201cafac720cf61f3a93fdcbad. @martinpitt, @jelly, @mvollmer please check.

@martinpitt
Copy link
Contributor

Yup, that's a regression in criu in podman-next. We see it in our nightly runs against podman-next as well, it fails on the same "criu restore" check. I'll investigate/report on Monday.

@rhatdan
Copy link
Member

rhatdan commented Dec 2, 2023

LGTM

Copy link
Member

@giuseppe giuseppe 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 Dec 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashley-cui, giuseppe

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:
  • OWNERS [ashley-cui,giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jeffmaury
Copy link

LGTM but I add to add ssh-keygen to get the machine created

@n1hility
Copy link
Member

Sorry this one fell through the cracks for me and the very late review. This LGTM

This is not a blocker at all, but just mentioning to factor in / consider for the future. Since the hyperv code uses gvproxy for ssh routing / port forwarding, if we wanted to save a process and drop usage of win-sshproxy the ssh options on gvproxy could be used for this purpose instead (like what we do on mac). It's still a trade-off to some extent since it would mean differences between the hyperv and wsl code.

@rhatdan
Copy link
Member

rhatdan commented Dec 14, 2023

/lgtm
/hold

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Dec 14, 2023
@jeffmaury
Copy link

I have 2 remarks:

C:\Users\Jeff\work\src\github.com\docker\awesome-compose\flask-redis>podman compose up -d
>>>> Executing external compose provider "C:\\Users\\Jeff\\AppData\\Local\\Microsoft\\WindowsApps\\docker-compose.exe". Please refer to the documentation for details. <<<<

[+] Running 2/0
 ✔ Network flask-redis_default    Created                                                                          0.0s
 ✔ Container flask-redis-redis-1  Created                                                                          0.1s
 - Container flask-redis-web-1    Creating                                                                         0.0s
Error response from daemon: fill out specgen: C:\Users\Jeff\work\src\github.com\docker\awesome-compose\flask-redis:/code:rw: incorrect volume format, should be [host-dir:]ctr-dir[:option]
Error: executing C:\Users\Jeff\AppData\Local\Microsoft\WindowsApps\docker-compose.exe up -d: exit status 1

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2023
@rhatdan
Copy link
Member

rhatdan commented Dec 19, 2023

@ashley-cui needs a rebase

@rhatdan rhatdan removed the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 4, 2024
@ashley-cui
Copy link
Member Author

@jeffmaury

  • when the machine is stopped the win-sshproxy processed is not stopped

Updated, thanks!

This is another followup issue, where the parsing of mounts on HyperV requires absolute paths. Will address this later.

@ashley-cui ashley-cui force-pushed the winapiforwarding branch 2 times, most recently from e2b1f5d to be94081 Compare January 5, 2024 16:02
Provides Docker API client access, allowing compose to work by default
for HyperV. Basically the HyperV equiv of the work done here by containers#12916.

[NO NEW TESTS NEEDED]

Signed-off-by: Ashley Cui <[email protected]>
@ashley-cui
Copy link
Member Author

Should be good to go, I think the mac failure is unrelated.

@jakecorrenti
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2024
@jakecorrenti
Copy link
Member

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 8, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 3b18c77 into containers:main Jan 8, 2024
91 of 92 checks passed
@ashley-cui
Copy link
Member Author

/cherry-pick v4.8

@openshift-cherrypick-robot
Copy link
Collaborator

@ashley-cui: #20878 failed to apply on top of branch "v4.8":

Applying: Add API forwarding support for HyperV
Using index info to reconstruct a base tree...
M	cmd/podman/compose.go
M	pkg/machine/hyperv/machine.go
M	pkg/machine/machine_windows.go
M	pkg/machine/wsl/machine.go
M	pkg/machine/wsl/usermodenet.go
M	pkg/machine/wsl/util_windows.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/machine/wsl/util_windows.go
Auto-merging pkg/machine/wsl/usermodenet.go
Auto-merging pkg/machine/wsl/machine.go
Auto-merging pkg/machine/machine_windows.go
Auto-merging pkg/machine/hyperv/machine.go
Auto-merging cmd/podman/compose.go
CONFLICT (content): Merge conflict in cmd/podman/compose.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add API forwarding support for HyperV
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick v4.8

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.

@ashley-cui ashley-cui deleted the winapiforwarding branch January 8, 2024 20:49
@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 Apr 8, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4.8 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. 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.

Cannot run compose projects with HyperV machines
10 participants