Francesco Romani has uploaded a new change for review.
Change subject: vm: janitorial: simply the recovering flow ......................................................................
vm: janitorial: simply the recovering flow
This patch reorganizes and simplifies the control flow as proposed during the review of http://gerrit.ovirt.org/#/c/22783/ in order to have simpler and cleaner code.
Change-Id: Ic2c1bc48b9f4df7cb236f27c44755d9778e7e6c9 Signed-off-by: Francesco Romani fromani@redhat.com --- M vdsm/vm.py 1 file changed, 10 insertions(+), 8 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/15/23915/1
diff --git a/vdsm/vm.py b/vdsm/vm.py index 6e4feda..4914653 100644 --- a/vdsm/vm.py +++ b/vdsm/vm.py @@ -3093,7 +3093,11 @@ self.conf['smp'] = self.conf.get('smp', '1') devices = self.buildConfDevices()
- if not self.recovering: + if self.recovering: + for drive in devices[DISK_DEVICES]: + if drive['device'] == 'disk' and isVdsmImage(drive): + self.sdIds.append(drive['domainID']) + else: self.preparePaths(devices[DISK_DEVICES]) self._prepareTransientDisks(devices[DISK_DEVICES]) # Update self.conf with updated devices @@ -3111,10 +3115,6 @@ # So, to get proper device objects during VM recovery flow # we must to have updated conf before VM run self.saveState() - else: - for drive in devices[DISK_DEVICES]: - if drive['device'] == 'disk' and isVdsmImage(drive): - self.sdIds.append(drive['domainID'])
for devType, devClass in self.DeviceMapping: for dev in devices[devType]: @@ -3126,9 +3126,7 @@
if self.conf.get('migrationDest'): return - if not self.recovering: - domxml = hooks.before_vm_start(self._buildCmdLine(), self.conf) - self.log.debug(domxml) + if self.recovering: self._dom = NotifyingVirDomain( self._connection.lookupByUUIDString(self.id), @@ -3156,6 +3154,9 @@ self._connection.lookupByUUIDString(self.id), self._timeoutExperienced) else: + domxml = hooks.before_vm_start(self._buildCmdLine(), self.conf) + self.log.debug(domxml) + flags = libvirt.VIR_DOMAIN_NONE if 'launchPaused' in self.conf: flags |= libvirt.VIR_DOMAIN_START_PAUSED @@ -3165,6 +3166,7 @@ self._connection.createXML(domxml, flags), self._timeoutExperienced) hooks.after_vm_start(self._dom.XMLDesc(0), self.conf) + for dev in self._customDevices(): hooks.after_device_create(dev._deviceXML, self.conf, dev.custom)
oVirt Jenkins CI Server has posted comments on this change.
Change subject: vm: janitorial: simply the recovering flow ......................................................................
Patch Set 1:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6125/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7018/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6912/ : SUCCESS
Vinzenz Feenstra has posted comments on this change.
Change subject: vm: janitorial: simply the recovering flow ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
http://gerrit.ovirt.org/#/c/23915/1/vdsm/vm.py File vdsm/vm.py:
Line 3157: before_vm_start Moving this here, changes the semantics. It's not called when it's restoring the state any more.
Vinzenz Feenstra has posted comments on this change.
Change subject: vm: janitorial: simply the recovering flow ......................................................................
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/23915/1//COMMIT_MSG Commit Message:
Line 7: simply simplify?
Francesco Romani has posted comments on this change.
Change subject: vm: janitorial: simply the recovering flow ......................................................................
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/23915/1/vdsm/vm.py File vdsm/vm.py:
Line 3157: before_vm_start
Moving this here, changes the semantics. It's not called when it's restorin
Agreed. I do have a question however (not to argue but to achieve a better understanding). The original code called those two lines also in the restore state pathm but:
* the domxml object returned is never used directly; do we rely here on a side effect of buildCmdLine, side effect which has to be reserved? * the before_vm_start hook is called, but not the after_vm_start one.
Is this really the intended behaviour or there is a bug lurking here?
Francesco Romani has posted comments on this change.
Change subject: vm: janitorial: simply the recovering flow ......................................................................
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/23915/1//COMMIT_MSG Commit Message:
Line 3: AuthorDate: 2014-01-30 16:37:40 +0100 Line 4: Commit: Francesco Romani fromani@redhat.com Line 5: CommitDate: 2014-01-30 16:37:40 +0100 Line 6: Line 7: vm: janitorial: simply the recovering flow
simplify?
Typo :) will fix, thanks. Line 8: Line 9: This patch reorganizes and simplifies the control flow Line 10: as proposed during the review of Line 11: http://gerrit.ovirt.org/#/c/22783/
Francesco Romani has posted comments on this change.
Change subject: vm: janitorial: simplify the recovering flow ......................................................................
Patch Set 2:
Patch Set 2:
* restored the behaviour as Vinzenz pointed out. My question in the comments of Patch Set 1 will be addressed in a different change
oVirt Jenkins CI Server has posted comments on this change.
Change subject: vm: janitorial: simplify the recovering flow ......................................................................
Patch Set 2:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6130/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7023/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6917/ : SUCCESS
Nir Soffer has posted comments on this change.
Change subject: vm: janitorial: simplify the recovering flow ......................................................................
Patch Set 2:
(1 comment)
Using if self.recovering is little better then if not self.recovering, but the second change does not improve anything if not little worse.
The real problem in this method is trying to do 3 things: run new vm, revover running vm, and recoverState(?). The real fix would be to split the big method into 2 or 3 smaller methods that do one thing.
This may require extracting some helper methods to avoid code duplication and make the code more clear.
It is not easy as the code have no tests and is messy.
http://gerrit.ovirt.org/#/c/23915/2/vdsm/vm.py File vdsm/vm.py:
Line 3127 Line 3128 Line 3129 Line 3130 Line 3131 This was better here without duplicating the code.
Francesco Romani has posted comments on this change.
Change subject: vm: janitorial: simplify the recovering flow ......................................................................
Patch Set 2:
Nir, I am willing to try to do the way you suggested, which was, in my head, the logical next step after the one being discussed here; I'd also to add/extend some tests along the way.
Should I abandon this change or may I go ahead here, also amending the commit message if it's the case?
Nir Soffer has posted comments on this change.
Change subject: vm: janitorial: simplify the recovering flow ......................................................................
Patch Set 2:
Do what is easier for you.
Francesco Romani has abandoned this change.
Change subject: vm: janitorial: simplify the recovering flow ......................................................................
Abandoned
abandoned. We need deeper changes of the code.
vdsm-patches@lists.fedorahosted.org
-
fromani＠redhat.com -
Jenkins CI RO -
Nir Soffer -
Vinzenz Feenstra