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 <from...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches