From 59434672b405f3734d1c319aac311af75a88325f Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Thu, 13 Aug 2015 16:58:02 -0700 Subject: [PATCH] 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 --- neutron/plugins/ml2/plugin.py | 108 ++++++++++++++++++---------------- 1 file changed, 56 insertions(+), 52 deletions(-) diff --git a/neutron/plugins/ml2/plugin.py b/neutron/plugins/ml2/plugin.py index 79741afba..eb2085c26 100644 --- 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 -- 2.45.2