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

michal.skrivanek at redhat.com michal.skrivanek at redhat.com
Tue Apr 5 07:30:12 UTC 2016


Michal Skrivanek has posted comments on this change.

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


Patch Set 23: Code-Review-1

(5 comments)

https://gerrit.ovirt.org/#/c/52799/23/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 124: 
Line 125:         ('max_incoming_migrations', '3',
Line 126:             'Maximum concurrent incoming migrations'),
Line 127: 
Line 128:         ('migration_retry_timeout', '5',
I would use 10. Completely arbitrary, but sounds better to me:)
Line 129:             'Time (in sec) to wait before retrying failed migration.'),
Line 130: 
Line 131:         ('sys_shutdown_timeout', '120',
Line 132:             'Destroy and shutdown timeouts (in sec) before completing the '


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

Line 69: class MigrationLimitExceeded(RuntimeError):
Line 70:     """
Line 71:     Cannot migrate right now: no resources on destination.
Line 72:     """
Line 73:     # FIXME: should subclass MigrationDestinationSetupError?
so should it or not?
Line 74: 
Line 75: 
Line 76: class SourceThread(threading.Thread):
Line 77:     """


PS23, Line 338: retry_timeout
> here I just wanted to avoid the repeated call to config.getint inside a whi
if you move it to line 366 it doesn't get called when everything is smooth. And parsing an integer every 10s when there is a several orders of magnitude bigger overhead of starting that migration is not worth it. readability is more important


Line 353:                         with self._vm.migration_parameters(self.parameters):
Line 354:                             self._vm.saveState()
Line 355:                             self._startUnderlyingMigration(time.time())
Line 356:                             self._finishSuccessfully()
Line 357:                             self._completed = True
we already have finishSuccessfully:
- I would prefer not add yet another variable.
    check for VM being down should be enough
- if we have to - then use the same wording...finish vs completed
Line 358:                 except libvirt.libvirtError as e:
Line 359:                     if e.get_error_code() == libvirt.VIR_ERR_OPERATION_ABORTED:
Line 360:                         self.status = response.error(
Line 361:                             'migCancelErr', message='Migration canceled')


PS23, Line 673: is_retry_error
> Thank you for not minding :-) I really think it conveys the *intent* of the
since you have response.is_error() at line 399 I don't think it conveys anything but confusion;-) And you anyway have an explanation comment at line 401....


-- 
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: 23
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: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek 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