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