Change in vdsm[ovirt-3.5]: vm: Libvirt quering after disk detach operation addition.

aaviram at redhat.com aaviram at redhat.com
Thu Oct 22 09:07:02 UTC 2015


Hello Francesco Romani,

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

    https://gerrit.ovirt.org/47629

to review the following change.

Change subject: vm: Libvirt quering after disk detach operation addition.
......................................................................

vm: Libvirt quering after disk detach operation addition.

As stated in libvirt documentary, after detaching a device using
virDomainDetachDeviceFlags, we need to verify that this device
has actually been detached.

Currently we use virDomainDetachDevice. However- That function behaves
the same in that matter. (Currently it is not documented at libvirt's
API docs- but after contacting libvirt's guys it turned out that this
is true. Bug 1257280 opened for fixing the documentation.)

Not verifying that the device was detached, as mentioned above, cause
various problems, as hotunplugDisk could return a success result
while it did not actually succeeds to detach the disk.

This patch adds this functionallity to hotunplugDisk, and after some
timeout fails the operation if the disk was not detached.

Change-Id: I393ce55dd761ac825cb96bd499976fd74c366b09
Bug-Url: https://bugzilla.redhat.com/1044466
Signed-off-by: Amit Aviram <aaviram at redhat.com>
Reviewed-on: https://gerrit.ovirt.org/45138
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer <nsoffer at redhat.com>
Reviewed-by: Francesco Romani <fromani at redhat.com>
Reviewed-on: https://gerrit.ovirt.org/45646
---
M lib/vdsm/config.py.in
M vdsm/virt/vm.py
2 files changed, 45 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/29/47629/1

diff --git a/lib/vdsm/config.py.in b/lib/vdsm/config.py.in
index ef05a4a..49a210b 100644
--- a/lib/vdsm/config.py.in
+++ b/lib/vdsm/config.py.in
@@ -154,6 +154,9 @@
             'command, 30 secs is a nice default. Set to 300 if the vm is '
             'expected to freeze during cluster failover.'),
 
+        ('hotunplug_timeout', '30',
+            'Time to wait (in seconds) for a VM to detach its disk'),
+
         ('vm_watermark_interval', '2',
             'How often should we sample each vm for statistics (seconds).'),
 
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index ef62bc8..54a4f8e 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -1830,6 +1830,10 @@
     pass
 
 
+class HotunplugTimeout(Exception):
+    pass
+
+
 class MissingLibvirtDomainError(Exception):
     def __init__(self, reason=vmexitreason.LIBVIRT_DOMAIN_MISSING):
         super(Exception, self).__init__(
@@ -4079,6 +4083,13 @@
                                     params=drive.custom)
         try:
             self._dom.detachDevice(driveXml)
+            self._waitForDriveRemoval(drive)
+        except HotunplugTimeout as e:
+            self.log.error("%s", e)
+            return {'status': {
+                'code': errCode['hotunplugDisk']['status']['code'],
+                'message': "%s" % e
+            }}
         except libvirt.libvirtError as e:
             self.log.error("Hotunplug failed", exc_info=True)
             if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
@@ -4107,6 +4118,37 @@
 
         return {'status': doneCode, 'vmList': self.status()}
 
+    def _waitForDriveRemoval(self, drive):
+        """
+        As stated in libvirt documentary, after detaching a device using
+        virDomainDetachDeviceFlags, we need to verify that this device
+        has actually been detached:
+        libvirt.org/html/libvirt-libvirt-domain.html#virDomainDetachDeviceFlags
+
+        This function waits for the disk device to be detached.
+
+        Currently we use virDomainDetachDevice. However- That function behaves
+        the same in that matter. (Currently it is not documented at libvirt's
+        API docs- but after contacting libvirt's guys it turned out that this
+        is true. Bug 1257280 opened for fixing the documentation.)
+        TODO: remove this comment when the documentation will be fixed.
+
+        :param drive: The drive that should be detached.
+        """
+        self.log.debug("Waiting for hotunplug to finish")
+        with utils.stopwatch("Hotunplug disk %s" % drive.name):
+            deadline = (utils.monotonic_time() +
+                        config.getint('vars', 'hotunplug_timeout'))
+            while self._isDriveAttached(drive):
+                time.sleep(1)
+                if utils.monotonic_time() > deadline:
+                    raise HotunplugTimeout("Timeout detaching drive %s"
+                                           % drive.name)
+
+    def _isDriveAttached(self, drive):
+        root = ET.fromstring(self._dom.XMLDesc(0))
+        return bool(root.findall("./devices/disk[serial='%s']" % drive.serial))
+
     def _readPauseCode(self, timeout):
         # libvirt does not not export yet the I/O error reason code.
         # we need a way to


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I393ce55dd761ac825cb96bd499976fd74c366b09
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.5
Gerrit-Owner: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>


More information about the vdsm-patches mailing list