From b22c7ae42813ca7c9c791b5acd4da24566784e9c Mon Sep 17 00:00:00 2001 From: Inessa Vasilevskaya Date: Wed, 1 Jun 2016 20:54:43 +0300 Subject: [PATCH] OVS: UnboundLocalError on switch timeout fixed In case there is some cached datapath id in OVSAgentBridge and openflow switch does not respond in time for some reason, a call to OVSAgentBridge._dpid() method will result in UnboundLocalError. This patch addresses the issue by calculating dpid_str value from cached dpid instead of referencing unassigned variable. Some minor refactor also took place. Closes-Bug: #1588042 Change-Id: If50183bf95cbe50c3a2393be8c2ab913c9715a10 --- .../agent/openflow/native/ovs_bridge.py | 18 +++++----- .../agent/openflow/native/test_ovs_bridge.py | 35 +++++++++++++++++++ 2 files changed, 43 insertions(+), 10 deletions(-) create mode 100644 neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_ovs_bridge.py diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/ovs_bridge.py b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/ovs_bridge.py index 26cfb3e5d0a..0431fb8c90c 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/ovs_bridge.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/ovs_bridge.py @@ -42,33 +42,31 @@ class OVSAgentBridge(ofswitch.OpenFlowSwitchMixin, A convenient method for openflow message composers. """ while True: - dpid_int = self._cached_dpid - if dpid_int is None: + if self._cached_dpid is None: dpid_str = self.get_datapath_id() LOG.info(_LI("Bridge %(br_name)s has datapath-ID %(dpid)s"), {"br_name": self.br_name, "dpid": dpid_str}) - dpid_int = int(dpid_str, 16) + self._cached_dpid = int(dpid_str, 16) try: - dp = self._get_dp_by_dpid(dpid_int) + dp = self._get_dp_by_dpid(self._cached_dpid) + return dp, dp.ofproto, dp.ofproto_parser except RuntimeError: with excutils.save_and_reraise_exception() as ctx: - self._cached_dpid = None # Retry if dpid has been changed. # NOTE(yamamoto): Open vSwitch change its dpid on # some events. # REVISIT(yamamoto): Consider to set dpid statically. + old_dpid_str = format(self._cached_dpid, '0x') new_dpid_str = self.get_datapath_id() - if new_dpid_str != dpid_str: + if new_dpid_str != old_dpid_str: LOG.info(_LI("Bridge %(br_name)s changed its " "datapath-ID from %(old)s to %(new)s"), { "br_name": self.br_name, - "old": dpid_str, + "old": old_dpid_str, "new": new_dpid_str, }) ctx.reraise = False - else: - self._cached_dpid = dpid_int - return dp, dp.ofproto, dp.ofproto_parser + self._cached_dpid = int(new_dpid_str, 16) def setup_controllers(self, conf): controllers = [ diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_ovs_bridge.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_ovs_bridge.py new file mode 100644 index 00000000000..cfbb90a9291 --- /dev/null +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_ovs_bridge.py @@ -0,0 +1,35 @@ +# Copyright (c) 2016 Mirantis, Inc. +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import mock + +from neutron.tests.unit.plugins.ml2.drivers.openvswitch.agent \ + import ovs_test_base + + +class OVSAgentBridgeTestCase(ovs_test_base.OVSRyuTestBase): + def test__get_dp(self): + mock.patch( + 'neutron.agent.common.ovs_lib.OVSBridge.get_datapath_id', + return_value="3e9").start() + mock.patch( + "neutron.plugins.ml2.drivers.openvswitch.agent.openflow.native." + "ofswitch.OpenFlowSwitchMixin._get_dp_by_dpid", + side_effect=RuntimeError).start() + br = self.br_int_cls('br-int') + br._cached_dpid = int("3e9", 16) + # make sure it correctly raises RuntimeError, not UnboundLocalError as + # in LP https://bugs.launchpad.net/neutron/+bug/1588042 + self.assertRaises(RuntimeError, br._get_dp)