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