Francesco Romani has posted comments on this change. Change subject: migration: added support for convergance schedule ......................................................................
Patch Set 4: Code-Review-1 (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 if 'convergenceSchedule' in kwargs: ... 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')) Ugh, this is thought to swallow. Can't we have the convergenceSchedule expressed in a form easier to parse, like json? I *really* want to avoid literal_eval (and worse, hand made parsers) unless it is absolutely needed 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]) this is hard to read, we need to simplify this. But first let's agree about how to express the schedule. 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 :) 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/) names in new code 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 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. 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 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) ? the rest comes for free 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 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 <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
