Change in vdsm[master]: monitor: Add udev monitor
nsoffer at redhat.com
nsoffer at redhat.com
Tue Oct 27 23:56:54 UTC 2015
Nir Soffer has posted comments on this change.
Change subject: monitor: Add udev monitor
......................................................................
Patch Set 7:
(14 comments)
https://gerrit.ovirt.org/#/c/47729/7/lib/vdsm/Makefile.am
File lib/vdsm/Makefile.am:
Line 18: # Refer to the README and COPYING files for full details of the license
Line 19: #
Line 20: include $(top_srcdir)/build-aux/Makefile.subs
Line 21:
Line 22: SUBDIRS=netlink udev tool infra profiling
We don't need udev package, udev module with Monitor class is fine and save lot of boilerplate.
Line 23:
Line 24: dist_vdsmpylib_PYTHON = \
Line 25: __init__.py \
Line 26: cmdutils.py \
https://gerrit.ovirt.org/#/c/47729/7/lib/vdsm/udev/__init__.py
File lib/vdsm/udev/__init__.py:
Line 15: # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
Line 16: #
Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19: from __future__ import absolute_import
The import is not needed, and empty files do not need a license.
https://gerrit.ovirt.org/#/c/47729/7/lib/vdsm/udev/monitor.py
File lib/vdsm/udev/monitor.py:
Line 27:
Line 28: """
Line 29: ``udev`` event monitor. Usage:
Line 30:
Line 31: The monitor is a thread-save thin wrapper arount pyudev.MonitorObserver.
thread-safe
Line 32: This allows multiple callbacks for the same netlink socket. To avoid
Line 33: listening for udev events the application is not interested in, the
Line 34: monitoring thread only starts listening on the socket when the monitor is
Line 35: started and at least one subscriber is added.
Line 34: monitoring thread only starts listening on the socket when the monitor is
Line 35: started and at least one subscriber is added.
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:
This is called on the observer thread - right?
This will end in people blocking each other by invoking blocking calls during event handling .
It would be safer if we provide only a way to pull events from the monitor, instead of pushing the events to the subscribers.
I wonder if we should simply use multiple monitors instead of adding this wrapper - I guess it will be also more efficient.
This wrapper makes sense only if we have many monitors listening for the same events. But currently we don't have any monitors. I plan to monitor multipath devices regularly for updating lvm filter, and you wanted to monitor cpu events. So lets create 2 monitors and wait with this infrastructure until we need it.
Line 39:
Line 40: dev listen_for_disabled_cpu_events(device):
Line 41: if device.action == 'offline':
Line 42: print('CPU {0.name} is now offline'.format(device))
Line 60: monitor.start()
Line 61: """
Line 62:
Line 63: def __init__(self):
Line 64: self._subsystems = {}
This makes subscription handling little harder. Why not have a flat list of filters?
Line 65: self._context = Context()
Line 66: self._monitor = Monitor.from_netlink(self._context)
Line 67: self._observer = MonitorObserver(self._monitor, callback=partial(
Line 68: UdevMonitor._event_loop, self), name='udev-monitor')
Line 61: """
Line 62:
Line 63: def __init__(self):
Line 64: self._subsystems = {}
Line 65: self._context = Context()
Do we need to keep the context? Isn't it enough to create it and pass it to Monitor.from_netlink()?
Line 66: self._monitor = Monitor.from_netlink(self._context)
Line 67: self._observer = MonitorObserver(self._monitor, callback=partial(
Line 68: UdevMonitor._event_loop, self), name='udev-monitor')
Line 69: self._filter_lock = threading.Lock()
Line 64: self._subsystems = {}
Line 65: self._context = Context()
Line 66: self._monitor = Monitor.from_netlink(self._context)
Line 67: self._observer = MonitorObserver(self._monitor, callback=partial(
Line 68: UdevMonitor._event_loop, self), name='udev-monitor')
Why do you need partial? Why not MonitorObserver(self._event_loop)?
Line 69: self._filter_lock = threading.Lock()
Line 70: self._is_started = False
Line 71: self._can_start = False
Line 72:
Line 69: self._filter_lock = threading.Lock()
Line 70: self._is_started = False
Line 71: self._can_start = False
Line 72:
Line 73: def _event_loop(self, device):
This is not a good name, since this is not an event loop (it does not loop). Maybe _notify_subscribers? _handle_event?
Line 74: subsystem = self._subsystems[device.subsystem]
Line 75: for callback in subsystem.get(device.device_type, []):
Line 76: _execute_callback(callback, device)
Line 77: if device.device_type:
Line 77: if device.device_type:
Line 78: for callback in subsystem.get(None, []):
Line 79: _execute_callback(callback, device)
Line 80:
Line 81: def subscribe(self, callback, subsystem, device_type=None):
If there is a way to subscribe, we need a way to unsubscribe.
Line 82: """
Line 83: Raise :exc:`~exceptions.ValueError` if the callback is None
Line 84:
Line 85: :param callback: function to invoke
Line 92: """
Line 93: if callback is None:
Line 94: raise ValueError('callback missing')
Line 95: with self._filter_lock:
Line 96: self._monitor.filter_by(subsystem, device_type)
Does udev support removing filters?
Line 97: device_types = self._subsystems.get(subsystem, {})
Line 98: callbacks = device_types.get(device_type, [])
Line 99: callbacks.append(callback)
Line 100: device_types[device_type] = callbacks
Line 97: device_types = self._subsystems.get(subsystem, {})
Line 98: callbacks = device_types.get(device_type, [])
Line 99: callbacks.append(callback)
Line 100: device_types[device_type] = callbacks
Line 101: self._subsystems[subsystem] = device_types
If we are using flat subscription list, this could be simplified to:
self._monitor.filter_by(subsystem, device_type)
self._subscriptions[(subsystem, device_type)].append(callback)
Line 102: self._start_if_necessary()
Line 103:
Line 104: def start(self):
Line 105: self._can_start = True
Line 109:
Line 110: def _start_if_necessary(self):
Line 111: if self._can_start and not self._is_started:
Line 112: self._observer.start()
Line 113: self._is_started = True
When unsubscribing the last filter, we can stop the observer.
Line 114:
Line 115: def stop(self):
Line 116: """
Line 117: Stops the monitoring thread. It is guaranteed that callbacks are no
Line 128: try:
Line 129: callback(device)
Line 130: except Exception as callbackException:
Line 131: logging.error(
Line 132: 'Callback execution threw an exception: %s', callbackException)
We like to see a traceback in this case, so this should be:
except Exception:
logging.exception("Unhandled error in %s", callback)
https://gerrit.ovirt.org/#/c/47729/7/tests/Makefile.am
File tests/Makefile.am:
Line 109: tcTests.py \
Line 110: testlibTests.py \
Line 111: toolTests.py \
Line 112: transportWrapperTests.py \
Line 113: udevMonitorTests.py \
If we move the monitor to udev.py, this should be udevTests.py
Line 114: utilsTests.py \
Line 115: vdscliTests.py \
Line 116: vdsClientTests.py \
Line 117: vdsmDumpChainsTests.py \
--
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: 7
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: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Roman Mohr <rmohr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list