Skip to content

Commit

Permalink
Merge #2101
Browse files Browse the repository at this point in the history 
2101: Fix eclipsing vms in ctor r=townsend2010 a=ricab

Let exceptions that arise when creating a VM in the daemon constructor through. This avoids erasing instance data that could be recoverable and important for the user, but aborts the daemon since it can't honor its database, which would otherwise be out of sync. More robust approaches for particular causes can always added. Fixes #1658.

Co-authored-by: Ricardo Abreu <ricardo.abreu@canonical.com>
  • Loading branch information
@bors @ricab
bors[bot] and ricab authored May 17, 2021
2 parents 5d69ce9 + 178cd47 commit 906a570
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 41 deletions.
23 changes: 11 additions & 12 deletions src/daemon/daemon.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -901,6 +901,14 @@ mp::Daemon::Daemon(std::unique_ptr<const DaemonConfig> the_config)
}

auto vm_image = fetch_image_for(name, config->factory->fetch_type(), *config->vault);
if (!QFile::exists(vm_image.image_path))
{
mpl::log(mpl::Level::warning, category,
fmt::format("Could not find image for '{}'. Expected location: {}", name, vm_image.image_path));
invalid_specs.push_back(name);
continue;
}

const auto instance_dir = mp::utils::base_dir(vm_image.image_path);
const auto cloud_init_iso = instance_dir.filePath("cloud-init-config.iso");
mp::VirtualMachineDescription vm_desc{spec.num_cores,
Expand All @@ -917,18 +925,8 @@ mp::Daemon::Daemon(std::unique_ptr<const DaemonConfig> the_config)
{},
{}};

try
{
auto& instance_record = spec.deleted ? deleted_instances : vm_instances;
instance_record[name] = config->factory->create_virtual_machine(vm_desc, *this);
}
catch (const std::exception& e)
{
mpl::log(mpl::Level::error, category, fmt::format("Removing instance {}: {}", name, e.what()));
invalid_specs.push_back(name);
config->vault->remove(name);
continue;
}
auto& instance_record = spec.deleted ? deleted_instances : vm_instances;
instance_record[name] = config->factory->create_virtual_machine(vm_desc, *this);

allocated_mac_addrs = std::move(new_macs); // Add the new macs to the daemon's list only if we got this far

Expand All @@ -955,6 +953,7 @@ mp::Daemon::Daemon(std::unique_ptr<const DaemonConfig> the_config)

for (const auto& bad_spec : invalid_specs)
{
mpl::log(mpl::Level::warning, category, fmt::format("Removing invalid instance: {}", bad_spec));
vm_instance_specs.erase(bad_spec);
}

