Milan Zamazal has posted comments on this change. Change subject: virt: vm: Update time on VM after resume ......................................................................
Patch Set 4: (5 comments) https://gerrit.ovirt.org/#/c/48860/4/tests/vmTests.py File tests/vmTests.py: Line 987: Line 988: def test_sync_time(self): Line 989: # No easy check it works, so let's just test it doesn't crash. Line 990: with fake.VM() as fakevm: Line 991: fakevm._syncTime() > This does not test anything. You should check that this invokes the proper Thanks for the hint. I tried to implement more meaningful tests. Line 992: Line 993: Line 994: VM_EXITS = tuple(product((define.NORMAL, define.ERROR), Line 995: vmexitreason.exitReasons.keys())) https://gerrit.ovirt.org/#/c/48860/4/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 1190: finally: Line 1191: if not guestCpuLocked: Line 1192: self._guestCpuLock.release() Line 1193: Line 1194: def _syncTime(self): > maybe _syncGuestTime? Yes, that's a better name. Done. Line 1195: """ Line 1196: Try to set VM time to the current value. This is typically useful when Line 1197: clock wasn't running on the VM for some time (e.g. during suspension or Line 1198: migration), especially if the time delay exceeds NTP tolerance. Line 1202: very precise (NTP in the guest should take care of it if needed). Line 1203: """ Line 1204: t = time.time() Line 1205: seconds = int(t) Line 1206: nseconds = int((t - seconds) * 10**9) > Use NSEC_PER_SEC? You mean introducing it as new constant? Not sure it's worth (well, it doesn't harm either). Line 1207: try: Line 1208: self._dom.setTime(time={'seconds': seconds, 'nseconds': nseconds}) Line 1209: except libvirt.libvirtError: Line 1210: # It may fail for various reasons, e.g. QEMU guest agent not Line 1208: self._dom.setTime(time={'seconds': seconds, 'nseconds': nseconds}) Line 1209: except libvirt.libvirtError: Line 1210: # It may fail for various reasons, e.g. QEMU guest agent not Line 1211: # running on the VM. Line 1212: self.log.exception("Failed to set time") > If this depends on qemu-guest-agent, it should use the error handling used Good idea, done. I'm not sure about debug level though. Shouldn't the user be warned when time is not updated? Line 1213: except virdomain.NotConnectedError: Line 1214: self.log.debug("Failed to set time: not connected") Line 1215: else: Line 1216: self.log.debug('Time updated to: %d.%09d', seconds, nseconds) Line 2802: hooks.after_vm_dehibernate(self._dom.XMLDesc(0), self.conf, Line 2803: {'FROM_SNAPSHOT': fromSnapshot}) Line 2804: # TODO: _syncTime() should probably be called for both the Line 2805: # restore_state and migration_dest paths, but let's handle just the Line 2806: # safer case first. > This comment does not belong here. Either move it to the commit message, or Moved to the other path and adjusted. Line 2807: self._syncTime() Line 2808: elif 'migrationDest' in self.conf: Line 2809: if self._needToWaitForMigrationToComplete(): Line 2810: usedTimeout = self._waitForUnderlyingMigration() -- 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: 4 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: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@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
