Merge #2877

2877: Handle Qemu native mount when resuming from suspended state r=townsend2010 a=andrei-toterman fix #2876 Co-authored-by: Andrei Toterman <[email protected]>
canonical · Jan 14, 2023 · e359350 · e359350
1 parent 9bcabbf
commit e359350
Show file tree

Hide file tree

Showing 9 changed files with 105 additions and 34 deletions.
diff --git a/include/multipass/utils.h b/include/multipass/utils.h
@@ -129,7 +129,7 @@ std::string qenum_to_string(RegisteredQtEnum val);
 
 // other helpers
 QString get_multipass_storage();
-QString make_uuid();
+QString make_uuid(const std::optional<std::string>& seed = std::nullopt);
 template <typename OnTimeoutCallable, typename TryAction, typename... Args>
 void try_action_for(OnTimeoutCallable&& on_timeout, std::chrono::milliseconds timeout, TryAction&& try_action,
                     Args&&... args);

diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp
@@ -2638,9 +2638,29 @@ grpc::Status mp::Daemon::cmd_vms(const std::vector<std::string>& tgts, std::func
 void mp::Daemon::init_mounts(const std::string& name)
 {
     auto& vm_mounts = mounts[name];
-    for (const auto& [target, vm_mount] : vm_instance_specs[name].mounts)
+    auto& vm_spec_mounts = vm_instance_specs[name].mounts;
+    std::vector<std::string> mounts_to_remove;
+    for (const auto& [target, vm_mount] : vm_spec_mounts)
+    {
         if (vm_mounts.find(target) == vm_mounts.end())
-            vm_mounts[target] = make_mount(vm_instances[name].get(), target, vm_mount);
+            try
+            {
+                vm_mounts[target] = make_mount(vm_instances[name].get(), target, vm_mount);
+            }
+            catch (const std::exception& e)
+            {
+                mpl::log(mpl::Level::warning, category,
+                         fmt::format(R"(Removing mount "{}" => "{}" from '{}': {})", vm_mount.source_path, target, name,
+                                     e.what()));
+                mounts_to_remove.push_back(target);
+            }
+    }
+
+    for (const auto& mount_target : mounts_to_remove)
+        vm_spec_mounts.erase(mount_target);
+
+    if (!mounts_to_remove.empty())
+        persist_instances();
 }
 
 multipass::MountHandler::UPtr multipass::Daemon::make_mount(VirtualMachine* vm, const std::string& target,

diff --git a/src/platform/backends/qemu/qemu_mount_handler.cpp b/src/platform/backends/qemu/qemu_mount_handler.cpp
@@ -36,10 +36,19 @@ QemuMountHandler::QemuMountHandler(QemuVirtualMachine* vm, const SSHKeyProvider*
     : MountHandler{vm, ssh_key_provider, target, mount},
       vm_mount_args{vm->modifiable_mount_args()},
       // Create a reproducible unique mount tag for each mount. The cmd arg can only be 31 bytes long so part of the
-      // uuid must be truncated. First character of tag must also be alpabetical.
-      tag{mp::utils::make_uuid().remove("-").left(30).prepend('m').toStdString()}
+      // uuid must be truncated. First character of tag must also be alphabetical.
+      tag{mp::utils::make_uuid(target).remove("-").left(30).prepend('m').toStdString()}
 {
-    if (vm->current_state() != VirtualMachine::State::off && vm->current_state() != VirtualMachine::State::stopped)
+    auto state = vm->current_state();
+    if (state == VirtualMachine::State::suspended && vm_mount_args.find(tag) != vm_mount_args.end())
+    {
+        mpl::log(
+            mpl::Level::info, category,
+            fmt::format("Found native mount {} => {} in '{}' while suspended", mount.source_path, target, vm->vm_name));
+        return;
+    }
+
+    if (state != VirtualMachine::State::off && state != VirtualMachine::State::stopped)
         throw std::runtime_error("Please shutdown the instance before attempting native mounts.");
 
     // Need to ensure no more than one uid/gid map is passed in here.
@@ -64,17 +73,16 @@ void QemuMountHandler::start_impl(ServerVariant, std::chrono::milliseconds)
     SSHSession session{vm->ssh_hostname(), vm->ssh_port(), vm->ssh_username(), *ssh_key_provider};
 
     // Split the path in existing and missing parts
-    const auto& [leading, missing] = mpu::get_path_split(session, target);
-    const auto default_uid = std::stoi(mpu::run_in_ssh_session(session, "id -u"));
-    mpl::log(mpl::Level::debug, category,
-             fmt::format("{}:{} {}(): `id -u` = {}", __FILE__, __LINE__, __FUNCTION__, default_uid));
-    const auto default_gid = std::stoi(mpu::run_in_ssh_session(session, "id -g"));
-    mpl::log(mpl::Level::debug, category,
-             fmt::format("{}:{} {}(): `id -g` = {}", __FILE__, __LINE__, __FUNCTION__, default_gid));
-
     // We need to create the part of the path which does not still exist, and set then the correct ownership.
-    if (missing != ".")
+    if (const auto& [leading, missing] = mpu::get_path_split(session, target); missing != ".")
     {
+        const auto default_uid = std::stoi(mpu::run_in_ssh_session(session, "id -u"));
+        mpl::log(mpl::Level::debug, category,
+                 fmt::format("{}:{} {}(): `id -u` = {}", __FILE__, __LINE__, __FUNCTION__, default_uid));
+        const auto default_gid = std::stoi(mpu::run_in_ssh_session(session, "id -g"));
+        mpl::log(mpl::Level::debug, category,
+                 fmt::format("{}:{} {}(): `id -g` = {}", __FILE__, __LINE__, __FUNCTION__, default_gid));
+
         mpu::make_target_dir(session, leading, missing);
         mpu::set_owner_for(session, leading, missing, default_uid, default_gid);
     }

diff --git a/src/platform/backends/qemu/qemu_virtual_machine.cpp b/src/platform/backends/qemu/qemu_virtual_machine.cpp
@@ -40,7 +40,6 @@
 #include <QString>
 #include <QStringList>
 #include <QTemporaryFile>
-#include <QUuid>
 
 #include <cassert>
 
@@ -54,6 +53,9 @@ namespace
 constexpr auto suspend_tag = "suspend";
 constexpr auto machine_type_key = "machine_type";
 constexpr auto arguments_key = "arguments";
+constexpr auto mount_data_key = "mount_data";
+constexpr auto mount_source_key = "source";
+constexpr auto mount_arguments_key = "arguments";
 
 bool use_cdrom_set(const QJsonObject& metadata)
 {
@@ -84,6 +86,23 @@ QStringList get_arguments(const QJsonObject& metadata)
     return args;
 }
 
+auto mount_args_from_json(const QJsonObject& object)
+{
+    mp::QemuVirtualMachine::MountArgs mount_args;
+    auto mount_data_map = object[mount_data_key].toObject();
+    for (const auto& tag : mount_data_map.keys())
+    {
+        const auto mount_data = mount_data_map[tag].toObject();
+        const auto source = mount_data[mount_source_key];
+        const auto args = mount_data[mount_arguments_key].toArray();
+        if (!source.isString() || !std::all_of(args.begin(), args.end(), std::mem_fn(&QJsonValue::isString)))
+            continue;
+        mount_args[tag.toStdString()] = {source.toString().toStdString(),
+                                         QVariant{args.toVariantList()}.toStringList()};
+    }
+    return mount_args;
+}
+
 auto make_qemu_process(const mp::VirtualMachineDescription& desc, const std::optional<QJsonObject>& resume_metadata,
                        const mp::QemuVirtualMachine::MountArgs& mount_args, const QStringList& platform_args)
 {
@@ -177,11 +196,25 @@ auto get_qemu_machine_type(const QStringList& platform_args)
     return machine_type;
 }
 
-auto generate_metadata(const QStringList& platform_args, const QStringList& proc_args)
+auto mount_args_to_json(const mp::QemuVirtualMachine::MountArgs& mount_args)
+{
+    QJsonObject object;
+    for (const auto& [tag, mount_data] : mount_args)
+    {
+        const auto& [source, args] = mount_data;
+        object[QString::fromStdString(tag)] = QJsonObject{{mount_source_key, QString::fromStdString(source)},
+                                                          {mount_arguments_key, QJsonArray::fromStringList(args)}};
+    }
+    return object;
+}
+
+auto generate_metadata(const QStringList& platform_args, const QStringList& proc_args,
+                       const mp::QemuVirtualMachine::MountArgs& mount_args)
 {
     QJsonObject metadata;
     metadata[machine_type_key] = get_qemu_machine_type(platform_args);
     metadata[arguments_key] = QJsonArray::fromStringList(proc_args);
+    metadata[mount_data_key] = mount_args_to_json(mount_args);
     return metadata;
 }
 } // namespace
@@ -194,7 +227,8 @@ mp::QemuVirtualMachine::QemuVirtualMachine(const VirtualMachineDescription& desc
       mac_addr{desc.default_mac_address},
       username{desc.ssh_username},
       qemu_platform{qemu_platform},
-      monitor{&monitor}
+      monitor{&monitor},
+      mount_args{mount_args_from_json(monitor.retrieve_metadata_for(vm_name))}
 {
     QObject::connect(
         this, &QemuVirtualMachine::on_delete_memory_snapshot, this,
@@ -259,8 +293,14 @@ void mp::QemuVirtualMachine::start()
     }
     else
     {
-        monitor->update_metadata_for(
-            vm_name, generate_metadata(qemu_platform->vmstate_platform_args(), vm_process->arguments()));
+        // remove the mount arguments from the rest of the arguments, as they are stored separately for easier retrieval
+        auto proc_args = vm_process->arguments();
+        for (const auto& [_, mount_data] : mount_args)
+            for (const auto& arg : mount_data.second)
+                proc_args.removeOne(arg);
+
+        monitor->update_metadata_for(vm_name,
+                                     generate_metadata(qemu_platform->vmstate_platform_args(), proc_args, mount_args));
     }
 
     vm_process->start();

diff --git a/src/platform/backends/qemu/qemu_virtual_machine.h b/src/platform/backends/qemu/qemu_virtual_machine.h
@@ -78,11 +78,11 @@ class QemuVirtualMachine final : public QObject, public BaseVirtualMachine
 
     VirtualMachineDescription desc;
     std::unique_ptr<Process> vm_process{nullptr};
-    MountArgs mount_args;
     const std::string mac_addr;
     const std::string username;
     QemuPlatform* qemu_platform;
     VMStatusMonitor* monitor;
+    MountArgs mount_args;
     std::string saved_error_msg;
     bool update_shutdown_status{true};
     bool is_starting_from_suspend{false};

diff --git a/src/platform/backends/qemu/qemu_vm_process_spec.cpp b/src/platform/backends/qemu/qemu_vm_process_spec.cpp
@@ -89,12 +89,12 @@ QStringList mp::QemuVMProcessSpec::arguments() const
              << "-nographic";
         // Cloud-init disk
         args << "-cdrom" << desc.cloud_init_iso;
+    }
 
-        for (const auto& [_, mount_data] : mount_args)
-        {
-            const auto& [__, mount_args] = mount_data;
-            args << mount_args;
-        }
+    for (const auto& [_, mount_data] : mount_args)
+    {
+        const auto& [__, mount_args] = mount_data;
+        args << mount_args;
     }
 
     return args;

diff --git a/src/utils/utils.cpp b/src/utils/utils.cpp
@@ -421,9 +421,10 @@ QString mp::utils::get_multipass_storage()
     return QString::fromUtf8(qgetenv(mp::multipass_storage_env_var));
 }
 
-QString mp::utils::make_uuid()
+QString mp::utils::make_uuid(const std::optional<std::string>& seed)
 {
-    return QUuid::createUuid().toString(QUuid::WithoutBraces);
+    auto uuid = seed ? QUuid::createUuidV3(QUuid{}, QString::fromStdString(*seed)) : QUuid::createUuid();
+    return uuid.toString(QUuid::WithoutBraces);
 }
 
 std::string mp::utils::contents_of(const multipass::Path& file_path)

diff --git a/tests/qemu/test_qemu_backend.cpp b/tests/qemu/test_qemu_backend.cpp
@@ -452,7 +452,8 @@ TEST_F(QemuBackend, verify_qemu_arguments_when_resuming_suspend_image_uses_metad
     process_factory->register_callback(handle_external_process_calls);
     NiceMock<mpt::MockVMStatusMonitor> mock_monitor;
 
-    EXPECT_CALL(mock_monitor, retrieve_metadata_for(_)).WillOnce(Return(QJsonObject({{"machine_type", machine_type}})));
+    EXPECT_CALL(mock_monitor, retrieve_metadata_for(_))
+        .WillRepeatedly(Return(QJsonObject({{"machine_type", machine_type}})));
 
     mp::QemuVirtualMachineFactory backend{data_dir.path()};
 
@@ -494,7 +495,7 @@ TEST_F(QemuBackend, verify_qemu_arguments_from_metadata_are_used)
     NiceMock<mpt::MockVMStatusMonitor> mock_monitor;
 
     EXPECT_CALL(mock_monitor, retrieve_metadata_for(_))
-        .WillOnce(Return(QJsonObject({{"arguments", QJsonArray{"-hi_there", "-hows_it_going"}}})));
+        .WillRepeatedly(Return(QJsonObject({{"arguments", QJsonArray{"-hi_there", "-hows_it_going"}}})));
 
     mp::QemuVirtualMachineFactory backend{data_dir.path()};
 

diff --git a/tests/qemu/test_qemu_vm_process_spec.cpp b/tests/qemu/test_qemu_vm_process_spec.cpp
@@ -88,7 +88,8 @@ TEST_F(TestQemuVMProcessSpec, resume_arguments_taken_from_resumedata)
 
     mp::QemuVMProcessSpec spec(desc, platform_args, mount_args, resume_data);
 
-    EXPECT_EQ(spec.arguments(), QStringList({"-one", "-two", "-loadvm", "suspend_tag", "-machine", "machine_type"}));
+    EXPECT_EQ(spec.arguments(), QStringList({"-one", "-two", "-loadvm", "suspend_tag", "-machine", "machine_type"})
+                                    << mount_args.begin()->second.second);
 }
 
 TEST_F(TestQemuVMProcessSpec, resume_with_missing_machine_type_guesses_correctly)
@@ -99,7 +100,7 @@ TEST_F(TestQemuVMProcessSpec, resume_with_missing_machine_type_guesses_correctly
 
     mp::QemuVMProcessSpec spec(desc, platform_args, mount_args, resume_data_missing_machine_info);
 
-    EXPECT_EQ(spec.arguments(), QStringList({"-args", "-loadvm", "suspend_tag"}));
+    EXPECT_EQ(spec.arguments(), QStringList({"-args", "-loadvm", "suspend_tag"}) << mount_args.begin()->second.second);
 }
 
 TEST_F(TestQemuVMProcessSpec, ResumeFixesVmnetFormat)
@@ -109,8 +110,8 @@ TEST_F(TestQemuVMProcessSpec, ResumeFixesVmnetFormat)
 
     mp::QemuVMProcessSpec spec(desc, platform_args, mount_args, resume_data);
 
-    EXPECT_EQ(spec.arguments(),
-              QStringList({"vmnet-shared,foo", "-loadvm", "suspend_tag", "-machine", "machine_type"}));
+    EXPECT_EQ(spec.arguments(), QStringList({"vmnet-shared,foo", "-loadvm", "suspend_tag", "-machine", "machine_type"})
+                                    << mount_args.begin()->second.second);
 }
 
 TEST_F(TestQemuVMProcessSpec, apparmorProfileIncludesFileMountPerms)