Change in vdsm[master]: vm: detect migration completed on recovery

fromani at redhat.com fromani at redhat.com
Mon Jun 30 13:44:32 UTC 2014


Francesco Romani has posted comments on this change.

Change subject: vm: detect migration completed on recovery
......................................................................


Patch Set 9:

(4 comments)

http://gerrit.ovirt.org/#/c/28511/9//COMMIT_MSG
Commit Message:

Line 17: 
Line 18: To handle this situation, we
Line 19: - connect early to the domain, on recovery. We know
Line 20:   this is safe because on recovery we iterate on the very
Line 21:   domain list libvirt provided to us moments before,
> but in what way this is "safe"? it might be only a problematic choice of wo
You understood my intent. But now, I've come to think about the changes made with 8ddfbf06915b76a6cbe4efa17bfbbcdad59dd965
and how well they play with this change, because that scenario wasn't covered by verification yet.
Line 22:   so the domain will be present
Line 23: - inspect the domain state *before* waitingr migration
Line 24:   termination, and skip the wait if the domain is detected
Line 25:   running.


Line 19: - connect early to the domain, on recovery. We know
Line 20:   this is safe because on recovery we iterate on the very
Line 21:   domain list libvirt provided to us moments before,
Line 22:   so the domain will be present
Line 23: - inspect the domain state *before* waitingr migration
> waitingr -> waiting for
will change.
Line 24:   termination, and skip the wait if the domain is detected
Line 25:   running.
Line 26: 
Line 27: A nice side-effect, this patch also tries to clarify a


http://gerrit.ovirt.org/#/c/28511/9/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 3010:         if self.recovering:
Line 3011:             self._dom = NotifyingVirDomain(
Line 3012:                 self._connection.lookupByUUIDString(self.id),
Line 3013:                 self._timeoutExperienced)
Line 3014:         elif 'migrationDest' in self.conf:
> that's simply "not initDomain", which I find clearer.
It is. But here I'm trying to have the code following and delimited by the four creation flows (create, recover, restoreState, migrationDestination), that's why I kept, on purpose, the 'migrationDest' name.
Line 3015:             pass  # self._dom will be None until migration ends.
Line 3016:         elif 'restoreState' in self.conf:
Line 3017:             fromSnapshot = self.conf.get('restoreFromSnapshot', False)
Line 3018:             srcDomXML = self.conf.pop('_srcDomXML')


Line 3587:                 try:
Line 3588:                     if self._isDomainRunning():
Line 3589:                         waitMigration = False
Line 3590:                         self.log.info('migration completed while recovering!')
Line 3591:                 except (libvirt.libvirtError, AttributeError):
> what is this evil AttributeError? due to self._dom being set to None asynch
Yes. I can switch to the LBYL idiom and check inside _isDomainRunning() for _dom not None if you like. I think it makes a little more sense.
Line 3592:                     self.log.exception('migration failed while recovering!')
Line 3593:                     self.setDownStatus(ERROR, vmexitreason.MIGRATION_FAILED)
Line 3594:                     return
Line 3595:             if waitMigration:


-- 
To view, visit http://gerrit.ovirt.org/28511
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I451c2a940693842e9bf7c63ccc117e75026bb11b
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list