Francesco Romani has posted comments on this change. Change subject: virt: migration: always run migration monitor ......................................................................
Patch Set 12: (3 comments) http://gerrit.ovirt.org/#/c/25975/12//COMMIT_MSG Commit Message: Line 9: Before this patch the migration monitor thread is always Line 10: run, unless the migration monitor interval configuration option Line 11: is explicitely set to zero. Line 12: Line 13: As far as we know, this is rarely done, and there are hidden > could you un-hide such an assumption? Well, turns out we currently have just one explicit, being the migrationProgress reporting. Will investigate more to discover if it's just me expecting the monitor always be present, and, given our future goal to leverage migration progress/stall detection to improve the downtime (and general migration behaviour/convergence), if make sense to support the disabling of the migration monitor. Line 14: assumptions scattered in the code about the monitor thread Line 15: always be present. Line 16: Line 17: This patch enforce those assumptions, and let this thread always http://gerrit.ovirt.org/#/c/25975/12/vdsm/virt/migration.py File vdsm/virt/migration.py: Line 394: def run(self): Line 395: if self.enabled: Line 396: self.monitor_migration() Line 397: else: Line 398: self.idle() > Could you suggest at least a single case where it is anyhow helpful to have On second thought there is no real value in having this "idle" loop, and we can simplify the code anyway. Will remove, by pushing the check inside the monitor class. Line 399: Line 400: def monitor_migration(self): Line 401: def calculateProgress(remaining, total): Line 402: if remaining == 0: Line 464: (timeElapsed / 1000, self.progress)) Line 465: Line 466: def idle(self): Line 467: while not self._stop.isSet(): Line 468: time.sleep(1.0) > how about self._stop.wait(forever) ? No real need. Will remove. Line 469: Line 470: def stop(self): Line 471: self._vm.log.debug('stopping migration monitor thread') -- To view, visit http://gerrit.ovirt.org/25975 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib4953ddda5a5c6c0ecd7ea0f95377309e18f771a Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
