[snapshots] resolve instance dir in vm #3130

sharder996 · 2023-06-19T14:31:12Z

The motivation behind this change is to allow a vm to resolve the directory in which it and all it's relevant files are stored, which is needed for snapshots and retrospectively, native mounts on hyperv instances.

In order to achieve this, the idea was to take away the responsibility of creating/deleting instance directories from the vm image vault and give it to the vm factory. This conceptually makes more sense as the factory is the class responsible for creating/removing vm resources in the first place and is also the place where platform/backend specific directories locations are resolved. The vm image vault then queries the factory for a instance specific directories and using it as a download directory.

As this is not a small architectural change, I want to get some feedback on this and improve it.

Public side of #549

ricab · 2023-06-20T10:34:25Z

Hey @sharder996, it looks like this needs a rebase and conflict fix.

ricab · 2023-06-27T12:13:11Z

Hey @sharder996, I pushed a new commit to account for new tests that are now present after rebasing. However, there are still a few other tests that failed when I tried them locally:

ImageVault
- returned_image_contains_instance_name
- caches_prepared_images
- remembers_prepared_images
QemuBackend
- get_backend_directory_name_calls_qemu_platform

I am not sure if they broke with my rebase or if they were already failing. Would you be able to take a look, please? Thank you!

sharder996 · 2023-06-27T14:59:55Z

Ho @ricab, thanks for fixing those tests! When I created this PR, I had not prioritized fixing tests since I'm anticipating there being review changes which would require me to fix tests again.

ricab · 2023-06-27T15:16:56Z

Ah OK, if it's not caused by my rebase I am happy.

ricab

Alright, thanks @sharder996! It took me a little to wrap my head around this one, but I think I finally formed a cohesive opinion...

This is tricky stuff but you made good progress here. I understand the option for creating the directory where it is derived (in get_instance_directory_name()), but I am not sure it's worth all the additional trips to disk (even to remove resources, the code tries to create the directory before removing it). We've been operating without that safety net for a long time and if we failed to create the directory now, that would be quickly noticed.

IIUC, the instance directory needs to be created only before it is used, i.e.:

when fetching, just before placing the instance image there
when launching with LXD (which does not place the image there), just before calling the instance constructor
when starting up the daemon with LXD, on creation of pre-existing (LXD) VMs that still do not have a directory, just before calling the instance constructor (only needed the first time after upgrading)

Additionally, I don't see a need to create parent directories separately:

.../vault is the vault's business
.../vault/instances is not needed until there is an instance
creating .../vault/instances/<instance> with MP_UTILS.make_dir creates those parents

Therefore, it seems to me that, strictly speaking, the only new place we need to create a directory is in LXDVMFactory::create_vm(). Other backends already require the image to be in place when creating the VM, whichever way it got there (DefaultVMImageVault today, clone tomorrow, etc.). So there is no need to create the directory in those cases.

If this all makes sense to you, I think you could still keep the instance directory derivation, parameters, and fields where you added them, but actually create the directory only in DefaultVMImageVault, as before (image_instance_from and extract_image_from) and in the LXD factory.

WDYT?

include/multipass/virtual_machine.h

include/multipass/vm_image_vault.h

src/platform/backends/libvirt/libvirt_virtual_machine_factory.cpp

include/multipass/virtual_machine_factory.h

include/multipass/virtual_machine.h

src/platform/backends/shared/base_virtual_machine_factory.h

src/platform/backends/shared/base_virtual_machine.h

src/platform/backends/lxd/lxd_vm_image_vault.cpp

src/platform/backends/lxd/lxd_virtual_machine_factory.cpp

Signed-off-by: sharder996 <[email protected]>

… to factory Signed-off-by: sharder996 <[email protected]>

ricab · 2023-09-05T14:26:47Z

Hey @sharder996, is this ready for another pass?

…ction calls to base class

src/platform/backends/shared/base_virtual_machine_factory.h

Remove `explicit` keyword from constructor with two parameters.

…d modified vm factory

codecov · 2023-09-14T19:19:05Z

Codecov Report

Merging #3130 (463ef1c) into snapshots (7546c56) will increase coverage by 0.15%.
Report is 21 commits behind head on snapshots.
The diff coverage is 70.64%.

@@              Coverage Diff              @@
##           snapshots    #3130      +/-   ##
=============================================
+ Coverage      84.61%   84.77%   +0.15%     
=============================================
  Files            247      247              
  Lines          13107    13228     +121     
=============================================
+ Hits           11091    11214     +123     
+ Misses          2016     2014       -2

