Break down _bind_port_if_needed in ML2

Separate the looping and retry logic in _bind_port_if_needed
from the actual binding attempts. This also eliminates the
'while True' loop with a regular for loop counter to make it
a little easier to reason about.

A suggestion to do this came up in a code review for
I437290affd8eb87177d0626bf7935a165859cbdd because the function
was difficult to reason about.

Change-Id: I6171cf39a4562ed1da9467e8e604d4519a813977

diff --git a/neutron/plugins/ml2/plugin.py b/neutron/plugins/ml2/plugin.py
index 79741afba7f386e87fcc4d2a7f685633030cb44c..eb2085c268f25839a00ad7a9805f37c2e920c7bb 100644 (file)
--- a/neutron/plugins/ml2/plugin.py
+++ b/neutron/plugins/ml2/plugin.py
@@ -259,9 +259,29 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2,
 
     def _bind_port_if_needed(self, context, allow_notify=False,
                              need_notify=False):
-        plugin_context = context._plugin_context
-        port_id = context.current['id']
+        # Binding limit does not need to be tunable because no
+        # more than a couple of attempts should ever be required in
+        # normal operation.
+        for count in range(1, MAX_BIND_TRIES + 1):
+            if count > 1:
+                # multiple attempts shouldn't happen very often so we log each
+                # attempt after the 1st.
+                greenthread.sleep(0)  # yield
+                LOG.info(_LI("Attempt %(count)s to bind port %(port)s"),
+                         {'count': count, 'port': context.current['id']})
+            context, need_notify, try_again = self._attempt_binding(
+                context, need_notify)
+            if not try_again:
+                if allow_notify and need_notify:
+                    self._notify_port_updated(context)
+                return context
+
+        LOG.error(_LE("Failed to commit binding results for %(port)s "
+                      "after %(max)s tries"),
+                  {'port': context.current['id'], 'max': MAX_BIND_TRIES})
+        return context
 
+    def _attempt_binding(self, context, need_notify):
         # Since the mechanism driver bind_port() calls must be made
         # outside a DB transaction locking the port state, it is
         # possible (but unlikely) that the port's state could change
@@ -270,57 +290,41 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2,
         # thread commits its results, the already committed results are
         # used. If attributes such as binding:host_id,
         # binding:profile, or binding:vnic_type are updated
-        # concurrently, this loop retries binding using the new
-        # values.
-        count = 0
-        while True:
-            # First, determine whether it is necessary and possible to
-            # bind the port.
-            binding = context._binding
-            if (binding.vif_type != portbindings.VIF_TYPE_UNBOUND
+        # concurrently, the try_again flag is returned to indicate that
+        # the commit was unsuccessful.
+        plugin_context = context._plugin_context
+        port_id = context.current['id']
+        binding = context._binding
+        try_again = False
+        # First, determine whether it is necessary and possible to
+        # bind the port.
+        if (binding.vif_type != portbindings.VIF_TYPE_UNBOUND
                 or not binding.host):
-                # We either don't need to bind the port, or can't, so
-                # notify if needed and return.
-                if allow_notify and need_notify:
-                    self._notify_port_updated(context)
-                return context
-
-            # Limit binding attempts to avoid any possibility of
-            # infinite looping and to ensure an error is logged
-            # instead. This does not need to be tunable because no
-            # more than a couple attempts should ever be required in
-            # normal operation. Log at info level if not 1st attempt.
-            count += 1
-            if count > MAX_BIND_TRIES:
-                LOG.error(_LE("Failed to commit binding results for %(port)s "
-                              "after %(max)s tries"),
-                          {'port': port_id, 'max': MAX_BIND_TRIES})
-                return context
-            if count > 1:
-                greenthread.sleep(0)  # yield
-                LOG.info(_LI("Attempt %(count)s to bind port %(port)s"),
-                         {'count': count, 'port': port_id})
-
-            # The port isn't already bound and the necessary
-            # information is available, so attempt to bind the port.
-            bind_context = self._bind_port(context)
-
-            # Now try to commit result of attempting to bind the port.
-            new_context, did_commit = self._commit_port_binding(
-                plugin_context, port_id, binding, bind_context)
-            if not new_context:
-                # The port has been deleted concurrently, so just
-                # return the unbound result from the initial
-                # transaction that completed before the deletion.
-                LOG.debug("Port %s has been deleted concurrently",
-                          port_id)
-                return context
-            # Need to notify if we succeed and our results were
-            # committed.
-            if did_commit and (new_context._binding.vif_type !=
-                               portbindings.VIF_TYPE_BINDING_FAILED):
-                need_notify = True
-            context = new_context
+            # We either don't need to bind the port or can't
+            return context, need_notify, try_again
+
+        # The port isn't already bound and the necessary
+        # information is available, so attempt to bind the port.
+        bind_context = self._bind_port(context)
+        # Now try to commit result of attempting to bind the port.
+        new_context, did_commit = self._commit_port_binding(
+            plugin_context, port_id, binding, bind_context)
+        if not new_context:
+            # The port has been deleted concurrently, so just
+            # return the unbound result from the initial
+            # transaction that completed before the deletion.
+            LOG.debug("Port %s has been deleted concurrently",
+                      port_id)
+            need_notify = False
+            return context, need_notify, try_again
+        # Need to notify if we succeed and our results were
+        # committed.
+        if did_commit and (new_context._binding.vif_type !=
+                           portbindings.VIF_TYPE_BINDING_FAILED):
+            need_notify = True
+            return new_context, need_notify, try_again
+        try_again = True
+        return new_context, need_notify, try_again
 
     def _bind_port(self, orig_context):
         # Construct a new PortContext from the one from the previous