From ef361f9fb969c07a0930bdbec6a4c2ab49815d8e Mon Sep 17 00:00:00 2001 From: Chris Townsend Date: Thu, 14 Dec 2023 12:46:24 -0500 Subject: [PATCH] Merge #3329 Complain on snapshot operations if unsupported a=ricab r=townsend2010 --- include/multipass/virtual_machine.h | 2 +- include/multipass/virtual_machine_factory.h | 1 + src/client/cli/formatter/csv_formatter.cpp | 20 ++++---- src/client/cli/formatter/json_formatter.cpp | 4 +- src/client/cli/formatter/table_formatter.cpp | 4 +- src/client/cli/formatter/yaml_formatter.cpp | 5 +- src/daemon/daemon.cpp | 48 +++++++++++++++---- .../backends/qemu/qemu_virtual_machine.h | 5 ++ .../qemu/qemu_virtual_machine_factory.h | 5 ++ .../backends/shared/base_virtual_machine.cpp | 11 ++++- .../backends/shared/base_virtual_machine.h | 12 ++++- .../shared/base_virtual_machine_factory.h | 7 +++ src/rpc/multipass.proto | 2 +- tests/mock_virtual_machine.h | 2 +- tests/mock_virtual_machine_factory.h | 1 + tests/stub_virtual_machine.h | 2 +- 16 files changed, 103 insertions(+), 28 deletions(-) diff --git a/include/multipass/virtual_machine.h b/include/multipass/virtual_machine.h index baf3dec502..ab08866298 100644 --- a/include/multipass/virtual_machine.h +++ b/include/multipass/virtual_machine.h @@ -89,7 +89,7 @@ class VirtualMachine : private DisabledCopyMove using SnapshotVista = std::vector>; // 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 get_snapshot(const std::string& name) const = 0; virtual std::shared_ptr get_snapshot(int index) const = 0; diff --git a/include/multipass/virtual_machine_factory.h b/include/multipass/virtual_machine_factory.h index 123e354d86..80d711a016 100644 --- a/include/multipass/virtual_machine_factory.h +++ b/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 networks() const = 0; + virtual void require_snapshots_support() const = 0; protected: VirtualMachineFactory() = default; diff --git a/src/client/cli/formatter/csv_formatter.cpp b/src/client/cli/formatter/csv_formatter.cpp index b768a12dff..940c8b976d 100644 --- a/src/client/cli/formatter/csv_formatter.cpp +++ b/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); diff --git a/src/client/cli/formatter/json_formatter.cpp b/src/client/cli/formatter/json_formatter.cpp index 5f112592da..b9d5bc9dec 100644 --- a/src/client/cli/formatter/json_formatter.cpp +++ b/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()) diff --git a/src/client/cli/formatter/table_formatter.cpp b/src/client/cli/formatter/table_formatter.cpp index 5429b12797..f14b609586 100644 --- a/src/client/cli/formatter/table_formatter.cpp +++ b/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) : "--"); diff --git a/src/client/cli/formatter/yaml_formatter.cpp b/src/client/cli/formatter/yaml_formatter.cpp index 4a7bb1b53f..3b7b91be5f 100644 --- a/src/client/cli/formatter/yaml_formatter.cpp +++ b/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"] = diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index 6944bdcac3..1709b1a05f 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -1209,10 +1209,21 @@ mp::SettingsHandler* register_instance_mod(std::unordered_map& operative_instances, const std::unordered_map& deleted_instances, - const std::unordered_set& preparing_instances) + const std::unordered_set& preparing_instances, + const mp::VirtualMachineFactory& vm_factory) { - return MP_SETTINGS.register_handler( - std::make_unique(operative_instances, deleted_instances, preparing_instances)); + try + { + vm_factory.require_snapshots_support(); + return MP_SETTINGS.register_handler( + std::make_unique(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 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 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); diff --git a/src/platform/backends/qemu/qemu_virtual_machine.h b/src/platform/backends/qemu/qemu_virtual_machine.h index ce0c73f01a..a795ba241a 100644 --- a/src/platform/backends/qemu/qemu_virtual_machine.h +++ b/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 make_specific_snapshot(const QString& filename) override; std::shared_ptr 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 diff --git a/src/platform/backends/qemu/qemu_virtual_machine_factory.h b/src/platform/backends/qemu/qemu_virtual_machine_factory.h index bc09eedf16..e90a6e2c01 100644 --- a/src/platform/backends/qemu/qemu_virtual_machine_factory.h +++ b/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 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 diff --git a/src/platform/backends/shared/base_virtual_machine.cpp b/src/platform/backends/shared/base_virtual_machine.cpp index 6ac33af7eb..b82079b61d 100644 --- a/src/platform/backends/shared/base_virtual_machine.cpp +++ b/src/platform/backends/shared/base_virtual_machine.cpp @@ -116,6 +116,7 @@ std::vector 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 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 BaseVirtualMachine::get_snapshot(const std::stri std::shared_ptr 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 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 BaseVirtualMachine::get_childrens_names(const Snapshot* parent) const { + require_snapshots_support(); std::vector 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(); diff --git a/src/platform/backends/shared/base_virtual_machine.h b/src/platform/backends/shared/base_virtual_machine.h index 8649a0bfcb..46696db4b6 100644 --- a/src/platform/backends/shared/base_virtual_machine.h +++ b/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 get_snapshot(const std::string& name) const override; std::shared_ptr 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 make_specific_snapshot(const QString& filename); virtual std::shared_ptr 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(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 diff --git a/src/platform/backends/shared/base_virtual_machine_factory.h b/src/platform/backends/shared/base_virtual_machine_factory.h index 7dff290779..ff7e53e25d 100644 --- a/src/platform/backends/shared/base_virtual_machine_factory.h +++ b/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 diff --git a/src/rpc/multipass.proto b/src/rpc/multipass.proto index 3fab3bb7b1..461cdf25e6 100644 --- a/src/rpc/multipass.proto +++ b/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 { diff --git a/tests/mock_virtual_machine.h b/tests/mock_virtual_machine.h index 76e63ad328..c124000710 100644 --- a/tests/mock_virtual_machine.h +++ b/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, get_snapshot, (const std::string&), (const, override)); MOCK_METHOD(std::shared_ptr, get_snapshot, (int index), (const, override)); MOCK_METHOD(std::shared_ptr, get_snapshot, (const std::string&), (override)); diff --git a/tests/mock_virtual_machine_factory.h b/tests/mock_virtual_machine_factory.h index 37ad0d89d9..3413fd4e19 100644 --- a/tests/mock_virtual_machine_factory.h +++ b/tests/mock_virtual_machine_factory.h @@ -47,6 +47,7 @@ struct MockVirtualMachineFactory : public VirtualMachineFactory (std::vector, URLDownloader*, const Path&, const Path&, const days&), (override)); MOCK_METHOD(void, configure, (VirtualMachineDescription&), (override)); MOCK_METHOD(std::vector, networks, (), (const, override)); + MOCK_METHOD(void, require_snapshots_support, (), (const, override)); // originally protected: MOCK_METHOD(std::string, create_bridge_with, (const NetworkInterfaceInfo&), (override)); diff --git a/tests/stub_virtual_machine.h b/tests/stub_virtual_machine.h index 3009b79d1a..1cc193780b 100644 --- a/tests/stub_virtual_machine.h +++ b/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; }