Change in vdsm[master]: virt: vm: Update time on VM after resume

michal.skrivanek at redhat.com michal.skrivanek at redhat.com
Fri Dec 11 13:45:00 UTC 2015


Michal Skrivanek has posted comments on this change.

Change subject: virt: vm: Update time on VM after resume
......................................................................


Patch Set 9: -Code-Review

(2 comments)

https://gerrit.ovirt.org/#/c/48860/9/vdsm/virt/vm.py
File vdsm/virt/vm.py:

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 ever
ok, disconnects shouldn't happen and are a reason to propagate all the way up. I agree.

Such assumption is not always ok though, as e.g. VM related functions are failing around lifecycle events and we don't want to bail out. But this should never be the case for libvirt connectivity
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()
> But syncGuestTime does not know anything about migration, this is not the p
syncGuestTime never supposed to fail the operation. It won't fail migration resume, cont from EIO...so I don't see a reason why to make the code complicated.
So it makes sense the "handling" is within syncGuestTime.
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 <mzamazal at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list