Add network bridges #3195

luis4a0 · 2023-08-17T17:07:12Z

No description provided.

codecov · 2023-08-17T17:43:41Z

Codecov Report

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

Comparison is base (4b38e63) 83.92% compared to head (89f1b78) 84.05%.

Files	Patch %	Lines
src/daemon/daemon.cpp	94.25%	5 Missing ⚠️
src/client/cli/cmd/launch.cpp	0.00%	4 Missing ⚠️
src/client/cli/cmd/delete.cpp	0.00%	3 Missing ⚠️
src/daemon/instance_settings_handler.cpp	89.65%	3 Missing ⚠️
...rc/platform/backends/qemu/qemu_virtual_machine.cpp	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3195      +/-   ##
==========================================
+ Coverage   83.92%   84.05%   +0.13%     
==========================================
  Files         251      251              
  Lines       13678    13793     +115     
==========================================
+ Hits        11479    11594     +115     
  Misses       2199     2199

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

ricab

Hey @luis4a0, thank you for your work on this. This is a light review only and I did not try the feature myself. I have a few requests that shouldn't be very difficult to implement.

include/multipass/cli/prompters.h

src/client/cli/cmd/launch.cpp

src/client/cli/cmd/start.cpp

include/multipass/cli/prompters.h

src/utils/utils.cpp

src/daemon/daemon.cpp

ricab · 2023-11-09T09:26:22Z

src/daemon/daemon.cpp

+        }
+        catch (const mp::NotImplementedOnThisBackendException&)
+        {
+            run_at_boot.erase(name);


Shouldn't this rethrow? And actually, couldn't this be structured such that we only add run_at_booy[name] after (if) everything else succeeds?

Rethrowing would mean to abort the execution of the start command. The problem with adding commands to the vector if everything succeeds would be possible, but the daemon would have to carry a lot of information about interfaces. Rolling back this way is much simpler.

Oh I didn't realize this was called at start. We should instead detect that bridging isn't implemented when the user tries to set bridged=true and refuse it at that point. What happens now in that case?

For adding only after success, I was probably not very clear. What I mean is that this should compose the commands var but only add them to run_at_boot at the end, when we know everything succeeded. That would avoid having to catch and erase, or leaving lingering commands behind if other exceptions are thrown.

luis4a0 · 2023-11-17T17:46:46Z

Hey @ricab and @georgeliao! I addressed your comments, thanks for taking the time to review! This is ready for another review pass. Thanks!

ricab

Hey @luis4a0, there are a couple of things that I think still need to be addressed (one tiny the other more important).

ricab · 2023-11-17T19:13:25Z

src/daemon/daemon.cpp

+        }
+        catch (const mp::NotImplementedOnThisBackendException&)
+        {
+            run_at_boot.erase(name);


Oh I didn't realize this was called at start. We should instead detect that bridging isn't implemented when the user tries to set bridged=true and refuse it at that point. What happens now in that case?

For adding only after success, I was probably not very clear. What I mean is that this should compose the commands var but only add them to run_at_boot at the end, when we know everything succeeded. That would avoid having to catch and erase, or leaving lingering commands behind if other exceptions are thrown.

luis4a0 · 2023-11-23T12:42:45Z

Hey @ricab! I implemented, as you proposed, refusing adding the bridged interface just in the moment the user issues the set command. So this PR might be ready for another round of review. Thanks!

luis4a0 · 2023-11-27T18:07:10Z

Hey @georgeliao @ricab! Done the changes. I also added some tweaks for this to look like what I imagined :) And I finally rebased upong the latest main. So this is ready for reviewing.

Note: the private side will not yet compile, I still need to do the rebase on the private branch, but I'll do it after this gets merged.

Thanks!

If the execution of the commands to configure the new interfaces through SSH fails, an annoying message is shown. This commit makes that message appear only when the execution failed; as opposed to show it in case of any failure.

Message only on SSH execution error

…aces Add bridged interfaces in macOS QEMU

This assures persistence in the data structure. This means that commands to be run at boot will be executed even if the daemon is restarted.

Fixes issue 2 of #3195 (review)

ricab

Alright, LGTM now (modulo issues we agreed to deal with separately).

Thank you for all your work @luis4a0!

luis4a0 force-pushed the add-network-bridges branch from 4a943ee to eb421ee Compare September 6, 2023 12:10

luis4a0 force-pushed the add-network-bridges branch 2 times, most recently from c722db6 to 3c0ab54 Compare September 26, 2023 14:59

luis4a0 force-pushed the add-network-bridges branch from 6f3b12d to 9d23fef Compare October 6, 2023 14:47

georgeliao force-pushed the add-network-bridges branch from 008aa0f to da8fc75 Compare October 9, 2023 11:01

luis4a0 force-pushed the add-network-bridges branch 2 times, most recently from 50cfcfc to b47e35d Compare October 18, 2023 12:37

townsend2010 mentioned this pull request Oct 24, 2023

Snapshots #2993

Merged

luis4a0 force-pushed the add-network-bridges branch from 75cb878 to e9f1c2d Compare October 28, 2023 21:58

luis4a0 marked this pull request as ready for review October 31, 2023 13:24

