Nir Soffer has posted comments on this change. Change subject: vm.py: State saving in hotunplugDisk. ......................................................................
Patch Set 3: Code-Review-1 (3 comments) https://gerrit.ovirt.org/#/c/45077/3/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2633 Line 2634 Line 2635 Line 2636 Line 2637 This also fixes a bug in the old code, calling the before_disk_hotunplug hook with the drive removed from conf, *before* it was removed. Now the hook will be called before the drive was removed. However, every bug you fix in vdsm can reveal another bug - please check that that we don't document this buggy behavior, and that vdsm does not have bad hooks assuming this buggy behavior. Line 2632: else: Line 2633: hooks.after_disk_hotunplug(driveXml, self.conf, Line 2634: params=drive.custom) Line 2635: Line 2636: self._devices[hwclass.DISK].remove(drive) > we should keep the order as before, removal of the disk before the hook is Right, the hook should the state after the removal. But even more important, if the hook will raise, we end with incorrect state in memory (self._devices) and on disk (self.conf). Line 2637: Line 2638: # Find and remove disk device from vm's conf Line 2639: for dev in self.conf['devices'][:]: Line 2640: if (dev['type'] == hwclass.DISK and Line 2643: self.conf['devices'].remove(dev) Line 2644: break Line 2645: Line 2646: self._cleanupDrives(drive) Line 2647: self.saveState() > please keep _saveState() before _cleanupDrives(). This is another function that may raise and leave the disk state out of sync. Line 2648: Line 2649: return {'status': doneCode, 'vmList': self.status()} Line 2650: Line 2651: def _readPauseCode(self): -- To view, visit https://gerrit.ovirt.org/45077 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Amit Aviram <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Amit Aviram <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
