Multipass clone #3264

georgeliao · 2023-10-20T11:05:17Z

Please read the spec first to get a sense of the context.

The code can be divided into several parts:

client module, the clone command line part.
daemon::clone method in daemon.cpp,
2.1. generate destination name, this step processes the name options of the input, use the given name or generate a new one if no name is provided. This step also needs the name list of instances to check if the name exist, it also requires the source spec VMSpecs::clone_count field to generate a new name.
2.2. Once the new destination name is produced, we can clone the vmspec object, this step requires copying the vmspec instance and updating the VMSpecs::default_mac_address and VMSpecs::extra_interfaces fields. Ideally, this method can be a data member method (called clone maybe) in VMSpecs class. However, given the fact that the dependency code (generate_unused_mac_address method and allocated_mac_addrs data) still resides in daemon.cpp, so clone_spec is a local lambda method in daemon.cpp for now.
2.3. clone image record through the VMImageVault::clone method.
2.4. calls create_vm_and_clone_instance_dir_data from VirtualMachineFactory.
remove_all_snapshots_from_the_image function is added to VirtualMachine class, it gets called in the factory class and it removes the snapshots from the image file.
qemu_virtual_machine_factory::create_vm_and_clone_instance_dir_data, it copies the whole instance folder with the exclusion of snapshot files and updates the unique identifiers in cloud-init-config.iso file. Besides that, it creates the new VirtualMachine instance and also removes the snapshots from the image file.
auto-completion script of multipass clone command line.

Functional testing should cover at least the following scenarios:

multipass clone command line part, --help, and options, explanation of these options. Error message in different cases.
Animated spinner works properly in a successful clone case.
In the case of cloning without a designated name, check if the name generator works right.
In the case of cloning a general vm instance that has mounts, and snapshots, check if the new instance can run properly, check whether the new instance disk data have the updated unique identifiers.

Small note:
The unit tests are not finished yet considering that they might change if the code structure is required to change according to the review. More unit tests will be added once the review is finished.

Some further thoughts on the code architecture (open for discussion):

Using VMSpecs::clone_count to count cloned instances and the BaseVirtualMachine::snapshot_count to count snapshots is cumbersome. We need a variable and a file or json item to track the value. Besides that, the current name-generation scheme also does not persist on trying. What I mean by that is if the next about-to-be-generated name already exists (if the user already created the snapshot/cloned instance by specifying that name), then the auto name-generation will fail. To fix these things, we can try a different name-generation scheme, that is always looping over the existing instances/snapshots to seek the next available name, this scheme will have to do a linear search but it no longer depends on the counter variable/file and will persist on name generation. The corresponding name-generation behavior will change when there is a gap in the integer sequence (when user deletes the snapshots/instances in between) , the new scheme will reuse that deleted name and the counter based scheme will not. I think this approach is better overall, but it requires some changes in already released behavior.
In the multipassd-vm-instances.json file, the metadata::arguments contains the image and clou-init file absolute path, the multipassd-instance-image-records.json file also has the path item points to the image file path. These paths in a way added more unnecessary instance name occurrences (the instance folder name in the path) which need to be updated during the clone and renaming process. Then maybe a better disk data layout and C++ code structure can remove these paths. For example, if we split these json files into one item per file and move that into the instance directory, and meanwhile we can move VMSpecs into VirtualMachine class. With this new data layout and code structure, the VMSpecs can access the instance directory and further access the multipassd-vm-instances.json file (with his own item), besides that, the data sync between the VMSpecs and VirtualMachine class becomes much easier. On top of these, there are more benefits like the rollback mechanism and thread-safety can be much easier.
Make a global Mac_address_generator class that can be accessed from everywhere and refactor the corresponding code. If this can be done, then the clone_spec does not have to be a local lambda in daemon.cpp.

codecov · 2023-11-02T09:42:12Z

Codecov Report

Attention: Patch coverage is 70.15504% with 77 lines in your changes missing coverage. Please review.

Project coverage is 88.48%. Comparing base (62c4fc6) to head (db63cc7).

Files	Patch %	Lines
...orm/backends/qemu/qemu_virtual_machine_factory.cpp	0.00%	37 Missing ⚠️
...rc/platform/backends/qemu/qemu_virtual_machine.cpp	0.00%	17 Missing ⚠️
src/daemon/daemon.cpp	84.81%	12 Missing ⚠️
...orm/backends/shared/base_virtual_machine_factory.h	0.00%	4 Missing ⚠️
src/utils/json_utils.cpp	82.60%	4 Missing ⚠️
...rc/platform/backends/shared/base_virtual_machine.h	0.00%	2 Missing ⚠️
...tform/backends/qemu/qemu_virtual_machine_factory.h	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3264      +/-   ##
==========================================
- Coverage   88.82%   88.48%   -0.34%     
==========================================
  Files         253      255       +2     
  Lines       14121    14365     +244     
==========================================
+ Hits        12543    12711     +168     
- Misses       1578     1654      +76

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

… check.

…re branch of cmd dispatch.

…rce and destination instance folders.

… function.