Files Changed	Coverage Δ
include/multipass/utils.h	`100.00% <ø> (ø)`
include/multipass/virtual_machine_factory.h	`100.00% <ø> (ø)`
include/multipass/vm_image_vault.h	`100.00% <ø> (ø)`
src/platform/backends/lxd/lxd_vm_image_vault.cpp	`94.54% <ø> (ø)`
...rc/platform/backends/shared/base_virtual_machine.h	`0.00% <ø> (ø)`
.../platform/backends/shared/base_virtual_machine.cpp	`8.39% <18.51%> (+1.06%)`	⬆️
src/daemon/daemon.cpp	`68.58% <71.42%> (+0.14%)`	⬆️
src/daemon/default_vm_image_vault.cpp	`87.38% <76.19%> (+0.99%)`	⬆️
...ckends/libvirt/libvirt_virtual_machine_factory.cpp	`62.66% <83.33%> (+1.02%)`	⬆️
include/multipass/virtual_machine.h	`100.00% <100.00%> (ø)`
... and 11 more

... and 9 files with indirect coverage changes

src/daemon/default_vm_image_vault.cpp

tests/blueprint_test_lambdas.cpp

tests/lxd/test_lxd_image_vault.cpp

tests/test_image_vault.cpp

include/multipass/vm_image_vault.h

Remove artificial constructors from base VM and factory classes. Let stub classes pass in stub arguments when needed.

src/platform/backends/qemu/qemu_virtual_machine_factory.cpp

include/multipass/virtual_machine_factory.h

ricab

OK, this is getting there, only a few loose ends left to tie (please see comments above too).

src/platform/backends/lxd/lxd_virtual_machine_factory.cpp

src/platform/backends/libvirt/libvirt_virtual_machine_factory.cpp

include/multipass/virtual_machine_factory.h

src/daemon/daemon.cpp

src/platform/backends/lxd/lxd_virtual_machine_factory.cpp

ricab

Getting real close, but something is fishy now:

$ multipass launch -n a
Launched: a
$ ll /var/snap/multipass/common/data/multipassd/vault/instances/a/
total 703784
drwxr-xr-x 3 root root      4096 set 20 11:43 .
drwxr-xr-x 3 root root      4096 set 20 11:43 ..
drwxr-xr-x 2 root root      4096 set 20 11:43 a
-rw-r--r-- 1 root root     53248 set 20 11:43 cloud-init-config.iso
-rw-r--r-- 1 root root 720699392 set 20 11:46 ubuntu-22.04-server-cloudimg-amd64.img

Notice the extra "a" subdir.

src/utils/utils.cpp

ricab · 2023-09-20T10:23:22Z

include/multipass/utils.h

@@ -154,6 +154,8 @@ class Utils : public Singleton<Utils>
    // virtual machine helpers
    virtual void wait_for_cloud_init(VirtualMachine* virtual_machine, std::chrono::milliseconds timeout,
                                     const SSHKeyProvider& key_provider) const;
+    virtual Path derive_instances_dir(const Path& data_dir, const Path& backend_directory_name,


Perfect, well done adding this!

ricab

Great Scott :} Looking good on this side, so approving but postponing merging until the other side is reviewed too.

[snapshots] resolve instance dir in vm r=ricab a=sharder996 Co-authored-by: ScottH <[email protected]>

sharder996 changed the title ~~[snapshots] Resolve instance dir~~ [snapshots] resolve instance dir in vm Jun 19, 2023

ricab self-requested a review June 20, 2023 10:33

sharder996 force-pushed the resolve-instance-dir branch from 9365e06 to 7af15d3 Compare June 21, 2023 17:33

ricab force-pushed the snapshots branch from af9a74d to e327953 Compare June 23, 2023 21:23

ricab force-pushed the resolve-instance-dir branch 2 times, most recently from 8757363 to 5220596 Compare June 23, 2023 23:05

ricab force-pushed the snapshots branch from a30ab9b to 9e749f9 Compare June 28, 2023 18:31

sharder996 force-pushed the resolve-instance-dir branch 4 times, most recently from 1adce11 to fa3de91 Compare June 30, 2023 19:17

ricab force-pushed the resolve-instance-dir branch from fa3de91 to d9926b8 Compare July 10, 2023 18:31

ricab requested changes Jul 20, 2023

View reviewed changes

ricab force-pushed the snapshots branch from 42d592b to 38f90ee Compare July 26, 2023 18:19

sharder996 force-pushed the resolve-instance-dir branch from d9926b8 to b1ef49d Compare July 31, 2023 16:18

ricab force-pushed the snapshots branch from f2ad7a6 to 7546c56 Compare August 29, 2023 18:02

sharder996 added 9 commits August 30, 2023 00:41

[image-vault] drive by fix pass by value

0da1f3d

Signed-off-by: sharder996 <[email protected]>

[image-vault] drive by move functions in unnamed namespace together

6cc1de0

Signed-off-by: sharder996 <[email protected]>

[vm] have vms be aware of where they are stored

0180540

Signed-off-by: sharder996 <[email protected]>

[vm] reference internally stored instance directory location

44746a7

Signed-off-by: sharder996 <[email protected]>

[vm factory] drive by fix add const modifier

9fd4194

Signed-off-by: sharder996 <[email protected]>

