Improve cross platform support of QEMU machine #21252
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: arixmkii 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 |
arixmkii
force-pushed
the
cross-platform-qemu-fixes
branch
2 times, most recently
from
bec9053 to
576db31
Compare
|
LGTM |
| @@ -1101,18 +1115,18 @@ func getDiskSize(path string) (uint64, error) { | |||
|
|
|||
| // startHostNetworking runs a binary on the host system that allows users | |||
| // to set up port forwarding to the podman virtual machine | |||
| func (v *MachineVM) startHostNetworking() (string, machine.APIForwardingState, error) { | |||
| func (v *MachineVM) startHostNetworking(vlanSocket *define.VMFile) (string, machine.APIForwardingState, *os.Process, error) { | |||
There was a problem hiding this comment.
Why are we passing in the qmp socket into the function if we already have access to it?
There was a problem hiding this comment.
I have implementation, which supports UNIX socket VLAN from newer QEMU QAPI, which is compatible also with Windows OS supporting AF_UNIX. In this case it is impossible to use hack with reusing QMP address and need to provide dedicated VLAN socket address as we need both in the same command line arixmkii@9e3e20f#diff-4a3677ed5171641eba40e911f19bbf5970550a9e1ce75a30959e6c8032b2e39cR524
| } | ||
| return forwardSock, state, nil | ||
| return forwardSock, state, c.Process, nil |
There was a problem hiding this comment.
What was the decision behind returning the os.Process? it currently isn't being used by the caller
There was a problem hiding this comment.
I have implementation based of UNIX socket VLAN based which checks the pid of this process to understand if we have the process alive arixmkii@9e3e20f#diff-4a3677ed5171641eba40e911f19bbf5970550a9e1ce75a30959e6c8032b2e39cR542
|
I would like to make sure we get a nod from @baude before this goes through |
|
@arixmkii there is a large refactor going on rn for this code in question ... here would be my offer ... I'm working on QEMU and should have that done soonish. It is the worst because it is the first. That PR will go to a dev branch. Once that PR is reviewed and merged, lets work to bring your qemu stuff in at that point and on the dev branch where the machine refactoring is going on. then it should go in smoothly, ok? |
|
@baude sounds reasonable to me 👍 Thank you! |
Signed-off-by: Arthur Sengileyev <[email protected]>
arixmkii
force-pushed
the
cross-platform-qemu-fixes
branch
from
576db31 to
48aa60b
Compare
openshift-merge-robot
added
the
needs-rebase
|
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. |
|
Will be continued in #21594 |
stale-locking-app
bot
added
the
locked - please file new issue/PR
This is a subset of bugfixes and technical changes from #17473 as the PR in question is really struggling to get merged and will probably have some more rebase iteration. Adding this commit with technical changes will allow to reduce the PR in question to actual functional changes w/o bugfixes.
Recap of changes:
Does this PR introduce a user-facing change?
[NO NEW TESTS NEEDED]