Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multipass clone #3264

Draft
wants to merge 110 commits into
base: main
Choose a base branch
from
Draft

Multipass clone #3264

wants to merge 110 commits into from

Conversation

georgeliao
Copy link
Contributor

@georgeliao georgeliao commented Oct 20, 2023

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

The code can be divided into several modules:

  1. client module, the clone command line part.
  2. 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.
  3. load_snapshots_and_update_unique_identifiers is added to base_snap_shot.cpp, it loads the snapshot files and updates the cloud_init_instance_id item, and file path, default mac address, and extra interface mac address of the metadata in the case of qemu.
  4. qemu_virtual_machine_factory::create_vm_and_clone_instance_dir_data, it copies the whole instance folder and updates the unique identifiers in cloud-init-config.iso file. Besides that, it creates the new VirtualMachine instance and also loads and modifies snapshot files.
  5. auto-completion script of multipass clone command line.

Functional testing should cover at least the following scenarios:

  1. multipass clone command line part, --help, and options, explanation of these options. Error message in different cases.
  2. Animated spinner works properly in a successful clone case.
  3. In the case of cloning without a designated name, check if the name generator works right.
  4. 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.

Further thoughts on the code architecture (open for discussion):

  1. The VMSpecs::clone_count to count cloned instances and the BaseVirtualMachine::snapshot_count to count snapshots are cumbersome. We need a variable and the file or json item to track the value. If we change the name generation scheme to seeking the next available integer.
  2. In multipassd-vm-instances.json file the metadata::arguments contains the image and clou-init file absolute path, the multipassd-instance-image-records.json file 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 or renaming process. Maybe a better disk data layout and C++ code structure can truly remove this. 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 a rollback mechanism and thread-safety can be much easier.
  3. 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.

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Attention: Patch coverage is 73.51779% with 67 lines in your changes are missing coverage. Please review.

Project coverage is 88.54%. Comparing base (981b86c) to head (b059d3c).

Files Patch % Lines
...orm/backends/qemu/qemu_virtual_machine_factory.cpp 0.00% 21 Missing ⚠️
src/platform/backends/shared/base_snapshot.cpp 0.00% 13 Missing ⚠️
src/daemon/daemon.cpp 84.61% 12 Missing ⚠️
src/platform/backends/qemu/qemu_snapshot.cpp 0.00% 4 Missing ⚠️
.../platform/backends/shared/base_virtual_machine.cpp 69.23% 4 Missing ⚠️
...orm/backends/shared/base_virtual_machine_factory.h 0.00% 4 Missing ⚠️
src/utils/json_utils.cpp 82.60% 4 Missing ⚠️
include/multipass/virtual_machine.h 0.00% 2 Missing ⚠️
...rc/platform/backends/qemu/qemu_virtual_machine.cpp 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.81%   88.54%   -0.28%     
==========================================
  Files         253      255       +2     
  Lines       14121    14354     +233     
==========================================
+ Hits        12542    12710     +168     
- Misses       1579     1644      +65     

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

…from QJsonObject to QJsonValue for consistency.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant