Nir Soffer has posted comments on this change. Change subject: Live Merge: Suspend disk stats collection during pivot ......................................................................
Patch Set 8: (3 comments) http://gerrit.ovirt.org/#/c/31367/8/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 5630: Line 5631: # A pivot changes the top volume being used for the VM Disk. Until Line 5632: # we can correct our metadata following the pivot we should not Line 5633: # attempt to collect disk stats. Line 5634: self.stopDisksStatsCollection() > In this series we are now doing the pivot synchronously. Therefore if stop Maybe - see bellow Line 5635: Line 5636: self.log.info("Requesting pivot to complete active layer commit " Line 5637: "(job %s)", jobID) Line 5638: flags = libvirt.VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT Line 5635: Line 5636: self.log.info("Requesting pivot to complete active layer commit " Line 5637: "(job %s)", jobID) Line 5638: flags = libvirt.VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT Line 5639: if self._dom.blockJobAbort(drive.name, flags) != 0: if this line raises - do we start disk stats collection? I think you need try finally block to ensure this. Line 5640: self.startDisksStatsCollection() Line 5641: raise RuntimeError("pivot failed") Line 5642: if mode in (LiveMergeCleanupThread.MODE_CLEANUP, Line 5643: LiveMergeCleanupThread.MODE_PIVOT): Line 5642: if mode in (LiveMergeCleanupThread.MODE_CLEANUP, Line 5643: LiveMergeCleanupThread.MODE_PIVOT): Line 5644: self.log.info("Live merge job completed (job %s)", jobID) Line 5645: self._syncVolumeChain(drive) Line 5646: self.startDisksStatsCollection() Do we have try finally block ensuring that we reach this line after line 5639 succeeded? I asked in a previous version of this patch (or maybe in another one), why the logic for pivot and cleanup is not written explicitly in the code of the cleanup thread, instead of having these confusing modes: def run(self): if self.pivot: stop stats collection... try: if self.pivot: do pivot... do cleanup... finally: if self.pivot: start stats collection... I probably miss something that prevent us from doing this in such a simple way - or not? Line 5647: else: Line 5648: raise RuntimeError("Invalid mode: '%s'" % mode) Line 5649: Line 5650: def _initLegacyConf(self): -- To view, visit http://gerrit.ovirt.org/31367 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I08689aec4d61871a568a6f92661b560fbf4d0b57 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[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: Greg Padgett <[email protected]> Gerrit-Reviewer: Nir Soffer <[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
