Merge pull request #3265 from canonical/snapshots-todos

[snapshots] Address pending TODOs

r=sharder996 a=ricab

include/multipass/mount_handler.h

@@ -67,7 +67,7 @@ class MountHandler : private DisabledCopyMove
         active = false;
     }
 
-    const VMMount& get_mount_spec() const
+    const VMMount& get_mount_spec() const noexcept
     {
         return mount_spec;
     }

include/multipass/snapshot.h

@@ -35,13 +35,13 @@ struct VMMount;
 
 class Snapshot : private DisabledCopyMove
 {
-public: // TODO@snapshots drop any accessors that we end up not needing
+public:
     virtual ~Snapshot() = default;
 
-    virtual int get_index() const = 0;
+    virtual int get_index() const noexcept = 0;
     virtual std::string get_name() const = 0;
     virtual std::string get_comment() const = 0;
-    virtual QDateTime get_creation_timestamp() const = 0;
+    virtual QDateTime get_creation_timestamp() const noexcept = 0;
     virtual int get_num_cores() const noexcept = 0;
     virtual MemorySize get_mem_size() const noexcept = 0;
     virtual MemorySize get_disk_space() const noexcept = 0;

include/multipass/virtual_machine.h

@@ -88,7 +88,7 @@ class VirtualMachine : private DisabledCopyMove
                                                                     const VMMount& mount) = 0;
 
     using SnapshotVista = std::vector<std::shared_ptr<const Snapshot>>; // using vista to avoid confusion with C++ views
-    virtual SnapshotVista view_snapshots() const noexcept = 0;
+    virtual SnapshotVista view_snapshots() const = 0;
     virtual int get_num_snapshots() const noexcept = 0;
 
     virtual std::shared_ptr<const Snapshot> get_snapshot(const std::string& name) const = 0;
@@ -99,7 +99,8 @@ class VirtualMachine : private DisabledCopyMove
     virtual std::shared_ptr<const Snapshot> take_snapshot(const VMSpecs& specs,
                                                           const std::string& snapshot_name,
                                                           const std::string& comment) = 0;
-    virtual void rename_snapshot(const std::string& old_name, const std::string& new_name) = 0; // TODO@snapshots remove
+    virtual void rename_snapshot(const std::string& old_name,
+                                 const std::string& new_name) = 0; // only VM can avoid repeated names
     virtual void delete_snapshot(const std::string& name) = 0;
     virtual void restore_snapshot(const std::string& name, VMSpecs& specs) = 0;
     virtual void load_snapshots() = 0;

include/multipass/vm_mount.h

@@ -20,6 +20,8 @@
 
 #include <multipass/id_mappings.h>
 
+#include <QJsonObject>
+
 #include <string>
 
 namespace multipass
@@ -32,6 +34,12 @@ struct VMMount
         Native = 1
     };
 
+    VMMount() = default;
+    VMMount(const QJsonObject& json);
+    VMMount(const std::string& sourcePath, id_mappings gidMappings, id_mappings uidMappings, MountType mountType);
+
+    QJsonObject serialize() const;
+
     std::string source_path;
     id_mappings gid_mappings;
     id_mappings uid_mappings;

src/daemon/vm_specs.h → include/multipass/vm_specs.h

@@ -18,10 +18,10 @@
 #ifndef MULTIPASS_VM_SPECS_H
 #define MULTIPASS_VM_SPECS_H
 
-#include <multipass/memory_size.h>
-#include <multipass/network_interface.h>
-#include <multipass/virtual_machine.h>
-#include <multipass/vm_mount.h>
+#include "memory_size.h"
+#include "network_interface.h"
+#include "virtual_machine.h"
+#include "vm_mount.h"
 
 #include <string>
 #include <tuple>
