Change in vdsm[master]: vm: move device setup into __init__

nsoffer at redhat.com nsoffer at redhat.com
Tue Nov 4 16:45:02 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: vm: move device setup into __init__
......................................................................


Patch Set 4:

(8 comments)

The nature of the error is very clear, and it is easy to fix by iterating over a copy of the keys or the values.

The solution you suggest is not clear.

http://gerrit.ovirt.org/#/c/34753/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2662
Line 2663
Line 2664
Line 2665
Line 2666
> Up to here old code was changing devices which is a hierarchy of conf data,
I don't understand this comment - can you explain what the old code was doing, and how the new code change it?


Line 2667
Line 2668
Line 2669
Line 2670
Line 2671
> This makes the Device instances from Conf data, so they are created correct
So why did you remove it?


Line 1301:         self._powerDownEvent = threading.Event()
Line 1302:         self._liveMergeCleanupThreads = {}
Line 1303:         self._shutdownReason = None
Line 1304: 
Line 1305:         self._diskMap = {}
When do we clear this cache? It looks like a temporary cache to speed up some operation, but its exits for the entire life of the vm object - right?
Line 1306:         self._setupDevices()
Line 1307: 
Line 1308:     def _get_lastStatus(self):
Line 1309:         PAUSED_STATES = (vmstatus.POWERING_DOWN, vmstatus.REBOOT_IN_PROGRESS,


Line 1302:         self._liveMergeCleanupThreads = {}
Line 1303:         self._shutdownReason = None
Line 1304: 
Line 1305:         self._diskMap = {}
Line 1306:         self._setupDevices()
Adding more stuff to __init__ is not a good idea. Doing stuff in __init__ is always fragile and make code harder to test.

Why we must do this here?
Line 1307: 
Line 1308:     def _get_lastStatus(self):
Line 1309:         PAUSED_STATES = (vmstatus.POWERING_DOWN, vmstatus.REBOOT_IN_PROGRESS,
Line 1310:                          vmstatus.UP)


Line 1518:         return [{
Line 1519:             'type': GRAPHICS_DEVICES,
Line 1520:             'device': (
Line 1521:                 'spice'
Line 1522:                 if self.conf.get('display') in ('qxl', 'qxlnc')
This seems unrelated to this patch.
Line 1523:                 else 'vnc'),
Line 1524:             'specParams': makeSpecParams(self.conf)}]
Line 1525: 
Line 1526:     def getConfSound(self):


Line 2654: 
Line 2655:         for devType, devClass in self.DeviceMapping:
Line 2656:             for dev in devices[devType]:
Line 2657:                 devObj = devClass(self.conf, self.log, **dev)
Line 2658:                 self._devices[devType].append(devObj)
> we need this _diskMap cache only to speedup updateDrivesFromConf below. Oth
We did not have this map before - why do we need it now?
Line 2659:                 if devType == DISK_DEVICES and isVdsmImage(dev):
Line 2660:                     self._diskMap[(dev['domainID'], dev['poolID'],
Line 2661:                                    dev['imageID'], dev['volumeID'])] = devObj
Line 2662: 


Line 5377: 
Line 5378:     def getDiskDevicesConf(self):
Line 5379:         for dev in self.conf['devices']:
Line 5380:             if dev['type'] == DISK_DEVICES:
Line 5381:                 yield dev
This is not safe - while some code iterate over the list of devices using this generator, the  list may be modified by another thread.

It is better to avoid such code in multithreaded program.

The name also does not suggest that this is a generator. It looks like an accessor.
Line 5382: 
Line 5383:     @property
Line 5384:     def sdIds(self):
Line 5385:         """


Line 5401:                 if dev:
Line 5402:                     dev.path = drv['path']
Line 5403:                     dev.format = drv['format']
Line 5404:                     dev.apparentsize = drv['apparentsize']
Line 5405:                     dev.truesize = drv['truesize']
> these are he fields (potentially) changed into lines 2667-2675
This code should be in the Drive class. If a drive need to be updated using a dict, the drive should have an update(dict) method.

This will change this to:

    for drive in drives:
        drive.update(conf)
Line 5406:                 else:
Line 5407:                     self.log.error('missing Drive from conf: %s', drv)
Line 5408: 
Line 5409: 


-- 
To view, visit http://gerrit.ovirt.org/34753
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e5a8e7916bb0c0d2519949397a5c1c64085e08e
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list