tests/storage-volumes-vm: Root volume disk device attachments #366

MggMuggins · 2024-12-05T00:07:15Z

This should have a check for all corner cases around VM root volume attachments (coverage for canonical/lxd#14532):

security.protection.start allows one other VM to attach the machine's root disk, and can only be removed if the disk is not attached
security.shared allows unchecked attachments of root disks
VM attachments are correctly reported in used_by
hotplug of VM root attachments works (as this is the method reccomended by the docs to avoid UUID/LABEL conflicts)

I ran into an interesting behavior of udev in Noble VMs while testing that slowed this down a bit. In order for udev to create a symlink for SCSI_IDENT_SERIAL (/dev/disk/by-id/scsi-SQEMU_QEMU_HARDDISK_lxd_virtual--machine-vm5), I had to hotplug the device in an ubuntu:24.04 VM. Starting the VM from cold didn't create the link, and ubuntu-minimal:24.04 VMs never created it at all.

Without hotplug or in an ubuntu-minimal:24.04:

# udevadm info /dev/sdb
...
S: disk/by-id/scsi-0QEMU_QEMU_HARDDISK_lxd_virtual--machine
S: disk/by-path/pci-0000:02:00.0-scsi-0:0:1:1
S: disk/by-diskseq/10
E: DEVPATH=/devices/pci0000:00/0000:00:01.1/0000:02:00.0/virtio6/host0/target0:0:1/0:0:1:1/block/sdb
E: DEVNAME=/dev/sdb
...
E: ID_SERIAL=0QEMU_QEMU_HARDDISK_lxd_virtual--machine
E: ID_SERIAL_SHORT=lxd_virtual--machine
E: ID_SCSI_SERIAL=lxd_virtual--machine-v2
E: ID_BUS=scsi
...
E: DEVLINKS=/dev/disk/by-id/scsi-0QEMU_QEMU_HARDDISK_lxd_virtual--machine /dev/disk/by-path/pci-0000:02:00.0-scsi-0:0:1:1 /dev/disk/by-diskseq/10
...

With hotplug:

# udevadm info /dev/sdb
...
S: disk/by-id/scsi-SQEMU_QEMU_HARDDISK_lxd_virtual--machine-vm5
S: disk/by-path/pci-0000:02:00.0-scsi-0:0:1:1
S: disk/by-id/scsi-0QEMU_QEMU_HARDDISK_lxd_virtual--machine
S: disk/by-diskseq/15
...
E: DEVPATH=/devices/pci0000:00/0000:00:01.1/0000:02:00.0/virtio6/host0/target0:0:1/0:0:1:1/block/sdb
E: DEVNAME=/dev/sdb
...
E: SCSI_IDENT_SERIAL=lxd_virtual--machine-vm5
E: SCSI_IDENT_LUN_VENDOR=lxd_virtual--machine
...
E: ID_SERIAL=0QEMU_QEMU_HARDDISK_lxd_virtual--machine
E: ID_SERIAL_SHORT=lxd_virtual--machine
E: ID_SCSI_SERIAL=lxd_virtual--machine-vm5
...
E: DEVLINKS=/dev/disk/by-id/scsi-SQEMU_QEMU_HARDDISK_lxd_virtual--machine-vm5 /dev/disk/by-path/pci-0000:02:00.0-scsi-0:0:1:1 /dev/disk/by-id/scsi-0QEMU_QEMU_HARDDISK_lxd_virtual--machin>
...

I worked around it by using a shorter LXD device name so that the symlink name wouldn't be truncated; not sure if this is worth chasing further.

simondeziel

LGTM, I made some comments but they are mostly nit picks.

I haven't seen it run in GHA due to the extension not being in latest/edge just yet. I'm assuming it all works locally but wanted to check how much longer it made the test :)

tests/storage-volumes-vm

tomponline · 2024-12-06T13:40:51Z

I ran into an interesting behavior of udev in Noble VMs while testing that slowed this down a bit. In order for udev to create a symlink for SCSI_IDENT_SERIAL (/dev/disk/by-id/scsi-SQEMU_QEMU_HARDDISK_lxd_virtual--machine-vm5), I had to hotplug the device in an ubuntu:24.04 VM. Starting the VM from cold didn't create the link, and ubuntu-minimal:24.04 VMs never created it at all.