Expand Down
2 changes: 1 addition & 1 deletion tests/mock_vm_image_vault.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class MockVMImageVault : public VMImageVault
MockVMImageVault()
{
ON_CALL(*this, fetch_image(_, _, _, _)).WillByDefault([this](auto, auto, const PrepareAction& prepare, auto) {
return prepare({dummy_image.name(), dummy_image.name(), dummy_image.name(), {}, {}, {}, {}, {}});
return VMImage{dummy_image.name(), dummy_image.name(), dummy_image.name(), {}, {}, {}, {}, {}};
});
ON_CALL(*this, has_record_for(_)).WillByDefault(Return(true));
ON_CALL(*this, minimum_image_size_for(_)).WillByDefault(Return(MemorySize{"1048576"}));
Expand Down
101 changes: 73 additions & 28 deletions tests/test_daemon.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,17 @@ std::string fake_json_contents(const std::string& default_mac, const std::vector
return contents.toStdString();
}

std::pair<std::unique_ptr<mpt::TempDir>, QString> // unique_ptr bypasses missing move ctor
plant_instance_json(const std::string& contents)
{
auto temp_dir = std::make_unique<mpt::TempDir>();
QString filename(temp_dir->path() + "/multipassd-vm-instances.json");

mpt::make_file_with_content(filename, contents);

return {std::move(temp_dir), filename};
}

void check_interfaces_in_json(const QString& file, const std::string& mac,
const std::vector<mp::NetworkInterface>& extra_interfaces)
{
Expand Down Expand Up @@ -982,15 +993,10 @@ TEST_F(Daemon, reads_mac_addresses_from_json)
mp::NetworkInterface{"wlx60e3270f55fe", "52:54:00:bd:19:41", true},
mp::NetworkInterface{"enp3s0", "01:23:45:67:89:ab", false}};

std::string json_contents = fake_json_contents(mac_addr, extra_interfaces);

mpt::TempDir temp_dir;
QString filename(temp_dir.path() + "/multipassd-vm-instances.json");

mpt::make_file_with_content(filename, json_contents);
const auto [temp_dir, filename] = plant_instance_json(fake_json_contents(mac_addr, extra_interfaces));

// Make the daemon look for the JSON on our temporary directory. It will read the contents of the file.
config_builder.data_directory = temp_dir.path();
config_builder.data_directory = temp_dir->path();
mp::Daemon daemon{config_builder.build()};

// By issuing the `list` command, we check at least that the instance was indeed read and there were no errors.
Expand Down Expand Up @@ -1068,16 +1074,6 @@ std::vector<std::string> old_releases{"10.04", "lucid", "11.10", "oneiric",
INSTANTIATE_TEST_SUITE_P(DaemonRefuseRelease, RefuseBridging, Combine(Values("release", ""), ValuesIn(old_releases)));
INSTANTIATE_TEST_SUITE_P(DaemonRefuseSnapcraft, RefuseBridging, Values(std::make_tuple("snapcraft", "core")));

std::unique_ptr<mpt::TempDir> plant_instance_json(const std::string& contents) // unique_ptr bypasses missing move ctor
{
auto temp_dir = std::make_unique<mpt::TempDir>();
QString filename(temp_dir->path() + "/multipassd-vm-instances.json");

mpt::make_file_with_content(filename, contents);

return temp_dir;
}

constexpr auto ghost_template = R"(
"{}": {{
"deleted": false,
Expand Down Expand Up @@ -1113,8 +1109,8 @@ TEST_F(Daemon, skips_over_instance_ghosts_in_db) // which will have been sometim
auto ghost2 = fmt::format(ghost_template, "ghost2");
auto valid1 = fmt::format(valid_template, id1, "56");
auto valid2 = fmt::format(valid_template, id2, "78");
auto temp_dir = plant_instance_json(fmt::format("{{\n{},\n{},\n{},\n{}\n}}", std::move(ghost1), std::move(ghost2),
std::move(valid1), std::move(valid2)));
const auto [temp_dir, filename] = plant_instance_json(fmt::format(
"{{\n{},\n{},\n{},\n{}\n}}", std::move(ghost1), std::move(ghost2), std::move(valid1), std::move(valid2)));

config_builder.data_directory = temp_dir->path();
auto mock_factory = use_a_mock_vm_factory();
Expand All @@ -1126,6 +1122,53 @@ TEST_F(Daemon, skips_over_instance_ghosts_in_db) // which will have been sometim
mp::Daemon daemon{config_builder.build()};
}

TEST_F(Daemon, ctor_lets_exceptions_arising_from_vm_creation_through)
{
config_builder.vault = std::make_unique<NiceMock<mpt::MockVMImageVault>>();
const auto [temp_dir, filename] = plant_instance_json(fake_json_contents("ab:ab:ab:ab:ab:ab", {}));
config_builder.data_directory = temp_dir->path();

const std::string msg = "asdf";
auto mock_factory = use_a_mock_vm_factory();
EXPECT_CALL(*mock_factory, create_virtual_machine).WillOnce(Throw(std::runtime_error{msg}));

MP_EXPECT_THROW_THAT(mp::Daemon{config_builder.build()}, std::runtime_error, mpt::match_what(msg));
}

TEST_F(Daemon, ctor_drops_removed_instances)
{
const std::string stayed{"foo"}, gone{"fighters"};
auto stayed_json = fmt::format(valid_template, stayed, "12");
auto gone_json = fmt::format(valid_template, gone, "34");
const auto [temp_dir, filename] =
plant_instance_json(fmt::format("{{\n{},\n{}\n}}", std::move(stayed_json), std::move(gone_json)));
config_builder.data_directory = temp_dir->path();

auto mock_image_vault = std::make_unique<NiceMock<mpt::MockVMImageVault>>();
EXPECT_CALL(*mock_image_vault, fetch_image(_, Field(&mp::Query::name, stayed), _, _))
.WillRepeatedly(DoDefault()); // returns an image that can be verified to exist for this instance
EXPECT_CALL(*mock_image_vault, fetch_image(_, Field(&mp::Query::name, gone), _, _))
.WillOnce(Return(mp::VMImage{})); // an image that can't be verified to exist for this instance
config_builder.vault = std::move(mock_image_vault);

auto mock_factory = use_a_mock_vm_factory();
EXPECT_CALL(*mock_factory, create_virtual_machine(Field(&mp::VirtualMachineDescription::vm_name, stayed), _))
.Times(1);
EXPECT_CALL(*mock_factory, create_virtual_machine(Field(&mp::VirtualMachineDescription::vm_name, gone), _))
.Times(0);

mp::Daemon daemon{config_builder.build()};

std::stringstream stream;
send_command({"list"}, stream);

auto instance_matchers = AllOf(HasSubstr(stayed), Not(HasSubstr(gone)));
EXPECT_THAT(stream.str(), instance_matchers);

auto updated_json = mpt::load(filename);
EXPECT_THAT(updated_json.toStdString(), instance_matchers);
}

TEST_P(ListIP, lists_with_ip)
{
auto mock_factory = use_a_mock_vm_factory();
Expand Down Expand Up @@ -1168,7 +1211,7 @@ TEST_F(Daemon, prevents_repetition_of_loaded_mac_addresses)
config_builder.vault = std::make_unique<NiceMock<mpt::MockVMImageVault>>();

std::string repeated_mac{"52:54:00:bd:19:41"};
auto temp_dir = plant_instance_json(fake_json_contents(repeated_mac, {}));
const auto [temp_dir, filename] = plant_instance_json(fake_json_contents(repeated_mac, {}));
config_builder.data_directory = temp_dir->path();

auto mock_factory = use_a_mock_vm_factory();
Expand All @@ -1187,7 +1230,7 @@ TEST_F(Daemon, does_not_hold_on_to_repeated_mac_addresses_when_loading)
std::string mac_addr("52:54:00:73:76:28");
std::vector<mp::NetworkInterface> extra_interfaces{mp::NetworkInterface{"eth0", mac_addr, true}};

auto temp_dir = plant_instance_json(fake_json_contents(mac_addr, extra_interfaces));
const auto [temp_dir, filename] = plant_instance_json(fake_json_contents(mac_addr, extra_interfaces));
config_builder.data_directory = temp_dir->path();

auto mock_factory = use_a_mock_vm_factory();
Expand All @@ -1199,19 +1242,21 @@ TEST_F(Daemon, does_not_hold_on_to_repeated_mac_addresses_when_loading)

TEST_F(Daemon, does_not_hold_on_to_macs_when_loading_fails)
{
config_builder.vault = std::make_unique<NiceMock<mpt::MockVMImageVault>>();

std::string mac1{"52:54:00:73:76:28"}, mac2{"52:54:00:bd:19:41"};
std::vector<mp::NetworkInterface> extra_interfaces{mp::NetworkInterface{"eth0", mac2, true}};

auto temp_dir = plant_instance_json(fake_json_contents(mac1, extra_interfaces));
const auto [temp_dir, filename] = plant_instance_json(fake_json_contents(mac1, extra_interfaces));
config_builder.data_directory = temp_dir->path();

auto mock_image_vault = std::make_unique<NiceMock<mpt::MockVMImageVault>>();
EXPECT_CALL(*mock_image_vault, fetch_image)
.WillOnce(Return(mp::VMImage{})) // cause the Daemon's ctor to fail verifying that the img exists
.WillRepeatedly(DoDefault());
config_builder.vault = std::move(mock_image_vault);

auto mock_factory = use_a_mock_vm_factory();
EXPECT_CALL(*mock_factory, create_virtual_machine)
.Times(3) // expect one call in the constructor and three in launch
.WillOnce(Throw(std::exception{})) // fail the first one
.WillRepeatedly(DoDefault()); // succeed the rest (this avoids gmock warnings)
EXPECT_CALL(*mock_factory, create_virtual_machine).Times(2); // no launch in ctor, two launch commands

mp::Daemon daemon{config_builder.build()};

for (const auto& mac : {mac1, mac2})
Expand Down

0 comments on commit 906a570

Please sign in to comment.