Change in vdsm[master]: migration: Add incoming migration semaphore

mpolednik at redhat.com mpolednik at redhat.com
Wed Sep 9 23:57:58 UTC 2015


Martin Polednik has posted comments on this change.

Change subject: migration: Add incoming migration semaphore
......................................................................


Patch Set 2: Code-Review-1

(5 comments)

https://gerrit.ovirt.org/#/c/45954/2/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 1271:     else:
Line 1272:         yield
Line 1273: 
Line 1274: 
Line 1275: @contextmanager
See comment in api.py
Line 1276: def releaseOnError(resource):
Line 1277:     try:
Line 1278:         yield
Line 1279:     except Exception:


https://gerrit.ovirt.org/#/c/45954/2/vdsm/API.py
File vdsm/API.py:

Line 574: 
Line 575:         if not migration.incomingMigrations.acquire(False):
Line 576:             return response.error('migrationLimit')
Line 577: 
Line 578:         with utils.releaseOnError(migration.incomingMigrations):
This feels unpythonic. You should create a contextmanager function that is able to handle this behavior without the need for the releaseOnError. Something like

    @contextmanager
    def acquire(semaphore, block):
        lock = semaphore.acquire(block)
        try:
            yield lock
        except:
            semaphore.release()
            raise

and then 

    with acquire(s, block=False) as acquired:
        if not acquired:
            return response.error('migrationLimit')
        ...
Line 579:             params['vmId'] = self._UUID
Line 580:             result = self.create(params)
Line 581:             if result['status']['code']:
Line 582:                 self.log.debug('Migration create - Failed')


https://gerrit.ovirt.org/#/c/45954/2/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 54: VIR_MIGRATE_PARAM_GRAPHICS_URI = 'graphics_uri'
Line 55: 
Line 56: 
Line 57: mig = min(config.getint('vars', 'max_incoming_migrations'),
Line 58:           caps.CpuTopology().cores())
I don't think that there is any relationship between the number of incoming migration and CPU cores, unlike outgoing migrations where one could argue the CPU cores can be equal to maximum number of compressions running.
Line 59: 
Line 60: incomingMigrations = threading.BoundedSemaphore(mig)
Line 61: 
Line 62: 


Line 330:                     dev._deviceXML, self._vm.conf, dev.custom)
Line 331:             hooks.before_vm_migrate_source(self._vm._dom.XMLDesc(0),
Line 332:                                            self._vm.conf)
Line 333: 
Line 334:             while True:
Why doesn't engine handle the case of too many incoming migrations on the host?
Line 335:                 # Do not measure the time spent for creating the VM on the
Line 336:                 # destination. In some cases some expensive operations can
Line 337:                 # cause the migration to get cancelled right after the
Line 338:                 # transfer started.


Line 347:                     SourceThread._ongoingMigrations.release()
Line 348:                     # the destination is busy with incoming migrations
Line 349:                     # release semaphore and give other outgoing migrations
Line 350:                     # a chance
Line 351:                     time.sleep(5)
seems very arbitrary and possibly works for short migrations, how about long migrations?
Line 352:                     SourceThread._ongoingMigrations.acquire()
Line 353:                 else:
Line 354:                     break
Line 355: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8952f732033ed160292b11fbc0c4deac099b2b3e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <mbetak at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjelinek at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list