Francesco Romani has posted comments on this change.
Change subject: virt: Try to detect non guest iniated shutdowns ......................................................................
Patch Set 6: Code-Review-1
(2 comments)
I'm fine with the concept, Vinzenz explained the logic and the background and this is a good short term fix while we wait for a long term fix from libvirt. It is also suitable for backport. I may have found a bug in the logic and I'm not happy about the placement of the said logic. Besides that no problems, hence -1 for visibility only.
https://gerrit.ovirt.org/#/c/64991/6/lib/vdsm/virt/guestagent.py File lib/vdsm/virt/guestagent.py:
PS6, Line 191: time.time() in another patch, we should move to utils.monotonic_time()
https://gerrit.ovirt.org/#/c/64991/6/vdsm/virt/vm.py File vdsm/virt/vm.py:
PS6, Line 630: if stopped_shutdown: : # do not overwrite admin shutdown, if present : if reason is None: : # seen_shutdown is used to detect VMs that have been : # stopped by sending them SIG_TERM (e.g. system shutdown). : # In that case libvirt and qemu report a user initiated : # shutdown that is not correct. : seen_shutdown = not self.guestAgent or \ : self.guestAgent.has_seen_shutdown() : if seen_shutdown: : self._shutdownReason = vmexitreason.USER_SHUTDOWN : else: : self._shutdownReason = vmexitreason.HOST_SHUTDOWN the only concern I have with this patch is this logic should not go there: this method should just consume self._shutdownReason, not update it.
Please add one simple method like
def _set_shutdown_reason_from_event(self, detail): pass
(or whatever name you see fit) and move this logic here.
Please also note that you (correctly) set self._shutdownReason, yet, below (line 647) you use the old value, fetched in line 629. Is this intentional?