[snapshots] resolve instance dir in vm

include/multipass/virtual_machine.h

@@ -20,6 +20,7 @@
 
 #include "disabled_copy_move.h"
 #include "ip_address.h"
+#include "path.h"
 
 #include <QDir>
 #include <QJsonObject>
@@ -91,11 +92,11 @@ class VirtualMachine : private DisabledCopyMove
     virtual int get_num_snapshots() const noexcept = 0;
     virtual std::shared_ptr<const Snapshot> get_snapshot(const std::string& name) const = 0;
     virtual std::shared_ptr<Snapshot> get_snapshot(const std::string& name) = 0;
-    virtual std::shared_ptr<const Snapshot> take_snapshot(const QDir& snapshot_dir, const VMSpecs& specs,
-                                                          const std::string& name, const std::string& comment) = 0;
-    virtual void delete_snapshot(const QDir& snapshot_dir, const std::string& name) = 0;
-    virtual void restore_snapshot(const QDir& snapshot_dir, const std::string& name, VMSpecs& specs) = 0;
-    virtual void load_snapshots(const QDir& snapshot_dir) = 0;
+    virtual std::shared_ptr<const Snapshot> take_snapshot(const VMSpecs& specs, const std::string& name,
+                                                          const std::string& comment) = 0;
+    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;
     virtual std::vector<std::string> get_childrens_names(const Snapshot* parent) const = 0;
 
     VirtualMachine::State state;
@@ -106,8 +107,13 @@ class VirtualMachine : private DisabledCopyMove
     bool shutdown_while_starting{false};
 
 protected:
-    VirtualMachine(VirtualMachine::State state, const std::string& vm_name) : state{state}, vm_name{vm_name} {};
-    VirtualMachine(const std::string& vm_name) : VirtualMachine(State::off, vm_name){};
+    const QDir instance_dir;
+
+    VirtualMachine(VirtualMachine::State state, const std::string& vm_name, const Path& instance_dir)
+        : state{state}, vm_name{vm_name}, instance_dir{QDir{instance_dir}} {};
+    VirtualMachine(const std::string& vm_name, const Path& instance_dir)
+        : VirtualMachine(State::off, vm_name, instance_dir){};
+    VirtualMachine(const std::string& vm_name) : VirtualMachine(vm_name, ""){};
 };
 } // namespace multipass
 #endif // MULTIPASS_VIRTUAL_MACHINE_H

include/multipass/virtual_machine_factory.h

@@ -59,8 +59,10 @@ class VirtualMachineFactory : private DisabledCopyMove
     virtual VMImage prepare_source_image(const VMImage& source_image) = 0;
     virtual void prepare_instance_image(const VMImage& instance_image, const VirtualMachineDescription& desc) = 0;
     virtual void hypervisor_health_check() = 0;
-    virtual QString get_backend_directory_name() = 0;
-    virtual QString get_backend_version_string() = 0;
+    virtual QString get_backend_directory_name() const = 0;
+    virtual Path
+    get_instance_directory_name(const std::string& name) const = 0; // postcondition: return directory exists
+    virtual QString get_backend_version_string() const = 0;
     virtual VMImageVault::UPtr create_image_vault(std::vector<VMImageHost*> image_hosts, URLDownloader* downloader,
                                                   const Path& cache_dir_path, const Path& data_dir_path,
                                                   const days& days_to_expire) = 0;

include/multipass/vm_image_vault.h

@@ -82,7 +82,7 @@ class VMImageVault : private DisabledCopyMove
     virtual ~VMImageVault() = default;
     virtual VMImage fetch_image(const FetchType& fetch_type, const Query& query, const PrepareAction& prepare,
                                 const ProgressMonitor& monitor, const bool unlock,
-                                const std::optional<std::string>& checksum) = 0;
+                                const std::optional<std::string>& checksum, const Path& save_dir) = 0;
     virtual void remove(const std::string& name) = 0;
     virtual bool has_record_for(const std::string& name) = 0;
     virtual void prune_expired_images() = 0;

src/daemon/daemon.cpp

@@ -437,19 +437,15 @@ QJsonObject vm_spec_to_json(const mp::VMSpecs& specs)
     return json;
 }
 
