Merge #3329

Complain on snapshot operations if unsupported

a=ricab r=townsend2010

include/multipass/virtual_machine.h

@@ -89,7 +89,7 @@ class VirtualMachine : private DisabledCopyMove
 
     using SnapshotVista = std::vector<std::shared_ptr<const Snapshot>>; // using vista to avoid confusion with C++ views
     virtual SnapshotVista view_snapshots() const = 0;
-    virtual int get_num_snapshots() const noexcept = 0;
+    virtual int get_num_snapshots() const = 0;
 
     virtual std::shared_ptr<const Snapshot> get_snapshot(const std::string& name) const = 0;
     virtual std::shared_ptr<const Snapshot> get_snapshot(int index) const = 0;

include/multipass/virtual_machine_factory.h

@@ -69,6 +69,7 @@ class VirtualMachineFactory : private DisabledCopyMove
 
     // List all the network interfaces seen by the backend.
     virtual std::vector<NetworkInterfaceInfo> networks() const = 0;
+    virtual void require_snapshots_support() const = 0;
 
 protected:
     VirtualMachineFactory() = default;

src/client/cli/formatter/csv_formatter.cpp

@@ -96,17 +96,21 @@ std::string generate_snapshot_details(const mp::InfoReply reply)
 
 std::string generate_instance_details(const mp::InfoReply reply)
 {
-    fmt::memory_buffer buf;
+    assert(reply.details_size() && "shouldn't call this if there are not entries");
+    assert(reply.details(0).has_instance_info() &&
+           "outputting instance and snapshot details together is not supported in csv format");
+
+    auto have_num_snapshots = reply.details(0).instance_info().has_num_snapshots();
 
+    fmt::memory_buffer buf;
     fmt::format_to(
         std::back_inserter(buf),
         "Name,State,Ipv4,Ipv6,Release,Image hash,Image release,Load,Disk usage,Disk total,Memory usage,Memory "
-        "total,Mounts,AllIPv4,CPU(s),Snapshots\n");
+        "total,Mounts,AllIPv4,CPU(s){}\n",
+        have_num_snapshots ? ",Snapshots" : "");
 
     for (const auto& info : mp::format::sorted(reply.details()))
     {
-        assert(info.has_instance_info() &&
-               "outputting instance and snapshot details together is not supported in csv format");
         const auto& instance_details = info.instance_info();
 
         fmt::format_to(std::back_inserter(buf),
@@ -126,11 +130,11 @@ std::string generate_instance_details(const mp::InfoReply reply)
 
         fmt::format_to(std::back_inserter(buf), format_mounts(info.mount_info()));
 
+        fmt::format_to(std::back_inserter(buf), ",{},{}", fmt::join(instance_details.ipv4(), ";"), info.cpu_count());
+
         fmt::format_to(std::back_inserter(buf),
-                       ",{},{},{}\n",
-                       fmt::join(instance_details.ipv4(), ";"),
-                       info.cpu_count(),
-                       instance_details.num_snapshots());
+                       "{}\n",
+                       have_num_snapshots ? fmt::format(",{}", instance_details.num_snapshots()) : "");
     }
 
     return fmt::to_string(buf);

src/client/cli/formatter/json_formatter.cpp

@@ -101,7 +101,9 @@ QJsonObject generate_instance_details(const mp::DetailedInfoItem& item)
     instance_info.insert("image_release", QString::fromStdString(instance_details.image_release()));
     instance_info.insert("release", QString::fromStdString(instance_details.current_release()));
     instance_info.insert("cpu_count", QString::fromStdString(item.cpu_count()));
-    instance_info.insert("snapshot_count", QString::number(instance_details.num_snapshots()));
+
+    if (instance_details.has_num_snapshots())
+        instance_info.insert("snapshot_count", QString::number(instance_details.num_snapshots()));
 
     QJsonArray load;
     if (!instance_details.load().empty())

src/client/cli/formatter/table_formatter.cpp

@@ -125,7 +125,9 @@ std::string generate_instance_details(const mp::DetailedInfoItem& item)
                    "{:<16}{}\n",
                    "State:",
                    mp::format::status_string_for(item.instance_status()));
-    fmt::format_to(std::back_inserter(buf), "{:<16}{}\n", "Snapshots:", instance_details.num_snapshots());
+
+    if (instance_details.has_num_snapshots())
+        fmt::format_to(std::back_inserter(buf), "{:<16}{}\n", "Snapshots:", instance_details.num_snapshots());
 
     int ipv4_size = instance_details.ipv4_size();
     fmt::format_to(std::back_inserter(buf), "{:<16}{}\n", "IPv4:", ipv4_size ? instance_details.ipv4(0) : "--");

