From 106f6507db7db61f3fb6f55ebf6b6d9405d66079 Mon Sep 17 00:00:00 2001 From: Thomas Morin Date: Thu, 26 Jan 2017 15:19:54 +0100 Subject: [PATCH] Refactor OVSCookieBridge: always use bridge cookie Instead of having OVSCookieBridge as a passthrough class that does not provide the intended behavior (see bug 1557620), this change implements a cookie bridge as a patched copy of the underlying bridge: - the underlying bridge is copied - the copy is given an extension-specific cookie The 'extension bridge should only touch its flows' effect is obtained by a separate change (Idd0531cedda87224531cb8fb6a912ccd0f1554d5). The two problems in the bug are addressed: - the extension-specific cookie is now applied even for calls to methods other than add/delete/mod_flows - the extension-specific cookie is now applied in the case of the native/ryu implementation This commit also re-enable the use of uninstall_flows in the QoS OVS driver, which had to be disabled in Idd0531cedda87224531cb8fb6a912ccd0f1554d5, but can now be re-enabled with this bug addressed. This change complements the unit tests to confirm that the bug is fixed. Change-Id: I55835a34d8fba7a139dce93f99cbff54584d695c Closes-Bug: #1557620 Needed-By: I8570441a0b8d5ee3ad7f88e07affac2f1b782021 --- .../agent/extension_drivers/qos_driver.py | 5 +- .../openvswitch/agent/openflow/br_cookie.py | 8 + .../agent/ovs_agent_extension_api.py | 61 ++----- .../agent/test_ovs_agent_extension_api.py | 162 ++++++++++-------- 4 files changed, 109 insertions(+), 127 deletions(-) diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/extension_drivers/qos_driver.py b/neutron/plugins/ml2/drivers/openvswitch/agent/extension_drivers/qos_driver.py index fb21810bee3..d43e1eff8a3 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/extension_drivers/qos_driver.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/extension_drivers/qos_driver.py @@ -123,10 +123,7 @@ class QosOVSAgentDriver(qos.QosLinuxAgentDriver): RULE_TYPE_DSCP_MARKING, 0) if dscp_port: port_num = dscp_port['vif_port'].ofport - # TODO(tmorin): revert to uninstall_flows will be doable once - # https://review.openstack.org/#/c/425756/ merges - # self.br_int.uninstall_flows(in_port=port_num, table_id=0, reg2=0) - self.br_int.delete_flows(in_port=port_num, table=0, reg2=0) + self.br_int.uninstall_flows(in_port=port_num, table_id=0, reg2=0) else: LOG.debug("delete_dscp_marking was received for port %s but " "no port information was stored to be deleted", diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/br_cookie.py b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/br_cookie.py index eafba8f54f5..8815bf82e97 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/br_cookie.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/br_cookie.py @@ -13,6 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. +import copy + from neutron.agent.common import ovs_lib @@ -47,3 +49,9 @@ class OVSBridgeCookieMixin(object): if self._default_cookie in self._reserved_cookies: self._reserved_cookies.remove(self._default_cookie) super(OVSBridgeCookieMixin, self).set_agent_uuid_stamp(val) + + def clone(self): + '''Used by OVSCookieBridge, can be overridden by subclasses if a + behavior different from copy.copy is needed. + ''' + return copy.copy(self) diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_agent_extension_api.py b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_agent_extension_api.py index 56910fc5a53..ae0e812781c 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_agent_extension_api.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_agent_extension_api.py @@ -13,61 +13,24 @@ # License for the specific language governing permissions and limitations # under the License. -from neutron.agent.common import ovs_lib - class OVSCookieBridge(object): - '''Passthrough bridge adding cookies before calling the underlying bridge + '''Bridge restricting flow operations to its own distinct cookie - This class creates a bridge that will pass all calls to its underlying - bridge, except (add/mod/del/dump)_flow calls for which a cookie (reserved - at init from the underlying bridge) will be added before calling the - underlying bridge. + This class creates a bridge derived from a bridge passed at init (which + has to inherit from OVSBridgeCookieMixin), but that has its own cookie, + registered to the underlying bridge, and that will use this cookie in all + flow operations. ''' + def __new__(cls, bridge): + cookie_bridge = bridge.clone() + cookie_bridge.set_agent_uuid_stamp(bridge.request_cookie()) + + return cookie_bridge + def __init__(self, bridge): - """:param bridge: underlying bridge - :type bridge: OVSBridge - """ - self.bridge = bridge - self._cookie = self.bridge.request_cookie() - - @property - def default_cookie(self): - return self._cookie - - def do_action_flows(self, action, kwargs_list): - # NOTE(tmorin): the OVSBridge code is excluding the 'del' - # action from this step where a cookie - # is added, but I think we need to keep it so that - # an extension does not delete flows of another - # extension - for kw in kwargs_list: - kw.setdefault('cookie', self._cookie) - - if action is 'mod' or action is 'del': - kw['cookie'] = ovs_lib.check_cookie_mask(str(kw['cookie'])) - - self.bridge.do_action_flows(action, kwargs_list) - - def add_flow(self, **kwargs): - self.do_action_flows('add', [kwargs]) - - def mod_flow(self, **kwargs): - self.do_action_flows('mod', [kwargs]) - - def delete_flows(self, **kwargs): - self.do_action_flows('del', [kwargs]) - - def __getattr__(self, name): - # for all other methods this class is a passthrough - return getattr(self.bridge, name) - - def deferred(self, **kwargs): - # NOTE(tmorin): we can't passthrough for deferred() or else the - # resulting DeferredOVSBridge apply_flows method would call - # the (non-cookie-filtered) do_action_flow of the underlying bridge - return ovs_lib.DeferredOVSBridge(self, **kwargs) + pass class OVSAgentExtensionAPI(object): diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_agent_extension_api.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_agent_extension_api.py index 25906b8360c..60c34436966 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_agent_extension_api.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_agent_extension_api.py @@ -16,32 +16,30 @@ import mock -from neutron.plugins.ml2.drivers.openvswitch.agent.openflow.ovs_ofctl \ - import ovs_bridge from neutron.plugins.ml2.drivers.openvswitch.agent \ import ovs_agent_extension_api as ovs_ext_agt -from neutron.tests import base +from neutron.tests.unit.plugins.ml2.drivers.openvswitch.agent \ + .openflow.native import ovs_bridge_test_base as native_ovs_bridge_test_base + +from neutron.tests.unit.plugins.ml2.drivers.openvswitch.agent \ + import ovs_test_base -class TestOVSAgentExtensionAPI(base.BaseTestCase): +class TestOVSAgentExtensionAPI(ovs_test_base.OVSOFCtlTestBase): def setUp(self): - super(base.BaseTestCase, self).setUp() - conn_patcher = mock.patch( - 'neutron.agent.ovsdb.native.connection.Connection.start') - conn_patcher.start() - self.addCleanup(conn_patcher.stop) - self.br_int = ovs_bridge.OVSAgentBridge("br-int") - self.br_tun = ovs_bridge.OVSAgentBridge("br-tun") + super(TestOVSAgentExtensionAPI, self).setUp() + self.br_int = self.br_int_cls("br-int") + self.br_tun = self.br_tun_cls("br-tun") def _test_bridge(self, orig_bridge, new_bridge): self.assertIsNotNone(new_bridge) self.assertEqual(orig_bridge.br_name, new_bridge.br_name) - self.assertIn(new_bridge.default_cookie, + self.assertIn(new_bridge._default_cookie, orig_bridge.reserved_cookies) - self.assertNotEqual(orig_bridge.default_cookie, - new_bridge.default_cookie) + self.assertNotEqual(orig_bridge._default_cookie, + new_bridge._default_cookie) def test_request_int_br(self): agent_extension_api = ovs_ext_agt.OVSAgentExtensionAPI(self.br_int, @@ -61,52 +59,66 @@ class TestOVSAgentExtensionAPI(base.BaseTestCase): self.assertIsNone(agent_extension_api.request_tun_br()) -class TestOVSCookieBridge(base.DietTestCase): +class TestOVSCookieBridgeOFCtl(ovs_test_base.OVSOFCtlTestBase): def setUp(self): - super(TestOVSCookieBridge, self).setUp() - conn_patcher = mock.patch( - 'neutron.agent.ovsdb.native.connection.Connection.start') - conn_patcher.start() - self.addCleanup(conn_patcher.stop) - self.bridge = ovs_bridge.OVSAgentBridge("br-foo") - self.bridge.do_action_flows = mock.Mock() + super(TestOVSCookieBridgeOFCtl, self).setUp() + self.bridge = self.br_int_cls("br-int") + mock.patch.object(self.bridge, "run_ofctl").start() + self.tested_bridge = ovs_ext_agt.OVSCookieBridge(self.bridge) + # mocking do_action_flows does not work, because this method is + # later wrapped by the cookie bridge code, and six.wraps apparently + # can't wrap a mock, so we mock deeper + self.mock_build_flow_expr_str = mock.patch( + 'neutron.agent.common.ovs_lib._build_flow_expr_str', + return_value="").start() + + def test_cookie(self): + self.assertNotEqual(self.bridge._default_cookie, + self.tested_bridge._default_cookie) + def test_reserved(self): - self.assertIn(self.tested_bridge.default_cookie, + self.assertIn(self.tested_bridge._default_cookie, self.bridge.reserved_cookies) + def assert_mock_build_flow_expr_str_call(self, action, kwargs_list): + self.mock_build_flow_expr_str.assert_called_once_with( + kwargs_list[0], + action + ) + def test_add_flow_without_cookie(self): self.tested_bridge.add_flow(in_port=1, actions="output:2") - self.bridge.do_action_flows.assert_called_once_with( + self.assert_mock_build_flow_expr_str_call( 'add', [{"in_port": 1, "actions": "output:2", - "cookie": self.tested_bridge.default_cookie}] + "cookie": self.tested_bridge._default_cookie}] ) def test_mod_flow_without_cookie(self): self.tested_bridge.mod_flow(in_port=1, actions="output:2") - self.bridge.do_action_flows.assert_called_once_with( + self.assert_mock_build_flow_expr_str_call( 'mod', [{"in_port": 1, "actions": "output:2", - "cookie": str(self.tested_bridge.default_cookie) + '/-1'}] + "cookie": self.tested_bridge._default_cookie}] ) def test_del_flows_without_cookie(self): self.tested_bridge.delete_flows(in_port=1) - self.bridge.do_action_flows.assert_called_once_with( + self.assert_mock_build_flow_expr_str_call( 'del', [{"in_port": 1, - "cookie": str(self.tested_bridge.default_cookie) + '/-1'}] + "cookie": str(self.tested_bridge._default_cookie) + '/-1'}] ) def test_add_flow_with_cookie(self): self.tested_bridge.add_flow(cookie=1234, in_port=1, actions="output:2") - self.bridge.do_action_flows.assert_called_once_with( + self.assert_mock_build_flow_expr_str_call( 'add', [{"in_port": 1, "actions": "output:2", @@ -116,78 +128,80 @@ class TestOVSCookieBridge(base.DietTestCase): def test_mod_flow_with_cookie(self): self.tested_bridge.mod_flow(cookie='1234', in_port=1, actions="output:2") - self.bridge.do_action_flows.assert_called_once_with( + self.assert_mock_build_flow_expr_str_call( 'mod', [{"in_port": 1, "actions": "output:2", - "cookie": str(1234) + '/-1'}] + "cookie": "1234"}] ) def test_del_flows_with_cookie(self): self.tested_bridge.delete_flows(cookie=1234, in_port=1) - self.bridge.do_action_flows.assert_called_once_with( + self.assert_mock_build_flow_expr_str_call( 'del', [{"in_port": 1, - "cookie": str(1234) + '/-1'}] + "cookie": "1234/-1"}] ) - def test_mod_flow_with_mask(self): + def test_mod_flow_with_cookie_mask(self): self.tested_bridge.mod_flow(cookie='1234/3', in_port=1, actions="output:2") - self.bridge.do_action_flows.assert_called_once_with( + self.assert_mock_build_flow_expr_str_call( 'mod', [{"in_port": 1, "actions": "output:2", "cookie": str(1234) + '/3'}] ) - def test_del_flows_with_mask(self): + def test_del_flows_with_cookie_mask(self): self.tested_bridge.delete_flows(cookie='1234/7', in_port=1) - self.bridge.do_action_flows.assert_called_once_with( + self.assert_mock_build_flow_expr_str_call( 'del', [{"in_port": 1, "cookie": str(1234) + '/7'}] ) + def test_install_drop(self): + self.tested_bridge.install_drop() + self.assert_mock_build_flow_expr_str_call( + 'add', + [{"table": 0, + "priority": 0, + "actions": "drop", + "cookie": self.tested_bridge._default_cookie}] + ) -class TestOVSDeferredCookieBridge(base.DietTestCase): + +class TestOVSCookieBridgeRyu(native_ovs_bridge_test_base.OVSBridgeTestBase): def setUp(self): - super(TestOVSDeferredCookieBridge, self).setUp() - conn_patcher = mock.patch( - 'neutron.agent.ovsdb.native.connection.Connection.start') - conn_patcher.start() - self.addCleanup(conn_patcher.stop) - self.bridge = ovs_bridge.OVSAgentBridge("br-foo") - self.bridge.do_action_flows = mock.Mock() - self.cookie_bridge = ovs_ext_agt.OVSCookieBridge(self.bridge) - self.tested_bridge = self.cookie_bridge.deferred() + super(TestOVSCookieBridgeRyu, self).setUp() + self.setup_bridge_mock('br-int', self.br_int_cls) + self.tested_bridge = ovs_ext_agt.OVSCookieBridge(self.br) - def test_add_flow(self): - self.tested_bridge.add_flow(in_port=1, actions="output:2") - self.tested_bridge.apply_flows() - self.bridge.do_action_flows.assert_called_once_with( - 'add', - [{"in_port": 1, - "actions": "output:2", - "cookie": self.cookie_bridge.default_cookie}] - ) + def test_cookie(self): + self.assertNotEqual(self.br._default_cookie, + self.tested_bridge._default_cookie) - def test_mod_flow(self): - self.tested_bridge.mod_flow(in_port=1, actions="output:2") - self.tested_bridge.apply_flows() - self.bridge.do_action_flows.assert_called_once_with( - 'mod', - [{"in_port": 1, - "actions": "output:2", - "cookie": str(self.cookie_bridge.default_cookie) + '/-1'}] - ) + def test_reserved(self): + self.assertIn(self.tested_bridge._default_cookie, + self.br.reserved_cookies) - def test_del_flows(self): - self.tested_bridge.delete_flows(in_port=1) - self.tested_bridge.apply_flows() - self.bridge.do_action_flows.assert_called_once_with( - 'del', - [{"in_port": 1, - "cookie": str(self.cookie_bridge.default_cookie) + '/-1'}] - ) + def test_install_drop(self): + priority = 99 + in_port = 666 + self.tested_bridge.install_drop(priority=priority, + in_port=in_port) + (dp, ofp, ofpp) = self._get_dp() + expected = [ + mock.call._send_msg( + ofpp.OFPFlowMod( + dp, + # this is the interesting part of the check: + cookie=self.tested_bridge._default_cookie, + instructions=[], + match=ofpp.OFPMatch(in_port=in_port), + priority=priority, + table_id=0)), + ] + self.assertEqual(expected, self.mock.mock_calls)