Francesco Romani has posted comments on this change. Change subject: vm: safe(r) Vm.conf update in creation thread ......................................................................
Patch Set 3: (1 comment) http://gerrit.ovirt.org/#/c/34813/3/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 1399: try: Line 1400: # while this code is running, Vm is queryable for status(), Line 1401: # thus we must fix devices in an atomic way, hence the deepcopy Line 1402: with self._confLock: Line 1403: devConf = deepcopy(self.conf['devices']) > I'm not convinced that we need to use deepcopy when using confLock in devic Without deepcopy() here, the creation thread will have 'devices' as a shallow copy of Vm.conf['devices']. The container will be a new instance; the contained device conf, however, will be shared among threads. Thus, every use of 'devices' on this thread needs to be confLock'd. This basically means lines 2649-2672 of _run(), and quite long critical sectiond because preparePath is included in that range. This is also harder to follow. If we use deepcopy(), the creation thread will have a hidden private copy of the device data, and it will atomically replace it into Vm.conf once all the setup is done, which is easier and nicer. Lastly, from the perspective of someone just querying the Vm: using deepcopy, the client sees either the initial configuration or the last fixed configuration, with no intermediate steps, which IMO is the best thing. Line 1404: except KeyError: Line 1405: # (very) old Engines do not send device configuration Line 1406: devices[DISK_DEVICES] = self.getConfDrives() Line 1407: devices[NIC_DEVICES] = self.getConfNetworkInterfaces() -- To view, visit http://gerrit.ovirt.org/34813 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie9dee2aa01b2c231b99e02a879dcfbd7ecc7f70a Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[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