-auto fetch_image_for(const std::string& name, const mp::FetchType& fetch_type, mp::VMImageVault& vault)
+auto fetch_image_for(const std::string& name, mp::VirtualMachineFactory& factory, mp::VMImageVault& vault)
 {
     auto stub_prepare = [](const mp::VMImage&) -> mp::VMImage { return {}; };
     auto stub_progress = [](int download_type, int progress) { return true; };
 
     mp::Query query{name, "", false, "", mp::Query::Type::Alias, false};
 
-    return vault.fetch_image(fetch_type, query, stub_prepare, stub_progress, false, std::nullopt);
-}
-
-QDir instance_directory(const std::string& instance_name, const mp::DaemonConfig& config)
-{ // TODO should we establish a more direct way to get to the instance's directory?
-    return mp::utils::base_dir(fetch_image_for(instance_name, config.factory->fetch_type(), *config.vault).image_path);
+    return vault.fetch_image(factory.fetch_type(), query, stub_prepare, stub_progress, false, std::nullopt,
+                             factory.get_instance_directory_name(name));
 }
 
 auto try_mem_size(const std::string& val) -> std::optional<mp::MemorySize>
@@ -1383,7 +1379,7 @@ mp::Daemon::Daemon(std::unique_ptr<const DaemonConfig> the_config)
             continue;
         }
 
-        auto vm_image = fetch_image_for(name, config->factory->fetch_type(), *config->vault);
+        auto vm_image = fetch_image_for(name, *config->factory, *config->vault);
         if (!vm_image.image_path.isEmpty() && !QFile::exists(vm_image.image_path))
         {
             mpl::log(mpl::Level::warning, category,
@@ -1410,7 +1406,7 @@ mp::Daemon::Daemon(std::unique_ptr<const DaemonConfig> the_config)
 
         auto& instance_record = spec.deleted ? deleted_instances : operative_instances;
         auto instance = instance_record[name] = config->factory->create_virtual_machine(vm_desc, *this);
-        instance->load_snapshots(instance_directory(name, *config));
+        instance->load_snapshots();
 
         allocated_mac_addrs = std::move(new_macs); // Add the new macs to the daemon's list only if we got this far
 
@@ -1818,7 +1814,7 @@ try // clang-format on
         entry->mutable_instance_status()->set_status(grpc_instance_status_for(present_state));
 
         // FIXME: Set the release to the cached current version when supported
-        auto vm_image = fetch_image_for(name, config->factory->fetch_type(), *config->vault);
+        auto vm_image = fetch_image_for(name, *config->factory, *config->vault);
         auto current_release = vm_image.original_release;
 
         if (!vm_image.id.empty() && current_release.empty())
@@ -2264,7 +2260,7 @@ try // clang-format on
                     assert(purge && "precondition: snapshots can only be purged");
 
                     for (const auto& snapshot_name : pick)
-                        vm_it->second->delete_snapshot(instance_directory(instance_name, *config), snapshot_name);
+                        vm_it->second->delete_snapshot(snapshot_name);
                 }
             }
         }
@@ -2499,8 +2495,7 @@ try
         SnapshotReply reply;
 
         {
-            const auto snapshot = vm_ptr->take_snapshot(instance_directory(instance_name, *config), spec_it->second,
-                                                        snapshot_name, request->comment());
+            const auto snapshot = vm_ptr->take_snapshot(spec_it->second, snapshot_name, request->comment());
 
             reply.set_snapshot(snapshot->get_name());
         }
@@ -2550,7 +2545,6 @@ try
         // Only need to check if the snapshot exists so the result is discarded
         vm_ptr->get_snapshot(request->snapshot());
 
