Change in vdsm[master]: vm: check operation result for vm nic hotunplug

nsoffer at redhat.com nsoffer at redhat.com
Wed Nov 18 01:13:06 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
......................................................................


Patch Set 2: Code-Review-1

(3 comments)

Please write tests for this code before you change it.

https://gerrit.ovirt.org/#/c/48473/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2250:         try:
Line 2251:             self._dom.detachDevice(nicXml)
Line 2252:             mac = nicParams['macAddr']
Line 2253:             self._waitForDeviceRemoval("NIC (mac='" + mac + "')",
Line 2254:                                        self._construct_nic_search_path(mac))
Having common waitForDeviceRemoval makes sense, but the arguments to this function are too strange.

The arguments should be something like predicate function that return True when the device is removed, and a device dict/object.

Something like this:

    self._waitForDeviceRemoval(self.drive_attached, drive)

    self._waitForDeviceRemoval(self.nic_attached, nic)

Or maybe just the object you want to wait for. This object should have
a method is_attached, accepting the domain xml.
Line 2255:         except HotunplugTimeout as e:
Line 2256:             self.log.error("%s", e)
Line 2257:             return response.error('hotunplugNic', "%s" % e)
Line 2258:         except libvirt.libvirtError as e:


Line 2711:         source_key = {DISK_TYPE.FILE: 'file',
Line 2712:                       DISK_TYPE.BLOCK: 'dev',
Line 2713:                       DISK_TYPE.NETWORK: 'name'}
Line 2714:         return "./devices/disk/source[@%s='%s']" \
Line 2715:                % (source_key[drive.diskType], drive.path)
This should be a method or property of Drive object.
Line 2716: 
Line 2717:     def _construct_nic_search_path(self, mac):
Line 2718:         return "./devices/interface/mac[@address='%s']" % mac
Line 2719: 


Line 2714:         return "./devices/disk/source[@%s='%s']" \
Line 2715:                % (source_key[drive.diskType], drive.path)
Line 2716: 
Line 2717:     def _construct_nic_search_path(self, mac):
Line 2718:         return "./devices/interface/mac[@address='%s']" % mac
This should be a method or property of a Nic object.
Line 2719: 
Line 2720:     def _is_device_attached(self, device_search_path):
Line 2721:         root = ET.fromstring(self._dom.XMLDesc(0))
Line 2722:         return bool(root.findall(device_search_path))


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki <mmirecki at redhat.com>
Gerrit-Reviewer: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list