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