From 271a4ffd6d700b5d092ff24592ced324ded41e43 Mon Sep 17 00:00:00 2001 From: Thomas Morin Date: Fri, 16 Sep 2016 09:46:27 +0200 Subject: [PATCH] OVS: merge the required OpenFlow version rather than replace This change modifies the behavior of OVS native and ovs-ofctl bridge implementations so that instead of configuring the bridge only for the required OVS protocol version, they add the required version to the already configured versions. To achieve this, an add_protocols method is added to the OVSBridge class, relying on the OVSDB add_db_attribute added in Ib6ce75846f9b13c1c33f0ced5ccc619ee7860dc1, with the behavior of making the provided set of versions supported in addition to already configured ones. It is aimed to be a cleaner solution to bug 1622644 than the quickfix merge from I4475865c4f83cb9f3e12c709af752bc490692ca3 . After this change, the set_protocols method appears useless and is hence marked for future removal. Depends-On: I4386aa293f9b18d2e17b4a80d9c7da4b9b46f3c9 Change-Id: Id5ac7e6431c97fc70d8404b16f89533b6f270eee Related-Bug: 1622644 --- neutron/agent/common/ovs_lib.py | 20 +++++++++++++- .../linux/openvswitch_firewall/firewall.py | 2 +- neutron/cmd/sanity/checks.py | 2 +- .../agent/openflow/native/ovs_bridge.py | 2 +- .../agent/openflow/ovs_ofctl/ovs_bridge.py | 3 --- .../tests/functional/agent/test_ovs_lib.py | 27 +++++++++++++++++++ .../openflow/native/ovs_bridge_test_base.py | 4 +-- 7 files changed, 51 insertions(+), 9 deletions(-) diff --git a/neutron/agent/common/ovs_lib.py b/neutron/agent/common/ovs_lib.py index 6261fabdd57..ce1a17f2c92 100644 --- a/neutron/agent/common/ovs_lib.py +++ b/neutron/agent/common/ovs_lib.py @@ -19,6 +19,7 @@ import operator import time import uuid +from debtcollector import removals from neutron_lib import exceptions from oslo_config import cfg from oslo_log import log as logging @@ -206,15 +207,32 @@ class OVSBridge(BaseOVS): def set_standalone_mode(self): self._set_bridge_fail_mode(FAILMODE_STANDALONE) + @removals.remove( + message=("Consider using add_protocols instead, or if replacing " + "the whole set of supported protocols is the desired " + "behavior, using set_db_attribute"), + version="Ocata", + removal_version="Queens") def set_protocols(self, protocols): self.set_db_attribute('Bridge', self.br_name, 'protocols', protocols, check_error=True) + def add_protocols(self, *protocols): + self.ovsdb.db_add('Bridge', self.br_name, + 'protocols', *protocols).execute(check_error=True) + def create(self, secure_mode=False): with self.ovsdb.transaction() as txn: txn.add( self.ovsdb.add_br(self.br_name, - datapath_type=self.datapath_type)) + datapath_type=self.datapath_type)) + # the ovs-ofctl commands below in run_ofctl use OF10, so we + # need to ensure that this version is enabled ; we could reuse + # add_protocols, but doing ovsdb.db_add avoids doing two + # transactions + txn.add( + self.ovsdb.db_add('Bridge', self.br_name, + 'protocols', constants.OPENFLOW10)) if secure_mode: txn.add(self.ovsdb.set_fail_mode(self.br_name, FAILMODE_SECURE)) diff --git a/neutron/agent/linux/openvswitch_firewall/firewall.py b/neutron/agent/linux/openvswitch_firewall/firewall.py index 502c69fd6d0..d2bbb27a603 100644 --- a/neutron/agent/linux/openvswitch_firewall/firewall.py +++ b/neutron/agent/linux/openvswitch_firewall/firewall.py @@ -236,7 +236,7 @@ class OVSFirewallDriver(firewall.FirewallDriver): @staticmethod def initialize_bridge(int_br): - int_br.set_protocols(OVSFirewallDriver.REQUIRED_PROTOCOLS) + int_br.add_protocols(*OVSFirewallDriver.REQUIRED_PROTOCOLS) return int_br.deferred(full_ordered=True) def _drop_all_unmatched_flows(self): diff --git a/neutron/cmd/sanity/checks.py b/neutron/cmd/sanity/checks.py index c4b26880fb8..6b6018552b7 100644 --- a/neutron/cmd/sanity/checks.py +++ b/neutron/cmd/sanity/checks.py @@ -380,7 +380,7 @@ def ovs_conntrack_supported(): with ovs_lib.OVSBridge(br_name) as br: try: - br.set_protocols(["OpenFlow%d" % i for i in range(10, 15)]) + br.add_protocols(*["OpenFlow%d" % i for i in range(10, 15)]) except RuntimeError as e: LOG.debug("Exception while checking ovs conntrack support: %s", e) return False 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 3a055fc55f8..df9ab36afda 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 @@ -75,7 +75,7 @@ class OVSAgentBridge(ofswitch.OpenFlowSwitchMixin, "port": conf.OVS.of_listen_port, } ] - self.set_protocols([ovs_consts.OPENFLOW10, ovs_consts.OPENFLOW13]) + self.add_protocols(ovs_consts.OPENFLOW13) self.set_controller(controllers) # NOTE(ivc): Force "out-of-band" controller connection mode (see diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/ovs_bridge.py b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/ovs_bridge.py index b60a1f55621..e5da02c175c 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/ovs_bridge.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/ovs_bridge.py @@ -16,8 +16,6 @@ from neutron.agent.common import ovs_lib -from neutron.plugins.ml2.drivers.openvswitch.agent.common import constants \ - as ovs_consts from neutron.plugins.ml2.drivers.openvswitch.agent.openflow \ import br_cookie from neutron.plugins.ml2.drivers.openvswitch.agent.openflow.ovs_ofctl \ @@ -29,7 +27,6 @@ class OVSAgentBridge(ofswitch.OpenFlowSwitchMixin, """Common code for bridges used by OVS agent""" def setup_controllers(self, conf): - self.set_protocols([ovs_consts.OPENFLOW10, ovs_consts.OPENFLOW13]) self.del_controller() def drop_port(self, in_port): diff --git a/neutron/tests/functional/agent/test_ovs_lib.py b/neutron/tests/functional/agent/test_ovs_lib.py index fdf64bece61..703e7ca6bea 100644 --- a/neutron/tests/functional/agent/test_ovs_lib.py +++ b/neutron/tests/functional/agent/test_ovs_lib.py @@ -159,6 +159,33 @@ class OVSBridgeTestCase(OVSBridgeTestBase): self.br.db_get_val('Bridge', self.br.br_name, 'protocols'), "OpenFlow10") + def test_add_protocols_start_with_one(self): + self.br.set_db_attribute('Bridge', self.br.br_name, 'protocols', + ['OpenFlow10'], + check_error=True) + self.br.add_protocols('OpenFlow13') + self.assertEqual( + self.br.db_get_val('Bridge', self.br.br_name, 'protocols'), + ['OpenFlow10', 'OpenFlow13']) + + def test_add_protocols_start_with_two_add_two(self): + self.br.set_db_attribute('Bridge', self.br.br_name, 'protocols', + ['OpenFlow10', 'OpenFlow12'], + check_error=True) + self.br.add_protocols('OpenFlow13', 'OpenFlow14') + self.assertEqual( + self.br.db_get_val('Bridge', self.br.br_name, 'protocols'), + ['OpenFlow10', 'OpenFlow12', 'OpenFlow13', 'OpenFlow14']) + + def test_add_protocols_add_existing(self): + self.br.set_db_attribute('Bridge', self.br.br_name, 'protocols', + ['OpenFlow10', 'OpenFlow12', 'OpenFlow13'], + check_error=True) + self.br.add_protocols('OpenFlow13') + self.assertEqual( + self.br.db_get_val('Bridge', self.br.br_name, 'protocols'), + ['OpenFlow10', 'OpenFlow12', 'OpenFlow13']) + def test_get_datapath_id(self): brdev = ip_lib.IPDevice(self.br.br_name) dpid = brdev.link.attributes['link/ether'].replace(':', '') diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/ovs_bridge_test_base.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/ovs_bridge_test_base.py index 892167011d8..aaf7fd46958 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/ovs_bridge_test_base.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/ovs_bridge_test_base.py @@ -135,12 +135,12 @@ class OVSBridgeTestBase(ovs_test_base.OVSRyuTestBase): cfg.OVS.of_listen_address = "" cfg.OVS.of_listen_port = "" - m_set_protocols = mock.patch.object(self.br, 'set_protocols') + m_add_protocols = mock.patch.object(self.br, 'add_protocols') m_set_controller = mock.patch.object(self.br, 'set_controller') m_set_ccm = mock.patch.object(self.br, 'set_controllers_connection_mode') - with m_set_ccm as set_ccm, m_set_controller, m_set_protocols: + with m_set_ccm as set_ccm, m_set_controller, m_add_protocols: self.br.setup_controllers(cfg) set_ccm.assert_called_once_with("out-of-band")