-        const auto& vm_dir = instance_directory(instance_name, *config);
         if (!request->destructive())
         {
             RestoreReply confirm_action{};
@@ -2566,8 +2560,8 @@ try
             {
                 reply_msg(server, fmt::format("Taking snapshot before restoring {}", instance_name));
 
-                const auto snapshot = vm_ptr->take_snapshot(vm_dir, vm_specs, "",
-                                                            fmt::format("Before restoring {}", request->snapshot()));
+                const auto snapshot =
+                    vm_ptr->take_snapshot(vm_specs, "", fmt::format("Before restoring {}", request->snapshot()));
 
                 reply_msg(server, fmt::format("Snapshot taken: {}.{}", instance_name, snapshot->get_name()),
                           /* sticky = */ true);
@@ -2577,7 +2571,7 @@ try
         // Actually restore snapshot
         reply_msg(server, "Restoring snapshot");
         auto old_specs = vm_specs;
-        vm_ptr->restore_snapshot(vm_dir, request->snapshot(), vm_specs);
+        vm_ptr->restore_snapshot(request->snapshot(), vm_specs);
 
         auto mounts_it = mounts.find(instance_name);
         assert(mounts_it != mounts.end() && "uninitialized mounts");
@@ -2662,8 +2656,8 @@ void mp::Daemon::persist_instances()
 
 void mp::Daemon::release_resources(const std::string& instance)
 {
-    config->factory->remove_resources_for(instance);
     config->vault->remove(instance);
+    config->factory->remove_resources_for(instance);
 
     auto spec_it = vm_instance_specs.find(instance);
     if (spec_it != cend(vm_instance_specs))
@@ -2921,8 +2915,9 @@ void mp::Daemon::create_vm(const CreateRequest* request,
             if (!vm_desc.image.id.empty())
                 checksum = vm_desc.image.id;
 
-            auto vm_image = config->vault->fetch_image(fetch_type, query, prepare_action, progress_monitor,
-                                                       launch_from_blueprint, checksum);
+            auto vm_image =
+                config->vault->fetch_image(fetch_type, query, prepare_action, progress_monitor, launch_from_blueprint,
+                                           checksum, config->factory->get_instance_directory_name(name));
 
             const auto image_size = config->vault->minimum_image_size_for(vm_image.id);
             vm_desc.disk_space = compute_final_image_size(
@@ -3363,7 +3358,7 @@ void mp::Daemon::populate_instance_info(VirtualMachine& vm, mp::DetailedInfoItem
     else
         info->mutable_instance_status()->set_status(grpc_instance_status_for(present_state));
 
-    auto vm_image = fetch_image_for(name, config->factory->fetch_type(), *config->vault);
+    auto vm_image = fetch_image_for(name, *config->factory, *config->vault);
     auto original_release = vm_image.original_release;
 
     if (!vm_image.id.empty() && original_release.empty())

src/daemon/default_vm_image_vault.cpp

@@ -218,15 +218,27 @@ mp::MemorySize get_image_size(const mp::Path& image_path)
 
     return image_size;
 }
+
+template <typename T>
+void persist_records(const T& records, const QString& path)
+{
+    QJsonObject json_records;
+    for (const auto& record : records)
+    {
+        auto key = QString::fromStdString(record.first);
+        json_records.insert(key, record_to_json(record.second));
+    }
+    mp::write_json(json_records, path);
+}
 } // namespace
 
 mp::DefaultVMImageVault::DefaultVMImageVault(std::vector<VMImageHost*> image_hosts, URLDownloader* downloader,
-                                             mp::Path cache_dir_path, mp::Path data_dir_path, mp::days days_to_expire)
+                                             const mp::Path& cache_dir_path, const mp::Path& data_dir_path,
+                                             const mp::days& days_to_expire)
     : BaseVMImageVault{image_hosts},
       url_downloader{downloader},
       cache_dir{QDir(cache_dir_path).filePath("vault")},
       data_dir{QDir(data_dir_path).filePath("vault")},
-      instances_dir(data_dir.filePath("instances")),
       images_dir(cache_dir.filePath("images")),
       days_to_expire{days_to_expire},
       prepared_image_records{load_db(cache_dir.filePath(image_db_name))},
@@ -241,7 +253,8 @@ mp::DefaultVMImageVault::~DefaultVMImageVault()
 
 mp::VMImage mp::DefaultVMImageVault::fetch_image(const FetchType& fetch_type, const Query& query,
                                                  const PrepareAction& prepare, const ProgressMonitor& monitor,
-                                                 const bool unlock, const std::optional<std::string>& checksum)
+                                                 const bool unlock, const std::optional<std::string>& checksum,
+                                                 const mp::Path& download_dir)
 {
     {
         std::lock_guard<decltype(fetch_mutex)> lock{fetch_mutex};
@@ -269,11 +282,11 @@ mp::VMImage mp::DefaultVMImageVault::fetch_image(const FetchType& fetch_type, co
 
         if (source_image.image_path.endsWith(".xz"))
         {
-            source_image.image_path = extract_image_from(query.name, source_image, monitor);
+            source_image.image_path = extract_image_from(query.name, source_image, monitor, download_dir);
         }
         else
         {
-            source_image = image_instance_from(query.name, source_image);
+            source_image = image_instance_from(query.name, source_image, download_dir);
         }
 
         vm_image = prepare(source_image);
@@ -314,7 +327,7 @@ mp::VMImage mp::DefaultVMImageVault::fetch_image(const FetchType& fetch_type, co
 
                 if (last_modified.isValid() && (last_modified.toString().toStdString() == record.image.release_date))
                 {
-                    return finalize_image_records(query, record.image, id);
+                    return finalize_image_records(query, record.image, id, download_dir);
                 }
             }
 
@@ -376,7 +389,7 @@ mp::VMImage mp::DefaultVMImageVault::fetch_image(const FetchType& fetch_type, co
                         const auto prepared_image = record.second.image;
                         try
                         {
-                            return finalize_image_records(query, prepared_image, record.first);
+                            return finalize_image_records(query, prepared_image, record.first, download_dir);
                         }
                         catch (const std::exception& e)
                         {
@@ -414,7 +427,7 @@ mp::VMImage mp::DefaultVMImageVault::fetch_image(const FetchType& fetch_type, co
             auto prepared_image = future.result();
             std::lock_guard<decltype(fetch_mutex)> lock{fetch_mutex};
             in_progress_image_fetches.erase(id);
-            return finalize_image_records(query, prepared_image, id);
+            return finalize_image_records(query, prepared_image, id, download_dir);
         }
         catch (const std::exception&)
         {
@@ -431,10 +444,6 @@ void mp::DefaultVMImageVault::remove(const std::string& name)
     if (name_entry == instance_image_records.end())
         return;
 
-    QDir instance_dir{instances_dir};
-    if (instance_dir.cd(QString::fromStdString(name)))
-        instance_dir.removeRecursively();
-
     instance_image_records.erase(name);
     persist_instance_records();
 }
@@ -523,7 +532,8 @@ void mp::DefaultVMImageVault::update_images(const FetchType& fetch_type, const P
         mpl::log(mpl::Level::info, category, fmt::format("Updating {} source image to latest", record.query.release));
         try
         {
-            fetch_image(fetch_type, record.query, prepare, monitor, false, std::nullopt);
+            fetch_image(fetch_type, record.query, prepare, monitor, false, std::nullopt,
+                        QFileInfo{record.image.image_path}.absolutePath());
 
             // Remove old image
             std::lock_guard<decltype(fetch_mutex)> lock{fetch_mutex};
@@ -621,24 +631,23 @@ mp::VMImage mp::DefaultVMImageVault::download_and_prepare_source_image(
 }
 
 QString mp::DefaultVMImageVault::extract_image_from(const std::string& instance_name, const VMImage& source_image,
-                                                    const ProgressMonitor& monitor)
+                                                    const ProgressMonitor& monitor, const mp::Path& dest_dir)
 {
     const auto name = QString::fromStdString(instance_name);
-    const QDir output_dir{MP_UTILS.make_dir(instances_dir, name)};
+    MP_UTILS.make_dir(dest_dir, name);
     QFileInfo file_info{source_image.image_path};
     const auto image_name = file_info.fileName().remove(".xz");
-    const auto image_path = output_dir.filePath(image_name);
+    const auto image_path = QDir(dest_dir).filePath(image_name);
 
     return mp::vault::extract_image(image_path, monitor);
 }
 
 mp::VMImage mp::DefaultVMImageVault::image_instance_from(const std::string& instance_name,
-                                                         const VMImage& prepared_image)
+                                                         const VMImage& prepared_image, const mp::Path& dest_dir)
 {
-    auto name = QString::fromStdString(instance_name);
-    auto output_dir = MP_UTILS.make_dir(instances_dir, name);
+    MP_UTILS.make_dir(dest_dir, QString::fromStdString(instance_name));
 
-    return {mp::vault::copy(prepared_image.image_path, output_dir),
+    return {mp::vault::copy(prepared_image.image_path, dest_dir),
             prepared_image.id,
             prepared_image.original_release,
             prepared_image.current_release,
@@ -658,13 +667,13 @@ std::optional<QFuture<mp::VMImage>> mp::DefaultVMImageVault::get_image_future(co
 }
 
 mp::VMImage mp::DefaultVMImageVault::finalize_image_records(const Query& query, const VMImage& prepared_image,
-                                                            const std::string& id)
+                                                            const std::string& id, const mp::Path& dest_dir)
 {
     VMImage vm_image;
 
     if (!query.name.empty())
     {
-        vm_image = image_instance_from(query.name, prepared_image);
+        vm_image = image_instance_from(query.name, prepared_image, dest_dir);
         instance_image_records[query.name] = {vm_image, query, std::chrono::system_clock::now()};
     }
 
@@ -679,21 +688,6 @@ mp::VMImage mp::DefaultVMImageVault::finalize_image_records(const Query& query,
     return vm_image;
 }
 
-namespace
-{
-template <typename T>
-void persist_records(const T& records, const QString& path)
-{
-    QJsonObject json_records;
-    for (const auto& record : records)
-    {
-        auto key = QString::fromStdString(record.first);
-        json_records.insert(key, record_to_json(record.second));
-    }
-    mp::write_json(json_records, path);
-}
-} // namespace
-
 void mp::DefaultVMImageVault::persist_instance_records()
 {
     persist_records(instance_image_records, data_dir.filePath(instance_db_name));