Mark Wu has posted comments on this change.
Change subject: Add an option to create a watchdog device.
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(13 inline comments)
....................................................
Commit Message
Line 8:
Line 9: A support for a watchdog device was added to the "devices"
Line 10: configuration.
Line 11:
Line 12: Also add the watchdog event callback.
Add a description for what you do in the callback function.
Line 13:
Line 14: model of watchdog:
Line 15: 'i6300esb' default model, emulating a PCI Intel 6300ESB
Line 16: 'ib700' emulating an ISA iBase IB700
Line 13:
Line 14: model of watchdog:
Line 15: 'i6300esb' default model, emulating a PCI Intel 6300ESB
Line 16: 'ib700' emulating an ISA iBase IB700
Line 17: only one watchdog device was support.
Only one watchdog device is supported for each VM.
Line 18:
Line 19: action of watchdog:
Line 20: 'reset' default, forcefully reset the guest
Line 21: 'shutdown' gracefully shutdown the guest (not recommended)
Line 15: 'i6300esb' default model, emulating a PCI Intel 6300ESB
Line 16: 'ib700' emulating an ISA iBase IB700
Line 17: only one watchdog device was support.
Line 18:
Line 19: action of watchdog:
action of watchdog timeout ?
Line 20: 'reset' default, forcefully reset the guest
Line 21: 'shutdown' gracefully shutdown the guest (not recommended)
Line 22: 'poweroff' forcefully power off the guest
Line 23: 'pause' pause the guest
Line 26:
Line 27: the parameter of 'watchdog' device as follow:
Line 28: {'device': 'watchdog', 'type': 'watchdog',
Line 29: 'specParams': {'model': 'i6300esb', 'action': 'reset'}}
Line 30:
Move line 14 to line 30 to api schema
Line 31: The watchdog device can used to detect guest crash, and if choose
Line 32: 'dump' action, the libvirt will dump the guest automatically.
Line 33: The directory to save dump files can be configured by auto_dump_path
Line 34: in file /etc/libvirt/qemu.conf.
Line 27: the parameter of 'watchdog' device as follow:
Line 28: {'device': 'watchdog', 'type': 'watchdog',
Line 29: 'specParams': {'model': 'i6300esb', 'action': 'reset'}}
Line 30:
Line 31: The watchdog device can used to detect guest crash, and if choose
can be used ... guest crash or hang, and if 'dump' is chosen for the action of
watchdog timeout
Line 32: 'dump' action, the libvirt will dump the guest automatically.
Line 33: The directory to save dump files can be configured by auto_dump_path
Line 34: in file /etc/libvirt/qemu.conf.
Line 35:
Line 28: {'device': 'watchdog', 'type': 'watchdog',
Line 29: 'specParams': {'model': 'i6300esb', 'action': 'reset'}}
Line 30:
Line 31: The watchdog device can used to detect guest crash, and if choose
Line 32: 'dump' action, the libvirt will dump the guest automatically.
'dump' action, libvirt will dump guest's memory on timeout automatically.
Line 33: The directory to save dump files can be configured by auto_dump_path
Line 34: in file /etc/libvirt/qemu.conf.
Line 35:
Line 36: The watchdog device requires an additional driver and management
....................................................
File vdsm/libvirtconnection.py
Line 63: elif eventid == libvirt.VIR_DOMAIN_EVENT_ID_BLOCK_JOB:
Line 64: path, type, status = args[:-1]
Line 65: v._onBlockJobEvent(path, type, status)
Line 66: elif eventid == libvirt.VIR_DOMAIN_EVENT_ID_WATCHDOG:
Line 67: wdargs = args[:-1]
why introduce a new variable? Can you use agrs directly?
Line 68: action = libvirtev.watchdogActionToString(wdargs[0])
Line 69: cif.log.debug("watchdog event: Domain %s(%s). Action: %s",
Line 70: dom.name(), dom.ID(), action)
Line 71: v._onWatchdogEvent(dom.name(), dom.ID(), action)
Line 66: elif eventid == libvirt.VIR_DOMAIN_EVENT_ID_WATCHDOG:
Line 67: wdargs = args[:-1]
Line 68: action = libvirtev.watchdogActionToString(wdargs[0])
Line 69: cif.log.debug("watchdog event: Domain %s(%s). Action: %s",
Line 70: dom.name(), dom.ID(), action)
Why not using UUID?
Move the log into _onWatchdogEvent ?
Line 71: v._onWatchdogEvent(dom.name(), dom.ID(), action)
Line 72: else:
Line 73: v.log.warning('unknown eventid %s args %s', eventid, args)
Line 74: except:
....................................................
File vdsm/libvirtev.py
Line 452: "Guest is requested to gracefully shutdown",
Line 453: "No action, a debug message logged")
Line 454: return actionStrings[action]
Line 455:
Line 456:
Move this convert function to libvirtvm.py, and it should be only used for
onWatchdogEvent()
Line 457: def myDomainEventCallback1(conn, dom, event, detail, opaque):
Line 458: print ("myDomainEventCallback1 EVENT: Domain %s(%s) %s %d" %
Line 459: (dom.name(), dom.ID(), eventToString(event), detail))
Line 460:
....................................................
File vdsm/libvirtvm.py
Line 2364: getElementsByTagName('watchdog')
Line 2365: for x in watchdogxml:
Line 2366: action = x.getAttribute('action')
Line 2367:
Line 2368: # PCI watchdog has "address" different with ISA watchdog
s/different with ISA watchdog/different from ISA watchdog/
Line 2369: if x.getElementsByTagName('address'):
Line 2370: address = self._getUnderlyingDeviceAddress(x)
Line 2371: alias =
x.getElementsByTagName('alias')[0].getAttribute('name')
Line 2372:
....................................................
File vdsm/vm.py
Line 401: # Update indices for drives devices
Line 402: self.normalizeDrivesIndices(devices[DISK_DEVICES])
Line 403: return devices
Line 404:
Line 405: def checkWatchdogModel(self, dev):
is it worth to define a new function?
Line 406: if (dev['type'] == WATCHDOG_DEVICES):
Line 407: if not 'specParams' in dev:
Line 408: dev['specParams'] = {}
Line 409: if not 'model' in dev['specParams']:
Line 402: self.normalizeDrivesIndices(devices[DISK_DEVICES])
Line 403: return devices
Line 404:
Line 405: def checkWatchdogModel(self, dev):
Line 406: if (dev['type'] == WATCHDOG_DEVICES):
is this check necessary?
Line 407: if not 'specParams' in dev:
Line 408: dev['specParams'] = {}
Line 409: if not 'model' in dev['specParams']:
Line 410: dev['specParams']['model'] = 'i6300esb'
Line 434: devices = self.getConfDevices()
Line 435:
Line 436: # libvirt only support one watchdog device
Line 437: if len(devices[BALLOON_DEVICES]) > 1:
Line 438: raise Exception("only a single watchdog device is
supported")
You shouldn't raise a generic exception.
Line 439: if len(devices[BALLOON_DEVICES]) == 1:
Line 440: self.checkWatchdogModel(devices[BALLOON_DEVICES][0])
Line 441:
Line 442: # Normalize vdsm images
--
To view, visit http://gerrit.ovirt.org/7535
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b7970a9050ab0279fe03371b9a77692fba30af8
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng <[email protected]>
Gerrit-Reviewer: Adam Litke <[email protected]>
Gerrit-Reviewer: Bing Bu Cao <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: Ryan Harper <[email protected]>
Gerrit-Reviewer: ShaoHe Feng <[email protected]>
Gerrit-Reviewer: Shu Ming <[email protected]>
Gerrit-Reviewer: Xu He Jie <[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
