Fix get local.instance.bridged
#3513
Conversation
luis4a0
changed the title
get local.instance.bridged
luis4a0
force-pushed
the
fix-get-bridged
branch
2 times, most recently
from
6fd3996
to
6434e8e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3513 +/- ##
==========================================
- Coverage 88.82% 88.80% -0.02%
==========================================
Files 254 254
Lines 14115 14115
==========================================
- Hits 12537 12535 -2
- Misses 1578 1580 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hey @luis4a0, thanks for this PR. I think the idea of replacing the bridge name with a bridge member check is what we need. However, I am afraid I don't follow the new logic in is_bridged.
I think the idea is just (in pseudocode):
alt_net = is_bridge(br_interface) ? nullopt : find_bridge_with(br_interface);
return std::any_of(spec.extra_interfaces.cbegin(),
spec.extra_interfaces.cend(),
[&br_interface, &alt_net](const auto& network) -> bool {
return network.id == br_interface || network.id == alt_net;
}); // this is there alreadyRegarding backends (or backend/platform combinations) where no bridge is necessary to connect the instance to the desired interface... Suppose that a user sets up a bridge (say custom-bridge) linked to the interface some-net. And suppose that they multipass launch --network custom-bridge. I don't think it is wrong to say that the resulting instance is bridged with some-net, even if the backend is VirtualBox and the bridge is unnecessary. I think it is actually more correct to say it is bridged.
IOW, the logic above should be mostly independent from backends. The exception I see is the bridge type (bridge vs switch).
As discussed, I ignored tests for now.
src/daemon/daemon.cpp
Outdated
| return true; | ||
| } | ||
|
|
||
| const auto& br_it = config->factory->find_bridge_with(host_nets, br_interface, "bridge"); |
There was a problem hiding this comment.
I don't think we can hardcode "bridge" here. It would be "switch" in Hyper-V. But let's perhaps deal with this later, after the rest is figured out.
src/daemon/daemon.cpp
Outdated
| if (std::any_of(std::cbegin(host_nets), | ||
| std::cend(host_nets), | ||
| [&br_interface](const mp::NetworkInterfaceInfo& info) { | ||
| return info.id == br_interface && info.type == "bridge"; | ||
| })) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I don't get this. I read "if we find that the host has a bridge named like the configured bridge, return true". But this is unrelated to whether or not the instance is has that network. What am I missing?
There was a problem hiding this comment.
Yeah, you're right. I removed this and modified the last one in the function.
| if (br_it == host_nets.cend()) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I don't understand this either. If the configured network is a bridge, it won't be part of any other bridge. Yet we shouldn't just return false.
There was a problem hiding this comment.
Right. I changed the checks before this one.
luis4a0
force-pushed
the
fix-get-bridged
branch
4 times, most recently
from
12fd884
to
238f240
Compare
|
Hey @townsend2010!
On one hand, moving to platform detail is needed because the code will be different for macOS QEMU. On the other hand, you are right in the sense that the particular function implementation will be needed after bridging is implemented in Linux QEMU. I can make the detail function on Linux throw, and then in the other PR introduce the code again. |
|
Hey @townsend2010, @ricab! I addressed all the concerns above, and added some tests. This is ready for review now. |
The answer was wrong in some backends. This PR uses a more reliable mechanism, inspecting the contents of the platform networks data structure.
Fixes #3489.