Merge pull request #3625 from canonical/fix_bugs_of_force_stop

Fix the bug where VirtualMachine::shutdown function throw affects multipass delete

include/multipass/virtual_machine.h

@@ -58,12 +58,20 @@ class VirtualMachine : private DisabledCopyMove
         unknown
     };
 
+    enum class ShutdownPolicy
+    {
+        Powerdown, // gracefully shut down the vm
+        Poweroff,  // forcefully shut down the vm
+        Halt // halt the vm to non-running state. More specifically. suspended and stopped state will remain the same
+             // and running state will be shut down to stopped state
+    };
+
     using UPtr = std::unique_ptr<VirtualMachine>;
     using ShPtr = std::shared_ptr<VirtualMachine>;
 
     virtual ~VirtualMachine() = default;
     virtual void start() = 0;
-    virtual void shutdown(bool force = false) = 0;
+    virtual void shutdown(ShutdownPolicy shutdown_policy = ShutdownPolicy::Powerdown) = 0;
     virtual void suspend() = 0;
     virtual State current_state() = 0;
     virtual int ssh_port() = 0;

src/client/cli/cmd/delete.cpp

@@ -68,7 +68,12 @@ mp::ReturnCode cmd::Delete::run(mp::ArgParser* parser)
         return mp::ReturnCode::Ok;
     };
 
-    auto on_failure = [this](grpc::Status& status) { return standard_failure_handler_for(name(), cerr, status); };
+    auto on_failure = [this](grpc::Status& status) {
+        // grpc::StatusCode::FAILED_PRECONDITION matches mp::VMStateInvalidException
+        return status.error_code() == grpc::StatusCode::FAILED_PRECONDITION
+                   ? standard_failure_handler_for(name(), cerr, status, "Use --purge to forcefully delete it.")
+                   : standard_failure_handler_for(name(), cerr, status);
+    };
 
     using Client = grpc::ClientReaderWriterInterface<DeleteRequest, DeleteReply>;
     auto streaming_callback = [this](const mp::DeleteReply& reply, Client* client) {

src/client/cli/cmd/stop.cpp

@@ -44,7 +44,11 @@ mp::ReturnCode cmd::Stop::run(mp::ArgParser* parser)
     AnimatedSpinner spinner{cout};
     auto on_failure = [this, &spinner](grpc::Status& status) {
         spinner.stop();
-        return standard_failure_handler_for(name(), cerr, status);
+
+        // grpc::StatusCode::FAILED_PRECONDITION matches mp::VMStateInvalidException
+        return status.error_code() == grpc::StatusCode::FAILED_PRECONDITION
+                   ? standard_failure_handler_for(name(), cerr, status, "Use --force to power it off.")
+                   : standard_failure_handler_for(name(), cerr, status);
     };
 
     spinner.start(instance_action_message_for(request.instance_names(), "Stopping "));

src/daemon/daemon.cpp

@@ -32,6 +32,7 @@
 #include <multipass/exceptions/snapshot_exceptions.h>
 #include <multipass/exceptions/sshfs_missing_error.h>
 #include <multipass/exceptions/start_exception.h>
+#include <multipass/exceptions/virtual_machine_state_exceptions.h>
 #include <multipass/ip_address.h>
 #include <multipass/json_utils.h>
 #include <multipass/logging/client_logger.h>
@@ -2146,9 +2147,13 @@ try // clang-format on
 
     status_promise->set_value(status);
 }
+catch (const mp::VMStateInvalidException& e)
+{
+    status_promise->set_value(grpc::Status{grpc::StatusCode::FAILED_PRECONDITION, e.what()});
+}
 catch (const std::exception& e)
 {
-    status_promise->set_value(grpc::Status(grpc::StatusCode::FAILED_PRECONDITION, e.what(), ""));
+    status_promise->set_value(grpc::Status(grpc::StatusCode::INTERNAL, e.what()));
 }
 
 void mp::Daemon::suspend(const SuspendRequest* request,
@@ -2295,9 +2300,13 @@ try // clang-format on
     server->Write(response);
     status_promise->set_value(status);
 }
