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

fromani at redhat.com fromani at redhat.com
Wed Nov 5 10:31:33 UTC 2014


Francesco Romani has posted comments on this change.

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


Patch Set 4: Code-Review-1

(8 comments)

Yes, I just took a too big step. It is not time yet to clean up the device/conf data confusion, and not in this convoluted way.

The review of parent patch just highlighted that the main assumption behind this change doesn't hold,
so no point to keep going in this direction.

So, what's left to do is fix the access of Vm.conf the way you suggested, by properly copying data with proper locking.
See much simpler and much less ambitious http://gerrit.ovirt.org/#/c/34813/

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
> I don't understand this comment - can you explain what the old code was doi
Vm object is created with conf given from outside (Engine usually), but once its __init__() has run, it is not fully initialized yet.

Full initialization happens here, because here we're fixing up the important Vm.conf['devices'] field, and building the Device object tree based on up-to-date Vm.conf['devices'] data. It all happens in buildConfDevices().

Now, the problem is that modern Engines already supply VDSM with some devices. Almost all of them.
VDSM however needs to make sure that all of them are present (e.g. GraphicsDevices) and each of them have all the fields and the fields have the correct data (normalizeVdsmImages).

This makes the dictionaries change.
The case I observed was about Drive.
Code was failing with RuntimeError because the two attributes
'truesize' and 'apparentsize' sprung up midway.

This change was supposed to fix that by
- fixing the device hierarchy before _run() is invoked and Vm is queryable (added to vmContainer). There is no good reason for this to change, Vm configuration is fixed and this flow can't change that.
- updating the fields which needs to be updated, with potentially slow storage access (getVolumeSize) asyncrhonously on _run()


Line 2667
Line 2668
Line 2669
Line 2670
Line 2671
> So why did you remove it?
Not removed, but moved in _setupDevices(), which is invoked in Vm.__init__()


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 so
We clear this in _run(), once Devices are synced with the updated conf.
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__ i
In general, I fully agree with you.

In this case, we are 'just' transforming the data we're given to build the internal representation.
Basically, this is buildConfDevices()

Until this transformation is done, the Vm class is not really initialized, because the internal representation is not yet the expected one.

I was not sure, but everything considered the above seemed enough to have this method called in __init__.
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.
Yes, it is a misplaced fix.
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 did not have this map before - why do we need it now?
Because this patch was trying to split the device conf normalization (the act of fixing the fields and making sure they all exist) from the configuration update (the act of filling them with real data, e.g. Drive apparent/truesize).

The intent was to avoid alltogether mutating data, so no RuntimeError possible ever, but altough the goal is right, the implementation is heading in the wrong direction.
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 t
Right, this was wrong.
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']
> This code should be in the Drive class. If a drive need to be updated using
Right
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