Merge pull request #3276 from canonical/message-only-on-ssh-exec-error

Message only on SSH execution error

include/multipass/exceptions/ssh_exception.h

@@ -30,5 +30,13 @@ class SSHException : public std::runtime_error
     {
     }
 };
+
+class SSHExecFailure : public SSHException
+{
+public:
+    SSHExecFailure(const std::string& what_arg) : SSHException(what_arg)
+    {
+    }
+};
 } // namespace multipass
 #endif // MULTIPASS_SSH_EXCEPTION_H

src/client/cli/cmd/start.cpp

@@ -65,11 +65,6 @@ mp::ReturnCode cmd::Start::run(mp::ArgParser* parser)
             cout << update_notice(reply.update_info());
         }
 
-        if (!reply.log_line().empty())
-        {
-            cout << "Warning: " << reply.log_line();
-        }
-
         return ReturnCode::Ok;
     };
 

src/daemon/daemon.cpp

@@ -3385,7 +3385,7 @@ mp::Daemon::async_wait_for_ready_all(grpc::ServerReaderWriterInterface<Reply, Re
                         mpu::run_in_ssh_session(session, command);
                     }
                 }
-                catch (const std::runtime_error&) // In case there is an error executing the command, report it.
+                catch (const mp::SSHExecFailure&) // In case there is an error executing the command, report it.
                 {
                     // Currently, the only use of running commands at boot is to configure networks. For this reason,
                     // the warning shown here refers to that use. In the future, in case of using the feature for other
@@ -3394,12 +3394,20 @@ mp::Daemon::async_wait_for_ready_all(grpc::ServerReaderWriterInterface<Reply, Re
                     if (!warned_exec_failure)
                     {
                         add_fmt_to(warnings,
-                                   "failure configuring network interfaces in {}, you can still do it manually.\n",
+                                   "Failure configuring network interfaces in {}: is Netplan installed?\n"
+                                   "You can still configure them manually.\n",
                                    name);
                     }
 
                     warned_exec_failure = true;
                 }
+                catch (const SSHException&) // The SSH session could not be created.
+                {
+                    add_fmt_to(warnings,
+                               "Cannot create a SSH shell to execute commands on {}, you can configure new\n"
+                               "interfaces manually via Netplan once logged to the instance.\n",
+                               name);
+                }
 
                 run_at_boot.erase(name);
             }

src/utils/utils.cpp

@@ -374,7 +374,7 @@ std::string mp::utils::run_in_ssh_session(mp::SSHSession& session, const std::st
         auto error_msg = proc.read_std_error();
         mpl::log(mpl::Level::warning, category,
                  fmt::format("failed to run '{}', error message: '{}'", cmd, mp::utils::trim_end(error_msg)));
-        throw std::runtime_error(mp::utils::trim_end(error_msg));
+        throw mp::SSHExecFailure(mp::utils::trim_end(error_msg));
     }
 
     auto output = proc.read_std_output();

tests/test_daemon_start.cpp

@@ -83,6 +83,43 @@ TEST_F(TestDaemonStart, successfulStartOkStatus)
     EXPECT_TRUE(status.ok());
 }
 
+TEST_F(TestDaemonStart, messageOnSSHError)
+{
+    REPLACE(ssh_new, []() { return nullptr; }); // This makes creating the SSH session throw.
+
+    std::vector<mp::NetworkInterface> unconfigured{{"eth7", "", 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(_, _)).Times(1);
+
+    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;
+
+    EXPECT_CALL(server, Write(Property(&mp::StartReply::log_line, HasSubstr("Cannot create a SSH shell")), _)).Times(1);
+
+    auto status = call_daemon_slot(daemon, &mp::Daemon::start, request, std::move(server));
+
+    EXPECT_THAT(status.error_message(), StrEq(""));
+    EXPECT_TRUE(status.ok());
+}
+
 struct WithSSH : public TestDaemonStart, WithParamInterface<int>
 {
 };
@@ -126,7 +163,7 @@ TEST_P(WithSSH, startConfiguresInterfaces)
         return std::move(instance_ptr);
     });
 
-    EXPECT_CALL(*instance_ptr, wait_until_ssh_up(_)).WillRepeatedly(Return());
+    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(_, _)).Times(1);
@@ -144,7 +181,7 @@ TEST_P(WithSSH, startConfiguresInterfaces)
     if (expected_status)
     {
         EXPECT_CALL(server,
-                    Write(Property(&mp::StartReply::log_line, HasSubstr("failure configuring network interfaces")), _))
+                    Write(Property(&mp::StartReply::log_line, HasSubstr("Failure configuring network interfaces")), _))
             .Times(1);
     }
     else