diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index cd072d0e1e..a4455253e4 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}; @@ -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_warnings), + "Cannot bridge {} for instance {}. Please see the logs for more details.\n", + fmt::join(failed_interfaces, ", "), + name); + } vm.start(); } @@ -2221,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) { @@ -2325,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) @@ -2751,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())); } @@ -2923,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 { @@ -3316,37 +3345,61 @@ 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. +// 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) { - 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* @@ -3443,8 +3496,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; { @@ -3469,6 +3525,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 b19d791ed3..e7687f9b99 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 { @@ -185,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); 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: 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, diff --git a/tests/test_daemon_start.cpp b/tests/test_daemon_start.cpp index 02f67bec2d..d39cb0d291 100644 --- a/tests/test_daemon_start.cpp +++ b/tests/test_daemon_start.cpp @@ -235,6 +235,67 @@ 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; + + 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_TRUE(status.ok()); +} + TEST_F(TestDaemonStart, unknownStateDoesNotStart) { auto mock_factory = use_a_mock_vm_factory();