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

Change QEMU netdev to Unix domain socket #21594

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

arixmkii
Copy link
Contributor

@arixmkii arixmkii commented Feb 9, 2024

Fixes #21544

This change migrates to new QEMU stream netdev added in 7.2.0. It also unifies how gvproxy is used in QEMU and AppleHV.

This change will require manual fixes to QEMU machine configurations migrated from 4.x, but 5.0 version is the only good point to do so. My previous work was with preserving old FD code, but it makes the codebase really messy.

It replaces my previous work from:
#21252
#17473
#18488

Does this PR introduce a user-facing change?

QEMU machine uses Unix domain socket netdev device type

[NO NEW TESTS NEEDED]

@arixmkii
Copy link
Contributor Author

arixmkii commented Feb 9, 2024

I'm open to split this PR into separate commits like:

  • bugfixes
  • migration to Unix domain socket netdev
  • add QEMU on Windows support

@arixmkii
Copy link
Contributor Author

arixmkii commented Feb 9, 2024

This passed full e2e suite on a Windows machine.

@arixmkii
Copy link
Contributor Author

arixmkii commented Feb 9, 2024

@baude @n1hility @jakecorrenti Could you take a look as you were doing reviews of my previous PRs on the same topic.

@arixmkii
Copy link
Contributor Author

Removed Windows machine provider from this commit - reduced the scope of changes to refactoring only.

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@arixmkii
Copy link
Contributor Author

@baude do you see this getting into release or this change will not make it anyway?

@jakecorrenti
Copy link
Member

code LGTM but I want a @baude head nod

@arixmkii
Copy link
Contributor Author

Rebased to latest main to use extracted wait for the socket function. This changeset should be now as compact as possible.

@arixmkii
Copy link
Contributor Author

If this is being accepted, will also allow to close #18314 as "won't implement", as, so far, there were no newer features identified, which are breaking for the Podman machine.

@arixmkii
Copy link
Contributor Author

@baude did you by any chance have time to take a look at this?

@baude
Copy link
Member

baude commented Feb 26, 2024

@baude did you by any chance have time to take a look at this?

looking now!

@baude
Copy link
Member

baude commented Feb 26, 2024

my one concern is that business with the fd being passed as an extrafile was crucial at one time. does this approach actually mitigate the need or ignore it ?

@arixmkii
Copy link
Contributor Author

It was the only option before QEMU 7.2.0 (QEMU 7.2.0 is now the oldest LTS still receiving fixes in upstream QEMU).

This is one of the recaps of the changes why it had become possible to use AF_UNIX directly https://john-millikin.com/improved-unix-socket-networking-in-qemu-7.2

I can dig the PATCH series from QEMU mailing list as well. IIRC the main reason it was not exposed as netdev before, because of the incomplete parser for parameters, not that it was technically an issue to use AF_UNIX netdev directly.

@arixmkii
Copy link
Contributor Author

Here is the PATCH with details from the author https://lists.gnu.org/archive/html/qemu-devel/2022-10/msg03516.html I'm not sure if it is the latest revision, but the main idea was not changed through revision versions, only fixes and adjustments were applied. It is mostly old socket netdev combined with new QAPI for parameter input.

@arixmkii
Copy link
Contributor Author

arixmkii commented Feb 26, 2024

QEMU 7.2.0 has been released in December, 2022 (https://www.qemu.org/2022/12/14/qemu-7-2-0/). My assumption was that this is old enough to become the baseline for the next generation Podman Machine (where compatibility breakages already happening - like removing QEMU support from official MacOS version, where all these machines will be eliminated and the storage format changes in general).

If this is still a no go I would probably rework this into moving this change into Windows specific part and keeping Unix part with FDs. This is the part which makes current QEMU machine code not cross platform, but there are multiple ways mitigating this.

@arixmkii
Copy link
Contributor Author

To provide a summary:

Pros:

  • less magical, podman binary doesn't interact with gvproxy and QEMU processes only via API (command line, pidfiles and process API, QMP protocol, ready socket - no magical FD allocated is transferred between processes)
  • the code is as close as it could be to applehv
  • code will become really cross platform

Cons:

  • breaking for old machines (they could be fixed via editing command line in config)
  • requires QEMU 7.2.0+ (7.2.0 is the only old LTS in upstream, so, unless distributions provide their own fixes it should be the minimal QEMU version in a wild one would reasonably care about)

@jakecorrenti @baude I listed these pros and cons and provided technical details in my previous comment, now, I will leave it for your team to decide if this is a good fit for Podman now or not. Thank you a lot for your time and reviews!

@Luap99
Copy link
Member

Luap99 commented Feb 26, 2024

breaking for old machines (they could be fixed via editing command line in config)

Podman 5.0 breaks the config file anyway so this shouldn't be an issue. Only we if we decide to implement some migration we need to keep this in mind but I don't think we will.

This change migrates to new QEMU stream netdev added in 7.2.0.
It also unifies how gvproxy is used in QEMU and AppleHV.

Signed-off-by: Arthur Sengileyev <[email protected]>
@baude
Copy link
Member

baude commented Feb 26, 2024

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2024
Copy link
Contributor

openshift-ci bot commented Feb 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arixmkii, baude

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2024
@baude
Copy link
Member

baude commented Feb 26, 2024

@arixmkii is this rebased to the latest (as of today) main branch ?

@arixmkii
Copy link
Contributor Author

@baude yes, it is only 2 commits behind main.

@baude
Copy link
Member

baude commented Feb 26, 2024

ill babysit and assume this is a flake.

@openshift-merge-bot openshift-merge-bot bot merged commit 04f7032 into containers:main Feb 26, 2024
93 checks passed
@arixmkii arixmkii deleted the qemu-win-machine-5 branch March 1, 2024 13:58
@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 31, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.

Require QEMU 7.2.0+ in machine 5 and switch to Unix domain socket networking
4 participants