Add `local.instance.bridged` setting. #3193

luis4a0 · 2023-08-16T18:25:21Z

This PR implements the set interface to add a bridged network to an existing instance.

codecov · 2023-08-16T18:56:39Z

Codecov Report

Merging #3193 (1a28788) into add-network-bridges (57c464f) will decrease coverage by 0.04%.
The diff coverage is 77.77%.

@@                   Coverage Diff                   @@
##           add-network-bridges    #3193      +/-   ##
=======================================================
- Coverage                88.56%   88.53%   -0.04%     
=======================================================
  Files                      239      239              
  Lines                    12123    12143      +20     
=======================================================
+ Hits                     10737    10751      +14     
- Misses                    1386     1392       +6

Files Changed	Coverage Δ
src/daemon/daemon.cpp	`73.31% <45.45%> (-0.26%)`	⬇️
src/daemon/instance_settings_handler.cpp	`100.00% <100.00%> (ø)`

ricab

Hey @luis4a0, I was curious and I just had a quick look. The code looks good, I like that you're splitting things into small self contained units 👍 I spotted only a couple of wrinkles in this very superficial pass.

ricab · 2023-09-12T17:13:34Z

src/daemon/instance_settings_handler.cpp

@@ -153,6 +154,48 @@ void update_disk(const QString& key, const QString& val, mp::VirtualMachine& ins
    }
 }

+bool to_bool(const QString& key, const QString& val)


This is implemented in BoolSettingSpec. Instead of copy+pasting from there, why don't you instantiate a helper object and use that?

Hey @ricab, thanks for your quick review!

I thought in avoid copy+pasting, but I opted for not a QString and comparing against it. Just true or false. Do you still think it's OK to reuse the existing implementation?

I see what you mean: you want to return a bool and compare against that in the code, right? But you can still achieve that with an extra step. For example, just something like this to replace the contents of to_bool: return BoolSettingSpec{key, "false"}.interpret(val) == "true";

I wanted to avoid that string comparison, and throw directly instead of returning a value before throwing. But well, it is true that the code looks much better the way you propose. Since time is not critical here, I changed it. Thanks!

ricab · 2023-09-12T17:15:03Z

src/daemon/instance_settings_handler.cpp

-        auto size = get_memory_size(key, val);
-        if (property == mem_suffix)
-            update_mem(key, val, instance, spec, size);
+        if (property == bridged_suffix)


else if? To avoid one extra nesting level.

Ok, changed. I was hesitating whether to mix an inline else if with a structured else { stmt; if ... }. But right, we save four columns the way you mention.

It's not for saving spaces, it's that it makes it easier to understand that they are alternatives at the same level, with no further shared logic (unlike the following 2 properties, which do share the conversion of the memory size).

georgeliao · 2023-09-20T14:10:28Z

tests/test_instance_settings_handler.cpp

-    inline static constexpr auto properties = std::array{"cpus", "disk", "memory"};
+    inline static constexpr auto numeric_properties = std::array{"cpus", "disk", "memory"};
+    inline static constexpr auto boolean_properties = std::array{"bridged"};
+    inline static constexpr auto properties = std::array{"cpus", "disk", "memory", "bridged"};
 };


I know it stems from the original code, but maybe change the copy initialization to direct initialization to make the code cleaner.

inline static constexpr std::array numeric_properties{"cpus", "disk", "memory"}; inline static constexpr std::array boolean_properties{"bridged"}; inline static constexpr std::array properties{"cpus", "disk", "memory", "bridged"};

Right, looks better that way. Changed.

georgeliao · 2023-09-20T14:14:15Z

src/daemon/instance_settings_handler.cpp

+            return true;
+    }
+
+    return false;


std algorithm based code can be another alternative, the code is shorter, the readability probably is the same

return std::any_of(spec.extra_interfaces.cbegin(), spec.extra_interfaces.cend(), [](const auto& network) -> bool { return network.auto_mode; });

Looks nice, thanks! Changed.

georgeliao · 2023-09-20T14:30:46Z

src/daemon/instance_settings_handler.cpp

+    auto bridged = mp::BoolSettingSpec{key, "false"}.interpret(val) == "true";
+
+    if (!bridged && is_bridged(spec))
+    {


Is it possible to elaborate the roles of bridged boolean (it looks like is checking user's input) and is_bridged(spec) (looks like checking if the spec is bridged)?

Oh, sorry if it wasn't clear. The bridged boolean is the value given by the user. The is_bridged() function returns a boolean stating if the VM has a bridged interface.

I see, thanks.

georgeliao · 2023-09-20T15:14:45Z

tests/test_instance_settings_handler.cpp

+    // An extra interface in auto mode will result in bridging.
+    specs[target_instance_name].extra_interfaces = {{"id", "52:54:00:12:34:56", bridged}};
+
+    auto got = make_handler().get(make_key(target_instance_name, "bridged"));


const auto got is better

Right, better. Changed.

georgeliao · 2023-09-20T16:10:39Z

HI @luis4a0 , it looks good overall. I just left a handful minor comments.

luis4a0 · 2023-09-20T17:29:54Z

Hey @georgeliao, thanks for the review! I addressed your concerns, let me know if everything looks better now.

ricab · 2023-09-20T22:38:14Z

src/daemon/instance_settings_handler.cpp

+bool is_bridged(const mp::VMSpecs& spec)
+{
+    return std::any_of(spec.extra_interfaces.cbegin(), spec.extra_interfaces.cend(),
+                       [](const auto& network) -> bool { return network.auto_mode; });


Hey @luis4a0, I think for our purposes we'd better consider the instance bridged if and only if it has an extra_interface corresponding to the network that is currently set for local.bridged-network. That would make it symmetric to launch --bridged and allow adding multiple interfaces.

Hey @ricab, thanks for your comment. You're right, I didn't think on that. I changed the code to reflect this behavior.

Great, thanks!

ricab · 2023-09-22T11:38:25Z

src/daemon/daemon.cpp

+
+    if (bridged_id == "")
+    {
+        throw std::runtime_error(fmt::format("You have to `multipass set {}=<name>` to use the `--bridged` shortcut.",


--bridged is not applicable in the case of settings, so the message needs to be generalized. Perhaps just:

Suggested change

throw std::runtime_error(fmt::format("You have to `multipass set {}=<name>` to use the `--bridged` shortcut.",

throw std::runtime_error(fmt::format("You have to `multipass set {}=<name>` to use the "bridged" shortcut.",

Right, changed.

ricab · 2023-09-22T11:41:19Z

src/daemon/daemon.cpp

-                                                 preparing_instances, [this] { persist_instances(); })}
+      instance_mod_handler{register_instance_mod(
+          vm_instance_specs, operative_instances, deleted_instances, preparing_instances,
+          [this] { persist_instances(); }, [] { return get_bridged_interface_name(); })}


Do you really need the lambda here? Unlike to persist instances, there is no closure for the bridge, so you should be able to pass the function directly.

Yep, nice catch. [this] { persist_instances(); }, get_bridged_interface_name)} will do it.

Right, changed.

It must return true if and only if there exists in the instance an interface bridged to the current interface set in local.bridged-interface.

luis4a0 · 2023-09-22T14:25:55Z

Hey @ricab @georgeliao, thanks for the review! Let me note that all the uncovered code corresponds to the get_bridged_interface() function in daemon.cpp. It will be tested in the next PR, for doing it here would add lots of unnecessary complexity.

Add `local.instance.bridged` setting.

luis4a0 marked this pull request as draft August 16, 2023 18:30

luis4a0 force-pushed the add-bridged-setting branch from f1d86ea to 712df61 Compare August 18, 2023 12:50

luis4a0 marked this pull request as ready for review August 18, 2023 12:55

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

luis4a0 force-pushed the add-bridged-setting branch 2 times, most recently from 48ac4cd to 42bf8ec Compare September 12, 2023 16:42

ricab requested changes Sep 12, 2023

View reviewed changes

luis4a0 force-pushed the add-bridged-setting branch from 42bf8ec to af6c565 Compare September 13, 2023 11:58

luis4a0 force-pushed the add-network-bridges branch from eb421ee to 57c464f Compare September 20, 2023 13:01

luis4a0 force-pushed the add-bridged-setting branch from af6c565 to b1c8d6b Compare September 20, 2023 13:01

georgeliao reviewed Sep 20, 2023

View reviewed changes

luis4a0 added 2 commits September 20, 2023 14:05

[daemon] Add local.instance.bridged setting.

17d6139

[tests] Check set local.instance.bridged.

ac54295

luis4a0 force-pushed the add-bridged-setting branch from b1c8d6b to ac54295 Compare September 20, 2023 17:25

ricab reviewed Sep 20, 2023

View reviewed changes

ricab reviewed Sep 22, 2023

View reviewed changes

luis4a0 added 2 commits September 22, 2023 10:09

[settings] Fix get local.<instance>.bridged.

24a4062

It must return true if and only if there exists in the instance an interface bridged to the current interface set in local.bridged-interface.

[tests] Check get local.<instance>.bridged.

1a28788

luis4a0 force-pushed the add-bridged-setting branch from 957653f to 1a28788 Compare September 22, 2023 13:09

georgeliao approved these changes Sep 25, 2023

View reviewed changes

luis4a0 merged commit 5c86860 into add-network-bridges Sep 25, 2023
13 of 15 checks passed

bors bot deleted the add-bridged-setting branch September 25, 2023 17:40

luis4a0 added a commit that referenced this pull request Sep 26, 2023

Merge pull request #3193 from canonical/add-bridged-setting

0a2e2d7

Add `local.instance.bridged` setting.

luis4a0 added a commit that referenced this pull request Oct 6, 2023

Merge pull request #3193 from canonical/add-bridged-setting

db04f4d

Add `local.instance.bridged` setting.

luis4a0 added a commit that referenced this pull request Oct 16, 2023

Merge pull request #3193 from canonical/add-bridged-setting

63e0881

Add `local.instance.bridged` setting.

luis4a0 added a commit that referenced this pull request Oct 18, 2023

Merge pull request #3193 from canonical/add-bridged-setting

15edf58

Add `local.instance.bridged` setting.

luis4a0 added a commit that referenced this pull request Oct 28, 2023

Merge pull request #3193 from canonical/add-bridged-setting

d706710

Add `local.instance.bridged` setting.

luis4a0 added a commit that referenced this pull request Nov 7, 2023

Merge pull request #3193 from canonical/add-bridged-setting

d14c6fa

Add `local.instance.bridged` setting.

luis4a0 added a commit that referenced this pull request Nov 7, 2023

Merge pull request #3193 from canonical/add-bridged-setting

77823d6

Add `local.instance.bridged` setting.

luis4a0 added a commit that referenced this pull request Nov 7, 2023

Merge pull request #3193 from canonical/add-bridged-setting

2b5062e

Add `local.instance.bridged` setting.

luis4a0 added a commit that referenced this pull request Nov 8, 2023

Merge pull request #3193 from canonical/add-bridged-setting

a692f11

Add `local.instance.bridged` setting.

luis4a0 added a commit that referenced this pull request Nov 10, 2023

Merge pull request #3193 from canonical/add-bridged-setting

5464795

Add `local.instance.bridged` setting.

luis4a0 added a commit that referenced this pull request Nov 27, 2023

Merge pull request #3193 from canonical/add-bridged-setting

740085e

Add `local.instance.bridged` setting.

luis4a0 added a commit that referenced this pull request Dec 4, 2023

Merge pull request #3193 from canonical/add-bridged-setting

ac1ad70

Add `local.instance.bridged` setting.

luis4a0 added a commit that referenced this pull request Jan 4, 2024

Merge pull request #3193 from canonical/add-bridged-setting

96dd3bb

Add `local.instance.bridged` setting.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `local.instance.bridged` setting. #3193

Add `local.instance.bridged` setting. #3193

luis4a0 commented Aug 16, 2023

codecov bot commented Aug 16, 2023 •

edited

Loading

ricab left a comment

ricab Sep 12, 2023

luis4a0 Sep 12, 2023

ricab Sep 12, 2023

luis4a0 Sep 13, 2023

ricab Sep 12, 2023

luis4a0 Sep 12, 2023

ricab Sep 12, 2023

georgeliao Sep 20, 2023

luis4a0 Sep 20, 2023

georgeliao Sep 20, 2023

luis4a0 Sep 20, 2023

georgeliao Sep 20, 2023 •

edited

Loading

luis4a0 Sep 20, 2023

georgeliao Sep 21, 2023

georgeliao Sep 20, 2023

luis4a0 Sep 20, 2023

georgeliao commented Sep 20, 2023

luis4a0 commented Sep 20, 2023

ricab Sep 20, 2023

luis4a0 Sep 22, 2023

ricab Sep 22, 2023

ricab Sep 22, 2023

luis4a0 Sep 22, 2023

ricab Sep 22, 2023

georgeliao Sep 22, 2023

luis4a0 Sep 22, 2023

ricab Sep 22, 2023

luis4a0 commented Sep 22, 2023

	throw std::runtime_error(fmt::format("You have to `multipass set {}=<name>` to use the `--bridged` shortcut.",
	throw std::runtime_error(fmt::format("You have to `multipass set {}=<name>` to use the "bridged" shortcut.",

Add local.instance.bridged setting. #3193

Add local.instance.bridged setting. #3193

Conversation

luis4a0 commented Aug 16, 2023

codecov bot commented Aug 16, 2023 • edited Loading

Codecov Report

ricab 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

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

georgeliao Sep 20, 2023 • edited Loading

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

georgeliao commented Sep 20, 2023

luis4a0 commented Sep 20, 2023

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 Sep 22, 2023

Add `local.instance.bridged` setting. #3193

Add `local.instance.bridged` setting. #3193

codecov bot commented Aug 16, 2023 •

edited

Loading

georgeliao Sep 20, 2023 •

edited

Loading