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

tjelinek at redhat.com tjelinek at redhat.com
Mon Nov 2 10:51:09 UTC 2015


Tomas Jelinek has posted comments on this change.

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


Patch Set 4:

(10 comments)

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

Line 135:             with utils.running(self._monitorThread):
Line 136:                 self._perform_migration(duri, muri)
Line 137: 
Line 138:         self._do_perform_migration = _perform_with_downtime_thread
Line 139:         if kwargs.has_key('convergenceSchedule'):
> let's just use
Done
Line 140:             # example convergence schedule
Line 141:             # "[(1, ('setDowntime', (10))), (4, ('abort', ()))]"
Line 142:             schedule = literal_eval(kwargs.get('convergenceSchedule'))
Line 143:             self._do_perform_migration = \


Line 138:         self._do_perform_migration = _perform_with_downtime_thread
Line 139:         if kwargs.has_key('convergenceSchedule'):
Line 140:             # example convergence schedule
Line 141:             # "[(1, ('setDowntime', (10))), (4, ('abort', ()))]"
Line 142:             schedule = literal_eval(kwargs.get('convergenceSchedule'))
> Proposal:
right, also better to send json from the engine
Line 143:             self._do_perform_migration = \
Line 144:                 _perform_with_conv_schedule_monitor_thread
Line 145: 
Line 146:         self._convergenceSchedule = map(ConvergenceItem._make,


Line 145: 
Line 146:         self._convergenceSchedule = map(ConvergenceItem._make,
Line 147:                                         [(s[0],
Line 148:                                           map(ConvergenceAction._make,
Line 149:                                               s[1:])[0]) for s in schedule])
> whatever format we choose, we can also write:
MUCH better! Thank you, done.
Line 150: 
Line 151:         self.log.debug('convergence schedule set to: ' + str(self._convergenceSchedule))
Line 152: 
Line 153: 


Line 399:             self._vm.log.info('starting migration to %s '
Line 400:                               'with miguri %s', duri, muri)
Line 401: 
Line 402:             self._monitorThread = MonitorThread(self._vm, startTime, self._convergenceSchedule)
Line 403:             self._do_perform_migration(self, duri, muri)
> no need to have self twice :)
actually... :) the _do_perform_migration is assigned to self in the constructor and the actual function is a function nested inside the constructor and expects the self to be the one in which it can find the _vm in.

But based on your comment I suspect I did not find the most pythonic solution ;)
Line 404: 
Line 405:             self.log.info("migration took %d seconds to complete",
Line 406:                           (time.time() - startTime) + destCreationTime)
Line 407: 


Line 519:         self._vm = vm
Line 520:         self._startTime = startTime
Line 521:         self.daemon = True
Line 522:         self.progress = 0
Line 523:         self._convSchedule = convSchedule
> conv_schedule, let's use pep8 (https://www.python.org/dev/peps/pep-0008/) n
Done
Line 524: 
Line 525:     @property
Line 526:     def enabled(self):
Line 527:         return MonitorThread._MIGRATION_MONITOR_INTERVAL > 0


Line 557:              fileTotal, fileProcessed, _) = self._vm._dom.jobInfo()
Line 558:             # from libvirt sources: data* = file* + mem*.
Line 559:             # docs can be misleading due to misaligned lines.
Line 560:             now = time.time()
Line 561: 
> spurious extra line, please drop
Done
Line 562:             if 0 < migrationMaxTime < now - self._startTime:
Line 563:                 self._vm.log.warn('The migration took %d seconds which is '
Line 564:                                   'exceeding the configured maximum time '
Line 565:                                   'for migrations of %d seconds. The '


Line 577:                     'Migration stalling: remaining (%sMiB)'
Line 578:                     ' > lowmark (%sMiB).'
Line 579:                     ' Refer to RHBZ#919201.',
Line 580:                     dataRemaining / Mbytes, lowmark / Mbytes)
Line 581:                 self._next_action(now - lastProgressTime)
> not sure this needs to be in at this nesting level.
right
Line 582: 
Line 583:                 if self._stop.isSet():
Line 584:                     break
Line 585: 


Line 579:                     ' Refer to RHBZ#919201.',
Line 580:                     dataRemaining / Mbytes, lowmark / Mbytes)
Line 581:                 self._next_action(now - lastProgressTime)
Line 582: 
Line 583:                 if self._stop.isSet():
> ditto
Done
Line 584:                     break
Line 585: 
Line 586:             if jobType != libvirt.VIR_DOMAIN_JOB_NONE:
Line 587:                 self.progress = update_progress(dataRemaining, dataTotal)


Line 594:         self._vm.log.debug('stopping migration monitor thread')
Line 595:         self._stop.set()
Line 596: 
Line 597:     def _next_action(self, stalling):
Line 598:         head, tail = self._convSchedule[0], self._convSchedule[1:]
> head = self._convSchedule.pop(0)
I need to assign the tail to the conv_schedule only if head.stallingLimit < stalling while the pop would remove the head always.
Line 599:         self._vm.log.debug('Stalling for %d seconds, '
Line 600:                            'trying to make next action: '
Line 601:                            '%s, tail: %s', stalling, head, tail)
Line 602:         if head.stallingLimit < stalling:


Line 605:             self._convSchedule = tail
Line 606: 
Line 607:     def _execute_next_step(self, stalling, actionWithParams):
Line 608:         action = str(actionWithParams.action)
Line 609:         if action == CONVERGENCE_SCHEDULE_SET_DOWNTIME:
> you can pass around function and/or closures if you like/feel
hm, do you mean to provide some map like convergance_actions which would map the action name to action so it will look here like this?

convergance_actions[action]()

In this case not sure where to define this functions so it will not reduce the readability...
The functions are quite short and simple and there is just a few of them and it is not expected that they will grow.
What you think?
Line 610:             downtime = actionWithParams.params[0]
Line 611:             self.log.warn('Stalling for %d, setting downtime to %d',
Line 612:                            stalling, downtime)
Line 613:             self._vm._dom.migrateSetMaxDowntime(int(downtime), 0)


-- 
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek <tjelinek at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani 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