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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

MggMuggins
Copy link
Contributor

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.

@MggMuggins MggMuggins mentioned this pull request Dec 5, 2024
Open
3 tasks
simondeziel
simondeziel reviewed Dec 5, 2024
View reviewed changes
Copy link
Member

@simondeziel simondeziel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
tests/storage-volumes-vm Outdated Show resolved Hide resolved
tests/storage-volumes-vm Outdated Show resolved Hide resolved
tests/storage-volumes-vm Outdated Show resolved Hide resolved
tests/storage-volumes-vm Outdated Show resolved Hide resolved
tests/storage-volumes-vm Outdated Show resolved Hide resolved
tests/storage-volumes-vm Show resolved Hide resolved
@tomponline
Copy link
Member

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
Copy link
Contributor

hamistao commented Dec 6, 2024

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 MggMuggins force-pushed the attach-vm-root-volumes branch from 4ea7f64 to 2040505 Compare December 9, 2024 22:23
@MggMuggins
Copy link
Contributor Author

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
simondeziel previously approved these changes Dec 12, 2024
View reviewed changes
tests/storage-volumes-vm Outdated
Comment on lines 196 to 197
lxc init --empty --vm v2 --storage "${poolName}" --device root,size=8KiB
lxc init --empty --vm v3 --storage "${poolName}" --device root,size=8KiB
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

powerflex doesnt round up to 8gib afaik @roosterfish ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed a special case for powerflex.

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

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

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

Added a check for instance rename.

@MggMuggins MggMuggins force-pushed the attach-vm-root-volumes branch from ef82585 to 259b5b9 Compare December 19, 2024 06:23
@MggMuggins
tests/storage-volumes-vm: Root volume disk device attachments
6abe711 
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]>
@MggMuggins MggMuggins force-pushed the attach-vm-root-volumes branch from 259b5b9 to 6abe711 Compare December 19, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simondeziel simondeziel simondeziel approved these changes

@tomponline tomponline Awaiting requested review from tomponline tomponline is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MggMuggins @tomponline @hamistao @simondeziel