Nir Soffer has posted comments on this change. Change subject: virt: vm: Update time on VM after resume ......................................................................
Patch Set 9: Code-Review-1 (3 comments) https://gerrit.ovirt.org/#/c/48860/9/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 1206: nseconds = int((t - seconds) * 10**9) Line 1207: try: Line 1208: self._dom.setTime(time={'seconds': seconds, 'nseconds': nseconds}) Line 1209: except libvirt.libvirtError as e: Line 1210: log_method = self.log.debug We don't need to add this complexity just to log with different log level. If we want to use a common prefix ("Failed to set time"), can add it in a simple way: format = "Failed to set time: %s" if code == x: self.log.debug(format, "message 1" elif code = y: self.log.debug(format, "message 2" else: self.log.error(format, e) Line 1211: code = e.get_error_code() Line 1212: if code == libvirt.VIR_ERR_AGENT_UNRESPONSIVE: Line 1213: message = "QEMU agent unresponsive" Line 1214: elif code == libvirt.VIR_ERR_NO_SUPPORT: Line 1216: else: Line 1217: message = e Line 1218: log_method = self.log.error Line 1219: log_method("Failed to set time: %s", message) Line 1220: except virdomain.NotConnectedError: Why do need to handle this here? We don't want to handle this error in every method. It should bubble up to the main error handler. Line 1221: self.log.debug("Failed to set time: not connected") Line 1222: else: Line 1223: self.log.debug('Time updated to: %d.%09d', seconds, nseconds) Line 1224: Line 2814: del self.conf['restoreState'] Line 2815: fromSnapshot = self.conf.pop('restoreFromSnapshot', False) Line 2816: hooks.after_vm_dehibernate(self._dom.XMLDesc(0), self.conf, Line 2817: {'FROM_SNAPSHOT': fromSnapshot}) Line 2818: self._syncGuestTime() If we lost connection here, it should bubble up to the top level error handler. Here we continue with the normal flow, which seems like a bad idea. Line 2819: elif 'migrationDest' in self.conf: Line 2820: if self._needToWaitForMigrationToComplete(): Line 2821: usedTimeout = self._waitForUnderlyingMigration() Line 2822: self._attachLibvirtDomainAfterMigration( -- To view, visit https://gerrit.ovirt.org/48860 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieb583cd5d21e56d7730b0ba21d75ed93b9d34025 Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
