Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add local.instance.bridged setting. #3193

Merged
merged 4 commits into from
Sep 25, 2023
Merged

Add local.instance.bridged setting. #3193

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
17d6139
[daemon] Add local.instance.bridged setting.
luis4a0 Sep 13, 2023
ac54295
[tests] Check set local.instance.bridged.
luis4a0 Sep 13, 2023
24a4062
[settings] Fix `get local.<instance>.bridged`.
luis4a0 Sep 22, 2023
1a28788
[tests] Check `get local.<instance>.bridged`.
luis4a0 Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 31 additions & 2 deletions src/daemon/instance_settings_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include <multipass/constants.h>
#include <multipass/exceptions/invalid_memory_size_exception.h>
#include <multipass/settings/bool_setting_spec.h>

#include <QRegularExpression>
#include <QStringList>
Expand All @@ -30,6 +31,7 @@ namespace
constexpr auto cpus_suffix = "cpus";
constexpr auto mem_suffix = "memory";
constexpr auto disk_suffix = "disk";
constexpr auto bridged_suffix = "bridged";

enum class Operation
{
Expand All @@ -46,7 +48,7 @@ QRegularExpression make_key_regex()
{
const auto instance_pattern = QStringLiteral("(?<instance>.+)");
const auto prop_template = QStringLiteral("(?<property>%1)");
const auto either_prop = QStringList{cpus_suffix, mem_suffix, disk_suffix}.join("|");
const auto either_prop = QStringList{cpus_suffix, mem_suffix, disk_suffix, bridged_suffix}.join("|");
const auto prop_pattern = prop_template.arg(either_prop);

const auto key_template = QStringLiteral(R"(%1\.%2\.%3)");
Expand Down Expand Up @@ -153,6 +155,27 @@ void update_disk(const QString& key, const QString& val, mp::VirtualMachine& ins
}
}

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; });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

}

void update_bridged(const QString& key, const QString& val, mp::VirtualMachine& instance, mp::VMSpecs& spec)
{
auto bridged = mp::BoolSettingSpec{key, "false"}.interpret(val) == "true";

if (!bridged && is_bridged(spec))
{
Copy link
Contributor

@georgeliao georgeliao Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks.

throw mp::InvalidSettingException{key, val, "Bridged interface cannot be removed"};
}

if (bridged)
{
// TODO: add the interface
}
}

} // namespace

mp::InstanceSettingsException::InstanceSettingsException(const std::string& reason, const std::string& instance,
Expand Down Expand Up @@ -180,7 +203,7 @@ std::set<QString> mp::InstanceSettingsHandler::keys() const

std::set<QString> ret;
for (const auto& item : vm_instance_specs)
for (const auto& suffix : {cpus_suffix, mem_suffix, disk_suffix})
for (const auto& suffix : {cpus_suffix, mem_suffix, disk_suffix, bridged_suffix})
ret.insert(key_template.arg(item.first.c_str()).arg(suffix));

return ret;
Expand All @@ -191,6 +214,8 @@ QString mp::InstanceSettingsHandler::get(const QString& key) const
auto [instance_name, property] = parse_key(key);
const auto& spec = find_spec(instance_name);

if (property == bridged_suffix)
return is_bridged(spec) ? "true" : "false";
if (property == cpus_suffix)
return QString::number(spec.num_cores);
if (property == mem_suffix)
Expand All @@ -214,6 +239,10 @@ void mp::InstanceSettingsHandler::set(const QString& key, const QString& val)

if (property == cpus_suffix)
update_cpus(key, val, instance, spec);
else if (property == bridged_suffix)
{
update_bridged(key, val, instance, spec);
}
else
{
auto size = get_memory_size(key, val);
Expand Down
76 changes: 68 additions & 8 deletions tests/test_instance_settings_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,9 @@ struct TestInstanceSettingsHandler : public Test
std::unordered_map<std::string, mp::VirtualMachine::ShPtr> deleted_vms;
std::unordered_set<std::string> preparing_vms;
bool fake_persister_called = false;
inline static constexpr auto properties = std::array{"cpus", "disk", "memory"};
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"};
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, looks better that way. Changed.


QString make_key(const QString& instance_name, const QString& property)
Expand Down Expand Up @@ -193,6 +195,26 @@ TEST_F(TestInstanceSettingsHandler, getReturnsMemorySizesInHumanReadableFormat)
EXPECT_EQ(handler.get(make_key(target_instance_name, "memory")), "337.6KiB");
}

struct TestBridgedInstanceSettings : public TestInstanceSettingsHandler, public WithParamInterface<bool>
{
};

TEST_P(TestBridgedInstanceSettings, getFetchesBridged)
{
const auto bridged = GetParam();

constexpr auto target_instance_name = "lemmy";
specs.insert({{"mikkey", {}}, {"phil", {}}, {target_instance_name, {}}});

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

const auto got = make_handler().get(make_key(target_instance_name, "bridged"));
EXPECT_EQ(got, bridged ? "true" : "false");
}

INSTANTIATE_TEST_SUITE_P(getFetchesBridged, TestBridgedInstanceSettings, Values(true, false));

TEST_F(TestInstanceSettingsHandler, getFetchesPropertiesOfInstanceInSpecialState)
{
constexpr auto preparing_instance = "nouvelle", deleted_instance = "vague";
Expand Down Expand Up @@ -424,6 +446,19 @@ TEST_F(TestInstanceSettingsHandler, setRefusesWrongProperty)
EXPECT_EQ(original_specs, specs[target_instance_name]);
}

TEST_F(TestInstanceSettingsHandler, setRefusesToUnbridge)
{
constexpr auto target_instance_name = "hendrix";
specs.insert({{"voodoo", {}}, {"chile", {}}, {target_instance_name, {}}});
specs[target_instance_name].extra_interfaces = {{"id", "52:54:00:78:90:12", true}};

mock_vm(target_instance_name); // TODO: make this an expectation.

MP_EXPECT_THROW_THAT(make_handler().set(make_key(target_instance_name, "bridged"), "false"),
mp::InvalidSettingException,
mpt::match_what(HasSubstr("Bridged interface cannot be removed")));
}

using VMSt = mp::VirtualMachine::State;
using Property = const char*;
using PropertyAndState = std::tuple<Property, VMSt>; // no subliminal political msg intended :)
Expand Down Expand Up @@ -476,7 +511,8 @@ TEST_P(TestInstanceModOnStoppedInstance, setWorksOnOtherStates)
}

