Francesco Romani has posted comments on this change. Change subject: migration: added support for convergance schedule ......................................................................
Patch Set 1: (6 comments) initial review https://gerrit.ovirt.org/#/c/46940/1/vdsm/virt/migration.py File vdsm/virt/migration.py: Line 365: self._vm, Line 366: int(self._downtime), Line 367: config.getint('vars', 'migration_downtime_steps')) Line 368: with utils.running(downtimeThread): Line 369: with utils.running(self._monitorThread): we now require python 2.7+, so we can get rid of nested with-s. I really don't like None checks, I prefer an approach like https://en.wikipedia.org/wiki/Null_Object_pattern which also fits quite nicely into the python's duck typing approach. Feel free to reorganize the code flow here to avoid if something is None checks, maybe into a later patch. Line 370: # we need to support python 2.6, so two nested with-s. Line 371: self._perform_migration(duri, muri) Line 372: else: Line 373: with utils.running(self._monitorThread): Line 485: self._vm = vm Line 486: self._startTime = startTime Line 487: self.daemon = True Line 488: self.progress = 0 Line 489: self._convergenceSchedule = convergenceSchedule we want/need to access the data in convergenceSchedule in order, here and in the foreseeble future, so I'd just do self._convergenceSchedule = iter(convergenceSchedule) (of course dealing with None/default values) Line 490: Line 491: @property Line 492: def enabled(self): Line 493: return MonitorThread._MIGRATION_MONITOR_INTERVAL > 0 Line 543: elif (now - lastProgressTime) > progress_timeout: : # Migration is stuck, abort : self._vm.log.warn( : 'Migration is stuck: Hasn\'t progressed in %s seconds. ' : 'Aborting.' % (now - lastProgressTime)) : abort = True : : if abort: : self._vm._dom.abortJob() : self.stop() : break I think this snippet should be repacked as VDSM's default convergenceSchedule, in the case Engine sends nothing. This way the code is more regular and easier to follow. Line 571: self._stop.set() Line 572: Line 573: def _next_action(self, stalling): Line 574: it = iter(self._convergenceSchedule) Line 575: head = it.next() better to use head = next(it) the name 'next' for iterators in python 2 was a design mistake. In python 3 it is renamed __next__, and the recommended (and forward compatible) way to call it is using indeed next(it) Line 576: if head[0] < stalling: Line 577: self._execute_next_step(stalling, head[1:]) Line 578: self._convergenceSchedule = list(it) Line 579: Line 572: Line 573: def _next_action(self, stalling): Line 574: it = iter(self._convergenceSchedule) Line 575: head = it.next() Line 576: if head[0] < stalling: You may want to translate the Engine-provided convergenceSchedule into a collections.namedtuple for better readability. Sure, it is one O(n) repacking, but I think it is cheap enough and worth the readibility gain. Line 577: self._execute_next_step(stalling, head[1:]) Line 578: self._convergenceSchedule = list(it) Line 579: Line 580: def _execute_next_step(self, stalling, actionWithParams): Line 578: self._convergenceSchedule = list(it) Line 579: Line 580: def _execute_next_step(self, stalling, actionWithParams): Line 581: action = actionWithParams[0] Line 582: if action == 'setDowntime': better to have some module-level constants here, or maybe even a new module (as you find most fitting) Line 583: self.log.debug('Stalling for %d, setting downtime to %d', Line 584: stalling, actionWithParams[1]) Line 585: elif action == 'abort': Line 586: self.log.debug('Stalling for %d, aborting migration', stalling) -- 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: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Jelinek <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
