Milan Zamazal has posted comments on this change. Change subject: virt: vm: Update time on VM after resume ......................................................................
Patch Set 9: (5 comments) https://gerrit.ovirt.org/#/c/48860/9/tests/vmTests.py File tests/vmTests.py: Line 1564: current_time = time.time() Line 1565: self.assertLess(abs(passed_time - current_time), 1) Line 1566: orig_set_time(time=time_) Line 1567: Line 1568: def test_sync_guest_time(self): > Should be something like test_success, do not repeat the class name here. Done Line 1569: vm = self._make_vm() Line 1570: dom = vm._dom Line 1571: orig_set_time = dom.setTime Line 1572: with MonkeyPatchScope([ Line 1575: ]): Line 1576: vm._syncGuestTime() Line 1577: calls = dom.__calls__ Line 1578: self.assertEqual(len(calls), 1) Line 1579: self.assertEqual(calls[0][0], 'setTime') > You are spreading asserts all over, making it hard to understand. Indeed, patching time.time is a much better idea, thanks for it. Done. Line 1580: Line 1581: @permutations([[libvirt.VIR_ERR_AGENT_UNRESPONSIVE], Line 1582: [libvirt.VIR_ERR_NO_SUPPORT], Line 1583: [libvirt.VIR_ERR_INTERNAL_ERROR]]) Line 1582: [libvirt.VIR_ERR_NO_SUPPORT], Line 1583: [libvirt.VIR_ERR_INTERNAL_ERROR]]) Line 1584: def test_sync_guest_time_on_error(self, virt_error): Line 1585: vm = self._make_vm(virt_error=virt_error) Line 1586: vm._syncGuestTime() > It looks like you test that libvirt errors are swallowed. The test should b Thanks for letting me know about assertNotRaises, nice feature. Done. Line 1587: Line 1588: Line 1589: def _load_xml(name): Line 1590: test_path = os.path.realpath(__file__) 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. Done 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: > ok, disconnects shouldn't happen and are a reason to propagate all the way After additional discussion with Michal I left this untouched here. It doesn't break anything and it's safer. Maybe it's not needed but unless we're absolutely sure about consequences (we're not), we should remain on the safe side. I added comment about that. 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: -- 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