@@ -48,10 +48,25 @@ struct VMSpecs
 
 inline bool operator==(const VMSpecs& a, const VMSpecs& b)
 {
-    return std::tie(a.num_cores, a.mem_size, a.disk_space, a.default_mac_address, a.extra_interfaces, a.ssh_username,
-                    a.state, a.mounts, a.deleted, a.metadata) ==
-           std::tie(b.num_cores, b.mem_size, b.disk_space, b.default_mac_address, b.extra_interfaces, b.ssh_username,
-                    b.state, b.mounts, b.deleted, b.metadata);
+    return std::tie(a.num_cores,
+                    a.mem_size,
+                    a.disk_space,
+                    a.default_mac_address,
+                    a.extra_interfaces,
+                    a.ssh_username,
+                    a.state,
+                    a.mounts,
+                    a.deleted,
+                    a.metadata) == std::tie(b.num_cores,
+                                            b.mem_size,
+                                            b.disk_space,
+                                            b.default_mac_address,
+                                            b.extra_interfaces,
+                                            b.ssh_username,
+                                            b.state,
+                                            b.mounts,
+                                            b.deleted,
+                                            b.metadata);
 }
 
 inline bool operator!=(const VMSpecs& a, const VMSpecs& b) // TODO drop in C++20

src/client/cli/formatter/table_formatter.cpp

@@ -103,7 +103,8 @@ std::string generate_snapshot_details(const mp::DetailedInfoItem& item)
         fmt::format_to(std::back_inserter(buf), "{}\n", *child);
     }
 
