Change in vdsm[master]: netlink: event monitor

phoracek at redhat.com phoracek at redhat.com
Mon Jan 19 12:56:15 UTC 2015


Petr Horáček has posted comments on this change.

Change subject: netlink: event monitor
......................................................................


Patch Set 27:

(7 comments)

http://gerrit.ovirt.org/#/c/36197/27/lib/vdsm/netlink/monitor.py
File lib/vdsm/netlink/monitor.py:

Line 75:     ipv4-route ipv6-ifaddr, ipv6-mroute, ipv6-route, ipv6-ifinfo,
Line 76:     decnet-ifaddr, decnet-route, ipv6-prefix
Line 77:     """
Line 78:     def __init__(self, groups=frozenset()):
Line 79:         self._stopped = False
> should be renamed to 'stopped' as it is used (read) publically
it is meant to be used locally, tests are just an exception
Line 80:         if groups:
Line 81:             self._groups = groups
Line 82:         else:
Line 83:             self._groups = _GROUPS.keys()


http://gerrit.ovirt.org/#/c/36197/27/tests/netlinkTests.py
File tests/netlinkTests.py:

Line 15: class NetlinkEventMonitorTests(TestCaseBase):
Line 16: 
Line 17:     @ValidateRunningAsRoot
Line 18:     def test_iterate_after_events(self):
Line 19:         with _timed_monitor(1) as mon:
> just a matter of style, but passing this as a named argumet might be more r
Done
Line 20:             dummy_name = dummy.create()
Line 21:             dummy.remove(dummy_name)
Line 22:             for event in mon:
Line 23:                 if event.get('name') == dummy_name:


Line 18:     def test_iterate_after_events(self):
Line 19:         with _timed_monitor(1) as mon:
Line 20:             dummy_name = dummy.create()
Line 21:             dummy.remove(dummy_name)
Line 22:             for event in mon:
> Just so I understand: when no events are collected by the monitor, does the
If we did not catch dummy-related event, it fails with MonitorError, yes.

We are not able to test strictly no-events case, because we can't be sure, there will be no external event (but we can just ignore incoming events).

I think testing timeout is not necessary, because now it's just a test's feature, not a part of monitor.py.
Line 23:                 if event.get('name') == dummy_name:
Line 24:                     break
Line 25: 
Line 26:     @ValidateRunningAsRoot


Line 29:         iteration we start _add_device, which waits for iteration_ready event.
Line 30:         """
Line 31:         dummy_name = dummy.create()
Line 32: 
Line 33:         def _add_device():
> may rename this function to _add_and_remove?
if i do dummy.create() inside the thread's function, it would be hard to pass dummy's name to the main thread.
Line 34:             time.sleep(.2)
Line 35:             dummy.setLinkUp(dummy_name)
Line 36:             dummy.remove(dummy_name)
Line 37:         add_device_thread = threading.Thread(target=_add_device)


Line 38: 
Line 39:         with _timed_monitor(1) as mon:
Line 40:             add_device_thread.start()
Line 41:             for event in mon:
Line 42:                 print event
> why print?
Done
Line 43:                 if event.get('name') == dummy_name:
Line 44:                     break
Line 45:             add_device_thread.join()
Line 46: 


Line 72:                             "to link or route." % event['event'])
Line 73: 
Line 74:     @ValidateRunningAsRoot
Line 75:     def test_iteration(self):
Line 76:         with _timed_monitor(1) as mon:
> * is this timeout enough even for slow Jenkins vms?
* i don't think so, the test passes within 0.038 second on vdsm-function-tests Jenkins VM

* StopIteration could happen only after the monitor is stopped. If it is monitoring, iterator.next() stuck until there is an event available.
Line 77:             iterator = iter(mon)
Line 78: 
Line 79:             # Generate events to avoid blocking
Line 80:             dummy_name = dummy.create()


Line 123:                          % (1 + len(expected_events)))
Line 124: 
Line 125: 
Line 126: @contextmanager
Line 127: def _timed_monitor(time, groups=frozenset(), raise_exception=True):
> why renaming this to 'time'? Maybe 'interval' (the original Timer arg name)
Done
Line 128:     mon = monitor.Monitor(groups=groups)
Line 129:     mon.start()
Line 130:     try:
Line 131:         timer = Timer(time, mon.stop)


-- 
To view, visit http://gerrit.ovirt.org/36197
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f4fcfde87ad51eb832f54862371b4da1281826e
Gerrit-PatchSet: 27
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <toni at midokura.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list