Vinzenz Feenstra has posted comments on this change.
Change subject: dump the core of a VM
......................................................................
Patch Set 7: I would prefer that you didn't submit this
(6 inline comments)
....................................................
File vdsm/API.py
Line 263: *reset* - reset the domain after core dump
Line 264: *memory-only* - dump domain's memory only
Line 265: """
Line 266: def reportError(key='coreDumpErr', msg=None):
Line 267: self.log.error("Core dump failed. " + msg, exc_info=True)
self.log.exception for exception logging
Line 268: error = {'status':
Line 269: {'code': errCode[key]['status']['code'],
Line 270: 'message': msg}}
Line 271: return error
Line 276: 'reset': False,
Line 277: 'memory-only': False}
Line 278: for k, v in params.items():
Line 279: try:
Line 280: if k not in dumpParams or v is not bool:
type(v) is not bool
v is not bool would be always True even if v is True or False
Try it.
Line 281: raise ValueError("Core dump: Invalid argument:
%s=%s" %
Line 282: (k, v))
Line 283: except ValueError, e:
Line 284: return reportError(msg=e.message)
Line 286: dumpParams.update(params)
Line 287: exclusiveParams = ("live", "crash", "reset")
Line 288: if [dumpParams[x] for x in exclusiveParams].count(True) > 1:
Line 289: reportError(msg="'reset', 'crash', and 'live' "
Line 290: "are mutually exclusive.")
return missing?
Line 291: v = self._cif.vmContainer.get(self._UUID)
Line 292: if not v:
Line 293: return errCode['noVM']
Line 294: return v.coreDump(to, dumpParams)
....................................................
File vdsm_api/vdsmapi-schema.json
Line 5448: #
Line 5449: # @reset: #optional Reset the domain after core dump
Line 5450: #
Line 5451: # @memory-only: #optional Dump domain's memory only
Line 5452: #
I am missing the documentation that only one of crash, live and reset are
allowed
Line 5453: # Since: 4.10.4
Line 5454: #
Line 5455: ##
Line 5456: {'type': 'DumpParams',
....................................................
File vdsm/vm.py
Line 344: else:
Line 345: error = {'status':
Line 346: {'code': errCode[key]['status']['code'],
Line 347: 'message': msg}}
Line 348: self.log.error("Failed to perform core dump. " + msg,
why exc_info=True? I don't see a exception handler here, if this is called from
the exception handlers only, then you should log it with self.log.exception
Also you're using msg which might be "None" You probably wanted to use
error['status']['message'] instead.
Line 349: exc_info=True)
Line 350:
Line 351: return error
Line 352:
Line 1339: check = self._doCoredumpThread.getStat()
Line 1340: return check
Line 1341: except Exception, e:
Line 1342: self.log.error("Failed to perform core dump. " +
e.message,
Line 1343: exc_info=True)
self.log.exception
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Ryan Harper <[email protected]>
Gerrit-Reviewer: ShaoHe Feng <[email protected]>
Gerrit-Reviewer: Vinzenz Feenstra <[email protected]>
Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