-    // TODO@snapshots split and align string if it extends onto several lines
+    /* TODO split and align string if it extends onto several lines; but actually better implement generic word-wrapping
+       for all output, taking both terminal width and current indentation level into account */
     fmt::format_to(std::back_inserter(buf),
                    "{:<16}{}\n",
                    "Comment:",

src/daemon/daemon.cpp

@@ -325,30 +325,8 @@ std::unordered_map<std::string, mp::VMSpecs> load_db(const mp::Path& data_path,
 
         for (QJsonValueRef entry : record["mounts"].toArray())
         {
-            mp::id_mappings uid_mappings;
-            mp::id_mappings gid_mappings;
-
-            auto target_path = entry.toObject()["target_path"].toString().toStdString();
-            auto source_path = entry.toObject()["source_path"].toString().toStdString();
-
-            for (QJsonValueRef uid_entry : entry.toObject()["uid_mappings"].toArray())
-            {
-                uid_mappings.push_back(
-                    {uid_entry.toObject()["host_uid"].toInt(), uid_entry.toObject()["instance_uid"].toInt()});
-            }
-
-            for (QJsonValueRef gid_entry : entry.toObject()["gid_mappings"].toArray())
-            {
-                gid_mappings.push_back(
-                    {gid_entry.toObject()["host_gid"].toInt(), gid_entry.toObject()["instance_gid"].toInt()});
-            }
-
-            uid_mappings = mp::unique_id_mappings(uid_mappings);
-            gid_mappings = mp::unique_id_mappings(gid_mappings);
-            auto mount_type = mp::VMMount::MountType(entry.toObject()["mount_type"].toInt());
-
-            mp::VMMount mount{source_path, gid_mappings, uid_mappings, mount_type};
-            mounts[target_path] = mount;
+            const auto& json = entry.toObject();
+            mounts[json["target_path"].toString().toStdString()] = mp::VMMount{json};
         }
 
         reconstructed_records[key] = {num_cores,
@@ -400,37 +378,8 @@ QJsonObject vm_spec_to_json(const mp::VMSpecs& specs)
     QJsonArray json_mounts;
     for (const auto& mount : specs.mounts)
     {
-        QJsonObject entry;
-        entry.insert("source_path", QString::fromStdString(mount.second.source_path));
+        auto entry = mount.second.serialize();
         entry.insert("target_path", QString::fromStdString(mount.first));
-
-        QJsonArray uid_mappings;
-
-        for (const auto& map : mount.second.uid_mappings)
-        {
-            QJsonObject map_entry;
-            map_entry.insert("host_uid", map.first);
-            map_entry.insert("instance_uid", map.second);
-
-            uid_mappings.append(map_entry);
-        }
-
-        entry.insert("uid_mappings", uid_mappings);
-
-        QJsonArray gid_mappings;
-
-        for (const auto& map : mount.second.gid_mappings)
-        {
-            QJsonObject map_entry;
-            map_entry.insert("host_gid", map.first);
-            map_entry.insert("instance_gid", map.second);
-
-            gid_mappings.append(map_entry);
-        }
-
-        entry.insert("gid_mappings", gid_mappings);
-
-        entry.insert("mount_type", static_cast<int>(mount.second.mount_type));
         json_mounts.append(entry);
     }
 

src/daemon/daemon.h

@@ -20,13 +20,12 @@
 
 #include "daemon_config.h"
 #include "daemon_rpc.h"
-#include "multipass/virtual_machine.h"
-#include "vm_specs.h"
 
 #include <multipass/async_periodic_task.h>
 #include <multipass/delayed_shutdown_timer.h>
 #include <multipass/mount_handler.h>
 #include <multipass/virtual_machine.h>
+#include <multipass/vm_specs.h>
 #include <multipass/vm_status_monitor.h>
 
 #include <chrono>

src/daemon/instance_settings_handler.h

@@ -18,11 +18,10 @@
 #ifndef MULTIPASS_INSTANCE_SETTINGS_HANDLER_H
 #define MULTIPASS_INSTANCE_SETTINGS_HANDLER_H
 
-#include "vm_specs.h"
-
 #include <multipass/exceptions/settings_exceptions.h>
 #include <multipass/settings/settings_handler.h>
 #include <multipass/virtual_machine.h>
+#include <multipass/vm_specs.h>
 
 #include <QString>
 

src/daemon/snapshot_settings_handler.cpp

@@ -80,7 +80,7 @@ mp::SnapshotSettingsException::SnapshotSettingsException(const std::string& deta
 mp::SnapshotSettingsHandler::SnapshotSettingsHandler(
     std::unordered_map<std::string, VirtualMachine::ShPtr>& operative_instances,
     const std::unordered_map<std::string, VirtualMachine::ShPtr>& deleted_instances,
-    const std::unordered_set<std::string>& preparing_instances)
+    const std::unordered_set<std::string>& preparing_instances) noexcept
     : operative_instances{operative_instances},
       deleted_instances{deleted_instances},
       preparing_instances{preparing_instances}

src/daemon/snapshot_settings_handler.h

@@ -35,7 +35,7 @@ class SnapshotSettingsHandler : public SettingsHandler
 public:
     SnapshotSettingsHandler(std::unordered_map<std::string, VirtualMachine::ShPtr>& operative_instances,
                             const std::unordered_map<std::string, VirtualMachine::ShPtr>& deleted_instances,
-                            const std::unordered_set<std::string>& preparing_instances);
+                            const std::unordered_set<std::string>& preparing_instances) noexcept;
 
     std::set<QString> keys() const override;
     QString get(const QString& key) const override;

src/platform/backends/libvirt/libvirt_virtual_machine.cpp

@@ -552,10 +552,10 @@ auto mp::LibVirtVirtualMachine::make_specific_snapshot(const std::string& /*snap
                                                        std::shared_ptr<Snapshot> /*parent*/)
     -> std::shared_ptr<Snapshot>
 {
-    throw NotImplementedOnThisBackendException{"Snapshots"}; // TODO@snapshots
+    throw NotImplementedOnThisBackendException{"Snapshots"};
 }
 
 auto mp::LibVirtVirtualMachine::make_specific_snapshot(const QString& /*filename*/) -> std::shared_ptr<Snapshot>
 {
-    throw NotImplementedOnThisBackendException{"Snapshots"}; // TODO@snapshots
+    throw NotImplementedOnThisBackendException{"Snapshots"};
 }

src/platform/backends/lxd/lxd_virtual_machine.cpp

@@ -492,10 +492,10 @@ auto mp::LXDVirtualMachine::make_specific_snapshot(const std::string& snapshot_n
                                                    const VMSpecs& specs,
                                                    std::shared_ptr<Snapshot> parent) -> std::shared_ptr<Snapshot>
 {
-    throw NotImplementedOnThisBackendException{"Snapshots"}; // TODO@snapshots
+    throw NotImplementedOnThisBackendException{"Snapshots"};
 }
 
 std::shared_ptr<mp::Snapshot> mp::LXDVirtualMachine::make_specific_snapshot(const QString& /*filename*/)
 {
-    throw NotImplementedOnThisBackendException{"Snapshots"}; // TODO@snapshots
+    throw NotImplementedOnThisBackendException{"Snapshots"};
 }

src/platform/backends/shared/base_snapshot.cpp

@@ -16,16 +16,16 @@
  */
 
 #include "base_snapshot.h"
-#include "daemon/vm_specs.h" // TODO@snapshots move this
+#include "multipass/virtual_machine.h"
 
 #include <multipass/file_ops.h>
-#include <multipass/id_mappings.h> // TODO@snapshots may be able to drop after extracting JSON utilities
 #include <multipass/json_utils.h>
 #include <multipass/vm_mount.h>
+#include <multipass/vm_specs.h>
 
 #include <scope_guard.hpp>
 
-#include <QJsonArray> // TODO@snapshots may be able to drop after extracting JSON utilities
+#include <QJsonArray>
 #include <QString>
 
 #include <QFile>
@@ -68,35 +68,13 @@ QJsonObject read_snapshot_json(const QString& filename)
         return json["snapshot"].toObject();
 }
 
-std::unordered_map<std::string, mp::VMMount> load_mounts(const QJsonArray& json)
+std::unordered_map<std::string, mp::VMMount> load_mounts(const QJsonArray& mounts_json)
 {
     std::unordered_map<std::string, mp::VMMount> mounts;
-    for (const auto& entry : json)
+    for (const auto& entry : mounts_json)
     {
-        mp::id_mappings uid_mappings;
-        mp::id_mappings gid_mappings;
-
-        auto target_path = entry.toObject()["target_path"].toString().toStdString();
-        auto source_path = entry.toObject()["source_path"].toString().toStdString();
-
-        for (const QJsonValueRef uid_entry : entry.toObject()["uid_mappings"].toArray())
-        {
-            uid_mappings.push_back(
-                {uid_entry.toObject()["host_uid"].toInt(), uid_entry.toObject()["instance_uid"].toInt()});
-        }
-
-        for (const QJsonValueRef gid_entry : entry.toObject()["gid_mappings"].toArray())
-        {
-            gid_mappings.push_back(
-                {gid_entry.toObject()["host_gid"].toInt(), gid_entry.toObject()["instance_gid"].toInt()});
-        }
-
-        uid_mappings = mp::unique_id_mappings(uid_mappings);
-        gid_mappings = mp::unique_id_mappings(gid_mappings);
-        auto mount_type = mp::VMMount::MountType(entry.toObject()["mount_type"].toInt());
-
-        mp::VMMount mount{source_path, gid_mappings, uid_mappings, mount_type};
-        mounts[target_path] = std::move(mount);
+        const auto& json = entry.toObject();
+        mounts[json["target_path"].toString().toStdString()] = mp::VMMount{json};
     }
 
     return mounts;
@@ -146,9 +124,13 @@ mp::BaseSnapshot::BaseSnapshot(const std::string& name,    // NOLINT(modernize-p
       captured{captured}
 {
     assert(index > 0 && "snapshot indices need to start at 1");
+    using St = VirtualMachine::State;
+    if (state != St::off && state != St::stopped)
+        throw std::runtime_error{fmt::format("Unsupported VM state in snapshot: {}", static_cast<int>(state))};
+    if (index < 1)
+        throw std::runtime_error{fmt::format("Snapshot index not positive: {}", index)};
     if (index > max_snapshots)
-        throw std::runtime_error{fmt::format("Maximum number of snapshots exceeded: {}", max_snapshots)};
-
+        throw std::runtime_error{fmt::format("Maximum number of snapshots exceeded: {}", index)};
     if (name.empty())
         throw std::runtime_error{"Snapshot names cannot be empty"};
     if (num_cores < 1)
@@ -224,37 +206,8 @@ QJsonObject mp::BaseSnapshot::serialize() const
     QJsonArray json_mounts;
     for (const auto& mount : mounts)
     {
-        QJsonObject entry;
-        entry.insert("source_path", QString::fromStdString(mount.second.source_path));
+        auto entry = mount.second.serialize();
         entry.insert("target_path", QString::fromStdString(mount.first));
-
-        QJsonArray uid_mappings;
-
-        for (const auto& map : mount.second.uid_mappings)
-        {
-            QJsonObject map_entry;
-            map_entry.insert("host_uid", map.first);
-            map_entry.insert("instance_uid", map.second);
-
-            uid_mappings.append(map_entry);
-        }
-
-        entry.insert("uid_mappings", uid_mappings);
-
-        QJsonArray gid_mappings;
-
-        for (const auto& map : mount.second.gid_mappings)
-        {
-            QJsonObject map_entry;
-            map_entry.insert("host_gid", map.first);
-            map_entry.insert("instance_gid", map.second);
-
-            gid_mappings.append(map_entry);
-        }
-
-        entry.insert("gid_mappings", gid_mappings);
-
-        entry.insert("mount_type", static_cast<int>(mount.second.mount_type));
         json_mounts.append(entry);
     }