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

fromani at redhat.com fromani at redhat.com
Wed Oct 28 16:42:31 UTC 2015


Francesco Romani has posted comments on this change.

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


Patch Set 18:

(2 comments)

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

Line 54: VIR_MIGRATE_PARAM_GRAPHICS_URI = 'graphics_uri'
Line 55: 
Line 56: incomingMigrations = threading.BoundedSemaphore(
Line 57:     min(config.getint('vars', 'max_incoming_migrations'),
Line 58:         caps.CpuTopology().cores()))
I'm still concerned about the interplay between this sempahore and Vm.ongoingCreations. Did we discussed this interplay? (Sorry if we did, my memory failed me here).

(Sorry for repetition, just ignore if we indeed already discussed all of this)
OK, the purposes of the two semaphores are different. Yet, they have quite some overlap.

In the case of migrationDestination VM, the ongoingCreations semaphore still needs to be hold, in the chunk of code that does the path preparation.

I wonder if there can be any choking between the two semaphores.
Line 59: 
Line 60: 
Line 61: class MigrationDestinationSetupError(RuntimeError):
Line 62:     """


https://gerrit.ovirt.org/#/c/45954/18/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 752:                 self.log.exception("The vm start process failed")
Line 753:                 self.setDownStatus(ERROR, vmexitreason.GENERIC_ERROR, str(e))
Line 754:         finally:
Line 755:             if 'migrationDest' in self.conf:
Line 756:                 migration.incomingMigrations.release()
better, but we have a problem here. _completeIncomingMigration will remove 'migrationDest' from self conf, so we need to add one flag at the beginning of the function and check that flag:


    def _startUnderlyingVm(self):
        self.log.debug("Start")
        try:
            self.memCommit()
            isMigrationDest = 'migrationDest' in self.conf
            ...
        finally:
            if isMigrationDest:
                 migration.incomingMigrations.release()

Another problem: we must make sure to match acquire() and release(). (probably unlikely?) recover while migration can mess up the semaphore, we should make sure this is not possible.

I'm thinking about the case on which VDSM restarts while waiting for migration to complete. On this case, migrationCreate() won't be called, thus we will have unbalanced acquire() and release()

We may want to add code in the recovery flow to cover this case, but doesn't seem so easy to get right.
Line 757: 
Line 758:     def incomingMigrationPending(self):
Line 759:         return 'migrationDest' in self.conf or 'restoreState' in self.conf
Line 760: 


-- 
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: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <mbetak 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: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list