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?
--
To view, visit
https://gerrit.ovirt.org/64991
To unsubscribe, visit
https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie04b9806fbf0a81dc576aa28cfdda5edb079ce29
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vinzenz Feenstra <vfeenstr(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes