[lxc-devel] [lxd/master] VM: Fixes disk resize regression

[tomponline on Github]

A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 552 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20201009/84e6c427/attachment.bin>
-------------- next part --------------
From 57f1dc94a63d5faf175b096f280269adcca51786 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Fri, 9 Oct 2020 19:39:09 +0100
Subject: [PATCH 1/2] lxd/instance/drivers/driver/qemu: Restores ability to
 resize VM disks

 - Moves check for preventing update of devices when VM is running into updateDevices().
 - Fixes regression introduced by https://github.com/lxc/lxd/commit/6697599975c1199c226c00b68c7e3e278ea49c66#diff-8ac4860b5f669c17c92c37545fe2ecfd.
 - Removes 1 call to IsRunning() by passing into updateDevices() from Update().

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/instance/drivers/driver_qemu.go | 44 +++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/lxd/instance/drivers/driver_qemu.go b/lxd/instance/drivers/driver_qemu.go
index 0b697cbf58..7c641ef033 100644
--- a/lxd/instance/drivers/driver_qemu.go
+++ b/lxd/instance/drivers/driver_qemu.go
@@ -2836,7 +2836,37 @@ func (vm *qemu) Update(args db.InstanceArgs, userRequested bool) error {
 	}
 
 	// Diff the devices.
-	removeDevices, addDevices, updateDevices, allUpdatedKeys := oldExpandedDevices.Update(vm.expandedDevices, nil)
+	removeDevices, addDevices, updateDevices, allUpdatedKeys := oldExpandedDevices.Update(vm.expandedDevices, func(oldDevice deviceConfig.Device, newDevice deviceConfig.Device) []string {
+		// This function needs to return a list of fields that are excluded from differences
+		// between oldDevice and newDevice. The result of this is that as long as the
+		// devices are otherwise identical except for the fields returned here, then the
+		// device is considered to be being "updated" rather than "added & removed".
+		oldNICType, err := nictype.NICType(vm.state, vm.Project(), newDevice)
+		if err != nil {
+			return []string{} // Cannot hot-update due to config error.
+		}
+
+		newNICType, err := nictype.NICType(vm.state, vm.Project(), oldDevice)
+		if err != nil {
+			return []string{} // Cannot hot-update due to config error.
+		}
+
+		if oldDevice["type"] != newDevice["type"] || oldNICType != newNICType {
+			return []string{} // Device types aren't the same, so this cannot be an update.
+		}
+
+		d, err := device.New(vm, vm.state, "", newDevice, nil, nil)
+		if err != nil {
+			return []string{} // Couldn't create Device, so this cannot be an update.
+		}
+
+		// TODO modify device framework to differentiate between devices that can only be updated when VM
+		// is stopped, but don't need to be removed then re-added. For now we rely upon the disk device
+		// indicating that it supports hot plugging, even for VMs, which it cannot. However this is
+		// needed so that the disk device's Update() function is called to allow disk resizing.
+		_, updateFields := d.CanHotPlug()
+		return updateFields
+	})
 
 	if userRequested {
 		// Do some validation of the config diff.
@@ -2854,12 +2884,8 @@ func (vm *qemu) Update(args db.InstanceArgs, userRequested bool) error {
 
 	isRunning := vm.IsRunning()
 
-	if isRunning && len(allUpdatedKeys) > 0 {
-		return fmt.Errorf("Devices cannot be changed when VM is running")
-	}
-
 	// Use the device interface to apply update changes.
-	err = vm.updateDevices(removeDevices, addDevices, updateDevices, oldExpandedDevices)
+	err = vm.updateDevices(removeDevices, addDevices, updateDevices, oldExpandedDevices, isRunning)
 	if err != nil {
 		return err
 	}
@@ -3055,8 +3081,10 @@ func (vm *qemu) updateMemoryLimit(newLimit string) error {
 	return fmt.Errorf("Failed setting memory to %d bytes (currently %d bytes) as it was taking too long", newSizeBytes, curSizeBytes)
 }
 
-func (vm *qemu) updateDevices(removeDevices deviceConfig.Devices, addDevices deviceConfig.Devices, updateDevices deviceConfig.Devices, oldExpandedDevices deviceConfig.Devices) error {
-	isRunning := vm.IsRunning()
+func (vm *qemu) updateDevices(removeDevices deviceConfig.Devices, addDevices deviceConfig.Devices, updateDevices deviceConfig.Devices, oldExpandedDevices deviceConfig.Devices, isRunning bool) error {
+	if isRunning && (len(removeDevices) > 0 || len(addDevices) > 0 || len(updateDevices) > 0) {
+		return fmt.Errorf("Devices cannot be changed when VM is running")
+	}
 
 	// Remove devices in reverse order to how they were added.
 	for _, dev := range removeDevices.Reversed() {

From 68dde3234a2e89118806ee57f60e8f4cd04ef997 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Fri, 9 Oct 2020 19:44:42 +0100
Subject: [PATCH 2/2] lxd/device/disk: Adds comment about VM instances
 depending on CanHotPlug fields for stopped disk resize

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/device/disk.go | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lxd/device/disk.go b/lxd/device/disk.go
index 790618cc4a..4f728007e7 100644
--- a/lxd/device/disk.go
+++ b/lxd/device/disk.go
@@ -231,6 +231,8 @@ func (d *disk) validateEnvironment() error {
 
 // CanHotPlug returns whether the device can be managed whilst the instance is running, it also
 // returns a list of fields that can be updated without triggering a device remove & add.
+// Note: At current time VM instances rely on this function indicating live update of "size" field is possible
+// to allow disk resize when VM is stopped.
 func (d *disk) CanHotPlug() (bool, []string) {
 	return true, []string{"limits.max", "limits.read", "limits.write", "size"}
 }