georgeliao previously approved these changes Oct 31, 2023

View reviewed changes

townsend2010 added this to the 1.13.0 milestone Nov 6, 2023

luis4a0 dismissed georgeliao’s stale review via 44dcf21 November 7, 2023 10:24

luis4a0 force-pushed the add-network-bridges branch 3 times, most recently from 4e4fdfe to 74f81e2 Compare November 7, 2023 12:45

townsend2010 assigned luis4a0 Nov 8, 2023

luis4a0 force-pushed the add-network-bridges branch from 74f81e2 to 15875b5 Compare November 8, 2023 13:32

ricab requested changes Nov 9, 2023

View reviewed changes

townsend2010 linked an issue Nov 9, 2023 that may be closed by this pull request

Add ability to add --networks to existing instances #2476

Closed

luis4a0 force-pushed the add-network-bridges branch 2 times, most recently from d5411be to 0f495b4 Compare November 10, 2023 09:38

ricab requested changes Nov 17, 2023

View reviewed changes

ricab self-requested a review November 21, 2023 16:50

luis4a0 force-pushed the add-network-bridges branch from 2ffe7ca to cebe251 Compare November 27, 2023 18:04

townsend2010 linked an issue Nov 28, 2023 that may be closed by this pull request

Commands to add interfaces are gone after daemon restart #3293

Closed

luis4a0 added 23 commits January 4, 2024 15:14

[utils] Use SSHExecFailure exception.

3937738

[daemon] Warn about bridge failure only if needed.

1f961d8

If the execution of the commands to configure the new interfaces through SSH fails, an annoying message is shown. This commit makes that message appear only when the execution failed; as opposed to show it in case of any failure.

[qemu][linux] Throw when bridging.

9c95d24

[cli] Show warning only once.

9c2df22

[tests] Adapt QEMU platform mock to new interface.

ad4a958

[daemon] Improve error reporting.

0065cee

[daemon] Run commands at boot only if needed.

9f9287a

[tests] Check throwing when opening a SSH session.

7eeaaa8

[tests][qemu][linux] Adding interface must throw.

507a4cc

Merge pull request #3276 from canonical/message-only-on-ssh-exec-error

74fcbf7

Message only on SSH execution error

Merge pull request #3266 from canonical/qemu-macos-add-bridged-interf…

b613e4c

…aces Add bridged interfaces in macOS QEMU

Address reviews.

035499b

[factory] Add can_add_bridges().

c3a6d48

[lxd] Add can_add_bridges().

9d13219

[daemon] Refuse adding bridge when calling set.

6e3bfbf

[daemon] Move run_at_boot to VMSpec.

4d8540a

This assures persistence in the data structure. This means that commands to be run at boot will be executed even if the daemon is restarted.

[daemon] Configure bridge as early as possible.

ae49b97

[daemon] Properly clear run_at_boot after config.

5f253e3

Address reviews #2.

1312697

[tests][factory] Remove can_add_bridges().

2d7427c

[cli] Use mp::client:: namespace for *_answer.

f4eeb9f

[daemon] Make vm_specs a reference.

f36824a

[daemon] Filter out empty MAC addresses.

89f1b78

Fixes issue 2 of #3195 (review)

luis4a0 force-pushed the add-network-bridges branch from 5b0665d to 89f1b78 Compare January 4, 2024 18:24

luis4a0 mentioned this pull request Jan 5, 2024

Fix get local.<instance>.bridged command #3357

Merged

ricab approved these changes Jan 5, 2024

View reviewed changes

ricab merged commit d84f8f7 into main Jan 5, 2024
11 of 12 checks passed

ricab deleted the add-network-bridges branch January 5, 2024 17:25

luis4a0 mentioned this pull request Jan 12, 2024

Filter new extra interfaces before instance start #3371

Merged

luis4a0 mentioned this pull request Feb 1, 2024

Fix bridging authorization #3395

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add network bridges #3195

Add network bridges #3195

luis4a0 commented Aug 17, 2023

codecov bot commented Aug 17, 2023 •

edited

Loading

ricab left a comment

ricab Nov 9, 2023

luis4a0 Nov 17, 2023

ricab Nov 17, 2023

luis4a0 commented Nov 17, 2023

ricab left a comment •

edited

Loading

ricab Nov 17, 2023

luis4a0 commented Nov 23, 2023

luis4a0 commented Nov 27, 2023

ricab left a comment

Add network bridges #3195

Add network bridges #3195

Conversation

luis4a0 commented Aug 17, 2023

codecov bot commented Aug 17, 2023 • edited Loading

Codecov Report

ricab left a comment

Choose a reason for hiding this comment

ricab Nov 9, 2023

Choose a reason for hiding this comment

luis4a0 Nov 17, 2023

Choose a reason for hiding this comment

ricab Nov 17, 2023

Choose a reason for hiding this comment

luis4a0 commented Nov 17, 2023

ricab left a comment • edited Loading

Choose a reason for hiding this comment

ricab Nov 17, 2023

Choose a reason for hiding this comment

luis4a0 commented Nov 23, 2023

luis4a0 commented Nov 27, 2023

ricab left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 17, 2023 •

edited

Loading

ricab left a comment •

edited

Loading