Change in vdsm[master]: vm: janitorial: introduce a recovering attribute.

nsoffer at redhat.com nsoffer at redhat.com
Mon Dec 30 11:16:31 UTC 2013


Nir Soffer has posted comments on this change.

Change subject: vm: janitorial: introduce a recovering attribute.
......................................................................


Patch Set 1:

(5 comments)

....................................................
Commit Message
Line 3: AuthorDate: 2013-12-27 16:37:38 +0100
Line 4: Commit:     Francesco Romani <fromani at redhat.com>
Line 5: CommitDate: 2013-12-27 16:37:38 +0100
Line 6: 
Line 7: vm: janitorial: introduce a recovering attribute.
We don't use a period end the end of the title.
Line 8: 
Line 9: this patch introduces an explicit recovering attribute to identify
Line 10: recovering VMs.
Line 11: 


Line 7: vm: janitorial: introduce a recovering attribute.
Line 8: 
Line 9: this patch introduces an explicit recovering attribute to identify
Line 10: recovering VMs.
Line 11: 
It would nice if you explain why this is better than the previous way. This is explained in the patch you link to, but why waste people time visiting that link?
Line 12: Spawned from the discussion of http://gerrit.ovirt.org/#/c/22556/
Line 13: 
Line 14: Change-Id: I1a43c5f6427c81a6610d820d9475b375c8dc0947


....................................................
File vdsm/vm.py
Line 2975:         self.log.info("VM wrapper has started")
Line 2976:         self.conf['smp'] = self.conf.get('smp', '1')
Line 2977:         devices = self.buildConfDevices()
Line 2978: 
Line 2979:         if not self.recovering:
It would be nice to switch the check and checks:

    if self.recovering:
        ...
    else:
        ...

Next patch?
Line 2980:             self.preparePaths(devices[DISK_DEVICES])
Line 2981:             self._prepareTransientDisks(devices[DISK_DEVICES])
Line 2982:             # Update self.conf with updated devices
Line 2983:             # For old type vmParams, new 'devices' key will be


Line 3008:         self._pathsPreparedEvent.set()
Line 3009: 
Line 3010:         if self.conf.get('migrationDest'):
Line 3011:             return
Line 3012:         if not self.recovering:
Here we have 2 blocks for "not recovering" state - one here
Line 3013:             domxml = hooks.before_vm_start(self._buildCmdLine(), self.conf)
Line 3014:             self.log.debug(domxml)
Line 3015:         if self.recovering:
Line 3016:             self._dom = NotifyingVirDomain(


Line 3037: 
Line 3038:             self._dom = NotifyingVirDomain(
Line 3039:                 self._connection.lookupByUUIDString(self.id),
Line 3040:                 self._timeoutExperienced)
Line 3041:         else:
The other here - can we merge them in another patch?
Line 3042:             flags = libvirt.VIR_DOMAIN_NONE
Line 3043:             if 'launchPaused' in self.conf:
Line 3044:                 flags |= libvirt.VIR_DOMAIN_START_PAUSED
Line 3045:                 self.conf['pauseCode'] = 'NOERR'


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a43c5f6427c81a6610d820d9475b375c8dc0947
Gerrit-PatchSet: 1
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: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list