From 7692e3ae6c3e73d58dcca54ef199dceb1250daf7 Mon Sep 17 00:00:00 2001 From: Jakub Libosvar Date: Wed, 26 Jul 2023 18:28:29 +0000 Subject: [PATCH 1/2] dvr: Avoid installing non-dvr openflow rule on startup The tunneling bridge uses different openflow rules depending if the agent is running in DVR mode or not. With DVR enabled initial rule was installed that caused traffic coming from the integration bridge to be flooded to all tunnels. After a few miliseconds this flow was replaced by a DVR specific flow, correctly dropping the traffic. This small time window caused a network loop on the compute node with restarted agent. This patch skips installing the non-dvr specific flow in case OVS agent is working in DVR mode. Hence the traffic is never flooded to the tunnels. Closes-bug: #2028795 Conflicts: neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py Signed-off-by: Jakub Libosvar Change-Id: I3ce026054286c8e28ec1500f1a4aa607fe73f337 (cherry picked from commit ba6f7bf83e6f17048a97f781aa16bf4a643a53d2) --- .../agent/openflow/native/br_tun.py | 16 ++++++--- .../agent/ovs_dvr_neutron_agent.py | 18 ++++++---- .../openvswitch/agent/ovs_neutron_agent.py | 3 +- .../tests/functional/agent/test_ovs_flows.py | 9 +++-- .../agent/openflow/native/test_br_tun.py | 33 +++++++++++++++++-- .../openvswitch/agent/test_ovs_tunnel.py | 9 +++-- 6 files changed, 69 insertions(+), 19 deletions(-) diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/br_tun.py b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/br_tun.py index 8bdf38c9215..a0b2dc26303 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/br_tun.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/br_tun.py @@ -33,13 +33,19 @@ class OVSTunnelBridge(ovs_bridge.OVSAgentBridge, dvr_process_next_table_id = constants.PATCH_LV_TO_TUN of_tables = constants.TUN_BR_ALL_TABLES - def setup_default_table(self, patch_int_ofport, arp_responder_enabled): + def setup_default_table( + self, patch_int_ofport, arp_responder_enabled, dvr_enabled): (dp, ofp, ofpp) = self._get_dp() - # Table 0 (default) will sort incoming traffic depending on in_port - self.install_goto(dest_table_id=constants.PATCH_LV_TO_TUN, - priority=1, - in_port=patch_int_ofport) + if not dvr_enabled: + # Table 0 (default) will sort incoming traffic depending on in_port + # This table is needed only for non-dvr environment because + # OVSDVRProcessMixin overwrites this flow in its + # install_dvr_process() method. + self.install_goto(dest_table_id=constants.PATCH_LV_TO_TUN, + priority=1, + in_port=patch_int_ofport) + self.install_drop() # default drop if arp_responder_enabled: diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py index be0d5349932..a393356e1ee 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py @@ -263,16 +263,20 @@ def setup_dvr_flows_on_tun_br(self): if not self.enable_tunneling: return - self.tun_br.install_goto(dest_table_id=constants.DVR_PROCESS, - priority=1, - in_port=self.patch_int_ofport) + self._setup_dvr_flows_on_tun_br(self.tun_br, self.patch_int_ofport) + + @staticmethod + def _setup_dvr_flows_on_tun_br(tun_br, patch_int_ofport): + tun_br.install_goto(dest_table_id=constants.DVR_PROCESS, + priority=1, + in_port=patch_int_ofport) # table-miss should be sent to learning table - self.tun_br.install_goto(table_id=constants.DVR_NOT_LEARN, - dest_table_id=constants.LEARN_FROM_TUN) + tun_br.install_goto(table_id=constants.DVR_NOT_LEARN, + dest_table_id=constants.LEARN_FROM_TUN) - self.tun_br.install_goto(table_id=constants.DVR_PROCESS, - dest_table_id=constants.PATCH_LV_TO_TUN) + tun_br.install_goto(table_id=constants.DVR_PROCESS, + dest_table_id=constants.PATCH_LV_TO_TUN) def setup_dvr_flows_on_phys_br(self, bridge_mappings=None): '''Setup up initial dvr flows into br-phys''' diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py index 614e41b4a8c..0479a97bc9a 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py @@ -1475,7 +1475,8 @@ def setup_tunnel_br_flows(self): Add all flows to the tunnel bridge. ''' self.tun_br.setup_default_table(self.patch_int_ofport, - self.arp_responder_enabled) + self.arp_responder_enabled, + self.enable_distributed_routing) def _reconfigure_physical_bridges(self, bridges): try: diff --git a/neutron/tests/functional/agent/test_ovs_flows.py b/neutron/tests/functional/agent/test_ovs_flows.py index d779dbbb244..d0f5045eb3f 100644 --- a/neutron/tests/functional/agent/test_ovs_flows.py +++ b/neutron/tests/functional/agent/test_ovs_flows.py @@ -24,6 +24,8 @@ from neutron.cmd.sanity import checks from neutron.common import utils as common_utils from neutron.plugins.ml2.drivers.openvswitch.agent.common import constants +from neutron.plugins.ml2.drivers.openvswitch.agent \ + import ovs_dvr_neutron_agent as ovsdvragt from neutron.plugins.ml2.drivers.openvswitch.agent \ import ovs_neutron_agent as ovsagt from neutron.tests.common import base as common_base @@ -299,8 +301,9 @@ class OVSFlowTestCase(OVSAgentTestBase): """ def setUp(self): + dvr_enabled = True cfg.CONF.set_override('enable_distributed_routing', - True, + dvr_enabled, group='AGENT') super(OVSFlowTestCase, self).setUp() self.phys_br = self.useFixture(net_helpers.OVSBridgeFixture()).bridge @@ -322,7 +325,9 @@ def setUp(self): prefix=cfg.CONF.OVS.tun_peer_patch_port), common_utils.get_rand_device_name( prefix=cfg.CONF.OVS.int_peer_patch_port)) - self.br_tun.setup_default_table(self.tun_p, True) + self.br_tun.setup_default_table(self.tun_p, True, dvr_enabled) + ovsdvragt.OVSDVRNeutronAgent._setup_dvr_flows_on_tun_br(self.br_tun, + self.tun_p) def test_provision_local_vlan(self): kwargs = {'port': 123, 'lvid': 888, 'segmentation_id': 777} diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_br_tun.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_br_tun.py index eecba6214b8..8eec6806021 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_br_tun.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_br_tun.py @@ -52,7 +52,8 @@ def test_setup_default_table(self): patch_int_ofport = 5555 arp_responder_enabled = False self.br.setup_default_table(patch_int_ofport=patch_int_ofport, - arp_responder_enabled=arp_responder_enabled) + arp_responder_enabled=arp_responder_enabled, + dvr_enabled=False) (dp, ofp, ofpp) = self._get_dp() expected = [ call._send_msg(ofpp.OFPFlowMod(dp, @@ -160,7 +161,8 @@ def test_setup_default_table_arp_responder_enabled(self): patch_int_ofport = 5555 arp_responder_enabled = True self.br.setup_default_table(patch_int_ofport=patch_int_ofport, - arp_responder_enabled=arp_responder_enabled) + arp_responder_enabled=arp_responder_enabled, + dvr_enabled=False) (dp, ofp, ofpp) = self._get_dp() expected = [ call._send_msg(ofpp.OFPFlowMod(dp, @@ -280,6 +282,33 @@ def test_setup_default_table_arp_responder_enabled(self): ] self.assertEqual(expected, self.mock.mock_calls) + def _test_setup_default_table_dvr_helper(self, dvr_enabled): + patch_int_ofport = 5555 + arp_responder_enabled = True + self.br.setup_default_table(patch_int_ofport=patch_int_ofport, + arp_responder_enabled=arp_responder_enabled, + dvr_enabled=dvr_enabled) + (dp, ofp, ofpp) = self._get_dp() + non_dvr_specific_call = call._send_msg( + ofpp.OFPFlowMod( + dp, + cookie=self.stamp, + instructions=[ofpp.OFPInstructionGotoTable(table_id=2)], + match=ofpp.OFPMatch(in_port=patch_int_ofport), + priority=1, table_id=0), + active_bundle=None) + + if dvr_enabled: + self.assertNotIn(non_dvr_specific_call, self.mock.mock_calls) + else: + self.assertIn(non_dvr_specific_call, self.mock.mock_calls) + + def test_setup_default_table_dvr_enabled(self): + self._test_setup_default_table_dvr_helper(dvr_enabled=True) + + def test_setup_default_table_dvr_disabled(self): + self._test_setup_default_table_dvr_helper(dvr_enabled=False) + def test_provision_local_vlan(self): network_type = 'vxlan' lvid = 888 diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py index 0e3b0c0d89b..6669b095829 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py @@ -188,7 +188,8 @@ def lookup_br(br_name, *args, **kwargs): '_check_bridge_datapath_id').start() self._define_expected_calls() - def _define_expected_calls(self, arp_responder=False, igmp_snooping=False): + def _define_expected_calls( + self, arp_responder=False, igmp_snooping=False): self.mock_int_bridge_cls_expected = [ mock.call(self.INT_BRIDGE, datapath_type=mock.ANY), @@ -268,7 +269,11 @@ def _define_expected_calls(self, arp_responder=False, igmp_snooping=False): ] self.mock_tun_bridge_expected += [ - mock.call.setup_default_table(self.INT_OFPORT, arp_responder), + # NOTE: Parameters passed to setup_default_table() method are named + # in the production code. That's why we can't use keyword parameter + # here. The last parameter passed below is dvr_enabled set to False + mock.call.setup_default_table( + self.INT_OFPORT, arp_responder, False), ] self.ipdevice_expected = [] From 8ebd7dbdf007b440d0aa79270d7d55f742652f19 Mon Sep 17 00:00:00 2001 From: Slawek Kaplonski Date: Wed, 2 Aug 2023 16:27:50 +0200 Subject: [PATCH 2/2] [UT] Create network to make lazy loading in the models_v2 possible In the test_port_deletion_prevention_handles_missing_port test which is part of the classes: * neutron.tests.unit.extensions.test_l3.L3NatDBSepTestCase * neutron.tests.unit.extensions.test_extraroute.ExtraRouteDBSepTestCase it is needed to call pecan application to e.g. create network to make all models to be ready. Otherwise things like port_forwarding attribute in the Port class, which is loaded in lazy mode isn't available and test was failing when was run in the isolated environment. It wasn't failing in the gate as there were other tests run by the same worker before this one and then it was all initialized properly. Closes-Bug: #2028285 Change-Id: Ie2382540d7c0a8813f093ddf51d82fe530026d71 (cherry picked from commit 14b2f4f60f1f1c00f41545394bd9761f57b6e37f) --- neutron/tests/unit/extensions/test_l3.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/neutron/tests/unit/extensions/test_l3.py b/neutron/tests/unit/extensions/test_l3.py index 07a5aea5142..c0c40ba0da7 100644 --- a/neutron/tests/unit/extensions/test_l3.py +++ b/neutron/tests/unit/extensions/test_l3.py @@ -4462,9 +4462,15 @@ class L3NatDBSepTestCase(L3BaseForSepTests, L3NatTestCaseBase, def test_port_deletion_prevention_handles_missing_port(self): pl = directory.get_plugin(plugin_constants.L3) - self.assertIsNone( - pl.prevent_l3_port_deletion(context.get_admin_context(), 'fakeid') - ) + # NOTE(slaweq): it's needed to make at least one API call to the + # application to initialize all models which are using lazy loading of + # some attributes, + # check https://bugs.launchpad.net/neutron/+bug/2028285 for details + with self.network(): + self.assertIsNone( + pl.prevent_l3_port_deletion(context.get_admin_context(), + 'fakeid') + ) class L3TestExtensionManagerWithDNS(L3TestExtensionManager):