src/client/cli/formatter/yaml_formatter.cpp

@@ -96,7 +96,10 @@ YAML::Node generate_instance_details(const mp::DetailedInfoItem& item)
     YAML::Node instance_node;
 
     instance_node["state"] = mp::format::status_string_for(item.instance_status());
-    instance_node["snapshot_count"] = instance_details.num_snapshots();
+
+    if (instance_details.has_num_snapshots())
+        instance_node["snapshot_count"] = instance_details.num_snapshots();
+
     instance_node["image_hash"] = instance_details.id();
     instance_node["image_release"] = instance_details.image_release();
     instance_node["release"] =

src/daemon/daemon.cpp

@@ -1209,10 +1209,21 @@ mp::SettingsHandler* register_instance_mod(std::unordered_map<std::string, mp::V
 mp::SettingsHandler* register_snapshot_mod(
     std::unordered_map<std::string, mp::VirtualMachine::ShPtr>& operative_instances,
     const std::unordered_map<std::string, mp::VirtualMachine::ShPtr>& deleted_instances,
-    const std::unordered_set<std::string>& preparing_instances)
+    const std::unordered_set<std::string>& preparing_instances,
+    const mp::VirtualMachineFactory& vm_factory)
 {
-    return MP_SETTINGS.register_handler(
-        std::make_unique<mp::SnapshotSettingsHandler>(operative_instances, deleted_instances, preparing_instances));
+    try
+    {
+        vm_factory.require_snapshots_support();
+        return MP_SETTINGS.register_handler(
+            std::make_unique<mp::SnapshotSettingsHandler>(operative_instances, deleted_instances, preparing_instances));
+    }
+    catch (const mp::NotImplementedOnThisBackendException& e)
+    {
+        assert(std::string{e.what()}.find("snapshots") != std::string::npos);
+    }
+
+    return nullptr;
 }
 
 // Erase any outdated mount handlers for a given VM
@@ -1330,7 +1341,8 @@ mp::Daemon::Daemon(std::unique_ptr<const DaemonConfig> the_config)
                                                  deleted_instances,
                                                  preparing_instances,
                                                  [this] { persist_instances(); })},
