Storage: Attach VM snapshots as disk devices

[MggMuggins]

Attach a snapshot of v1 to another VM as a disk device using this device config:

devices:
  v1-snap0:
    pool: default
    source: v1
    source.type: virtual-machine
    source.snapshot: snap0
    type: disk

Attempting to attach custom volume snapshots using this API will fail. I am planning to complete that functionality next week.

Tests in the works for lxd-cicanonical/lxd-ci#413

[tomponline]

@MggMuggins when we added source-type field it looks like we didnt update the instance_root_volume_attachment API extension, as it still says "Introduces the <type>/<volume> syntax for the source property of
disk devices."

Also can you update that API extension to include the new snapshot field too? Then that single API extension can cover both new features.

[MggMuggins]

Good catch; should be good to go.

[MggMuggins]

Still chasing down a bug, this is not GTM.

[tomponline]

@MggMuggins please can you rebase

[MggMuggins]

Spun my wheels a bit today on checking that deleting a snapshot isn't possible if it's attached to an instance. Added a test to the lxd-ci PR to check that; this is good for review.

Still working on the LVM refcount bug. When the LVM driver deactivates a volume snapshot it checks to see if the parent volume is still in use. We need the inverse; the driver also needs to make sure when deactivating the parent that there aren't any snapshots in use. It looks like deactivating the parent also deactivates the snapshot (hence the error below), but I doubt the opposite is true. More on this tomorrow.

Putting this here for future travelers:

$ lxc storage volume detach vmpool292382 virtual-machine/v2 v1
Error: Failed to stop device "v2-root": Failed to deactivate LVM logical volume "/dev/vmpool292382/virtual-machines_test_v2": Failed to run: lvchange --activate n --ignoreactivationskip /dev/vmpool292382/virtual-machines_test_v2: exit status 5 (Logical volume vmpool292382/virtual-machines_test_v2-snap0 contains a filesystem in use.

[tomponline]

@MggMuggins did you consider supporting source: vol/snap rather than source-snapshot? As we tend to use vol/snap format most other places?

[tomponline]

@MggMuggins did you consider supporting source: vol/snap rather than source-snapshot? As we tend to use vol/snap format most other places?

Just want to check u didnt miss this question?

[MggMuggins]

I did not 😉

Here is this set of changes with that syntax instead (tests failing atm, def broken). From an implementation perspective it makes complete sense (much smaller delta with the existing disk device driver and lxc, consistent with StorageVolume.Name, etc); and there are a few places where <instance>/<snap> synxtax is exposed in the API.

I don't love it for the same reasons that I switched to source.type though: multiple discrete pieces of information in the same field is very opaque when reading the code. It also requires parsing the field before using it (even if, as is the case here, my changes aren't doing the parsing). Using source.snapshot is more of an ugly band-aid than a real fix though, so I'm willing to go with the delimited syntax if that's what you'd prefer. I will get that other branch in order tomorrow.

Fixed 2 bugs (ceph ro block vols and docs) in the process 🙂

[tomponline]

Using source.snapshot is more of an ugly band-aid than a real fix though, so I'm willing to go with the delimited syntax if that's what you'd prefer. I will get that other branch in order tomorrow.

How about we stick with source.snapshot because it makes the config and code itself clearer, and we can then still use vol/snapshot in the CLI if needed (like volume attach)?

[MggMuggins]

I'm happy with that; the [<type>/]<vol>[/<snapshot>] syntax will stick around regardless.

I'll get this ready to go ASAP.

[MggMuggins]

@tomponline This is ready to go.

Added tests for readonly ceph block devices to canonical/lxd-ci#413

[MggMuggins]

Nevermind, the readonly fix regressed something else and I don't have time today to dig into it. I'll pull those commits out so that this PR is unblocked and keep working on it on Monday.

[tomponline]

strings.Cut would also be an option here

[tomponline]

@MggMuggins shall we rename this to vm_root_volume_attachment as my understanding is that we dont support containers yet, and we dont plan to before LXD 6.3 is released?