Improve callback registry devref documentation and usability

Latest developments have revealed that the registry can be misused
under certain circumstances, and that it can be harder to use by
projects that extend Neutron.

This patch improves the devref documentation so that developers know
what to expect.

Change-Id: I565b6a2f2a58bf22eae5b36f03c4fd24ba0774d2

doc/source/devref/callbacks.rst

@@ -300,6 +300,14 @@ The output is:
 FAQ
 ===
 
+Can I use the callbacks registry to subscribe and notify non-core resources and events?
+
+   Short answer is yes. The callbacks module defines literals for what are considered core Neutron
+   resources and events. However, the ability to subscribe/notify is not limited to these as you
+   can use your own defined resources and/or events. Just make sure you use string literals, as
+   typos are common, and the registry does not provide any runtime validation. Therefore, make
+   sure you test your code!
+
 What is the relationship between Callbacks and Taskflow?
 
    There is no overlap between Callbacks and Taskflow or mutual exclusion; as matter of fact they
@@ -315,6 +323,16 @@ Is there any ordering guarantee during notifications?
   notified. Priorities can be a future extension, if a use case arises that require enforced
   ordering.
 
+How is the the notifying object expected to interact with the subscribing objects?
+
+  The ``notify`` method implements a one-way communication paradigm: the notifier sends a message
+  without expecting a response back (in other words it fires and forget). However, due to the nature
+  of Python, the payload can be mutated by the subscribing objects, and this can lead to unexpected
+  behavior of your code, if you assume that this is the intentional design. Bear in mind, that
+  passing-by-value using deepcopy was not chosen for efficiency reasons. Having said that, if you
+  intend for the notifier object to expect a response, then the notifier itself would need to act
+  as a subscriber.
+
 Is the registry thread-safe?
 
   Short answer is no: it is not safe to make mutations while callbacks are being called (more

neutron/callbacks/events.py

@@ -10,6 +10,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+# String literals representing core events.
 BEFORE_CREATE = 'before_create'
 BEFORE_READ = 'before_read'
 BEFORE_UPDATE = 'before_update'
@@ -27,18 +28,3 @@ ABORT_DELETE = 'abort_delete'
 
 ABORT = 'abort_'
 BEFORE = 'before_'
-
-VALID = (
-    BEFORE_CREATE,
-    BEFORE_READ,
-    BEFORE_UPDATE,
-    BEFORE_DELETE,
-    AFTER_CREATE,
-    AFTER_READ,
-    AFTER_UPDATE,
-    AFTER_DELETE,
-    ABORT_CREATE,
-    ABORT_READ,
-    ABORT_UPDATE,
-    ABORT_DELETE,
-)

neutron/callbacks/manager.py

@@ -17,7 +17,6 @@ from oslo_utils import reflection
 
 from neutron.callbacks import events
 from neutron.callbacks import exceptions
-from neutron.callbacks import resources
 from neutron.i18n import _LE
 
 LOG = logging.getLogger(__name__)
@@ -40,13 +39,15 @@ class CallbacksManager(object):
         """
         LOG.debug("Subscribe: %(callback)s %(resource)s %(event)s",
                   {'callback': callback, 'resource': resource, 'event': event})
-        if resource not in resources.VALID:
-            raise exceptions.Invalid(element='resource', value=resource)
-        if event not in events.VALID:
-            raise exceptions.Invalid(element='event', value=event)
 
         callback_id = _get_id(callback)
-        self._callbacks[resource][event][callback_id] = callback
+        try:
+            self._callbacks[resource][event][callback_id] = callback
+        except KeyError:
+            # Initialize the registry for unknown resources and/or events
+            # prior to enlisting the callback.
+            self._callbacks[resource][event] = {}
+            self._callbacks[resource][event][callback_id] = callback
         # We keep a copy of callbacks to speed the unsubscribe operation.
         if callback_id not in self._index:
             self._index[callback_id] = collections.defaultdict(set)
@@ -125,9 +126,6 @@ class CallbacksManager(object):
         """Brings the manager to a clean slate."""
         self._callbacks = collections.defaultdict(dict)
         self._index = collections.defaultdict(dict)
-        for resource in resources.VALID:
-            for event in events.VALID:
-                self._callbacks[resource][event] = collections.defaultdict()
 
     def _notify_loop(self, resource, event, trigger, **kwargs):
         """The notification loop."""
@@ -135,8 +133,9 @@ class CallbacksManager(object):
                   {'resource': resource, 'event': event})
 
         errors = []
+        callbacks = self._callbacks[resource].get(event, {}).items()
         # TODO(armax): consider using a GreenPile
-        for callback_id, callback in self._callbacks[resource][event].items():
+        for callback_id, callback in callbacks:
             try:
                 LOG.debug("Calling callback %s", callback_id)
                 callback(resource, event, trigger, **kwargs)

neutron/callbacks/resources.py

@@ -10,6 +10,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+# String literals representing core resources.
 PORT = 'port'
 ROUTER = 'router'
 ROUTER_GATEWAY = 'router_gateway'
@@ -17,13 +18,3 @@ ROUTER_INTERFACE = 'router_interface'
 SECURITY_GROUP = 'security_group'
 SECURITY_GROUP_RULE = 'security_group_rule'
 SUBNET = 'subnet'
-
-VALID = (
-    PORT,
-    ROUTER,
-    ROUTER_GATEWAY,
-    ROUTER_INTERFACE,
-    SECURITY_GROUP,
-    SECURITY_GROUP_RULE,
-    SUBNET,
-)

neutron/tests/unit/callbacks/test_manager.py

@@ -13,7 +13,6 @@
 #    under the License.
 
 import mock
-import testtools
 
 from neutron.callbacks import events
 from neutron.callbacks import exceptions
@@ -44,15 +43,6 @@ class CallBacksManagerTestCase(base.BaseTestCase):
         callback_1.counter = 0
         callback_2.counter = 0
 
-    def test_subscribe_invalid_resource_raise(self):
-        with testtools.ExpectedException(exceptions.Invalid):
-            self.manager.subscribe(mock.ANY, 'foo_resource', mock.ANY)
-
-    def test_subscribe_invalid_event_raise(self):
-        self.assertRaises(exceptions.Invalid,
-                  self.manager.subscribe,
-                  mock.ANY, mock.ANY, 'foo_event')
-
     def test_subscribe(self):
         self.manager.subscribe(
             callback_1, resources.PORT, events.BEFORE_CREATE)
@@ -60,6 +50,13 @@ class CallBacksManagerTestCase(base.BaseTestCase):
             self.manager._callbacks[resources.PORT][events.BEFORE_CREATE])
         self.assertIn(callback_id_1, self.manager._index)
 
+    def test_subscribe_unknown(self):
+        self.manager.subscribe(
+            callback_1, 'my_resource', 'my-event')
+        self.assertIsNotNone(
+            self.manager._callbacks['my_resource']['my-event'])
+        self.assertIn(callback_id_1, self.manager._index)
+
     def test_subscribe_is_idempotent(self):
         self.manager.subscribe(
             callback_1, resources.PORT, events.BEFORE_CREATE)