-      snapshot_mod_handler{register_snapshot_mod(operative_instances, deleted_instances, preparing_instances)}
+      snapshot_mod_handler{
+          register_snapshot_mod(operative_instances, deleted_instances, preparing_instances, *config->factory)}
 {
     connect_rpc(daemon_rpc, *this);
     std::vector<std::string> invalid_specs;
@@ -1702,6 +1714,9 @@ try // clang-format on
     bool snapshots_only = request->snapshots();
     response.set_snapshots(snapshots_only);
 
+    if (snapshots_only)
+        config->factory->require_snapshots_support();
+
     auto process_snapshot_pick = [&response, &have_mounts, snapshots_only](VirtualMachine& vm,
                                                                            const SnapshotPick& snapshot_pick) {
         for (const auto& snapshot_name : snapshot_pick.pick)
@@ -1785,7 +1800,14 @@ try // clang-format on
     config->update_prompt->populate_if_time_to_show(response.mutable_update_info());
 
     // Need to 'touch' a report in the response so formatters know what to do with an otherwise empty response
-    request->snapshots() ? (void)response.mutable_snapshot_list() : (void)response.mutable_instance_list();
+    if (request->snapshots())
+    {
+        config->factory->require_snapshots_support();
+        response.mutable_snapshot_list();
+    }
+    else
+        response.mutable_instance_list();
+
     bool deleted = false;
 
     auto fetch_instance = [this, request, &response, &deleted](VirtualMachine& vm) {
@@ -2499,6 +2521,7 @@ try
                                                              *config->logger,
                                                              server};
 
+    config->factory->require_snapshots_support();
     const auto& instance_name = request->instance();
     auto [instance_trail, status] = find_instance_and_react(operative_instances,
                                                             deleted_instances,
@@ -2563,6 +2586,9 @@ try
         auto* vm_ptr = std::get<0>(instance_trail)->second.get();
         assert(vm_ptr);
 
+        // Only need to check if snapshots are supported and if the snapshot exists, so the result is discarded
+        vm_ptr->get_snapshot(request->snapshot());
+
         using St = VirtualMachine::State;
         if (auto state = vm_ptr->current_state(); state != St::off && state != St::stopped)
             return status_promise->set_value(
@@ -2572,9 +2598,6 @@ try
         assert(spec_it != vm_instance_specs.end() && "missing instance specs");
         auto& vm_specs = spec_it->second;
 
-        // Only need to check if the snapshot exists so the result is discarded
-        vm_ptr->get_snapshot(request->snapshot());
-
         if (!request->destructive())
         {
             RestoreReply confirm_action{};
@@ -3441,7 +3464,14 @@ void mp::Daemon::populate_instance_info(VirtualMachine& vm,
         }
     }
 
-    instance_info->set_num_snapshots(vm.get_num_snapshots());
+    try
+    {
+        instance_info->set_num_snapshots(vm.get_num_snapshots());
+    }
+    catch (const NotImplementedOnThisBackendException& e)
+    {
+        assert(std::string{e.what()}.find("snapshots") != std::string::npos); // TODO per-feature exception type instead
+    }
     instance_info->set_image_release(original_release);
     instance_info->set_id(vm_image.id);
 

src/platform/backends/qemu/qemu_virtual_machine.h

@@ -77,6 +77,7 @@ class QemuVirtualMachine : public QObject, public BaseVirtualMachine
     {
     }
 
+    void require_snapshots_support() const override;
     std::shared_ptr<Snapshot> make_specific_snapshot(const QString& filename) override;
     std::shared_ptr<Snapshot> make_specific_snapshot(const std::string& snapshot_name,
                                                      const std::string& comment,
@@ -105,4 +106,8 @@ class QemuVirtualMachine : public QObject, public BaseVirtualMachine
 };
 } // namespace multipass
 
+inline void multipass::QemuVirtualMachine::require_snapshots_support() const
+{
+}
+
 #endif // MULTIPASS_QEMU_VIRTUAL_MACHINE_H

src/platform/backends/qemu/qemu_virtual_machine_factory.h

@@ -41,6 +41,7 @@ class QemuVirtualMachineFactory final : public BaseVirtualMachineFactory
     QString get_backend_version_string() const override;
     QString get_backend_directory_name() const override;
     std::vector<NetworkInterfaceInfo> networks() const override;
+    void require_snapshots_support() const override;
 
 protected:
     void remove_resources_for_impl(const std::string& name) override;
@@ -52,4 +53,8 @@ class QemuVirtualMachineFactory final : public BaseVirtualMachineFactory
 };
 } // namespace multipass
 
+inline void multipass::QemuVirtualMachineFactory::require_snapshots_support() const
+{
+}
+
 #endif // MULTIPASS_QEMU_VIRTUAL_MACHINE_FACTORY_H

src/platform/backends/shared/base_virtual_machine.cpp

@@ -116,6 +116,7 @@ std::vector<std::string> BaseVirtualMachine::get_all_ipv4(const SSHKeyProvider&
 
 auto BaseVirtualMachine::view_snapshots() const -> SnapshotVista
 {
+    require_snapshots_support();
     SnapshotVista ret;
 
     const std::unique_lock lock{snapshot_mutex};
@@ -129,6 +130,7 @@ auto BaseVirtualMachine::view_snapshots() const -> SnapshotVista
 
 std::shared_ptr<const Snapshot> BaseVirtualMachine::get_snapshot(const std::string& name) const
 {
+    require_snapshots_support();
     const std::unique_lock lock{snapshot_mutex};
     try
     {
@@ -142,6 +144,7 @@ std::shared_ptr<const Snapshot> BaseVirtualMachine::get_snapshot(const std::stri
 
 std::shared_ptr<const Snapshot> BaseVirtualMachine::get_snapshot(int index) const
 {
+    require_snapshots_support();
     const std::unique_lock lock{snapshot_mutex};
 
     auto index_matcher = [index](const auto& elem) { return elem.second->get_index() == index; };
@@ -195,6 +198,8 @@ std::shared_ptr<const Snapshot> BaseVirtualMachine::take_snapshot(const VMSpecs&
                                                                   const std::string& snapshot_name,
                                                                   const std::string& comment)
 {
+    require_snapshots_support();
+
     std::unique_lock lock{snapshot_mutex};
     assert_vm_stopped(state); // precondition
 
@@ -366,6 +371,7 @@ void BaseVirtualMachine::load_snapshots()
 
 std::vector<std::string> BaseVirtualMachine::get_childrens_names(const Snapshot* parent) const
 {
+    require_snapshots_support();
     std::vector<std::string> children;
 
     for (const auto& snapshot : view_snapshots())
@@ -512,10 +518,11 @@ void BaseVirtualMachine::restore_rollback_helper(const Path& head_path,
 void BaseVirtualMachine::restore_snapshot(const std::string& name, VMSpecs& specs)
 {
     const std::unique_lock lock{snapshot_mutex};
-    assert_vm_stopped(state); // precondition
 
     auto snapshot = get_snapshot(name);
-    assert(snapshot->get_state() == St::off || snapshot->get_state() == St::stopped);
+
+    assert_vm_stopped(state);                 // precondition
+    assert_vm_stopped(snapshot->get_state()); // precondition
 
     snapshot->apply();
 

src/platform/backends/shared/base_virtual_machine.h

@@ -51,7 +51,7 @@ class BaseVirtualMachine : public VirtualMachine
     };
 
     SnapshotVista view_snapshots() const override;
-    int get_num_snapshots() const noexcept override;
+    int get_num_snapshots() const override;
 
     std::shared_ptr<const Snapshot> get_snapshot(const std::string& name) const override;
     std::shared_ptr<const Snapshot> get_snapshot(int index) const override;
@@ -71,6 +71,7 @@ class BaseVirtualMachine : public VirtualMachine
     int get_snapshot_count() const override;
 
 protected:
+    virtual void require_snapshots_support() const;
     virtual std::shared_ptr<Snapshot> make_specific_snapshot(const QString& filename);
     virtual std::shared_ptr<Snapshot> make_specific_snapshot(const std::string& snapshot_name,
                                                              const std::string& comment,
@@ -129,15 +130,22 @@ class BaseVirtualMachine : public VirtualMachine
 
 } // namespace multipass
 
-inline int multipass::BaseVirtualMachine::get_num_snapshots() const noexcept
+inline int multipass::BaseVirtualMachine::get_num_snapshots() const
 {
+    require_snapshots_support();
     return static_cast<int>(snapshots.size());
 }
 
 inline int multipass::BaseVirtualMachine::get_snapshot_count() const
 {
+    require_snapshots_support();
     const std::unique_lock lock{snapshot_mutex};
     return snapshot_count;
 }
 
+inline void multipass::BaseVirtualMachine::require_snapshots_support() const
+{
+    throw NotImplementedOnThisBackendException{"snapshots"};
+}
+
 #endif // MULTIPASS_BASE_VIRTUAL_MACHINE_H

src/platform/backends/shared/base_virtual_machine_factory.h

@@ -73,6 +73,8 @@ class BaseVirtualMachineFactory : public VirtualMachineFactory
         throw NotImplementedOnThisBackendException("networks");
     };
 
+    void require_snapshots_support() const override;
+
 protected:
     static const Path instances_subdir;
 
@@ -102,4 +104,9 @@ inline void multipass::BaseVirtualMachineFactory::remove_resources_for(const std
     instance_dir.removeRecursively();
 }
 
+inline void multipass::BaseVirtualMachineFactory::require_snapshots_support() const
+{
+    throw NotImplementedOnThisBackendException{"snapshots"};
+}
+
 #endif // MULTIPASS_BASE_VIRTUAL_MACHINE_FACTORY_H

src/rpc/multipass.proto

@@ -216,7 +216,7 @@ message InstanceDetails {
     string disk_usage = 6;
     repeated string ipv4 = 7;
     repeated string ipv6 = 8;
-    int32 num_snapshots = 9;
+    optional int32 num_snapshots = 9;
 }
 
 message SnapshotFundamentals {

tests/mock_virtual_machine.h

@@ -76,7 +76,7 @@ struct MockVirtualMachineT : public T
                 (const SSHKeyProvider*, const std::string&, const VMMount&),
                 (override));
     MOCK_METHOD(VirtualMachine::SnapshotVista, view_snapshots, (), (const, override, noexcept));
-    MOCK_METHOD(int, get_num_snapshots, (), (const, override, noexcept));
+    MOCK_METHOD(int, get_num_snapshots, (), (const, override));
     MOCK_METHOD(std::shared_ptr<const Snapshot>, get_snapshot, (const std::string&), (const, override));
     MOCK_METHOD(std::shared_ptr<const Snapshot>, get_snapshot, (int index), (const, override));
     MOCK_METHOD(std::shared_ptr<Snapshot>, get_snapshot, (const std::string&), (override));

tests/mock_virtual_machine_factory.h

@@ -47,6 +47,7 @@ struct MockVirtualMachineFactory : public VirtualMachineFactory
                 (std::vector<VMImageHost*>, URLDownloader*, const Path&, const Path&, const days&), (override));
     MOCK_METHOD(void, configure, (VirtualMachineDescription&), (override));
     MOCK_METHOD(std::vector<NetworkInterfaceInfo>, networks, (), (const, override));
+    MOCK_METHOD(void, require_snapshots_support, (), (const, override));
 
     // originally protected:
     MOCK_METHOD(std::string, create_bridge_with, (const NetworkInterfaceInfo&), (override));

tests/stub_virtual_machine.h

@@ -131,7 +131,7 @@ struct StubVirtualMachine final : public multipass::VirtualMachine
         return {};
     }
 
-    int get_num_snapshots() const noexcept override
+    int get_num_snapshots() const override
     {
         return 0;
     }