INSTANTIATE_TEST_SUITE_P(TestInstanceSettingsHandler, TestInstanceModOnStoppedInstance,
Combine(ValuesIn(TestInstanceSettingsHandler::properties), Values(VMSt::off, VMSt::stopped)));
Combine(ValuesIn(TestInstanceSettingsHandler::numeric_properties),
Values(VMSt::off, VMSt::stopped)));

struct TestInstanceModPersists : public TestInstanceSettingsHandler, public WithParamInterface<Property>
{
Expand All @@ -496,7 +532,7 @@ TEST_P(TestInstanceModPersists, setPersistsInstances)
}

INSTANTIATE_TEST_SUITE_P(TestInstanceSettingsHandler, TestInstanceModPersists,
ValuesIn(TestInstanceSettingsHandler::properties));
ValuesIn(TestInstanceSettingsHandler::numeric_properties));

TEST_F(TestInstanceSettingsHandler, setRefusesToModifyInstancesInSpecialState)
{
Expand Down Expand Up @@ -554,12 +590,12 @@ TEST_F(TestInstanceSettingsHandler, getAndSetThrowOnBadKey)

using PlainValue = const char*;
using PropertyValue = std::tuple<Property, PlainValue>;
struct TestInstanceSettingsHandlerBadValues : public TestInstanceSettingsHandler,
public WithParamInterface<PropertyValue>
struct TestInstanceSettingsHandlerBadNumericValues : public TestInstanceSettingsHandler,
public WithParamInterface<PropertyValue>
{
};

TEST_P(TestInstanceSettingsHandlerBadValues, setRefusesBadValues)
TEST_P(TestInstanceSettingsHandlerBadNumericValues, setRefusesBadNumericValues)
{
constexpr auto target_instance_name = "xanana";
const auto& [property, bad_val] = GetParam();
Expand All @@ -574,8 +610,32 @@ TEST_P(TestInstanceSettingsHandlerBadValues, setRefusesBadValues)
EXPECT_EQ(original_specs, specs[target_instance_name]);
}

INSTANTIATE_TEST_SUITE_P(TestInstanceSettingsHandler, TestInstanceSettingsHandlerBadValues,
Combine(ValuesIn(TestInstanceSettingsHandler::properties),
INSTANTIATE_TEST_SUITE_P(TestInstanceSettingsHandler, TestInstanceSettingsHandlerBadNumericValues,
Combine(ValuesIn(TestInstanceSettingsHandler::numeric_properties),
Values("0", "2u", "1.5f", "2.0", "0xa", "0x8", "-4", "-1", "rubbish", " 123nonsense ",
"¤9", "\n", "\t", "^", "")));

struct TestInstanceSettingsHandlerBadBooleanValues : public TestInstanceSettingsHandler,
public WithParamInterface<PropertyValue>
{
};

TEST_P(TestInstanceSettingsHandlerBadBooleanValues, setRefusesBadBooleanValues)
{
constexpr auto target_instance_name = "zappa";
const auto& [property, bad_val] = GetParam();

const auto original_specs = specs[target_instance_name];
mock_vm(target_instance_name); // TODO: make this an expectation.

MP_EXPECT_THROW_THAT(make_handler().set(make_key(target_instance_name, property), bad_val),
mp::InvalidSettingException,
mpt::match_what(AllOf(HasSubstr(bad_val), HasSubstr("try \"true\" or \"false\""))));

EXPECT_EQ(original_specs, specs[target_instance_name]);
}

INSTANTIATE_TEST_SUITE_P(TestInstanceSettingsHandler, TestInstanceSettingsHandlerBadBooleanValues,
Combine(ValuesIn(TestInstanceSettingsHandler::boolean_properties),
Values("apostrophe", "(')", "1974")));
} // namespace