Change in vdsm[master]: vm: Libvirt quering after disk detach operation addition.

aaviram at redhat.com aaviram at redhat.com
Mon Aug 31 08:51:03 UTC 2015


Amit Aviram has posted comments on this change.

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


Patch Set 7: -Verified

(6 comments)

https://gerrit.ovirt.org/#/c/45138/7/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 343:             'Max number of tasks which can be queued on workers.'
Line 344:             ' This is for internal usage and may change without warning'),
Line 345: 
Line 346:         ('hotunplug_timeout', '20',
Line 347:             'Number of seconds to wait for a VM to detach its disk'),
> I don't think this belongs here, this is not about sampling. This fits bett
Done
Line 348:     ]),
Line 349: 
Line 350:     # Section: [devel]
Line 351:     ('devel', [


https://gerrit.ovirt.org/#/c/45138/7/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 28: import tempfile
Line 29: import threading
Line 30: import time
Line 31: import uuid
Line 32: import datetime
> We dont' need this import now - this probably cause the CI failures.  You c
Done
Line 33: 
Line 34: # 3rd party libs imports
Line 35: import libvirt
Line 36: import xml.etree.ElementTree as ET


Line 32: import datetime
Line 33: 
Line 34: # 3rd party libs imports
Line 35: import libvirt
Line 36: import xml.etree.ElementTree as ET
> This belongs to stdlib imports above, this is part of standard library.
Done
Line 37: 
Line 38: # vdsm imports
Line 39: from vdsm import constants
Line 40: from vdsm import libvirtconnection


Line 2626:                                     params=drive.custom)
Line 2627:         try:
Line 2628:             self._dom.detachDevice(driveXml)
Line 2629:             deadline = (utils.monotonic_time() +
Line 2630:                 config.getint('sampling', 'hotunplug_timeout'))
> Should be indented under utils. I don't think pep8 likes your indentation. 
Done
Line 2631:             while self.isDriveInDom(drive):
Line 2632:                 self.log.debug("Waiting for hotunplug to finish.")
Line 2633:                 time.sleep(1)
Line 2634:                 if utils.monotonic_time() > deadline:


Line 2628:             self._dom.detachDevice(driveXml)
Line 2629:             deadline = (utils.monotonic_time() +
Line 2630:                 config.getint('sampling', 'hotunplug_timeout'))
Line 2631:             while self.isDriveInDom(drive):
Line 2632:                 self.log.debug("Waiting for hotunplug to finish.")
> We may see 30 logs like this if a drive fail to detach, and I don't think i
This log will be redundant if we put it before the while, it will always log that message and that doesn't give us any indication of what happens.

Theoretically we are not supposed to see this message 30 times, only if the disk could not be removed at all. most of the times we are supposed not to see it at all or few times only.

The way it is now- it could give us an indication for how much time did it took to detach the disk, which could be helpful in some cases. (e.g why disk removal is taking to long)
Line 2633:                 time.sleep(1)
Line 2634:                 if utils.monotonic_time() > deadline:
Line 2635:                     return response.error('hotunplugDisk',
Line 2636:                                    "libvirt could not detach the disk.")


Line 2632:                 self.log.debug("Waiting for hotunplug to finish.")
Line 2633:                 time.sleep(1)
Line 2634:                 if utils.monotonic_time() > deadline:
Line 2635:                     return response.error('hotunplugDisk',
Line 2636:                                    "libvirt could not detach the disk.")
> Same indentation issue, should be aligned with 'hotunplugDisk'
Done
Line 2637:         except libvirt.libvirtError as e:
Line 2638:             self.log.exception("Hotunplug failed")
Line 2639:             if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
Line 2640:                 return response.error('noVM')


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I393ce55dd761ac825cb96bd499976fd74c366b09
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list