Bridging on Linux QEMU #3463

luis4a0 · 2024-04-05T17:20:08Z

This PR implements the bridging feature on the QEMU backend on Linux.

codecov · 2024-04-05T17:30:46Z

Codecov Report

Attention: Patch coverage is 28.12500% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 88.66%. Comparing base (2f4ac45) to head (04a9ddb).

Files	Patch %	Lines
...backends/qemu/linux/qemu_platform_detail_linux.cpp	26.92%	19 Missing ⚠️
...orm/backends/qemu/qemu_virtual_machine_factory.cpp	0.00%	4 Missing ⚠️

Additional details and impacted files

@@                 Coverage Diff                 @@
##           fix-get-bridged    #3463      +/-   ##
===================================================
- Coverage            88.80%   88.66%   -0.14%     
===================================================
  Files                  254      254              
  Lines                14115    14139      +24     
===================================================
+ Hits                 12535    12537       +2     
- Misses                1580     1602      +22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CMakeLists.txt

sharder996 · 2024-04-29T22:22:05Z

src/platform/backends/lxd/lxd_virtual_machine_factory.cpp

@@ -247,7 +238,7 @@ auto mp::LXDVirtualMachineFactory::networks() const -> std::vector<NetworkInterf
                ret.push_back(std::move(network));

        for (auto& net : ret)
-            if (net.needs_authorization && find_bridge_with(ret, net.id) != ret.cend())
+            if (net.needs_authorization && MP_BACKEND.find_bridge_with(ret, net.id) != ret.cend())
                net.needs_authorization = false;


Change this to match https://github.com/canonical/multipass/pull/3463/files#diff-77064bb2fbd2589b9a34679c3e6bde02dc7ad8b0b22931195d4d687e8aef2720R232? It makes it easier to see that the logic is the same in both places.

Right, changed in the base branch.

This still isn't changed

Oh, sorry, I didn't get it. I changed it as you propose, but I used &= instead of = to maintain exactly the same logic (i.e., the same exact truth table).

src/platform/backends/shared/linux/backend_utils.cpp

src/platform/backends/qemu/linux/qemu_platform_detail_linux.cpp

ricab · 2024-04-30T11:03:33Z

src/platform/backends/qemu/linux/qemu_platform_detail_linux.cpp

