Change in vdsm[master]: migration: Add incoming migration semaphore

fromani at redhat.com fromani at redhat.com
Tue Oct 6 16:15:04 UTC 2015


Francesco Romani has posted comments on this change.

Change subject: migration: Add incoming migration semaphore
......................................................................


Patch Set 8: Code-Review-1

(4 comments)

Good improvements! the throttle decorator is very nice.
Few comments inside, -1 for visibility

https://gerrit.ovirt.org/#/c/45954/8//COMMIT_MSG
Commit Message:

Line 7: migration: Add incoming migration semaphore
Line 8: 
Line 9: Added throttling of incoming migrations.
Line 10: 
Line 11: 'migrationCreate' werb can now return new error status to signify full
to report full occupation?
Line 12: occupation.
Line 13: 
Line 14: Wiki: http://www.ovirt.org/Features/Migration_Enhancements
Line 15: Change-Id: I8952f732033ed160292b11fbc0c4deac099b2b3e


https://gerrit.ovirt.org/#/c/45954/8/lib/vdsm/response.py
File lib/vdsm/response.py:

Line 69:         return code != doneCode["code"]
Line 70: 
Line 71: 
Line 72: def is_success(res):
Line 73:     return not is_error(res)
please just use

  if not response.is_error(res)


https://gerrit.ovirt.org/#/c/45954/8/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 1277:     else:
Line 1278:         yield
Line 1279: 
Line 1280: 
Line 1281: def throttle(semaphore, err, blocking):
brilliant! But please split to a separate patch, and make sure to add tests to cover all the major flows.
Furthermore, this should probably be introduced in vdsm/virt/utils.py, and only after moved in the general utils.
At first glance looks fine, will review later.
Line 1282:     """
Line 1283:     Helper decorator for API methods that should have limited number
Line 1284:     of concurrent executions.
Line 1285: 


https://gerrit.ovirt.org/#/c/45954/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2777:             del self.conf['username']
Line 2778:         self.saveState()
Line 2779:         self.log.info("End of migration")
Line 2780: 
Line 2781:     def _checkMigrationCompletedDuringRecovery(self):
nice cleanup (although not 100% happy about the name) but please move it into a separate patch.
Line 2782:         if not self.recovering:
Line 2783:             return False
Line 2784: 
Line 2785:         try:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8952f732033ed160292b11fbc0c4deac099b2b3e
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <mbetak at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak <mbetak at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
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