Change in vdsm[master]: monitor: Add udev monitor

rmohr at redhat.com rmohr at redhat.com
Mon Nov 2 16:25:09 UTC 2015


Roman Mohr has posted comments on this change.

Change subject: monitor: Add udev monitor
......................................................................


Patch Set 7:

(4 comments)

https://gerrit.ovirt.org/#/c/47729/7/lib/vdsm/udev/monitor.py
File lib/vdsm/udev/monitor.py:

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.

Yes, it definitely can end up like that.

> It would be safer if we provide only a way to pull events from the monitor, instead of pushing the events to the subscribers.

The idea is about having this as single point of source for non local sudden events. Events are handled one after the other (e.g. device is added and we have 10 listeners, then all that actions are executed before the next event comes. Would us allow to settle the new information throughout the application). Long running parts would be executed asynchronous or pushed to queues where a thread can poll, like you suggested.

> 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.

Valid objections. I have seen it as some kind of application wide service, where you only unsubscribe for shutdown. For actions of only local interest (like remove a device and wait until udev reports that it is really removed) someone can always just use the monitor from the pyudev library.

>  So lets create 2 monitors and wait with this infrastructure until we need it.

I am undecided. If you still insist after my comments, it is fine for me.
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
Will do that if we decide to introduce the monitor.
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 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.
Unsubscribe is not possible because we can only remove all filters at once. Since the idea was to make this a service where you only unsubscribe for shutdown that, the general stop() method would be enough.
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?
You can only remove all at once
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


-- 
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