Filter new extra interfaces before instance start

[luis4a0]

Adding interfaces after the instance was created introduced the possibility of some kinds of clashes when the instance starts. To fix this, we check first if the backend is able to add each one of the network interfaces requested by the user. The daemon then filters out the failing interfaces, letting only the interfaces which work be added to the instance.

This PR fixes issue 4 of this review.

[codecov]

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (5172bd3) 84.18% compared to head (2701104) 84.16%.

Files	Patch %	Lines
src/daemon/daemon.cpp	78.00%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3371      +/-   ##
==========================================
- Coverage   84.18%   84.16%   -0.03%     
==========================================
  Files         251      251              
  Lines       13816    13848      +32     
==========================================
+ Hits        11631    11655      +24     
- Misses       2185     2193       +8     

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

[townsend2010]

Kicking this to have it build with the proper private branch.

[townsend2010]

Hey @luis4a0!

I've tested this and I think we're getting closer. One thing though is the warning output is confusing, especially to end users. First, it looks like an error although it's not. Second, the message is not clear what the problem is.

If I look at the available networks as instructed in the docs, I get the following:

$ multipass networks
Name     Type     Description
lxdbr0   bridge   Network bridge
mpbr0    bridge   Network bridge for Multipass
virbr0   bridge   Network bridge

That to me would mean mpbr0 would be a candidate for setting as the bridged network. However, with this fix and following Ricardo's issue #4 instructions, I of course get the following:

$ multipass start test-net
Cannot bridge mpbr0 for instance test-net

There isn't any explanation as to why this is problematic. I think saying something to the effect of:
Warning: Cannot bridge mpbr0 for instance test-net: Bridge mpbr0 already used by internal network
or similar would be a bit more helpful.

As an aside and not related to this PR, I think showing mpbr0 in multipass networks is a bug.

[luis4a0]

Hey @townsend2010!

It's true that the message you propose looks better. But, for this, we should distinguish mpbr0 from the rest of the bridges which cannot be used. Moreover, the LXD bridge can be used only once per instance. And the other bridges, like virbr0, can be used any number of times. So I think with one message like `Warning: cannot bridge mpbr0, it is already being used for instance test-net." will suffice, without distinguishing between LXD and Multipass bridges (honestly, I don't know what do they have which makes them impossible to use twice). What do you think?

And finally, I agree that the Multipass bridge should be filtered out from networks. Same maybe for the LXD bridge, with the exception that it can be used once.

[townsend2010]

Hey @luis4a0!

I've made some comments and suggestions below.

Also, regarding the warning message, I see what you mean. I think we should even be more general here.

So maybe something like this:

Warning: Cannot bridge mpbr0 for instance test-net. Please see the logs for more details.

This way, we don't have to try to capture any backend specific error strings nor make generalizations that may not apply to other backends.

[luis4a0]

Hey @townsend2010, thanks for the review! I addressed all your comments and implemented your suggestions.

[townsend2010]

Hey @luis4a0! This is much better, thank you!

Just a note that isn't related to this PR, but the log message that gets put in the journal when the interface isn't added is not complete/corrupted. This is a snippet of what I see:

Jan 29 09:12:24 Razor-Crest systemd[5145]: Started snap.multipass.multipass-27753e77-b2fd-42e3-8275-436f01f59392.scope.
Jan 29 09:12:24 Razor-Crest multipassd[120858]: Using the 'default' storage pool.
 - Failed add validation for device "eth1": Instance DNS name "test1" conflict between "eth1" and "eth0" because both are connected to same networkroject=multipass: Conflict
Jan 29 09:12:24 Razor-Crest dnsmasq[4063]: read /etc/hosts - 7 addresses

As you can see, the warning is incomplete and some of the text is partially overwritten. This will need to be fixed since we are directing users to consult the log when they see the warning.

[luis4a0]

Hey @townsend2010, thanks for reviewing again and approving!

Indeed, the message is incorrectly logged. I suspect a newline is lacking somewhere. I'll investigate it and create a new PR.