Change in vdsm[master]: migration: Add incoming migration semaphore
by automation＠ovirt.org
automation(a)ovirt.org has posted comments on this change.
Change subject: migration: Add incoming migration semaphore
......................................................................
Patch Set 6:
* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
--
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: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjelinek(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No
8 years, 7 months
Change in vdsm[master]: migration: Add retry logic for incoming limit
by fromani＠redhat.com
Francesco Romani has posted comments on this change.
Change subject: migration: Add retry logic for incoming limit
......................................................................
Patch Set 1:
(2 comments)
initial review
https://gerrit.ovirt.org/#/c/46971/1/vdsm/virt/migration.py
File vdsm/virt/migration.py:
Line 328: dev._deviceXML, self._vm.conf, dev.custom)
Line 329: hooks.before_vm_migrate_source(self._vm._dom.XMLDesc(0),
Line 330: self._vm.conf)
Line 331:
Line 332: while not self._migrationCanceledEvt:
I'd reverse the loop logic.
Let's assume we want this retry logic (I'm still not sold, but for the sake of this patch let's assume we want it).
So we need to retry N times, being N="forever" by default (we may want to allow Engine to limit the retries in a future patch, that could be a nice compromise).
Do, I think it should go like this:
while True:
result = migrationCreate() # always at least one try, like before
if response.success(result):
break
elif result == response.error('migrationLimit'):
if not self.migrationCanceledEvt:
# we will retry. A log would be nice
continue
else:
# log this
break
elif response.is_error(result):
# generic error: log this and
break
You can rearrange the checks as you find nice, but my point here is that we should somehow pack all the cycle breaker conditions together.
Line 333: # Do not measure the time spent for creating the VM on the
Line 334: # destination. In some cases some expensive operations can
Line 335: # cause the migration to get cancelled right after the
Line 336: # transfer started.
Line 346: # so don't try again
Line 347: if self._migrationCanceledEvt:
Line 348: break
Line 349:
Line 350: SourceThread._ongoingMigrations.release()
I'm not sure we need this management here. Will get back on this later.
Line 351: # the destination is busy with incoming migrations
Line 352: # release semaphore and give other outgoing migrations
Line 353: # a chance
Line 354: time.sleep(5)
--
To view, visit https://gerrit.ovirt.org/46971
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icabe79dfccb61ce43489a9a242a5390e73979649
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjelinek(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes
8 years, 7 months
Change in vdsm[master]: migration: Add incoming migration semaphore
by fromani＠redhat.com
Francesco Romani has posted comments on this change.
Change subject: migration: Add incoming migration semaphore
......................................................................
Patch Set 5:
(2 comments)
https://gerrit.ovirt.org/#/c/45954/5/lib/vdsm/utils.py
File lib/vdsm/utils.py:
Line 1277: yield
Line 1278:
Line 1279:
Line 1280: @contextmanager
Line 1281: def acquired(resource, block):
acquired vs acquire
still, I'm not happy with this code. The problem is that a context manager inherently suggests that we are going to execute some code with some resource, or with a fixed preamble/postamble.
But here we just want to make sure to release the resource on error, while on success we will have a wildly different code path.
For this reason I'm more and more convinced that a plain try/except in the calling site is clearer.
Line 1282: success = resource.acquire(block)
Line 1283: try:
Line 1284: yield success
Line 1285: except Exception:
https://gerrit.ovirt.org/#/c/45954/5/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 2752: fromSnapshot = self.conf.pop('restoreFromSnapshot', False)
Line 2753: hooks.after_vm_dehibernate(self._dom.XMLDesc(0), self.conf,
Line 2754: {'FROM_SNAPSHOT': fromSnapshot})
Line 2755: elif 'migrationDest' in self.conf:
Line 2756: migration.incomingMigrations.release()
what if waitMigration == True?
If so, we still need to transfer some data, so migration is not done yet.
Line 2757: waitMigration = True
Line 2758: if self.recovering:
Line 2759: try:
Line 2760: if self._isDomainRunning():
--
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjelinek(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes
8 years, 7 months
Change in vdsm[master]: migration: Add retry logic for incoming limit
by automation＠ovirt.org
automation(a)ovirt.org has posted comments on this change.
Change subject: migration: Add retry logic for incoming limit
......................................................................
Patch Set 1:
* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
--
To view, visit https://gerrit.ovirt.org/46971
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icabe79dfccb61ce43489a9a242a5390e73979649
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No
8 years, 7 months
Change in vdsm[master]: migration: Add incoming migration semaphore
by automation＠ovirt.org
automation(a)ovirt.org has posted comments on this change.
Change subject: migration: Add incoming migration semaphore
......................................................................
Patch Set 5:
* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
--
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjelinek(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No
8 years, 7 months
Change in vdsm[master]: migration: Add incoming migration semaphore
by mbetak＠redhat.com
Martin Betak has posted comments on this change.
Change subject: migration: Add incoming migration semaphore
......................................................................
Patch Set 3:
(2 comments)
https://gerrit.ovirt.org/#/c/45954/3/vdsm/virt/migration.py
File vdsm/virt/migration.py:
Line 56:
Line 57: mig = min(config.getint('vars', 'max_incoming_migrations'),
Line 58: caps.CpuTopology().cores())
Line 59:
Line 60: incomingMigrations = threading.BoundedSemaphore(mig)
> let's avoid this temporary, please.
Done
Line 61:
Line 62:
Line 63: class MigrationDestinationSetupError(RuntimeError):
Line 64: """
Line 342: startTime += destCreationTime
Line 343: self.log.info('Creation of destination VM took: %d seconds',
Line 344: destCreationTime)
Line 345:
Line 346: if result['status'] == errCode['migrationLimit']['status']:
> please consider use (and probably enhance) response.is_error.
Done
Line 347: # we failed, but migration was cancelled in the meantime
Line 348: # so don't try again
Line 349: if self._migrationCanceledEvt:
Line 350: break
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjelinek(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes
8 years, 7 months
Change in vdsm[master]: migration: Add incoming migration semaphore
by automation＠ovirt.org
automation(a)ovirt.org has posted comments on this change.
Change subject: migration: Add incoming migration semaphore
......................................................................
Patch Set 4:
* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
--
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjelinek(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No
8 years, 7 months
Change in vdsm[master]: migration: Add incoming migration semaphore
by mbetak＠redhat.com
Martin Betak has posted comments on this change.
Change subject: migration: Add incoming migration semaphore
......................................................................
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/45954/3/vdsm/API.py
File vdsm/API.py:
Line 571: :type params: dict
Line 572: """
Line 573: self.log.debug('Migration create')
Line 574:
Line 575: with utils.acquire(migration.incomingMigrations,
> acquire or acquired? (see utils.py)
I would say that 'acquire'. I modeled this after the standard library where you have:
with open(...) as f:
and not
with opened(...) as f:
Line 576: block=False) as acquired:
Line 577: if not acquired:
Line 578: return response.error('migrationLimit')
Line 579:
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjelinek(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes
8 years, 7 months
Change in vdsm[master]: migration: Add incoming migration semaphore
by mbetak＠redhat.com
Martin Betak has posted comments on this change.
Change subject: migration: Add incoming migration semaphore
......................................................................
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/45954/3/lib/vdsm/utils.py
File lib/vdsm/utils.py:
Line 1273:
Line 1274:
Line 1275: @contextmanager
Line 1276: def acquired(resource, block):
Line 1277: success = resource.acquire(block)
> success is not a good name here. (And now I lack suggestions!)
I could call it 'acquired' but that would be probably one too many of variations to acquire[d] :-)
Line 1278: try:
Line 1279: yield success
Line 1280: except Exception:
Line 1281: resource.release()
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjelinek(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes
8 years, 7 months
Change in vdsm[master]: vdsm: isolate fake_kvm into a hook
by fromani＠redhat.com
Francesco Romani has posted comments on this change.
Change subject: vdsm: isolate fake_kvm into a hook
......................................................................
Patch Set 4: Code-Review+1
(3 comments)
looks good, only few comments/questions inside
https://gerrit.ovirt.org/#/c/46897/4/vdsm_hooks/fakearch/after_get_caps.py
File vdsm_hooks/fakearch/after_get_caps.py:
Line 45: 'rhel6.4.0',
Line 46: 'rhel6.0.0',
Line 47: 'rhel6.5.0']
Line 48:
Line 49: return ppc64le_machines + x86_64_machines
why not just one constant per arch?
_PPC64_LE_MACHINES = [...]
_X86_64_MACHINES = [...]
or maybe a dict whose keys are the supported arches.
Line 50:
Line 51:
Line 52: if __name__ == '__main__':
Line 53: if config.getboolean('vars', 'fake_kvm_support'):
Line 54: caps = hooking.read_json()
Line 55: arch = config.get('vars', 'fake_kvm_architecture')
Line 56:
Line 57: caps['kvmEnabled'] = str(config.getboolean('vars', 'fake_kvm_support'))
Line 58: caps['emulatedMachines'] = _get_fake_emulated_machines()
I don't think we need _get_fake_emulated_machines() function
Line 59:
Line 60: if arch == 'x86_64':
Line 61: caps['cpuModel'] = 'Intel(Fake) CPU'
Line 62:
Line 69:
Line 70: caps['cpuFlags'] = ','.join(flags) + ',model_486,model_pentium,' \
Line 71: 'model_pentium2,model_pentium3,model_pentiumpro,' \
Line 72: 'model_qemu32,model_coreduo,model_core2duo,model_n270,' \
Line 73: 'model_Conroe,model_Penryn,model_Nehalem,model_Opteron_G1'
for a future patch/version (if you have time): let's make this string a list/tuple of flags.
Line 74: elif arch in ('ppc64le', 'ppc64', 'ppc'):
Line 75: caps['cpuModel'] = 'POWER 8(fake)'
Line 76: caps['cpuFlags'] = 'powernv,model_POWER8'
Line 77: else:
--
To view, visit https://gerrit.ovirt.org/46897
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I178e0c1d5a9a8ffca2bf28b2fb404b42729dfff4
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes
8 years, 7 months