[vm factory] give responsibility of instances dir to vm factory

1f1b9e9

Signed-off-by: sharder996 <[email protected]>

[vm factory] provide external access to specific vm instance directory

fe9634f

Signed-off-by: sharder996 <[email protected]>

[vm factory] populate vm instance directory on creation

05f9156

Signed-off-by: sharder996 <[email protected]>

[vm factory] give responsibility of removing instance dir on deletion…

7d79236

… to factory Signed-off-by: sharder996 <[email protected]>

sharder996 force-pushed the resolve-instance-dir branch from b1ef49d to c505a13 Compare August 30, 2023 13:56

sharder996 force-pushed the resolve-instance-dir branch from 557c0ad to 54faf5f Compare August 31, 2023 03:57

[vm factory] use virtual{,-impl} architecture to remove duplicate fun…

30cbc96

…ction calls to base class

ricab reviewed Sep 14, 2023

View reviewed changes

src/platform/backends/shared/base_virtual_machine_factory.h Show resolved Hide resolved

ricab and others added 6 commits September 14, 2023 17:18

[qemu] Fix circular dependency in initialization

35c466c

[qemu] Remove unnecessary explicit

9ff29e0

Remove `explicit` keyword from constructor with two parameters.

[qemu] Make QemuPlatform::get_directory_name const

cf88eef

[tests] update tests to properly reference an instance's directory an…

84ac031

…d modified vm factory

[vm factory] make instance variable private

ff13fbc

[lxd vm image vault] label unused parameter

04a4d7b

linting changes

2020333

ricab reviewed Sep 18, 2023

View reviewed changes

[vms] Remove artificial constructors for tests

8e50791

Remove artificial constructors from base VM and factory classes. Let stub classes pass in stub arguments when needed.

ricab reviewed Sep 18, 2023

View reviewed changes

src/platform/backends/qemu/qemu_virtual_machine_factory.cpp Outdated Show resolved Hide resolved

include/multipass/virtual_machine_factory.h Outdated Show resolved Hide resolved

ricab requested changes Sep 18, 2023

View reviewed changes

ricab and others added 6 commits September 18, 2023 20:35

[tests] Use a temporary dir in remaining stub VMs

3029016

rename param to be more descriptive

4788cf3

[lxd] edit log string to be more descriptive

8a3f6a0

remove unnecessary comment

373a23d

[vm factory] rename function

feb86ea

[vm factory] extract instance directory derivation into util function

33af0f6

ricab reviewed Sep 20, 2023

View reviewed changes

sharder996 added 2 commits September 20, 2023 11:45

[vm image vault] correct instance directory creation location

400fbb3

[utils] avoid hardcoded directory separators

463ef1c

ricab approved these changes Sep 21, 2023

View reviewed changes

ricab merged commit 7a58568 into snapshots Sep 22, 2023
11 of 13 checks passed

bors bot deleted the resolve-instance-dir branch September 22, 2023 18:29

ricab added a commit that referenced this pull request Oct 25, 2023

Merge pull request #3130 from canonical/resolve-instance-dir

f5a964f

[snapshots] resolve instance dir in vm r=ricab a=sharder996 Co-authored-by: ScottH <[email protected]>

ricab added a commit that referenced this pull request Oct 30, 2023

Merge pull request #3130 from canonical/resolve-instance-dir

0a7ea5b

[snapshots] resolve instance dir in vm r=ricab a=sharder996 Co-authored-by: ScottH <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[snapshots] resolve instance dir in vm #3130

[snapshots] resolve instance dir in vm #3130

sharder996 commented Jun 19, 2023 •

edited

Loading

ricab commented Jun 20, 2023

ricab commented Jun 27, 2023 •

edited

Loading

sharder996 commented Jun 27, 2023

ricab commented Jun 27, 2023

ricab left a comment

ricab commented Sep 5, 2023

codecov bot commented Sep 14, 2023 •

edited

Loading

ricab left a comment

ricab left a comment

ricab Sep 20, 2023

ricab left a comment

[snapshots] resolve instance dir in vm #3130

[snapshots] resolve instance dir in vm #3130

Conversation

sharder996 commented Jun 19, 2023 • edited Loading

ricab commented Jun 20, 2023

ricab commented Jun 27, 2023 • edited Loading

sharder996 commented Jun 27, 2023

ricab commented Jun 27, 2023

ricab left a comment

Choose a reason for hiding this comment

ricab commented Sep 5, 2023

codecov bot commented Sep 14, 2023 • edited Loading

Codecov Report

ricab left a comment

Choose a reason for hiding this comment

ricab left a comment

Choose a reason for hiding this comment

ricab Sep 20, 2023

Choose a reason for hiding this comment

ricab left a comment

Choose a reason for hiding this comment

sharder996 commented Jun 19, 2023 •

edited

Loading

ricab commented Jun 27, 2023 •

edited

Loading

codecov bot commented Sep 14, 2023 •

edited

Loading