Mark Wu has posted comments on this change.
Change subject: dump the core of a VM
......................................................................
Patch Set 10: I would prefer that you didn't submit this
(18 inline comments)
most of the comments are just style issues.
....................................................
File vdsm/API.py
Line 257: :param to: a string designates the dump file
Line 258:
Line 259: :param params: a dictionary containing:
Line 260: *crash* - crash the domain after core dump
Line 261: *live* - perform a live core dump if supported
qemu-kvm definitely supports live core dump, so you could removed 'if supported'
Line 262: *bypass-cache* - avoid file system cache when saving
Line 263: *reset* - reset the domain after core dump
Line 264: *memory-only* - dump domain's memory only
Line 265: """
Line 273: 'live': False,
Line 274: 'bypass-cache': False,
Line 275: 'reset': False,
Line 276: 'memory-only': False}
Line 277: for k, v in params.items():
iteritems() is better.
Line 278: if k not in dumpParams or type(v) is not bool:
Line 279: msgstr = ("Core dump failed. Invalid argument: "
Line 280: "%s=%s" % (k, v))
Line 281: self.log.error(msgstr)
Line 274: 'bypass-cache': False,
Line 275: 'reset': False,
Line 276: 'memory-only': False}
Line 277: for k, v in params.items():
Line 278: if k not in dumpParams or type(v) is not bool:
better to change to isinstance(v, bool) since it supports old style class and class
inheritance.
Line 279: msgstr = ("Core dump failed. Invalid argument: "
Line 280: "%s=%s" % (k, v))
Line 281: self.log.error(msgstr)
Line 282: return reportError(msg=msgstr)
Line 281: self.log.error(msgstr)
Line 282: return reportError(msg=msgstr)
Line 283:
Line 284: dumpParams.update(params)
Line 285: exclusiveParams = ("live", "crash",
"reset")
for the exclusive options, you could use them as the value for a new option, like
'operation', then only of them could be given. You don't need validate the
exclusivity.
Line 286: if [dumpParams[x] for x in exclusiveParams].count(True) > 1:
Line 287: msgstr = "'reset', 'crash', and 'live'
are mutually exclusive."
Line 288: self.log.debug("Core dump failed. " + msgstr)
Line 289: return reportError(msg=msgstr)
....................................................
File vdsm/libvirtvm.py
Line 516: if not self._preparingMigrationEvt:
Line 517: raise
Line 518:
Line 519:
Line 520: class DoCoreDumpThread(vm.DoCoreDumpThread):
How about calling it CoreDumpThread?
Line 521:
Line 522: def dumpInfo(self):
Line 523: return self._vm._dom.jobInfo()
Line 524:
....................................................
File vdsm/vm.py
Line 297: self._vm = vm
Line 298: self.dumpfile = to
Line 299: self.dumpParams = kwargs
Line 300: self._mode = "memory" \
Line 301: if kwargs.get('memory-only') is True else
"core"
I don't understand why you need differentiate the modes and record it in vdsm?
The options is specified by the client, so it should know which mode of dump file it will
generate.
Line 302: self.dumpflag = 0
Line 303: self.status = {'status': {'code': 0, 'message':
'noJob'}}
Line 304:
Line 305: def getStat(self):
Line 303: self.status = {'status': {'code': 0, 'message':
'noJob'}}
Line 304:
Line 305: def getStat(self):
Line 306: """
Line 307: Get the status of the dump.
s/bump/core dump/
Line 308: """
Line 309: (jobType, timeElapsed, _,
Line 310: dataTotal, dataProcessed, dataRemaining,
Line 311: memTotal, memProcessed, memRemaining,
Line 314: dataProgress = (100 - 100 * dataRemaining / dataTotal
Line 315: if dataTotal else 0)
Line 316: self.log.info('dump status: job type: %d, dump Progress: '
Line 317: 'dataRemaining = %d, dataTotal=%d, %s%%
processed',
Line 318: jobType, dataRemaining, dataTotal, dataProgress)
if you don't want to add api to query the progress, then you don't need fetch the
jobinfo and log it.
Line 319: if self.isAlive():
Line 320: return self.status
Line 321: else:
Line 322: status = self.status
Line 329: "bypass-cache": libvirt.VIR_DUMP_BYPASS_CACHE,
Line 330: "reset": libvirt.VIR_DUMP_RESET,
Line 331: "memory-only": libvirt.VIR_DUMP_MEMORY_ONLY}
Line 332: self.dumpflag = 0
Line 333: for k, v in self.dumpParams.items():
the same as before. iteritems() could be better.
Line 334: if v is True:
Line 335: self.dumpflag |= params[k]
Line 336: if self.dumpParams.get('memory-only') is True:
Line 337: self._mode = "memory"
Line 330: "reset": libvirt.VIR_DUMP_RESET,
Line 331: "memory-only": libvirt.VIR_DUMP_MEMORY_ONLY}
Line 332: self.dumpflag = 0
Line 333: for k, v in self.dumpParams.items():
Line 334: if v is True:
you have validated the parameters in api level. So you could just use 'if v:'
Line 335: self.dumpflag |= params[k]
Line 336: if self.dumpParams.get('memory-only') is True:
Line 337: self._mode = "memory"
Line 338: else:
Line 338: else:
Line 339: self._mode = "core"
Line 340:
Line 341: def _status(self, msg=''):
Line 342: return {'status': {'code': 0, 'message': msg}}
I think you don't need add this function, and you can just update the status as this:
self.status['status']['message'] = msg or change the self.status =
{'code': 0, 'message': 'noJob'} and update it like
self.status['message'] = msg
Line 343:
Line 344: def run(self):
Line 345: def reportError(self, key='coreDumpErr', msg=None):
Line 346: if msg is None:
Line 348: else:
Line 349: error = {'status':
Line 350: {'code':
errCode[key]['status']['code'],
Line 351: 'message': msg}}
Line 352: self.log.exception("Failed to perform core dump. " + msg,
self.log.error?
Line 353: exc_info=True)
Line 354:
Line 355: return error
Line 356:
Line 357: try:
Line 358: self.log.debug("begin to perform core dump")
Line 359: if self._vm._dom is None:
Line 360: raise RuntimeError('noVM')
Line 361: self._setupDumpParams()
you could change to all it in the __init__ , and then you don't need keep dumpParams
any more.
Line 362: self.status = self._status(msg="ongoing")
Line 363: self._startUnderlyingDump()
Line 364: self.status = self._status(msg="finished")
Line 365: except libvirt.libvirtError, e:
Line 361: self._setupDumpParams()
Line 362: self.status = self._status(msg="ongoing")
Line 363: self._startUnderlyingDump()
Line 364: self.status = self._status(msg="finished")
Line 365: except libvirt.libvirtError, e:
libvirt.libvirtError as e:
Line 366: if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
Line 367: self.status = reportError(key='noVM')
Line 368: elif e.get_error_code() == libvirt.VIR_ERR_OPERATION_ABORTED:
Line 369: self.status = self._status(msg='canceled')
Line 1317: def isDoingDump(self):
Line 1318: return self._doCoredumpThread.isAlive()
Line 1319:
Line 1320: def dumpMode(self):
Line 1321: return self._doCoredumpThread._mode
why do you need this function? I don't see it's used by any function.
Line 1322:
Line 1323: def coreDump(self, to, params):
Line 1324: self._acquireCpuLockWithTimeout()
Line 1325: try:
Line 1335: # we do not care "Down"
Line 1336: self._doCoredumpThread = self.DoCoreDumpThreadClass(self, to,
Line 1337: **params)
Line 1338: self._doCoredumpThread.start()
Line 1339: check = self._doCoredumpThread.getStat()
status could be a better name. and you could move this call and the call in exception into
the finally block.
Line 1340: return check
Line 1341: except Exception, e:
Line 1342: self.log.exception("Failed to perform core dump. " +
e.message,
Line 1343: exc_info=True)
Line 1336: self._doCoredumpThread = self.DoCoreDumpThreadClass(self, to,
Line 1337: **params)
Line 1338: self._doCoredumpThread.start()
Line 1339: check = self._doCoredumpThread.getStat()
Line 1340: return check
move 'return' outside the try block, because it could not raise any exception
Line 1341: except Exception, e:
Line 1342: self.log.exception("Failed to perform core dump. " +
e.message,
Line 1343: exc_info=True)
Line 1344: check = self._doCoredumpThread.getStat()
Line 1338: self._doCoredumpThread.start()
Line 1339: check = self._doCoredumpThread.getStat()
Line 1340: return check
Line 1341: except Exception, e:
Line 1342: self.log.exception("Failed to perform core dump. " +
e.message,
the same log.error?
Line 1343: exc_info=True)
Line 1344: check = self._doCoredumpThread.getStat()
Line 1345: return check
Line 1346: finally:
--
To view, visit
http://gerrit.ovirt.org/7329
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If4aac9e747dc7aa64a6ff5ef256a7a4375aa2bb5
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Ryan Harper <ryanh(a)us.ibm.com>
Gerrit-Reviewer: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server