@hamistao any experience with this?

hamistao · 2024-12-06T18:01:09Z

Not particularly, but it is not the first time we have seen udev or other packages mangling with disk devices that LXD manages (canonical/lxd#13701 (comment)) and since it only happens on a specific distro version and not on minimal it is almost certainly a problem of the same nature and probably isn't worth digging too deep into.

On Without hotplug or in an ubuntu-minimal:24.04 we have S: disk/by-id/scsi-0QEMU_QEMU_HARDDISK_lxd_virtual--machine and E: DEVLINKS=/dev/disk/by-id/scsi-0QEMU_QEMU_HARDDISK_lxd_virtual--machine ... so LXD seems to be doing its job just fine.

MggMuggins · 2024-12-09T22:27:13Z

Thanks @tomponline @hamistao, I agree that this isn't worth chasing.

@simondeziel They do pass locally 😉 . It looks like it goes from 5 lxc starts to 6 lxc starts. So ~120% of the previous runtime for this test.

simondeziel · 2024-12-12T19:37:39Z

tests/storage-volumes-vm

+		lxc init --empty --vm v2 --storage "${poolName}" --device root,size=8KiB
+		lxc init --empty --vm v3 --storage "${poolName}" --device root,size=8KiB


It's good to test with such small volumes. I'm pretty sure LVM will round them up but that should happen all transparently, hopefully!

Dunno how it would fare with PowerFlex's minimum of 8GiB though. I guess we'll see when we run it there :)

powerflex doesnt round up to 8gib afaik @roosterfish ?

AFAIK, it only allows multiples of 8GiB which is why it's special cased everywhere in tests/storage-vm like in https://github.com/canonical/lxd-ci/blob/main/tests/storage-vm#L89-L95

Pushed a special case for powerflex.

MggMuggins · 2024-12-17T00:26:41Z

Added a check to make sure that security.shared can be unset when security.protection.start is set.

MggMuggins · 2024-12-19T03:43:48Z

Added a check for instance rename.

This should have a check for all corner cases around VM root volume attachments: - security.protection.start allows one other VM to attach the machine's root disk, and can only be removed if the disk is not attached - security.shared allows unchecked attachments of root disks - VM attachments are correctly reported in used_by - hotplug of VM root attachments works (as this is the method reccomended by the docs to avoid UUID/LABEL conflicts) - Rename/Delete of instances when their root disk is attached elsewhere is disallowed Signed-off-by: Wesley Hershberger <[email protected]>

Includes commits from #14491 This PR enables attaching a virtual machine's root storage volume to another virtual machine via a disk device: ``` # vm2 yaml ... devices: v1-root: type: disk pool: default source: virtual-machine/vm1 ``` This has some constraints because simultaneous access to storage volumes with content-type `block` is unsafe: - `vm1`'s root volume can be attached to exactly one other instance if `vm1` has `security.protection.start: true` - `vm1`'s root volume can be attached to any number of other instances if the storage volume `virtual-machine/vm1` has `security.shared: true` `security.protection.start` is recommended for interactive use; e.g. a user temporarily needs to access a bricked machine's root volume to fix it or recover data. `security.shared` can be used if more than one running instance must have access to the block volume. ## TODO - [x] Docs - [x] [lxd-ci](https://github.com/canonical/lxd-ci/) tests to validate attachments to running VMs. [Done](canonical/lxd-ci#366) - [x] Bootorder bug (see comment) - current workaround is to hotplug disk devices to avoid UUID/LABEL collisions at boot time.

tomponline · 2025-01-13T10:18:41Z

@MggMuggins seems to be failing:

lxc storage volume attach vmpool14779 virtual-machine/v2 v1 v2-root
Error: Failed to start device "v2-root": Failed to call monitor hook for block device: Failed adding block device for disk device "v2-root": Failed adding block device: error reading header from custom_test_v2.block: No such file or directory

MggMuggins · 2025-01-14T00:10:22Z

The qemu driver infers the volume type based on the source path (here). It seems to me that the "RBDFormat" (see here) should really be sending over the volume name as it appears to Ceph, rather than requiring the driver to figure out project and volume type info.

