Change in vdsm[master]: vdsm hooks: report hook stderr to Engine if it fails an action
fromani at redhat.com
fromani at redhat.com
Thu Jul 2 14:08:13 UTC 2015
Francesco Romani has posted comments on this change.
Change subject: vdsm hooks: report hook stderr to Engine if it fails an action
......................................................................
Patch Set 12: Code-Review-1
(9 comments)
This version is a bit worse, and maybe I've contributed to it with cryptic suggestions. See inside for more verbose comments.
https://gerrit.ovirt.org/#/c/42592/12/lib/vdsm/response.py
File lib/vdsm/response.py:
Line 40: "message": message or status["message"]
Line 41: }
Line 42: }
Line 43:
Line 44:
spurious change, please remove
Line 45: def error_raw(code, message):
Line 46: return {
Line 47: "status": {
Line 48: "code": code,
Line 49: "message": message
Line 50: }
Line 51: }
Line 52:
Line 53:
same
Line 54: def is_failure(res):
https://gerrit.ovirt.org/#/c/42592/12/vdsm/API.py
File vdsm/API.py:
Line 355: v = self._cif.vmContainer[self._UUID]
Line 356: except KeyError:
Line 357: return errCode['noVM']
Line 358:
Line 359: status = v.get_last_migration_status()
why is not v.migrateStatus() not good enough?
Line 360: # check for errors
Line 361: if response.is_failure(status):
Line 362: return status
Line 363: # if no error, return progress
Line 585: params['vmId'] = self._UUID
Line 586: result = self.create(params)
Line 587: if result['status']['code']:
Line 588: self.log.debug('Migration create - Failed')
Line 589: return result
please split these three in a (trivial) separate patch, which will prepare the soil to use response module inside API.py, and which will get almost instanteously my +1
Line 590:
Line 591: v = self._cif.vmContainer.get(self._UUID)
Line 592:
Line 593: try:
Line 591: v = self._cif.vmContainer.get(self._UUID)
Line 592:
Line 593: try:
Line 594: if not v.waitForMigrationDestinationPrepare():
Line 595: return response.errCode['createErr']
perhaps you mean
response.error('createErr')
?
Line 596: except hooks.HookError as e:
Line 597: self.log.debug('Destination VM creation failed due to hook' +
Line 598: ' error:' + str(e))
Line 599: return response.error('migrateErr', 'Destination hook failed: ' +
https://gerrit.ovirt.org/#/c/42592/12/vdsm/virt/migration.py
File vdsm/virt/migration.py:
Line 323
Line 324
Line 325
Line 326
Line 327
spurious change, please remove
Line 103: def hibernating(self):
Line 104: return self._mode == MODE_FILE
Line 105:
Line 106: def get_last_status(self):
Line 107: return self.lastStatus
Well, I feel that I've very likely derailed your work. Let me fix that.
It is true that we do have plans to de-entangle the mess of status reporting in the migration.SourceThread (
https://gerrit.ovirt.org/#/c/42794/2 , https://gerrit.ovirt.org/#/c/42796/2 ,
) but this code isn't close enough to get merged - days away at least. So, for the time being, there is no point in being forward looking: just access self.status wherever you need to.
Please note that SourceThread.status and SourceThread._vm.lastStatus are two wildly different beasts.
SourceThread.status it is a response value, the same value you get from the functions in the response module.
SourceThread._vm.lastStatus is an enumerated value (aka one integer), see vdsm/virt/vmstatus.py
I *think* you need SourceThread.status in your patch.
Line 108:
Line 109: def getStat(self):
Line 110: """
Line 111: Get the status of the migration.
Line 301: del self._vm.conf['_migrationParams']
Line 302: SourceThread._ongoingMigrations.release()
Line 303: except hooks.HookError as e:
Line 304: self._last_status = response.error('migrateErr', 'Hook error ' +
Line 305: 'during migration: ' + str(e))
you probably want to do
self.status = response.error(...)
(which looks OK)
Line 306: self._recover(str(e))
Line 307: self.log.error('Hook error during migration: ' + str(e))
Line 308:
Line 309: except Exception as e:
https://gerrit.ovirt.org/#/c/42592/12/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 1542: finally:
Line 1543: self._guestCpuLock.release()
Line 1544:
Line 1545: def get_last_migration_status(self):
Line 1546: return self._migrationSourceThread.get_last_status()
I think we don't need this (or please explain why we do)
Line 1547:
Line 1548: def migrateStatus(self):
Line 1549: return self._migrationSourceThread.getStat()
Line 1550:
--
To view, visit https://gerrit.ovirt.org/42592
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe2d5eb52cf2c8855d9d7d5e0ff1628a6cf1dc51
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki <mmirecki at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Feng Yang <yangfeng at cloud-times.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list