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 <from...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
