From a3833312b0d80ba3682e2c1dfdd9318e7d83c70c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Fri, 12 Jan 2024 14:34:13 -0300 Subject: [PATCH 1/7] [daemon] Remove failing bridged interfaces. The daemon removes now the new bridged interfaces requested by the user which could not be added to the instance. --- src/daemon/daemon.cpp | 69 +++++++++++++++++++++++++++++++------------ src/daemon/daemon.h | 4 ++- 2 files changed, 53 insertions(+), 20 deletions(-) diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index cd072d0e1e..d444f85aab 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -2212,7 +2212,15 @@ try // clang-format on mpl::log(mpl::Level::error, category, "Mounts have been disabled on this instance of Multipass"); } - configure_new_interfaces(name, vm, vm_instance_specs[name]); + auto failed_interfaces = configure_new_interfaces(name, vm, vm_instance_specs[name]); + + if (!failed_interfaces.empty()) + { + fmt::format_to(std::back_inserter(start_errors), + "Cannot bridge {} for instance {}", + fmt::join(failed_interfaces, ", "), + name); + } vm.start(); } @@ -3316,37 +3324,60 @@ mp::MountHandler::UPtr mp::Daemon::make_mount(VirtualMachine* vm, const std::str : vm->make_native_mount_handler(config->ssh_key_provider.get(), target, mount); } -void mp::Daemon::configure_new_interfaces(const std::string& name, mp::VirtualMachine& vm, mp::VMSpecs& specs) +// This function configures the network interfaces whose MAC address is empty. They will stay in specs.extra_interfaces +// only if the backend is able to add them to the virtual machine. If not, they will be removed. +std::unordered_set mp::Daemon::configure_new_interfaces(const std::string& name, + mp::VirtualMachine& vm, + mp::VMSpecs& specs) { - std::vector new_interfaces; + bool added_good_interfaces{false}; + size_t j = 0; /* The final index of the interface. This is needed because we will filter out the bad interfaces + from specs.extra_interfaces. */ + std::vector filtered_interfaces; + std::unordered_set bad_bridges; + auto& commands = specs.run_at_boot; - for (auto i = 0u; i < specs.extra_interfaces.size(); ++i) - { - auto& interface = specs.extra_interfaces[i]; + config->factory->prepare_networking(specs.extra_interfaces); // TODO: call this only if needed. - if (interface.mac_address.empty()) // An empty MAC address means the interface needs to be configured. + for (auto& iface : specs.extra_interfaces) + { + if (iface.mac_address.empty()) // An empty MAC address means the interface needs to be configured. { - new_interfaces.push_back(i); + iface.mac_address = generate_unused_mac_address(allocated_mac_addrs); - interface.mac_address = generate_unused_mac_address(allocated_mac_addrs); - } - } + try + { + vm.add_network_interface(j, iface); // This will throw if the interface wasn't added to the VM. - if (!new_interfaces.empty()) - { - config->factory->prepare_networking(specs.extra_interfaces); + added_good_interfaces = true; - auto& commands = specs.run_at_boot; + commands.push_back(generate_netplan_script(j, iface.mac_address)); + } + catch (const std::exception&) + { + // The generated MAC will not be used, so we remove it from the list of used addresses. + // TODO: avoid adding the new MAC to allocated_mac_addrs if the interface is bad. + allocated_mac_addrs.erase(iface.mac_address); - for (const auto i : new_interfaces) - { - vm.add_network_interface(i, specs.extra_interfaces[i]); + bad_bridges.insert(iface.id); - commands.push_back(generate_netplan_script(i, specs.extra_interfaces[i].mac_address)); + continue; + } } + filtered_interfaces.push_back(iface); + + ++j; + } + + if (added_good_interfaces) + { commands.push_back("sudo netplan apply"); } + + specs.extra_interfaces = filtered_interfaces; + + return bad_bridges; } QFutureWatcher* diff --git a/src/daemon/daemon.h b/src/daemon/daemon.h index b19d791ed3..19348f5170 100644 --- a/src/daemon/daemon.h +++ b/src/daemon/daemon.h @@ -172,7 +172,9 @@ public slots: VirtualMachine* vm); MountHandler::UPtr make_mount(VirtualMachine* vm, const std::string& target, const VMMount& mount); - void configure_new_interfaces(const std::string& name, VirtualMachine& vm, VMSpecs& specs); + std::unordered_set configure_new_interfaces(const std::string& name, + VirtualMachine& vm, + VMSpecs& specs); struct AsyncOperationStatus { From 145f449e3d0502a8f9dad79e43beacf585aabfd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Fri, 12 Jan 2024 14:35:48 -0300 Subject: [PATCH 2/7] [tests] Check bridged interface errors at start. --- tests/test_daemon_start.cpp | 60 +++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/test_daemon_start.cpp b/tests/test_daemon_start.cpp index 02f67bec2d..77f4e2ea3c 100644 --- a/tests/test_daemon_start.cpp +++ b/tests/test_daemon_start.cpp @@ -235,6 +235,66 @@ TEST_F(TestDaemonStart, exitlessSshProcessExceptionDoesNotShowMessage) INSTANTIATE_TEST_SUITE_P(TestDaemonStart, WithSSH, Values(0, 1, -1)); +TEST_F(TestDaemonStart, startShowsBridgingErrors) +{ + ssh_channel_callbacks callbacks{nullptr}; + auto add_channel_cbs = [&callbacks](ssh_channel, ssh_channel_callbacks cb) { + callbacks = cb; + return SSH_OK; + }; + REPLACE(ssh_add_channel_callbacks, add_channel_cbs); + + auto event_dopoll = [&callbacks](auto...) { + if (!callbacks) + return SSH_ERROR; + callbacks->channel_exit_status_function(nullptr, nullptr, 0, callbacks->userdata); + return SSH_OK; + }; + REPLACE(ssh_event_dopoll, event_dopoll); + + std::string fake_output{"some output"}; + auto remaining = fake_output.size(); + auto channel_read = [&fake_output, &remaining](ssh_channel, void* dest, uint32_t count, int is_stderr, int) { + const auto num_to_copy = std::min(count, static_cast(remaining)); + const auto begin = fake_output.begin() + fake_output.size() - remaining; + std::copy_n(begin, num_to_copy, reinterpret_cast(dest)); + remaining -= num_to_copy; + return num_to_copy; + }; + REPLACE(ssh_channel_read_timeout, channel_read); + + std::vector unconfigured{{"eth47", "", true}}; + + auto mock_factory = use_a_mock_vm_factory(); + const auto [temp_dir, filename] = plant_instance_json(fake_json_contents(mac_addr, unconfigured)); + + auto instance_ptr = std::make_unique>(mock_instance_name); + EXPECT_CALL(*mock_factory, create_virtual_machine(_, _)).WillOnce([&instance_ptr](const auto&, auto&) { + return std::move(instance_ptr); + }); + + EXPECT_CALL(*instance_ptr, wait_until_ssh_up).WillRepeatedly(Return()); + EXPECT_CALL(*instance_ptr, current_state()).WillRepeatedly(Return(mp::VirtualMachine::State::off)); + EXPECT_CALL(*instance_ptr, start()).Times(1); + EXPECT_CALL(*instance_ptr, add_network_interface(_, _)).WillOnce(Throw(std::runtime_error("panic show"))); + + config_builder.data_directory = temp_dir->path(); + config_builder.vault = std::make_unique>(); + + mp::Daemon daemon{config_builder.build()}; + + mp::StartRequest request; + request.mutable_instance_names()->add_instance_name(mock_instance_name); + + StrictMock> server; + + auto status = call_daemon_slot(daemon, &mp::Daemon::start, request, std::move(server)); + + EXPECT_THAT(status.error_message(), + StrEq("The following errors occurred:\nCannot bridge eth47 for instance real-zebraphant")); + EXPECT_FALSE(status.ok()); +} + TEST_F(TestDaemonStart, unknownStateDoesNotStart) { auto mock_factory = use_a_mock_vm_factory(); From 801c6a79f288d9e52d212bba4dc0f72b614a6355 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Fri, 19 Jan 2024 11:11:32 -0300 Subject: [PATCH 3/7] [lxd] Log network errors as warnings. They used to be logged as errors, what produced a confusing output to the user when failing to bridge an interface. --- src/platform/backends/lxd/lxd_request.cpp | 10 +++++++++- src/platform/backends/lxd/lxd_request.h | 8 ++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/platform/backends/lxd/lxd_request.cpp b/src/platform/backends/lxd/lxd_request.cpp index d486ff2379..857512b085 100644 --- a/src/platform/backends/lxd/lxd_request.cpp +++ b/src/platform/backends/lxd/lxd_request.cpp @@ -113,7 +113,9 @@ const QJsonObject lxd_request_common(const std::string& method, QUrl& url, int t mpl::log(mpl::Level::trace, request_category, fmt::format("Got reply: {}", QJsonDocument(json_reply).toJson())); if (reply->error() != QNetworkReply::NoError) - throw mp::LXDRuntimeError(fmt::format("Network error for {}: {} - {}", url.toString(), reply->errorString(), + throw mp::LXDNetworkError(fmt::format("Network error for {}: {} - {}", + url.toString(), + reply->errorString(), json_reply.object()["error"].toString())); return json_reply.object(); @@ -141,6 +143,12 @@ try return lxd_request_common(method, url, timeout, handle_request); } +catch (const LXDNetworkError& e) +{ + mpl::log(mpl::Level::warning, request_category, e.what()); + + throw; +} catch (const LXDRuntimeError& e) { mpl::log(mpl::Level::error, request_category, e.what()); diff --git a/src/platform/backends/lxd/lxd_request.h b/src/platform/backends/lxd/lxd_request.h index 4b5e884094..4c352c6a33 100644 --- a/src/platform/backends/lxd/lxd_request.h +++ b/src/platform/backends/lxd/lxd_request.h @@ -48,6 +48,14 @@ class LXDRuntimeError : public std::runtime_error } }; +class LXDNetworkError : public LXDRuntimeError +{ +public: + LXDNetworkError(const std::string& message) : LXDRuntimeError{message} + { + } +}; + class LXDJsonParseError : public std::runtime_error { public: From c647dbf4a559f069fe025d87020407a84ba30310 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Thu, 25 Jan 2024 14:12:10 -0300 Subject: [PATCH 4/7] [daemon] Add warning for bridging error. If a bridged interface couldn't be added to an instance, a concise warning message is shown at start. --- src/daemon/daemon.cpp | 54 ++++++++++++++++++++++++++++++++----------- src/daemon/daemon.h | 10 ++++---- 2 files changed, 46 insertions(+), 18 deletions(-) diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index d444f85aab..17eada7235 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -2179,7 +2179,7 @@ try // clang-format on std::vector starting_vms{}; starting_vms.reserve(instance_selection.operative_selection.size()); - fmt::memory_buffer start_errors; + fmt::memory_buffer start_errors, start_warnings; for (auto& vm_it : instance_selection.operative_selection) { std::lock_guard lock{start_mutex}; @@ -2216,8 +2216,8 @@ try // clang-format on if (!failed_interfaces.empty()) { - fmt::format_to(std::back_inserter(start_errors), - "Cannot bridge {} for instance {}", + fmt::format_to(std::back_inserter(start_warnings), + "Cannot bridge {} for instance {}. Please see the logs for more details.\n", fmt::join(failed_interfaces, ", "), name); } @@ -2229,9 +2229,14 @@ try // clang-format on } auto future_watcher = create_future_watcher(); - future_watcher->setFuture(QtConcurrent::run(&Daemon::async_wait_for_ready_all, this, - server, starting_vms, timeout, status_promise, - fmt::to_string(start_errors))); + future_watcher->setFuture(QtConcurrent::run(&Daemon::async_wait_for_ready_all, + this, + server, + starting_vms, + timeout, + status_promise, + fmt::to_string(start_errors), + fmt::to_string(start_warnings))); } catch (const std::exception& e) { @@ -2333,8 +2338,13 @@ try // clang-format on } auto future_watcher = create_future_watcher(); - future_watcher->setFuture(QtConcurrent::run(&Daemon::async_wait_for_ready_all, this, - server, names_from(instance_targets), timeout, status_promise, + future_watcher->setFuture(QtConcurrent::run(&Daemon::async_wait_for_ready_all, + this, + server, + names_from(instance_targets), + timeout, + status_promise, + std::string(), std::string())); } catch (const std::exception& e) @@ -2759,8 +2769,13 @@ void mp::Daemon::on_restart(const std::string& name) virtual_machine->state = VirtualMachine::State::running; virtual_machine->update_state(); }); - future_watcher->setFuture(QtConcurrent::run(&Daemon::async_wait_for_ready_all, this, - nullptr, std::vector{name}, mp::default_timeout, nullptr, + future_watcher->setFuture(QtConcurrent::run(&Daemon::async_wait_for_ready_all, + this, + nullptr, + std::vector{name}, + mp::default_timeout, + nullptr, + std::string(), std::string())); } @@ -2931,8 +2946,14 @@ void mp::Daemon::create_vm(const CreateRequest* request, server->Write(reply); }); future_watcher->setFuture( - QtConcurrent::run(&Daemon::async_wait_for_ready_all, this, server, - std::vector{name}, timeout, status_promise, std::string())); + QtConcurrent::run(&Daemon::async_wait_for_ready_all, + this, + server, + std::vector{name}, + timeout, + status_promise, + std::string(), + std::string())); } else { @@ -3474,8 +3495,11 @@ mp::Daemon::async_wait_for_ssh_and_start_mounts_for(const std::string& name, con template mp::Daemon::AsyncOperationStatus mp::Daemon::async_wait_for_ready_all(grpc::ServerReaderWriterInterface* server, - const std::vector& vms, const std::chrono::seconds& timeout, - std::promise* status_promise, const std::string& start_errors) + const std::vector& vms, + const std::chrono::seconds& timeout, + std::promise* status_promise, + const std::string& start_errors, + const std::string& start_warnings) { QFutureSynchronizer start_synchronizer; { @@ -3500,6 +3524,8 @@ mp::Daemon::async_wait_for_ready_all(grpc::ServerReaderWriterInterface lock{start_mutex}; for (const auto& name : vms) diff --git a/src/daemon/daemon.h b/src/daemon/daemon.h index 19348f5170..e7687f9b99 100644 --- a/src/daemon/daemon.h +++ b/src/daemon/daemon.h @@ -187,10 +187,12 @@ public slots: std::string async_wait_for_ssh_and_start_mounts_for(const std::string& name, const std::chrono::seconds& timeout, grpc::ServerReaderWriterInterface* server); template - AsyncOperationStatus - async_wait_for_ready_all(grpc::ServerReaderWriterInterface* server, - const std::vector& vms, const std::chrono::seconds& timeout, - std::promise* status_promise, const std::string& errors); + AsyncOperationStatus async_wait_for_ready_all(grpc::ServerReaderWriterInterface* server, + const std::vector& vms, + const std::chrono::seconds& timeout, + std::promise* status_promise, + const std::string& errors, + const std::string& start_warnings); void finish_async_operation(const std::string& async_future_key); QFutureWatcher* create_future_watcher(std::function const& finished_op = []() {}); void update_manifests_all(const bool is_force_update_from_network = false); From 43df7d4ae253d9cd85cd1d41f9e67d8f5cf0f9ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Thu, 25 Jan 2024 14:12:51 -0300 Subject: [PATCH 5/7] [tests][daemon] Adapt to new warning output. --- tests/test_daemon_start.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/test_daemon_start.cpp b/tests/test_daemon_start.cpp index 77f4e2ea3c..d39cb0d291 100644 --- a/tests/test_daemon_start.cpp +++ b/tests/test_daemon_start.cpp @@ -288,11 +288,12 @@ TEST_F(TestDaemonStart, startShowsBridgingErrors) StrictMock> server; + std::string log{"Cannot bridge eth47 for instance real-zebraphant. Please see the logs for more details.\n"}; + EXPECT_CALL(server, Write(Property(&mp::StartReply::log_line, HasSubstr(log)), _)).Times(1); + auto status = call_daemon_slot(daemon, &mp::Daemon::start, request, std::move(server)); - EXPECT_THAT(status.error_message(), - StrEq("The following errors occurred:\nCannot bridge eth47 for instance real-zebraphant")); - EXPECT_FALSE(status.ok()); + EXPECT_TRUE(status.ok()); } TEST_F(TestDaemonStart, unknownStateDoesNotStart) From 9b716203a89e7602b31a3c77fdee7774b991ee47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Fri, 19 Jan 2024 12:47:19 -0300 Subject: [PATCH 6/7] [tests][lxd] Adapt to new network error logging. --- tests/lxd/test_lxd_backend.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/lxd/test_lxd_backend.cpp b/tests/lxd/test_lxd_backend.cpp index af138ba36c..1ced6edaad 100644 --- a/tests/lxd/test_lxd_backend.cpp +++ b/tests/lxd/test_lxd_backend.cpp @@ -1362,7 +1362,8 @@ TEST_F(LXDBackend, lxd_request_bad_request_throws_and_logs) HasSubstr(": Error - Failure")); EXPECT_CALL(*logger_scope.mock_logger, - log(Eq(mpl::Level::error), mpt::MockLogger::make_cstring_matcher(StrEq("lxd request")), + log(_, + mpt::MockLogger::make_cstring_matcher(StrEq("lxd request")), mpt::MockLogger::make_cstring_matcher(error_matcher))); MP_EXPECT_THROW_THAT(mp::lxd_request(mock_network_access_manager.get(), "GET", base_url), std::runtime_error, From 2701104435fccac463e7fabe5adbfdc2e694dcad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Thu, 25 Jan 2024 14:35:50 -0300 Subject: [PATCH 7/7] [daemon] Comment function add_new_interface(). The return value was explained, because it is counter-intuitive. --- src/daemon/daemon.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index 17eada7235..a4455253e4 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -3347,6 +3347,7 @@ mp::MountHandler::UPtr mp::Daemon::make_mount(VirtualMachine* vm, const std::str // This function configures the network interfaces whose MAC address is empty. They will stay in specs.extra_interfaces // only if the backend is able to add them to the virtual machine. If not, they will be removed. +// The return value is a set of networks that couldn't be configured and thus were removed from specs.extra_interfaces. std::unordered_set mp::Daemon::configure_new_interfaces(const std::string& name, mp::VirtualMachine& vm, mp::VMSpecs& specs)