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 <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