+
+std::string mp::QemuPlatformDetail::bridge_name_for(const std::string& iface_name) const
+{
+    return MP_BACKEND.bridge_name(iface_name);


I don't really understand what is going on here. This bridge_name_for thing has become pretty complex. This is what I see:

the daemon calls VirtualMachineFactory::bridge_name_for. This is implemented to:

return an empty string by default.

this covers libvirt and VirtualBox, which have no specific implementation

In LXD, it returns mp::Backend::bridge_name().

In QEMU, it delegates on QemuPlatform::bridge_name_for, which:

returns an empty string by default (covers macOS, as there is no specific implementation there)

again calls mp::Backend::bridge_name() on Linux

On Hyper-V, it incorrectly returns a hardcoded "External Virtual Switch" string (bug we've discussed)

This complexity is complemented by tests and mocks. But the only functional bit here is mp::Backend::bridge_name(), on Linux, where we create bridges. However, and this is the crucial part, IIANM this function shouldn't be used at all in the first place! The daemon should not be looking for the bridge based on the name at all, but based on its members, as we've established when we discussed the bug in Hyper-V.

Note that information on bridge members is readily available on our return from VirtualMachineFactory::networks(). We just need to run a find that is the same for all backends. And this find is implemented. It's the one you just moved here.

So you could just replace this call [https://github.com/canonical/multipass/blob/main/src/daemon/daemon.cpp#L3523] with a call to find_bridge_with. That would fix the behavior and get rid of all this code... Am I missing something??

Hey @ricab, thanks! Well, what you are proposing is exactly what I am implementing as the fix to the Hyper-V bug here. What do you think in finishing that fix, merging and then rebasing this branch on top of that? That way, the history will reflect exactly what change does what and this PR will only add bridging to QEMU on Linux.

Otherwise, I can fix everything here. The resulting code will be the same but, you know, the change here can be seen as a drive-by change.

WDYT?

Or, I can leave that here, merge this and fix later on the other PR. In any case, both PR's will go to 1.14 IIRC.

Ah, I looked at that PR but did not see this. And since this one is adding to the bridge_name stuff... But OK, yes, what you propose is perfectly fine! As long as it does the right thing and removes the obsolete code in the end.

Of course, I'll let you know when I commit to the other PR. Thanks!

Ok, this was all addressed in the other PR. I changed the base of this PR, so now it's on top of the get fix.

This is not anymore needed because networks() is implemented in both Linux and macOS.

This commit removes expectations on throwing from add_network_interface() and networks() tests.

luis4a0 · 2024-05-10T13:14:44Z

Hey @sharder996! Thanks for the review! I addressed all your comments, plus rebasing on the fix for get. Now the code should look better, so it's ready for another round of review. Thanks!

sharder996

Ok, just a few more questions. Otherwise, the code looks good, but I am unable to functionally test since my laptop running Linux died and I haven't setup a replacement yet.

src/platform/backends/qemu/linux/qemu_platform_detail_linux.cpp

src/platform/backends/qemu/qemu_platform.h

luis4a0 · 2024-05-13T13:16:33Z

Hey @sharder996! I addressed your two points. this is ready for another round of review. Thanks!

townsend2010

Hey @luis4a0,

Just a couple of copyright things inline below. :) Thanks!

townsend2010 · 2024-05-10T12:45:55Z

src/platform/backends/qemu/linux/bridge_helper.c

+ * QEMU Bridge Helper
+ *
+ * Copyright IBM, Corp. 2011
+ *


Please add a Copyright (C) Canonical, Ltd. here.

Right, added.

townsend2010 · 2024-05-10T12:46:28Z

src/platform/backends/qemu/linux/bridge_helper.c

+ * Authors:
+ * Anthony Liguori   <aliguori@us.ibm.com>
+ * Richa Marwaha     <rmarwah@linux.vnet.ibm.com>
+ * Corey Bryant      <coreyb@linux.vnet.ibm.com>


Feel free to add your name here too, but up to you.

Well, I didn't make functional changes. But I added it in order to blame someone if something goes wrong ;)

It is a stripped-down version of the QEMU bridge helper.

This was made to match the logic with the Linux QEMU factory.

luis4a0 · 2024-05-14T08:06:13Z

@sharder996 I addressed the point I didn't address in the last round, so this is ready again for review. Thanks!

sharder996 · 2024-05-22T22:00:02Z

@luis4a0 Got my PC up and dual-booting with Ubuntu so I did some more functional testing. There are a couple of things that I noticed that seem a bit odd:

If I take a snapshot of an instance before and after setting local.foo.bridged=true, then restore the first snapshot (before adding the bridge) and then restore the second snapshot (after adding the bridge), when I start the instance the bridge does not get initialized. Only after restarting the instance does the bridge come up.
Also, since I recently moved I am on a different network and had to create a new bridge for that network. As such, I have an extra bridge in multipass networks which was for the old network and does nothing. Deleting the bridge with ip link will make it go away, but when I restart the Multipass daemon it comes back.

luis4a0 force-pushed the qemu-bridging branch 3 times, most recently from 4c55b42 to da6e35c Compare April 12, 2024 14:09

luis4a0 force-pushed the qemu-bridging branch 3 times, most recently from 0cb2e99 to 15de003 Compare April 19, 2024 12:59

luis4a0 force-pushed the qemu-bridging branch 3 times, most recently from 837f43c to 7c3919f Compare April 24, 2024 11:42

luis4a0 mentioned this pull request Apr 25, 2024

Linux QEMU bridging unit tests #3510

Open

sharder996 requested changes Apr 29, 2024

View reviewed changes

src/platform/backends/qemu/linux/qemu_platform_detail_linux.cpp Outdated Show resolved Hide resolved

src/platform/backends/qemu/linux/qemu_platform_detail_linux.cpp Outdated Show resolved Hide resolved

ricab requested changes Apr 30, 2024

View reviewed changes

townsend2010 mentioned this pull request May 9, 2024

Fix get local.instance.bridged #3513

Open

luis4a0 added 5 commits May 9, 2024 15:00

[qemu][linux] Implement networks().

6409132

[qemu][linux] Add command-line for extra networks.

df1024e

[qemu][linux] Add add_network_interface().

6decae3

[qemu] Remove throwing networks().

845e014

This is not anymore needed because networks() is implemented in both Linux and macOS.

[tests][qemu] Adapt tests to last changes.

f1819d9

This commit removes expectations on throwing from add_network_interface() and networks() tests.

luis4a0 force-pushed the qemu-bridging branch from 65e03ff to 29d97e1 Compare May 10, 2024 13:02

luis4a0 changed the base branch from main to fix-get-bridged May 10, 2024 13:03

luis4a0 force-pushed the qemu-bridging branch from 29d97e1 to 2b40732 Compare May 10, 2024 13:13

luis4a0 force-pushed the qemu-bridging branch from 2b40732 to a783fd5 Compare May 10, 2024 14:18

sharder996 requested changes May 10, 2024

View reviewed changes

src/platform/backends/qemu/linux/qemu_platform_detail_linux.cpp Show resolved Hide resolved

src/platform/backends/qemu/qemu_platform.h Outdated Show resolved Hide resolved

townsend2010 requested changes May 14, 2024

View reviewed changes

luis4a0 added 2 commits May 14, 2024 04:52

[qemu][linux] Add our own bridge helper.

d839347

It is a stripped-down version of the QEMU bridge helper.

[cmake] Build bridge helper on Linux.

59d07d7

luis4a0 added 10 commits May 14, 2024 04:52

[qemu][linux] Add bridge helper to command line.

de544bb

[qemu][linux] Create bridge when needed.

a2c4035

[apparmor] Let bridge helper run.

3f8dfd0

[qemu][linux] Ask permission to bridge.

45d7a21

[qemu][linux] Ask for bridging only when needed.

2dafed9

[qemu] Move Linux bits to QemuPlatformDetail.

4c3fc23

[qemu][linux] Implement find_bridge_with.

7b91f54

[tests][qemu][linux] Check find_bridge_with.

a6ad86d

[qemu] Make create_bridge_with pure virtual.

0ed07ae

[lxd][factory] Reword needs_bridging computation.

04a9ddb

This was made to match the logic with the Linux QEMU factory.

luis4a0 force-pushed the qemu-bridging branch from 10717b2 to 04a9ddb Compare May 14, 2024 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bridging on Linux QEMU #3463

Bridging on Linux QEMU #3463

luis4a0 commented Apr 5, 2024

codecov bot commented Apr 5, 2024 •

edited

sharder996 Apr 29, 2024

luis4a0 May 10, 2024

sharder996 May 10, 2024

luis4a0 May 14, 2024

ricab Apr 30, 2024

luis4a0 Apr 30, 2024

luis4a0 Apr 30, 2024

ricab Apr 30, 2024

luis4a0 Apr 30, 2024

luis4a0 May 10, 2024

luis4a0 commented May 10, 2024

sharder996 left a comment

luis4a0 commented May 13, 2024

townsend2010 left a comment

townsend2010 May 10, 2024

luis4a0 May 14, 2024

townsend2010 May 10, 2024

luis4a0 May 14, 2024

luis4a0 commented May 14, 2024

sharder996 commented May 22, 2024

Bridging on Linux QEMU #3463

Are you sure you want to change the base?

Bridging on Linux QEMU #3463

Conversation

luis4a0 commented Apr 5, 2024

codecov bot commented Apr 5, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luis4a0 commented May 10, 2024

sharder996 left a comment

Choose a reason for hiding this comment

luis4a0 commented May 13, 2024

townsend2010 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luis4a0 commented May 14, 2024

sharder996 commented May 22, 2024

codecov bot commented Apr 5, 2024 •

edited