…t test mocking.

… unit test.

…sistent with the metadata sub-string replacement in JsonUtils::update_unique_identifiers_of_metadata.

…work bug.

… ci use relwithdebinfo mode and that does hit the assertion.

…instead of the QemuVirtualMachineFactory::create_virtual_machine public function.

…ction and replace copying the whole directory call with that.

…d code.

…rom_the_image function.

…e implementation and call it in the factory class.

sharder996

Hey @georgeliao, thanks for all the work you've done with clone! Took a little while to go over everything, but I think I'm finally ready to submit my initial review.

Functionally it looks great! Just some comments/questions from my code review.

sharder996 · 2024-06-08T00:24:00Z

src/client/cli/cmd/clone.cpp

+
+QString cmd::Clone::short_help() const
+{
+    return QStringLiteral("Clone an Ubuntu instance");


I think we typical refer to them simply as an instance. See snapshot.cpp

sharder996 · 2024-06-08T00:33:49Z

src/client/cli/cmd/clone.cpp

+
+QString cmd::Clone::description() const
+{
+    return QStringLiteral("A clone is a complete independent copy of a whole virtual machine instance");


I think this should describe an action since it is a description of multipass clone; an action. Right now, it describes an object. How about something like:

Create a complete independent copy of an existing instance

WDYT

sharder996 · 2024-06-19T05:32:00Z

src/daemon/daemon.cpp

+                {
+                    if (!mp::utils::valid_hostname(request.destination_name()))
+                    {
+                        throw std::runtime_error("Invalid destination virtual machine instance name: " +


Just a small nitpick, but I feel like you're repeating yourself by saying "virtual machine instance" since we use "virtual machine" and "instance" interchangeably. There is already a precedent for referring to them as "instances" so that is what I would expect.

sharder996 · 2024-06-19T06:02:12Z

src/daemon/daemon.cpp

+                    config->vault->remove(destination_name);
+                });
+
+            const auto& source_vm_ptr = operative_instances[source_name];


You should already have a pointer to the vm from calling find_instance_and_react earlier in the function.

sharder996 · 2024-06-19T06:16:39Z

src/daemon/daemon.cpp

+                throw std::runtime_error("Please stop instance " + source_name + " before you clone it.");
+            }
+
+            auto clone_spec = [this](const mp::VMSpecs& src_vm_spec,


I don't see the point in having all these lambdas functions in clone(). It makes it harder to follow the execution sequence since there code in between different lambda functions. Each of the lambdas here are only called once, so I would say either just inline the code, or if you need to separate it for readability, put it in a helper function.

sharder996 · 2024-06-20T19:57:31Z

src/platform/backends/qemu/qemu_virtual_machine_factory.cpp

+
+            const fs::path filename = entry.path().filename();
+            // if the filename does not contains "snapshot" sub-string, then copy
+            if (filename.string().find("snapshot") == std::string::npos)


Not really an important issue right now, but maybe it would be better if we explicitly define which files to copy so that this doesn't need to change if/when more files are created and associated with a vm. I guess we are only filtering because of the exception with cloning snapshots, so maybe it's not a big issue. 🤷

sharder996 · 2024-06-20T20:33:07Z

src/iso/cloud_init_iso.cpp

@@ -727,6 +727,29 @@ void mp::CloudInitFileOps::update_cloud_init_with_new_extra_interfaces_and_new_i
    iso_file.write_to(QString::fromStdString(cloud_init_path.string()));
 }

+void mp::CloudInitFileOps::update_cloned_cloud_init_unique_identifiers(


Just for my own understanding; this is basically doing the same thing as in mp::JsonUtils::update_unique_identifiers_of_metadata(), just a different format, right? Cloud-init iso instead of json. Seems like a bit of code smell that we have two functions that functionally do the same thing, but I guess it can't be avoided...

sharder996 · 2024-06-20T20:52:15Z

src/utils/utils.cpp

@@ -22,6 +22,7 @@
 #include <multipass/file_ops.h>
 #include <multipass/format.h>
 #include <multipass/logging/log.h>
+#include <multipass/network_interface.h>


Leftover development code?

sharder996 · 2024-06-20T21:02:45Z

include/multipass/virtual_machine.h


    QDir instance_directory() const;
+    std::string get_vm_name() const;


vm_name is a public member so you shouldn't need this accessor method, right?

sharder996 · 2024-06-21T00:54:10Z

tests/test_daemon_clone.cpp

+namespace mpt = multipass::test;
+using namespace testing;
+
+struct TestDaemonClone : public mpt::DaemonTestFixture


There are still some uncovered lines in daemon::clone() that would be nice if they were covered.

georgeliao marked this pull request as draft October 20, 2023 11:07

georgeliao force-pushed the multipass_clone branch from d8739fd to c9f71b0 Compare November 2, 2023 08:41

georgeliao force-pushed the multipass_clone branch from 0e5a833 to 5e2a139 Compare November 16, 2023 11:46

georgeliao force-pushed the multipass_clone branch from 77e02fb to 69d81b9 Compare December 11, 2023 14:23

georgeliao force-pushed the multipass_clone branch 2 times, most recently from 5b2c7f7 to 2bb5b3a Compare January 3, 2024 10:04

georgeliao force-pushed the multipass_clone branch from eddafcc to fb663a5 Compare January 12, 2024 14:20

georgeliao force-pushed the multipass_clone branch 3 times, most recently from 794467a to dac94dd Compare February 6, 2024 15:48

georgeliao force-pushed the multipass_clone branch 6 times, most recently from 563f9f3 to f448929 Compare February 20, 2024 11:46

georgeliao force-pushed the multipass_clone branch 5 times, most recently from c28404e to a662819 Compare March 5, 2024 12:06

georgeliao force-pushed the multipass_clone branch from f96a5cf to a662819 Compare March 7, 2024 20:51

georgeliao force-pushed the multipass_clone branch 3 times, most recently from 6b0b4c7 to 963f599 Compare April 15, 2024 14:21

georgeliao force-pushed the multipass_clone branch 3 times, most recently from 560583e to 8c70116 Compare May 1, 2024 09:47

georgeliao force-pushed the multipass_clone branch from 92819f9 to f328b81 Compare May 8, 2024 10:02

georgeliao added 24 commits June 13, 2024 16:28

[unit test] added more unit tests for client failed command line args…

ab52644

… check.

[unit test] added clone invalid option unit test.

62723f3

[cmd] added AnimatedSpinner to clone command.

6df0592

[client][unit test] added command fail error to cover action_on_failu…

941f51c

…re branch of cmd dispatch.

[vm factory] use in class data member instances_dir to derive the sou…

93ab893

…rce and destination instance folders.

[vm image] improve one function parameter name.

dc244c2

[clone] function name improvement.

b623d65

[clone] added update_cloned_cloud_init utility function.

4cbe2d7

[cloud init] [unit test] added unit test for update_cloned_cloud_init…

d763550

… function.

[vm factory] use update_cloned_cloud_init function to prepare for uni…

f5519bd

…t test mocking.

[unit test] added FileOps::copy unit test.

af09064

[image vault] added unit test for DefaultVMImageVault::clone.

8640b2f

[image vault] [unit test] added imageCloneFailOnNonExistSrcImage case.

fa0dc91

[image vault] [unit test] added imageCloneFailOnAlreadyExistDestImage…

16812a6

… unit test.

[lxd image vault][unit test] added lxdImageVaultCloneThrow unit test.

b7504a0

[unit test] added coverage for DaemonRpc::clone.

dff898e

[daemon] small refactor on generate_next_clone_name function.

78faee2

[cloud init] small function name improvement and typo correction.

85bdf13

[vm image] improves the image_path sub-string replacement, get it con…

6800785

…sistent with the metadata sub-string replacement in JsonUtils::update_unique_identifiers_of_metadata.

[qemu][vm factory] fixed rollback instance directory removal did not …

e4098a4

…work bug.

[clone] added metadata non-empty check for the non-qemu backend cases.

d165585

[unit test] removed assertion line coverage unit test because windows…

a823965

… ci use relwithdebinfo mode and that does hit the assertion.

[qemu][vm factory] using the QemuVirtualMachine constructor directly …

956ccab

…instead of the QemuVirtualMachineFactory::create_virtual_machine public function.

[qemu][vm factory] added copy_instance_dir_without_snapshot_files fun…

0f619d0

…ction and replace copying the whole directory call with that.

georgeliao force-pushed the multipass_clone branch from 5067f65 to 0f619d0 Compare June 13, 2024 14:28

georgeliao added 3 commits June 13, 2024 18:21

[qemu] remove all load_snapshots_and_update_unique_identifiers relate…

b94370a

…d code.

[virtual machine] added the placeholder of the remove_all_snapshots_f…

01867b7

…rom_the_image function.

[qemu][virtual machine] added qemu remove_all_snapshots_from_the_imag…

db63cc7

…e implementation and call it in the factory class.

georgeliao force-pushed the multipass_clone branch from 40ab758 to db63cc7 Compare June 14, 2024 13:00

sharder996 requested changes Jun 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multipass clone #3264

Multipass clone #3264

georgeliao commented Oct 20, 2023 •

edited

codecov bot commented Nov 2, 2023 •

edited

sharder996 left a comment

sharder996 Jun 8, 2024

sharder996 Jun 8, 2024

sharder996 Jun 19, 2024

sharder996 Jun 19, 2024

sharder996 Jun 19, 2024

sharder996 Jun 20, 2024

sharder996 Jun 20, 2024

sharder996 Jun 20, 2024

sharder996 Jun 20, 2024

sharder996 Jun 21, 2024


		QDir instance_directory() const;
		std::string get_vm_name() const;

Multipass clone #3264

Are you sure you want to change the base?

Multipass clone #3264

Conversation

georgeliao commented Oct 20, 2023 • edited

codecov bot commented Nov 2, 2023 • edited

Codecov Report

sharder996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeliao commented Oct 20, 2023 •

edited

codecov bot commented Nov 2, 2023 •

edited