Change in vdsm[master]: monitor: Add udev monitor
mpolednik at redhat.com
mpolednik at redhat.com
Wed Feb 10 14:37:07 UTC 2016
Martin Polednik has posted comments on this change.
Change subject: monitor: Add udev monitor
......................................................................
Patch Set 9: Code-Review-1
(6 comments)
There are few code issues, but I like the overall idea of monitoring the events. What I'd really like to see is vdsm-wide udev monitor where various parts can register their events.
https://gerrit.ovirt.org/#/c/47729/9/lib/vdsm/udev/monitor.py
File lib/vdsm/udev/monitor.py:
Line 22:
Line 23: from pyudev import Context, Monitor, MonitorObserver
Line 24:
Line 25:
Line 26: class UdevMonitor(object):
I'd prefer the monitor to be module level functionality, not a class. There should only be one monitor running in a VDSM.
Line 27:
Line 28: """
Line 29: ``udev`` event monitor. Usage:
Line 30:
Line 36:
Line 37: The simplest way to use the monitor is to subscribe a callback to a
Line 38: specific subsystem event and let the callback do the work:
Line 39:
Line 40: dev listen_for_disabled_cpu_events(device):
(unimportant) s/dev/def
Line 41: if device.action == 'offline':
Line 42: print('CPU {0.name} is now offline'.format(device))
Line 43:
Line 44: monitor = UdevMonitor()
Line 92: ``'block'``)
Line 93: :return: None
Line 94: """
Line 95: if callback is None:
Line 96: raise ValueError('callback missing')
I'm not sure that we need this explicit check - when do you expect to actually see None?
Line 97: with self._filter_lock:
Line 98: self._monitor.filter_by(subsystem, device_type)
Line 99: callbacks = self._subscriptions.get((subsystem, device_type), [])
Line 100: callbacks.append(callback)
Line 98: self._monitor.filter_by(subsystem, device_type)
Line 99: callbacks = self._subscriptions.get((subsystem, device_type), [])
Line 100: callbacks.append(callback)
Line 101: self._subscriptions[(subsystem, device_type)] = callbacks
Line 102: self._start_if_necessary()
Not fan of this approach. Something more explicit would be nicer - for example only allow subscriptions to running monitor, raise MonitorNotRunning exception otherwise (example name!).
Line 103:
Line 104: def start(self):
Line 105: self._can_start = True
Line 106: with self._filter_lock:
Line 106: with self._filter_lock:
Line 107: if self._subscriptions:
Line 108: self._start_if_necessary()
Line 109:
Line 110: def _start_if_necessary(self):
Not needed if the code is moved to module level.
Line 111: if self._can_start and not self._is_started:
Line 112: self._observer.start()
Line 113: self._is_started = True
Line 114:
https://gerrit.ovirt.org/#/c/47729/9/tests/udevMonitorTests.py
File tests/udevMonitorTests.py:
Line 42: self._monitor.subscribe(
Line 43: partial(UdevMonitorTest._device_listener, self),
Line 44: subsystem='net',
Line 45: device_type='bridge')
Line 46: bridge = Bridge()
This (and following occurrences) needs an improvement. We should avoid touching the underlying system, and this*2 when it comes to networks. We need to mock the pyudev monitor to emit fake events and test on these.
Line 47: try:
Line 48: bridge.addDevice()
Line 49: device = None
Line 50: try:
--
To view, visit https://gerrit.ovirt.org/47729
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b91753424d83896fa538eb6b57f8653b6332fbb
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Roman Mohr <rmohr at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Roman Mohr <rmohr at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list