canonical / multipass

Multipass orchestrates virtual Ubuntu instances
https://multipass.run
GNU General Public License v3.0
7.51k stars 632 forks source link

Fix detection of whether an instance is bridged, take2 #3545

Closed ricab closed 1 week ago

ricab commented 4 weeks ago

An instance is considered bridged, at any point in time, iff the configured preferred network at that time is:

This PR implements that predicate in mp::Daemon::is_bridged (and is_bridged_impl), removing any previous logic relying on bridge names to relate bridges with other interfaces, and instead using readily available NetworkInterfaceInfo::links. To achieve this, the predicate calls find_bridge_with, which was refactored into a backend-agnostic utility.

Because Windows uses "switches" instead of "bridges", a platform function is added to obtain the correct nomenclature, replacing hard-coded uses in backend factories. Since the factories no longer need to call a base prepare_networking_guts with the correct nomenclature, that method is removed and its implementation moved to BaseVirtualMachine::prepare_networking, which is overridden to do nothing in VirtualBox.

The separation between mp::Daemon::is_bridged (and is_bridged_impl) is meant to avoid inspecting host networks or the preferred bridge setting twice when adding a bridge. To that end, the responsibility of checking whether and instance is already bridged, when attempting to bridge it, is split from the InstanceSettingsHandler to the Daemon, depending on the use case.

This scheme of having different entities checking if the instance is bridged is a compromise that avoids passing yet more lambdas to the settings handler, which would otherwise be required to obtain networks or settings only once from that level. A better implementation would instead extract all required functionality (all "lambdas") into a separate class that could be passed to the settings handler instead of disconnected callables. But that would require more time.

Finally, existing tests were adapted, removed where obsolete, and a couple of tests were added to cover everything. Codecov seems to be unhappy that I've removed previously covered, but wrong/unnecessary, code.

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 92.98246% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.82%. Comparing base (5fecf79) to head (759eabf). Report is 26 commits behind head on main.

Files Patch % Lines
...backends/qemu/linux/qemu_platform_detail_linux.cpp 33.33% 2 Missing :warning:
...orm/backends/qemu/qemu_virtual_machine_factory.cpp 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3545 +/- ## ======================================= Coverage 88.81% 88.82% ======================================= Files 253 253 Lines 14121 14165 +44 ======================================= + Hits 12542 12582 +40 - Misses 1579 1583 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ricab commented 2 weeks ago

Bahh freakin CI...

georgeliao commented 2 weeks ago

@ricab , Thanks for the good work. It looks good in general, just a few minor comments to address.

ricab commented 2 weeks ago

Hey @georgeliao, thanks for the review. I have answered your comments and changed a few things. Let me know what you think!

ricab commented 1 week ago

Hey @georgeliao, I have replied back in here. Thanks again.

ricab commented 3 days ago

Fixed #3489