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 <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Roman Mohr <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
