Change in vdsm[master]: vdsm hooks: report hook stderr to Engine if it fails an action

fromani at redhat.com fromani at redhat.com
Wed Jul 1 13:02:35 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 10:

(5 comments)

https://gerrit.ovirt.org/#/c/42592/10/vdsm/API.py
File vdsm/API.py:

Line 360:         # check for errors
Line 361:         if status and 'status' in status and 'code' in status['status'] and \
Line 362:            status['status']['code'] != 0:
Line 363:             return status
Line 364:         # if no error, return progress
this deserve a separate change. Moreover, this patch can be of further assistence: https://gerrit.ovirt.org/#/c/42795/
Line 365:         return {'status': doneCode, 'migrationStats': status}
Line 366: 
Line 367:     def getStats(self):
Line 368:         """


Line 379:             try:
Line 380:                 hooks.before_get_vm_stats()
Line 381:             except hooks.HookError as e:
Line 382:                 self.status = response.error('unexpected', 'Hook error ' +
Line 383:                                              'during migration: ' + str(e))
why assign to self.status?!

VERY minor: this formatting is IMHO a little nicer:

  response.error(
    'unexpected',
    'Hook error during migration: ' + str(e))
Line 384: 
Line 385:         stats = v.getStats().copy()
Line 386:         if runHooks:
Line 387:             stats = hooks.after_get_vm_stats([stats])[0]


Line 586:         params['vmId'] = self._UUID
Line 587:         result = self.create(params)
Line 588:         if result['status']['code']:
Line 589:             self.log.debug('Migration create - Failed')
Line 590:             return result
good change, but deserves separate patch (which I have somewehere stuck on gerrit...)
Line 591: 
Line 592:         v = self._cif.vmContainer.get(self._UUID)
Line 593: 
Line 594:         try:


Line 592:         v = self._cif.vmContainer.get(self._UUID)
Line 593: 
Line 594:         try:
Line 595:             if not v.waitForMigrationDestinationPrepare():
Line 596:                 return errCode['createErr']
better:

  return response.error('createErr')
Line 597:         except hooks.HookError as e:
Line 598:             self.log.debug('Destination VM creation failed due to hook' +
Line 599:                            ' error:' + str(e))
Line 600:             return response.error('migrateErr', 'Destination hook failed: ' +


https://gerrit.ovirt.org/#/c/42592/10/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 300:             self.status = response.error('migrateErr', 'Hook error ' +
Line 301:                                          'during migration: ' + str(e))
Line 302:             self.status = {'status': {'code': 12,
Line 303:                            'message': 'Hook error during migration: ' +
Line 304:                                       str(e)}}
Please use name for errors, and response.error.
If you need to use the straight code (it can rarely happen when translating errors) please use response.error_raw.


In general, this is the wrong direction, I'd rather get rid of self.status entirely.
Line 305:             self._recover(str(e))
Line 306:             self.log.error('Hook error during migration: ' + str(e))
Line 307: 
Line 308:         except Exception as e:


-- 
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: 10
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