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