Change in vdsm[master]: migration added migrateChangeParams verb

fromani at redhat.com fromani at redhat.com
Fri Oct 2 12:21:07 UTC 2015


Francesco Romani has posted comments on this change.

Change subject: migration added migrateChangeParams verb
......................................................................


Patch Set 2: Code-Review-1

(6 comments)

few comments inside, but prtty much all of them are minor. -1 for visibility.

https://gerrit.ovirt.org/#/c/46934/2/vdsm/API.py
File vdsm/API.py:

Line 560:         """
Line 561:         try:
Line 562:             v = self._cif.vmContainer[self._UUID]
Line 563:         except KeyError:
Line 564:             return errCode['noVM']
let's use response.error() in the new code
Line 565:         return v.migrateChangeParams(params)
Line 566: 
Line 567:     def migrateCancel(self):
Line 568:         """


https://gerrit.ovirt.org/#/c/46934/2/vdsm/rpc/vdsmapi-schema.json
File vdsm/rpc/vdsmapi-schema.json:

Line 7757: # Parameters for @VM.migrateChangeParams
Line 7758: #
Line 7759: # @maxBandwidth:    #optional the new bandwidth
Line 7760: #
Line 7761: # Since: TODO
> what should be the version? 4.0?
I *guess* 5.0, but we'll need input from Dan here.
Line 7762: ##
Line 7763: {'type': 'MigrateChangeParamsParams',
Line 7764:  'data': {'*maxBandwidth': 'int'}}
Line 7765: 


https://gerrit.ovirt.org/#/c/46934/2/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 403:             self._vm._dom.migrateToURI3(duri, params, flags)
Line 404:         else:
Line 405:             self._raiseAbortError()
Line 406: 
Line 407:     def migrateChangeParams(self, params):
IMO you don't need to have this facade here. You can do this in vm.py, and expose only the specific knobs (like set_max_bandwidth) here.
Line 408:         if 'maxBandwidth' in params:
Line 409:             self._set_max_bandwidth(params['maxBandwidth'])
Line 410: 
Line 411:     def _set_max_bandwidth(self, bandwidth):


Line 410: 
Line 411:     def _set_max_bandwidth(self, bandwidth):
Line 412:         self._vm.log.debug('setting migration max bandwidth to %d', bandwidth)
Line 413:         self._maxBandwidth = int(bandwidth)
Line 414:         self._vm._dom.migrateSetMaxSpeed(int(bandwidth))
this (self._vm._dom....) is a little ugly, but I'm afraid we can't do better.
Line 415: 
Line 416:     def stop(self):
Line 417:         # if its locks we are before the migrateToURI3()
Line 418:         # call so no need to abortJob()


https://gerrit.ovirt.org/#/c/46934/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1546:         finally:
Line 1547:             self._guestCpuLock.release()
Line 1548: 
Line 1549:     def migrateChangeParams(self, params):
Line 1550:         # TODO check if the self._acquireCpuLockWithTimeout() is needed here
> @Francesco - Im really not sure here. What you think?
I don't think you need. You may need to make sure that migration is in progress, and that it is not to file.
Let's check the libvirt docs to be sure.

BTW, and most likely out of scope of your patch(es), both the CpuLockWithTimeout and the migration.SourceThread handling are in need of a serious cleanup here.
Line 1551:         self._migrationSourceThread.migrateChangeParams(params)
Line 1552:         return {'status': doneCode}
Line 1553: 
Line 1554:     def _customDevices(self):


Line 1548: 
Line 1549:     def migrateChangeParams(self, params):
Line 1550:         # TODO check if the self._acquireCpuLockWithTimeout() is needed here
Line 1551:         self._migrationSourceThread.migrateChangeParams(params)
Line 1552:         return {'status': doneCode}
please use response.success() in new code
Line 1553: 
Line 1554:     def _customDevices(self):
Line 1555:         """
Line 1556:             Get all devices that have custom properties


-- 
To view, visit https://gerrit.ovirt.org/46934
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I52219c0e1f7d619dd30441dd4c70bd401f91e56d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek <tjelinek at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Tomas Jelinek <tjelinek at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list