Change in vdsm[ovirt-3.6]: vm.py: State saving in hotunplugDisk.

aaviram at redhat.com aaviram at redhat.com
Wed Sep 2 09:05:30 UTC 2015


Hello Nir Soffer, Francesco Romani,

I'd like you to do a code review.  Please visit

    https://gerrit.ovirt.org/45627

to review the following change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................

vm.py: State saving in hotunplugDisk.

VM's state was saved before detatching a disk in hotunplugDisk.

Saving vm's state should be performed only AFTER vdsm is actually
detaching the disk, otherwise the disk will remain in xmlDesc and
be calculated in the vm's hash.

This causes a bug in the engine's VM monitoring: the engine
gets the same hash as before the disk was unplugged, thus does not
update the disk's status.

Due to a possible race while hotunplugging, where VM monitoring starts
updating the VM devices while the disk is still plugged- then
hotplugging is performed and succeeds, then monitoring is resumed and
sets the disk to be still plugged, this patch is critical for the
engine to be synced with the actual state of the VM's disks.

Please note that this patch is partial, as detachDevice is async. That
means that still the hash can be calculated wrong in case the disk was
not detached yet. This fix will be included in a following patch.

Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Bug-Url:https://bugzilla.redhat.com/1206696
Signed-off-by: Amit Aviram <aaviram at redhat.com>
Reviewed-on: https://gerrit.ovirt.org/45077
Reviewed-by: Nir Soffer <nsoffer at redhat.com>
Reviewed-by: Francesco Romani <fromani at redhat.com>
Continuous-Integration: Jenkins CI
---
M vdsm/virt/vm.py
1 file changed, 10 insertions(+), 19 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/27/45627/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 903821f..2f245e6 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -2587,19 +2587,6 @@
         # see the XML even with 'info' as default level.
         self.log.info("Hotunplug disk xml: %s", driveXml)
 
-        self._devices[hwclass.DISK].remove(drive)
-        # Find and remove disk device from vm's conf
-        diskDev = None
-        for dev in self.conf['devices'][:]:
-            if (dev['type'] == hwclass.DISK and
-                    dev['path'] == drive.path):
-                with self._confLock:
-                    self.conf['devices'].remove(dev)
-                diskDev = dev
-                break
-
-        self.saveState()
-
         hooks.before_disk_hotunplug(driveXml, self.conf,
                                     params=drive.custom)
         try:
@@ -2608,14 +2595,18 @@
             self.log.exception("Hotunplug failed")
             if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
                 return response.error('noVM')
-            self._devices[hwclass.DISK].append(drive)
-            # Restore disk device in vm's conf and _devices
-            if diskDev:
-                with self._confLock:
-                    self.conf['devices'].append(diskDev)
-            self.saveState()
             return response.error('hotunplugDisk', e.message)
         else:
+            self._devices[hwclass.DISK].remove(drive)
+
+            # Find and remove disk device from vm's conf
+            for dev in self.conf['devices'][:]:
+                if dev['type'] == hwclass.DISK and dev['path'] == drive.path:
+                    with self._confLock:
+                        self.conf['devices'].remove(dev)
+                    break
+
+            self.saveState()
             hooks.after_disk_hotunplug(driveXml, self.conf,
                                        params=drive.custom)
             self._cleanupDrives(drive)


-- 
To view, visit https://gerrit.ovirt.org/45627
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>


More information about the vdsm-patches mailing list