Francesco Romani has posted comments on this change. Change subject: vm.py: State saving in hotunplugDisk. ......................................................................
Patch Set 1: Code-Review-1 (4 comments) questions inside, -1 for visibility. Please also briefly state how you verified this patch. https://gerrit.ovirt.org/#/c/45077/1//COMMIT_MSG Commit Message: Line 9: VM's state was saved before detatching a disk in hotunplugDisk. Line 10: Line 11: Saving vm's state should be performed only AFTER vdsm is actually detaching Line 12: the disk, otherwise the disk will remain in xmlDesc and be calculated in Line 13: the vm's hash. true. Another issue is that the xml refresh is well hidden inside saveState, and this coude would be clearer if the XML update would be made manifest. But maybe this is out of scope. Line 14: This causes a bug in the engine's VM monitoring: the engine Line 15: gets the same hash as before the disk was unplugged, thus does not Line 16: update the disk's status. Line 17: https://gerrit.ovirt.org/#/c/45077/1/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2644: # Restore disk device in vm's conf and _devices Line 2645: if diskDev: Line 2646: with self._confLock: Line 2647: self.conf['devices'].append(diskDev) Line 2648: self.saveState() if we move the saveState() out of the try block - as we should! this becomes unnecessary and should go away as well. Line 2649: return response.error('hotunplugDisk', e.message) Line 2650: else: Line 2651: hooks.after_disk_hotunplug(driveXml, self.conf, Line 2652: params=drive.custom) Line 2646: with self._confLock: Line 2647: self.conf['devices'].append(diskDev) Line 2648: self.saveState() Line 2649: return response.error('hotunplugDisk', e.message) Line 2650: else: I'd rather saveState here... Line 2651: hooks.after_disk_hotunplug(driveXml, self.conf, Line 2652: params=drive.custom) Line 2653: self._cleanupDrives(drive) Line 2654: Line 2649: return response.error('hotunplugDisk', e.message) Line 2650: else: Line 2651: hooks.after_disk_hotunplug(driveXml, self.conf, Line 2652: params=drive.custom) Line 2653: self._cleanupDrives(drive) ...or even here (disclaimer: need to check what cleanupDrives does). Line 2654: Line 2655: return {'status': doneCode, 'vmList': self.status()} Line 2656: Line 2657: 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: 1 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
