Filter new extra interfaces before instance start

src/daemon/daemon.cpp

@@ -2179,7 +2179,7 @@
     std::vector<std::string> 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 @@
                 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 @@
     }
 
     auto future_watcher = create_future_watcher();
-    future_watcher->setFuture(QtConcurrent::run(&Daemon::async_wait_for_ready_all<StartReply, StartRequest>, this,
-                                                server, starting_vms, timeout, status_promise,
-                                                fmt::to_string(start_errors)));
+    future_watcher->setFuture(QtConcurrent::run(&Daemon::async_wait_for_ready_all<StartReply, StartRequest>,
+                                                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 @@
     }
 
     auto future_watcher = create_future_watcher();
-    future_watcher->setFuture(QtConcurrent::run(&Daemon::async_wait_for_ready_all<RestartReply, RestartRequest>, this,
-                                                server, names_from(instance_targets), timeout, status_promise,
+    future_watcher->setFuture(QtConcurrent::run(&Daemon::async_wait_for_ready_all<RestartReply, RestartRequest>,
+                                                this,
+                                                server,
+                                                names_from(instance_targets),
+                                                timeout,
+                                                status_promise,
+                                                std::string(),
                                                 std::string()));
 }
 catch (const std::exception& e)
@@ -2751,8 +2769,13 @@
         virtual_machine->state = VirtualMachine::State::running;
         virtual_machine->update_state();
     });
-    future_watcher->setFuture(QtConcurrent::run(&Daemon::async_wait_for_ready_all<StartReply, StartRequest>, this,
-                                                nullptr, std::vector<std::string>{name}, mp::default_timeout, nullptr,
+    future_watcher->setFuture(QtConcurrent::run(&Daemon::async_wait_for_ready_all<StartReply, StartRequest>,
+                                                this,
+                                                nullptr,
+                                                std::vector<std::string>{name},
+                                                mp::default_timeout,
+                                                nullptr,
+                                                std::string(),
                                                 std::string()));
 }
 
@@ -2923,8 +2946,14 @@
                         server->Write(reply);
                     });
                     future_watcher->setFuture(
-                        QtConcurrent::run(&Daemon::async_wait_for_ready_all<LaunchReply, LaunchRequest>, this, server,
-                                          std::vector<std::string>{name}, timeout, status_promise, std::string()));
+                        QtConcurrent::run(&Daemon::async_wait_for_ready_all<LaunchReply, LaunchRequest>,
+                                          this,
+                                          server,
+                                          std::vector<std::string>{name},
+                                          timeout,
+                                          status_promise,
+                                          std::string(),
+                                          std::string()));
                 }
                 else
                 {
@@ -3316,37 +3345,61 @@
                : 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<std::string> mp::Daemon::configure_new_interfaces(const std::string& name,
+                                                                     mp::VirtualMachine& vm,
+                                                                     mp::VMSpecs& specs)
 {
-    std::vector<size_t> 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<mp::NetworkInterface> filtered_interfaces;
+    std::unordered_set<std::string> 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<mp::Daemon::AsyncOperationStatus>*
@@ -3443,8 +3496,11 @@
 template <typename Reply, typename Request>
 mp::Daemon::AsyncOperationStatus
 mp::Daemon::async_wait_for_ready_all(grpc::ServerReaderWriterInterface<Reply, Request>* server,
-                                     const std::vector<std::string>& vms, const std::chrono::seconds& timeout,
-                                     std::promise<grpc::Status>* status_promise, const std::string& start_errors)
+                                     const std::vector<std::string>& vms,
+                                     const std::chrono::seconds& timeout,
+                                     std::promise<grpc::Status>* status_promise,
+                                     const std::string& start_errors,
+                                     const std::string& start_warnings)
 {
     QFutureSynchronizer<std::string> start_synchronizer;
     {
@@ -3469,6 +3525,8 @@
 
     fmt::memory_buffer warnings;
 
+    fmt::format_to(std::back_inserter(warnings), "{}", start_warnings);
+
     {
         std::lock_guard<decltype(start_mutex)> lock{start_mutex};
         for (const auto& name : vms)

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<std::string> 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<Reply, Request>* server);
     template <typename Reply, typename Request>
-    AsyncOperationStatus
-    async_wait_for_ready_all(grpc::ServerReaderWriterInterface<Reply, Request>* server,
-                             const std::vector<std::string>& vms, const std::chrono::seconds& timeout,
-                             std::promise<grpc::Status>* status_promise, const std::string& errors);
+    AsyncOperationStatus async_wait_for_ready_all(grpc::ServerReaderWriterInterface<Reply, Request>* server,
+                                                  const std::vector<std::string>& vms,
+                                                  const std::chrono::seconds& timeout,
+                                                  std::promise<grpc::Status>* status_promise,
+                                                  const std::string& errors,
+                                                  const std::string& start_warnings);
     void finish_async_operation(const std::string& async_future_key);
     QFutureWatcher<AsyncOperationStatus>* create_future_watcher(std::function<void()> const& finished_op = []() {});
     void update_manifests_all(const bool is_force_update_from_network = false);

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());

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:

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,

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<uint32_t>(remaining));
+        const auto begin = fake_output.begin() + fake_output.size() - remaining;
+        std::copy_n(begin, num_to_copy, reinterpret_cast<char*>(dest));
+        remaining -= num_to_copy;
+        return num_to_copy;
+    };
+    REPLACE(ssh_channel_read_timeout, channel_read);
+
+    std::vector<mp::NetworkInterface> 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<NiceMock<mpt::MockVirtualMachine>>(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<NiceMock<mpt::MockVMImageVault>>();
+
+    mp::Daemon daemon{config_builder.build()};
+
+    mp::StartRequest request;
+    request.mutable_instance_names()->add_instance_name(mock_instance_name);
+
+    StrictMock<mpt::MockServerReaderWriter<mp::StartReply, mp::StartRequest>> 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();