Change in vdsm[master]: migration: Add retry on full capacity

fromani at redhat.com fromani at redhat.com
Mon Feb 29 16:46:58 UTC 2016


Francesco Romani has posted comments on this change.

Change subject: migration: Add retry on full capacity
......................................................................


Patch Set 18: Code-Review+1

(3 comments)

Looks good, but a couple of questions inside. Overall, -1 for visibility

https://gerrit.ovirt.org/#/c/52799/18/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 334:                             'migCancelErr', message='Migration canceled')
Line 335:                     raise
Line 336:                 except MigrationLimitExceeded:
Line 337:                     self.log.debug("Migration destination busy. Initiating "
Line 338:                                    "retry in %d seconds." % retry_timeout)
works. Better:

 self.log.debug("Migration destination busy. Initiating "
                "retry in %d seconds.", retry_timeout)

This is because python logging module will do the string formatting if and only if the message is going to be logged. Otherwise, the string formatting will always happen, and this is (indeed a [tiny] bit) wasteful.
Line 339:                     time.sleep(retry_timeout)
Line 340:         except MigrationDestinationSetupError as e:
Line 341:             self._recover(str(e))
Line 342:             # we know what happened, no need to dump hollow stack trace


Line 370:                           destCreationTime)
Line 371: 
Line 372:             if response.is_error(result):
Line 373:                 self.status = result
Line 374:                 # Differentiate between general migration failure and a retry
I don't think this comment is worth keeping. Which value does it add?
Line 375:                 if is_retry_error(result):
Line 376:                     raise MigrationLimitExceeded()
Line 377:                 else:
Line 378:                     raise MigrationDestinationSetupError(


Line 601:         self._vm.log.debug('stopping migration monitor thread')
Line 602:         self._stop.set()
Line 603: 
Line 604: 
Line 605: def is_retry_error(result):
I don't really see the point of this helper. Could you please elaborate why it is better to have it?
Btw, this is minor, I don't really mind that much.


-- 
To view, visit https://gerrit.ovirt.org/52799
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I988fa2e501eb77d121668b22cc533b744a3dc755
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak <mbetak at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjelinek at redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list