+catch (const mp::VMStateInvalidException& e)
+{
+    status_promise->set_value(grpc::Status{grpc::StatusCode::FAILED_PRECONDITION, e.what()});
+}
 catch (const std::exception& e)
 {
-    status_promise->set_value(grpc::Status(grpc::StatusCode::FAILED_PRECONDITION, e.what(), ""));
+    status_promise->set_value(grpc::Status(grpc::StatusCode::INTERNAL, e.what()));
 }
 
 void mp::Daemon::umount(const UmountRequest* request,
@@ -3075,8 +3084,9 @@ bool mp::Daemon::delete_vm(InstanceTable::iterator vm_it, bool purge, DeleteRepl
             delayed_shutdown_instances.erase(name);
 
         mounts[name].clear();
-        instance->shutdown(purge);
 
+        instance->shutdown(purge == true ? VirtualMachine::ShutdownPolicy::Poweroff
+                                         : VirtualMachine::ShutdownPolicy::Halt);
         if (!purge)
         {
             vm_instance_specs[name].deleted = true;
@@ -3120,50 +3130,27 @@ grpc::Status mp::Daemon::reboot_vm(VirtualMachine& vm)
 grpc::Status mp::Daemon::shutdown_vm(VirtualMachine& vm, const std::chrono::milliseconds delay)
 {
     const auto& name = vm.vm_name;
-    const auto& current_state = vm.current_state();
+    delayed_shutdown_instances.erase(name);
 
-    using St = VirtualMachine::State;
-    const auto skip_states = {St::off, St::stopped, St::suspended};
+    auto stop_all_mounts = [this](const std::string& name) { stop_mounts(name); };
+    auto& shutdown_timer = delayed_shutdown_instances[name] =
+        std::make_unique<DelayedShutdownTimer>(&vm, stop_all_mounts);
 
-    if (std::none_of(cbegin(skip_states), cend(skip_states), [&current_state](const auto& skip_state) {
-            return current_state == skip_state;
-        }))
-    {
+    QObject::connect(shutdown_timer.get(), &DelayedShutdownTimer::finished, [this, name]() {
         delayed_shutdown_instances.erase(name);
+    });
 
-        auto stop_all_mounts = [this](const std::string& name) { stop_mounts(name); };
-        auto& shutdown_timer = delayed_shutdown_instances[name] =
-            std::make_unique<DelayedShutdownTimer>(&vm, stop_all_mounts);
-
-        QObject::connect(shutdown_timer.get(), &DelayedShutdownTimer::finished,
-                         [this, name]() { delayed_shutdown_instances.erase(name); });
-
-        shutdown_timer->start(delay);
-    }
-    else
-        mpl::log(mpl::Level::debug, category, fmt::format("instance \"{}\" does not need stopping", name));
+    shutdown_timer->start(delay);
 
     return grpc::Status::OK;
 }
 
 grpc::Status mp::Daemon::switch_off_vm(VirtualMachine& vm)
 {
     const auto& name = vm.vm_name;
-    const auto& current_state = vm.current_state();
+    delayed_shutdown_instances.erase(name);
 
-    using St = VirtualMachine::State;
-    const auto skip_states = {St::off, St::stopped};
-
-    if (std::none_of(cbegin(skip_states), cend(skip_states), [&current_state](const auto& skip_state) {
-            return current_state == skip_state;
-        }))
-    {
-        delayed_shutdown_instances.erase(name);
-
-        vm.shutdown(true);
-    }
-    else
-        mpl::log(mpl::Level::debug, category, fmt::format("instance \"{}\" does not need stopping", name));
+    vm.shutdown(VirtualMachine::ShutdownPolicy::Poweroff);
 
     return grpc::Status::OK;
 }

src/platform/backends/libvirt/libvirt_virtual_machine.cpp

@@ -346,7 +346,7 @@ void mp::LibVirtVirtualMachine::start()
     monitor->on_resume();
 }
 
-void mp::LibVirtVirtualMachine::shutdown(bool force)
+void mp::LibVirtVirtualMachine::shutdown(ShutdownPolicy shutdown_policy)
 {
     std::unique_lock<std::mutex> lock{state_mutex};
     auto domain = checked_vm_domain();
@@ -355,15 +355,15 @@ void mp::LibVirtVirtualMachine::shutdown(bool force)
 
     try
     {
-        check_state_for_shutdown(force);
+        check_state_for_shutdown(shutdown_policy);
     }
     catch (const VMStateIdempotentException& e)
     {
         mpl::log(mpl::Level::info, vm_name, e.what());
         return;
     }
 
-    if (force) // TODO delete suspension state if it exists
+    if (shutdown_policy == ShutdownPolicy::Poweroff) // TODO delete suspension state if it exists
     {
         mpl::log(mpl::Level::info, vm_name, "Forcing shutdown");
 

src/platform/backends/libvirt/libvirt_virtual_machine.h

@@ -44,7 +44,7 @@ class LibVirtVirtualMachine final : public BaseVirtualMachine
     ~LibVirtVirtualMachine();
 
     void start() override;
-    void shutdown(bool force = false) override;
+    void shutdown(ShutdownPolicy shutdown_policy = ShutdownPolicy::Powerdown) override;
     void suspend() override;
     State current_state() override;
     int ssh_port() override;

src/platform/backends/lxd/lxd_virtual_machine.cpp

@@ -262,23 +262,24 @@ void mp::LXDVirtualMachine::start()
     update_state();
 }
 
-void mp::LXDVirtualMachine::shutdown(bool force)
+void mp::LXDVirtualMachine::shutdown(ShutdownPolicy shutdown_policy)
 {
     std::unique_lock<std::mutex> lock{state_mutex};
 
     const auto present_state = current_state();
 
     try
     {
-        check_state_for_shutdown(force);
+        check_state_for_shutdown(shutdown_policy);
     }
     catch (const VMStateIdempotentException& e)
     {
         mpl::log(mpl::Level::info, vm_name, e.what());
         return;
     }
 
-    request_state("stop", {{"force", force}});
+    // ShutdownPolicy::Poweroff is force and the other two values are non-force
+    request_state("stop", {{"force", shutdown_policy == ShutdownPolicy::Poweroff}});
 
     state = State::off;
 

src/platform/backends/lxd/lxd_virtual_machine.h

@@ -43,7 +43,7 @@ class LXDVirtualMachine : public BaseVirtualMachine
     ~LXDVirtualMachine() override;
 
     void start() override;
-    void shutdown(bool force = false) override;
+    void shutdown(ShutdownPolicy shutdown_policy = ShutdownPolicy::Powerdown) override;
     void suspend() override;
     State current_state() override;
     int ssh_port() override;

src/platform/backends/qemu/qemu_virtual_machine.cpp

@@ -345,29 +345,28 @@ void mp::QemuVirtualMachine::start()
     vm_process->write(qmp_execute_json("qmp_capabilities"));
 }
 
-void mp::QemuVirtualMachine::shutdown(bool force)
+void mp::QemuVirtualMachine::shutdown(ShutdownPolicy shutdown_policy)
 {
     std::unique_lock<std::mutex> lock{state_mutex};
 
-    force_shutdown = force;
-
     try
     {
-        check_state_for_shutdown(force);
+        check_state_for_shutdown(shutdown_policy);
     }
     catch (const VMStateIdempotentException& e)
     {
         mpl::log(mpl::Level::info, vm_name, e.what());
         return;
     }
 
-    if (force)
+    if (shutdown_policy == ShutdownPolicy::Poweroff)
     {
         mpl::log(mpl::Level::info, vm_name, "Forcing shutdown");
 
         if (vm_process)
         {
             mpl::log(mpl::Level::info, vm_name, "Killing process");
+            force_shutdown = true;
             lock.unlock();
             vm_process->kill();
             if (vm_process != nullptr && !vm_process->wait_for_finished(kill_process_timeout))

src/platform/backends/qemu/qemu_virtual_machine.h

@@ -50,7 +50,7 @@ class QemuVirtualMachine : public QObject, public BaseVirtualMachine
     ~QemuVirtualMachine();
 
     void start() override;
-    void shutdown(bool force = false) override;
+    void shutdown(ShutdownPolicy shutdown_policy = ShutdownPolicy::Powerdown) override;
     void suspend() override;
     State current_state() override;
     int ssh_port() override;

src/platform/backends/shared/base_virtual_machine.cpp

@@ -188,34 +188,39 @@ std::string mp::BaseVirtualMachine::get_instance_id_from_the_cloud_init() const
     return MP_CLOUD_INIT_FILE_OPS.get_instance_id_from_cloud_init(cloud_init_config_iso_file_path);
 }
 
-void mp::BaseVirtualMachine::check_state_for_shutdown(bool force)
+void mp::BaseVirtualMachine::check_state_for_shutdown(ShutdownPolicy shutdown_policy)
 {
-    const std::string force_statement{"Use --force to override."};
-
     // A mutex should already be locked by the caller here
     if (state == State::off || state == State::stopped)
     {
         throw VMStateIdempotentException{"Ignoring shutdown since instance is already stopped."};
     }
 
-    if (force)
+    if (shutdown_policy == ShutdownPolicy::Poweroff)
     {
         return;
     }
 
-    if (state == State::suspending)
+    if (state == State::suspended)
     {
-        throw VMStateInvalidException{fmt::format("Cannot stop instance while suspending. {}", force_statement)};
+        if (shutdown_policy == ShutdownPolicy::Halt)
+        {
+            throw VMStateIdempotentException{"Ignoring shutdown since instance is already suspended."};
+        }
+        else // else only can be ShutdownPolicy::Powerdown since ShutdownPolicy::Poweroff check was preemptively done.
+        {
+            throw VMStateInvalidException{fmt::format("Cannot shut down suspended instance {}.", vm_name)};
+        }
     }
 
-    if (state == State::suspended)
+    if (state == State::suspending)
     {
-        throw VMStateInvalidException{fmt::format("Cannot stop suspended instance. {}", force_statement)};
+        throw VMStateInvalidException{fmt::format("Cannot shut down instance {} while suspending.", vm_name)};
     }
 
     if (state == State::starting || state == State::restarting)
     {
-        throw VMStateInvalidException{fmt::format("Cannot stop instance while starting. {}", force_statement)};
+        throw VMStateInvalidException{fmt::format("Cannot shut down instance {} while starting.", vm_name)};
     }
 }
 

src/platform/backends/shared/base_virtual_machine.h

@@ -104,7 +104,7 @@ class BaseVirtualMachine : public VirtualMachine
         const std::string& new_instance_id) const;
     virtual std::string get_instance_id_from_the_cloud_init() const;
 
-    virtual void check_state_for_shutdown(bool force);
+    virtual void check_state_for_shutdown(ShutdownPolicy shutdown_policy);
 
 private:
     using SnapshotMap = std::unordered_map<std::string, std::shared_ptr<Snapshot>>;

tests/libvirt/test_libvirt_backend.cpp

@@ -448,7 +448,7 @@ TEST_F(LibVirtBackend, shutdown_while_starting_throws_and_sets_correct_state)
 
     ASSERT_EQ(machine->state, mp::VirtualMachine::State::starting);
 
-    mp::AutoJoinThread thread = [&machine] { machine->shutdown(true); };
+    mp::AutoJoinThread thread = [&machine] { machine->shutdown(mp::VirtualMachine::ShutdownPolicy::Poweroff); };
 
     using namespace std::chrono_literals;
     while (!destroy_called)

tests/lxd/test_lxd_backend.cpp

@@ -1702,7 +1702,7 @@ TEST_F(LXDBackend, shutdown_while_stopped_does_nothing_and_logs_debug)
 
 TEST_F(LXDBackend, shutdown_while_frozen_throws_and_logs_info)
 {
-    const std::string error_msg{"Cannot stop suspended instance. Use --force to override."};
+    const std::string sub_error_msg{"Cannot shut down suspended instance"};
     mpt::MockVMStatusMonitor mock_monitor;
 
     EXPECT_CALL(*mock_network_access_manager, createRequest(_, _, _)).WillRepeatedly([](auto, auto request, auto) {
@@ -1730,7 +1730,7 @@ TEST_F(LXDBackend, shutdown_while_frozen_throws_and_logs_info)
 
     EXPECT_CALL(mock_monitor, persist_state_for(_, _));
 
-    MP_EXPECT_THROW_THAT(machine.shutdown(), mp::VMStateInvalidException, mpt::match_what(StrEq(error_msg)));
+    MP_EXPECT_THROW_THAT(machine.shutdown(), mp::VMStateInvalidException, mpt::match_what(HasSubstr(sub_error_msg)));
 
     EXPECT_EQ(machine.current_state(), mp::VirtualMachine::State::suspended);
 }
@@ -1843,7 +1843,9 @@ TEST_F(LXDBackend, shutdown_while_starting_throws_and_sets_correct_state)
 
     ASSERT_EQ(machine.state, mp::VirtualMachine::State::starting);
 
-    mp::AutoJoinThread thread = [&machine] { machine.shutdown(true); }; // force shutdown
+    mp::AutoJoinThread thread = [&machine] {
+        machine.shutdown(mp::VirtualMachine::ShutdownPolicy::Poweroff);
+    }; // force shutdown
 
     while (machine.state != mp::VirtualMachine::State::off)
         std::this_thread::sleep_for(1ms);

tests/mock_virtual_machine.h

@@ -56,7 +56,7 @@ struct MockVirtualMachineT : public T
     }
 
     MOCK_METHOD(void, start, (), (override));
-    MOCK_METHOD(void, shutdown, (bool), (override));
+    MOCK_METHOD(void, shutdown, (VirtualMachine::ShutdownPolicy), (override));
     MOCK_METHOD(void, suspend, (), (override));
     MOCK_METHOD(multipass::VirtualMachine::State, current_state, (), (override));
     MOCK_METHOD(int, ssh_port, (), (override));