Change in vdsm[master]: migration: added support for convergence schedule

fromani at redhat.com fromani at redhat.com
Thu Feb 11 08:38:12 UTC 2016


Francesco Romani has posted comments on this change.

Change subject: migration: added support for convergence schedule
......................................................................


Patch Set 14: Code-Review+2

(4 comments)

more comments inside, but aiming for future patches.
I have concerns about readability, each time I find myself spending too much time reading the code and I need to switch between code and docs. We could and should improve this.

OTOH I don't see flaws in the implementation, we need this feature and we could and should refine it later. Please check the comment at like 122, that's my only mild concern.

https://gerrit.ovirt.org/#/c/46940/14/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 52: VIR_MIGRATE_PARAM_URI = 'migrate_uri'
Line 53: VIR_MIGRATE_PARAM_BANDWIDTH = 'bandwidth'
Line 54: VIR_MIGRATE_PARAM_GRAPHICS_URI = 'graphics_uri'
Line 55: 
Line 56: CONVERGENCE_SCHEDULE_SET_DOWNTIME = "setDowntime"
Future patch: maybe reformat like

class ConvergenceScheduleAction(object):
  SET_DOWNTIME = 'setDowntime'
  SET_ABORT = 'setAbort'
Line 57: CONVERGENCE_SCHEDULE_SET_ABORT = "abort"
Line 58: 
Line 59: ConvergenceAction = namedtuple('ConvergenceAction', 'action params')
Line 60: ConvergenceItem = namedtuple('ConvergenceItem',


Line 122: []
does it have to be empty list? I mean, below on the code we access sublists, is this right?


Line 119:         self._destServer = None
Line 120: 
Line 121:         progress_timeout = config.getint('vars', 'migration_progress_timeout')
Line 122:         schedule = [[], [[progress_timeout,
Line 123:                           [CONVERGENCE_SCHEDULE_SET_ABORT, []]]]]
maybe reformat like

  schedule = [
    [],
    [[progress_timeout,
     [CONVERGENCE_SCHEDULE_SET_ABORT, []]]
  ]

makes it easier to read?
Line 124: 
Line 125:         self._use_convergence_schedule = False
Line 126:         if 'convergenceSchedule' in kwargs:
Line 127:             # example convergence schedule


Line 132:         self._convergence_schedule = (
        :             [
        :                 ConvergenceAction(act, params)
        :                 for (act, params) in schedule[0]
        :             ], [
        :                 ConvergenceItem(lim, ConvergenceAction(act, params))
        :                 for lim, (act, params) in schedule[1]
        :             ]
        :         )
I suggested this format back in time, and still is the best I can think of at the moment. Yet I'm still not happy with it, it could be made more readable.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I989cff12d08ef1cab36bd10df7daaa999a8dac14
Gerrit-PatchSet: 14
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: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list