Change in vdsm[master]: vm: split slow disk access from normalization

fromani at redhat.com fromani at redhat.com
Wed Nov 5 08:14:14 UTC 2014


Francesco Romani has posted comments on this change.

Change subject: vm: split slow disk access from normalization
......................................................................


Patch Set 2:

(3 comments)

this change was the preparation step for the next one (34753).
But if, as it now seems clear, we can't split the device normalization (make sure the device config has all the correct fields) from the device initialization (filling the required fields with up to date values), then this is the wrong direction.

http://gerrit.ovirt.org/#/c/34752/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1337:         if 'device' not in drv:
Line 1338:             drv['device'] = 'disk'
Line 1339: 
Line 1340:         drv['truesize'] = 0
Line 1341:         drv['apparentsize'] = 0
> This is bad idea - this creates an unwanted state where disk size is set to
Good catch, this is a point I definitely overlooked. Please see below for the full context.
Line 1342: 
Line 1343:     def _getVdsmImgSizes(self, drv):
Line 1344:         if drv['device'] == 'disk':
Line 1345:             res = self.cif.irs.getVolumeSize(drv['domainID'], drv['poolID'],


Line 1339: 
Line 1340:         drv['truesize'] = 0
Line 1341:         drv['apparentsize'] = 0
Line 1342: 
Line 1343:     def _getVdsmImgSizes(self, drv):
> This name is also confusing, as this does not return anything. It should be
Right.
Line 1344:         if drv['device'] == 'disk':
Line 1345:             res = self.cif.irs.getVolumeSize(drv['domainID'], drv['poolID'],
Line 1346:                                              drv['imageID'], drv['volumeID'])
Line 1347:             if res['status']['code'] != 0:


Line 2645: 
Line 2646:     def _run(self):
Line 2647:         self.log.info("VM wrapper has started")
Line 2648:         devices = self.buildConfDevices()
Line 2649:         self._updateDiskSizes(devices[DISK_DEVICES])
> This is a good change - it is more clear that we do this step  here compare
This is a very good point, but this unfortunately also means that this change of mine doesn't really solve the problem I'm tackling.

In the following patch, I'm trying to solve https://bugzilla.redhat.com/show_bug.cgi?id=1143968
by building on this change.

The problem I observed is the Drive configuration is mutating, and this is the cause of RuntimeError I reported.
That's the reason why I added the bogus keys.

But bogus keys are worse than no keys, and you're right about the states we should export (either no data or correct data).

So I'm back to the drawing board :)
Line 2650: 
Line 2651:         # recovery flow note:
Line 2652:         # we do not start disk stats collection here since
Line 2653:         # in the recovery flow irs may not be ready yet.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6d8cb7a3c58e37540e56f4359aa67ea4dce875cc
Gerrit-PatchSet: 2
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