From 11fe2bff17d8ad27c0c49037fcd36203f9baa32d Mon Sep 17 00:00:00 2001 From: Slawek Kaplonski Date: Mon, 25 Oct 2021 10:59:24 +0200 Subject: [PATCH] Don't setup bridge controller if it is already set Setting new controller for bridge every time when neutron-ovs-agent is restarted or is doing full-sync may cause some short data plane connectivity loss and is not needed if same controller is already configured for the bridge. With this patch neutron-ovs-agent will first check if controller is configured for the bridge and if it's the same as what should be configured, it will skip setting it up. With this patch also protocols added to the bridge will be first checked if they aren't already there and only missing ones will be added if necessary. Setting of the connectivity mode and inactivity probe is always performed as this don't cause connectivity issues and is cheap so we can always ensure that those parameters are configured properly. Conflicts: neutron/agent/common/ovs_lib.py neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/ovs_bridge.py neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/ovs_bridge_test_base.py Closes-Bug: #1948642 Change-Id: Idfa763df8c60d8ae46cd6351d1b6dc7d950b4c67 (cherry picked from commit 11d166be683d36ba51e3f85ddea45cb0a1fcf8db) (cherry picked from commit b1eccf5a2deaf6d18ff18dfacfbcf7ef7fdb781f) --- neutron/agent/common/ovs_lib.py | 9 +++-- .../agent/openflow/native/ovs_bridge.py | 10 ++++-- .../tests/unit/agent/common/test_ovs_lib.py | 23 +++++++++++++ .../openflow/native/ovs_bridge_test_base.py | 33 +++++++++++++++---- 4 files changed, 63 insertions(+), 12 deletions(-) diff --git a/neutron/agent/common/ovs_lib.py b/neutron/agent/common/ovs_lib.py index c108c7a9dcf..e3ca362ae48 100644 --- a/neutron/agent/common/ovs_lib.py +++ b/neutron/agent/common/ovs_lib.py @@ -283,8 +283,13 @@ class OVSBridge(BaseOVS): self._set_bridge_fail_mode(FAILMODE_STANDALONE) def add_protocols(self, *protocols): - self.ovsdb.db_add('Bridge', self.br_name, - 'protocols', *protocols).execute(check_error=True) + existing_protocols = self.db_get_val( + 'Bridge', self.br_name, 'protocols') + diff = set(protocols).difference(existing_protocols) + if diff: + self.ovsdb.db_add( + 'Bridge', self.br_name, + 'protocols', *diff).execute(check_error=True) def use_at_least_protocol(self, protocol): """Calls to ovs-ofctl will use a protocol version >= 'protocol'""" 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 764417430e2..9eafe74e933 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 @@ -73,10 +73,14 @@ class OVSAgentBridge(ofswitch.OpenFlowSwitchMixin, def setup_controllers(self, conf): url = ipv6_utils.valid_ipv6_url(conf.OVS.of_listen_address, conf.OVS.of_listen_port) - controllers = ["tcp:" + url] - self.add_protocols(ovs_consts.OPENFLOW13) - self.set_controller(controllers) + controller = "tcp:" + url + existing_controllers = self.get_controller() + if controller not in existing_controllers: + LOG.debug("Setting controller %s for bridge %s.", + controller, self.br_name) + self.set_controller([controller]) + self.add_protocols(ovs_consts.OPENFLOW13) # NOTE(ivc): Force "out-of-band" controller connection mode (see # "In-Band Control" [1]). # diff --git a/neutron/tests/unit/agent/common/test_ovs_lib.py b/neutron/tests/unit/agent/common/test_ovs_lib.py index d861312b8b2..bcab17bdd59 100644 --- a/neutron/tests/unit/agent/common/test_ovs_lib.py +++ b/neutron/tests/unit/agent/common/test_ovs_lib.py @@ -207,6 +207,29 @@ class OVS_Lib_Test(base.BaseTestCase): return self.execute.assert_called_once_with(cmd, run_as_root=True, **kwargs) + def test_add_protocols_all_already_set(self): + self.br = ovs_lib.OVSBridge(self.BR_NAME) + with mock.patch.object(self.br, 'db_get_val') as db_get_val, \ + mock.patch.object(self.br.ovsdb, 'db_add') as db_add: + db_get_val.return_value = [p_const.OPENFLOW10, + p_const.OPENFLOW13] + self.br.add_protocols(p_const.OPENFLOW10, p_const.OPENFLOW13) + db_get_val.assert_called_once_with( + 'Bridge', self.BR_NAME, 'protocols') + db_add.assert_not_called() + + def test_add_protocols_some_already_set(self): + self.br = ovs_lib.OVSBridge(self.BR_NAME) + with mock.patch.object(self.br, 'db_get_val') as db_get_val, \ + mock.patch.object(self.br.ovsdb, 'db_add') as db_add: + db_get_val.return_value = [p_const.OPENFLOW10] + self.br.add_protocols(p_const.OPENFLOW10, p_const.OPENFLOW13) + db_get_val.assert_called_once_with( + 'Bridge', self.BR_NAME, 'protocols') + db_add.assert_has_calls([ + mock.call('Bridge', self.BR_NAME, + 'protocols', p_const.OPENFLOW13)]) + def test_add_flow_timeout_set(self): flow_dict = collections.OrderedDict([ ('cookie', 1234), 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 2b170691e71..7773f1fce2a 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 @@ -136,11 +136,12 @@ class OVSBridgeTestBase(ovs_test_base.OVSOSKenTestBase): self.assertEqual('192.168.0.1', f('192.168.0.1/32')) self.assertEqual(('192.168.0.0', '255.255.255.0'), f('192.168.0.0/24')) - def test__setup_controllers__out_of_band(self): + def _test_setup_controllers(self, existing_controllers): cfg = mock.MagicMock() - cfg.OVS.of_listen_address = "" - cfg.OVS.of_listen_port = "" + cfg.OVS.of_listen_address = "127.0.0.1" + cfg.OVS.of_listen_port = "6633" + m_get_controller = mock.patch.object(self.br, 'get_controller') m_add_protocols = mock.patch.object(self.br, 'add_protocols') m_set_controller = mock.patch.object(self.br, 'set_controller') m_set_probe = mock.patch.object(self.br, @@ -148,10 +149,28 @@ class OVSBridgeTestBase(ovs_test_base.OVSOSKenTestBase): m_set_ccm = mock.patch.object(self.br, 'set_controllers_connection_mode') - with m_set_ccm as set_ccm: - with m_set_controller, m_add_protocols, m_set_probe: - self.br.setup_controllers(cfg) - set_ccm.assert_called_once_with("out-of-band") + with m_set_ccm as set_ccm, \ + m_add_protocols as add_protocols, \ + m_set_controller as set_controller, \ + m_get_controller as get_controller, \ + m_set_probe: + get_controller.return_value = existing_controllers + + self.br.setup_controllers(cfg) + + if existing_controllers: + set_controller.assert_not_called() + else: + set_controller.assert_called_once_with(["tcp:127.0.0.1:6633"]) + set_ccm.assert_called_once_with("out-of-band") + add_protocols.assert_called_once_with(constants.OPENFLOW13) + + def test_setup_controllers(self): + self._test_setup_controllers(existing_controllers=[]) + + def test_setup_controllers_when_already_exists(self): + self._test_setup_controllers( + existing_controllers=["tcp:127.0.0.1:6633"]) class OVSDVRProcessTestMixin(object):