I'm working on building a reproducer and should have a fix up by EOD Wed.

MggMuggins · 2025-01-16T19:50:28Z

canonical/lxd#14807 fixes this.

Different calls of `DiskGetRBDFormat` and `DiskParseRBDFormat` are currently using both senses of "volume": - A Ceph block device (RBD Image) - A LXD storage volume Because a disk device can specify an RBD image with source `ceph:<pool_name>/<volume_name>"`, these functions should use the first meaning: the name passed should be the name of a RADOS block device (RBD image), not a LXD storage volume. These functions should (in future) be replaced with a more comprehensive abstraction for what `MountInfo.DevPath` is currently doing. This serialization doesn't seem to be written anywhere, it's just deserialized when it gets to the instance driver. This fixes the failures from canonical/lxd-ci#366; the QEMU driver was inferring a virtual-machine `VolumeType` only when `TargetPath == "/"`

tomponline

Thanks!

tomponline · 2025-02-05T11:19:13Z

@MggMuggins this will need to be updated now too since canonical/lxd#14886 right?

MggMuggins · 2025-02-11T16:24:23Z

Sorry, just saw this. As we discussed last week, canonical/lxd#14886 only changes the device config keys used by the server; the cli interface didn't change, so this is alright without updates.

MggMuggins requested review from simondeziel and tomponline as code owners December 5, 2024 00:07

MggMuggins mentioned this pull request Dec 5, 2024

Attach VM root volumes as disk devices canonical/lxd#14532

Merged

3 tasks

simondeziel reviewed Dec 5, 2024

View reviewed changes

MggMuggins force-pushed the attach-vm-root-volumes branch from 4ea7f64 to 2040505 Compare December 9, 2024 22:23

MggMuggins requested a review from simondeziel December 9, 2024 22:27

simondeziel previously approved these changes Dec 12, 2024

View reviewed changes

MggMuggins force-pushed the attach-vm-root-volumes branch from 2040505 to 533a96f Compare December 17, 2024 00:25

MggMuggins force-pushed the attach-vm-root-volumes branch from 533a96f to ef82585 Compare December 19, 2024 03:43

MggMuggins dismissed simondeziel’s stale review via 259b5b9 December 19, 2024 06:23

MggMuggins force-pushed the attach-vm-root-volumes branch from ef82585 to 259b5b9 Compare December 19, 2024 06:23

MggMuggins force-pushed the attach-vm-root-volumes branch from 259b5b9 to 6abe711 Compare December 19, 2024 16:10

simondeziel approved these changes Dec 19, 2024

View reviewed changes

MggMuggins mentioned this pull request Jan 16, 2025

Storage: Align Ceph block device name encoding canonical/lxd#14807

Merged

tomponline approved these changes Jan 22, 2025

View reviewed changes

tomponline merged commit b4a1e6e into canonical:main Jan 22, 2025
174 of 179 checks passed

MggMuggins deleted the attach-vm-root-volumes branch January 23, 2025 13:28

		lxc init --empty --vm v2 --storage "${poolName}" --device root,size=8KiB
		lxc init --empty --vm v3 --storage "${poolName}" --device root,size=8KiB

tests/storage-volumes-vm: Root volume disk device attachments #366

tests/storage-volumes-vm: Root volume disk device attachments #366

Uh oh!

Conversation

MggMuggins commented Dec 5, 2024

Uh oh!

simondeziel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomponline commented Dec 6, 2024

Uh oh!

hamistao commented Dec 6, 2024

Uh oh!

MggMuggins commented Dec 9, 2024

Uh oh!

simondeziel Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

tomponline Dec 13, 2024

Choose a reason for hiding this comment

Uh oh!

simondeziel Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

MggMuggins Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

MggMuggins commented Dec 17, 2024

Uh oh!

MggMuggins commented Dec 19, 2024

Uh oh!

tomponline commented Jan 13, 2025

Uh oh!

MggMuggins commented Jan 14, 2025

Uh oh!

MggMuggins commented Jan 16, 2025

Uh oh!

tomponline left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomponline commented Feb 5, 2025

Uh oh!

MggMuggins commented Feb 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants