From 5981bf416d39f8ec15c937dabaf16d39a101d6ee Mon Sep 17 00:00:00 2001 From: Thomas Morin Date: Wed, 29 Mar 2017 11:11:01 -0500 Subject: [PATCH] OVSBridge: add --strict to allow priority in delete_flows n8g-sfc currently has its own variant of OVSBridge to allow the use of priority in a delete_flows call This change is meant to make this available outside n8g-sfc and simplify n8g-sfc code. This change adds a 'strict' boolean parameter to mod_flow and delete_flows that results in ovs-ofctl to be run with --strict for del-flows and mod-flows actions. When strict is set, the use of priority is allowed and hence not rejected anymore. Note that for batched actions in a deffered bridge, we disallow mixing calls with strict and without strict, which can't be translated in one CLI call. Needed-By: I3bf939590dd43bff685f133bff86eb7e9068de91 Change-Id: I289d546780f10dc1002ab6bc2e1b38c9ef2d728f --- neutron/agent/common/ovs_lib.py | 29 +++++++++++++++---- neutron/cmd/sanity/checks.py | 3 +- .../tests/functional/agent/test_ovs_lib.py | 8 +++++ .../tests/unit/agent/common/test_ovs_lib.py | 27 ++++++++++++++--- .../agent/test_ovs_agent_extension_api.py | 6 ++-- 5 files changed, 61 insertions(+), 12 deletions(-) diff --git a/neutron/agent/common/ovs_lib.py b/neutron/agent/common/ovs_lib.py index 249a8ba64e1..b9a7c67fff4 100644 --- a/neutron/agent/common/ovs_lib.py +++ b/neutron/agent/common/ovs_lib.py @@ -338,6 +338,10 @@ class OVSBridge(BaseOVS): self.br_name, 'datapath_id') def do_action_flows(self, action, kwargs_list): + # we can't mix strict and non-strict, so we'll use the first kw + # and check against other kw being different + strict = kwargs_list[0].get('strict', False) + for kw in kwargs_list: if action is 'del': if kw.get('cookie') == COOKIE_ANY: @@ -357,14 +361,27 @@ class OVSBridge(BaseOVS): if 'cookie' not in kw: kw['cookie'] = self._default_cookie + if action in ('mod', 'del'): + if kw.pop('strict', False) != strict: + msg = ("cannot mix 'strict' and not 'strict' in a batch " + "call") + raise exceptions.InvalidInput(error_message=msg) + else: + if kw.pop('strict', False): + msg = "cannot use 'strict' with 'add' action" + raise exceptions.InvalidInput(error_message=msg) + + strict_param = ["--strict"] if strict else [] + if action == 'del' and {} in kwargs_list: # the 'del' case simplifies itself if kwargs_list has at least # one item that matches everything self.run_ofctl('%s-flows' % action, []) else: - flow_strs = [_build_flow_expr_str(kw, action) + flow_strs = [_build_flow_expr_str(kw, action, strict) for kw in kwargs_list] - self.run_ofctl('%s-flows' % action, ['-'], '\n'.join(flow_strs)) + self.run_ofctl('%s-flows' % action, strict_param + ['-'], + '\n'.join(flow_strs)) def add_flow(self, **kwargs): self.do_action_flows('add', [kwargs]) @@ -733,7 +750,7 @@ class DeferredOVSBridge(object): self.br.br_name) -def _build_flow_expr_str(flow_dict, cmd): +def _build_flow_expr_str(flow_dict, cmd, strict): flow_expr_arr = [] actions = None @@ -745,8 +762,10 @@ def _build_flow_expr_str(flow_dict, cmd): flow_expr_arr.append("priority=%s" % flow_dict.pop('priority', '1')) elif 'priority' in flow_dict: - msg = _("Cannot match priority on flow deletion or modification") - raise exceptions.InvalidInput(error_message=msg) + if not strict: + msg = _("Cannot match priority on flow deletion or modification " + "without 'strict'") + raise exceptions.InvalidInput(error_message=msg) if cmd != 'del': if "actions" not in flow_dict: diff --git a/neutron/cmd/sanity/checks.py b/neutron/cmd/sanity/checks.py index 504683f8881..961d83d8fcb 100644 --- a/neutron/cmd/sanity/checks.py +++ b/neutron/cmd/sanity/checks.py @@ -95,7 +95,8 @@ def ofctl_arg_supported(cmd, **kwargs): br_name = common_utils.get_rand_device_name(prefix='br-test-') with ovs_lib.OVSBridge(br_name) as test_br: full_args = ["ovs-ofctl", cmd, test_br.br_name, - ovs_lib._build_flow_expr_str(kwargs, cmd.split('-')[0])] + ovs_lib._build_flow_expr_str(kwargs, cmd.split('-')[0], + False)] try: agent_utils.execute(full_args, run_as_root=True) except RuntimeError as e: diff --git a/neutron/tests/functional/agent/test_ovs_lib.py b/neutron/tests/functional/agent/test_ovs_lib.py index ab4a7d73a3d..dd7b4add99a 100644 --- a/neutron/tests/functional/agent/test_ovs_lib.py +++ b/neutron/tests/functional/agent/test_ovs_lib.py @@ -474,6 +474,14 @@ class OVSBridgeTestCase(OVSBridgeTestBase): self.br.delete_flows(cookie=ovs_lib.COOKIE_ANY) self.assertEqual([], self.br.dump_all_flows()) + def test_delete_flows_strict(self): + self.br.delete_flows(cookie=ovs_lib.COOKIE_ANY) # remove NORMAL action + self.br.add_flow(in_port=1, actions="output:2") + self.br.add_flow(in_port=1, priority=100, actions="output:3") + self.assertEqual(2, len(self.br.dump_all_flows())) + self.br.delete_flows(in_port=1, priority=100, strict=True) + self.assertEqual(1, len(self.br.dump_all_flows())) + class OVSLibTestCase(base.BaseOVSLinuxTestCase): diff --git a/neutron/tests/unit/agent/common/test_ovs_lib.py b/neutron/tests/unit/agent/common/test_ovs_lib.py index 22483a164a0..e2ffe07d64c 100644 --- a/neutron/tests/unit/agent/common/test_ovs_lib.py +++ b/neutron/tests/unit/agent/common/test_ovs_lib.py @@ -374,13 +374,32 @@ class OVS_Lib_Test(base.BaseTestCase): ] self.execute.assert_has_calls(expected_calls) - def test_delete_flow_with_priority_set(self): - params = {'in_port': '1', - 'priority': '1'} + def test_mod_delete_flows_strict(self): + self.br.delete_flows(in_port=5, priority=1, strict=True) + self.br.mod_flow(in_port=5, priority=1, strict=True, actions='drop') + cookie_spec = "cookie=%s" % self.br._default_cookie + expected_calls = [ + self._ofctl_mock("del-flows", self.BR_NAME, '--strict', '-', + process_input=StringSetMatcher( + "%s/-1,in_port=5,priority=1" % cookie_spec)), + self._ofctl_mock("mod-flows", self.BR_NAME, '--strict', '-', + process_input=StringSetMatcher( + "%s,in_port=5,priority=1,actions=drop" % + cookie_spec)), + ] + self.execute.assert_has_calls(expected_calls) + def test_mod_delete_flows_priority_without_strict(self): self.assertRaises(exceptions.InvalidInput, self.br.delete_flows, - **params) + in_port=5, priority=1) + + def test_mod_delete_flows_mixed_strict(self): + deferred_br = self.br.deferred() + deferred_br.delete_flows(in_port=5) + deferred_br.delete_flows(in_port=5, priority=1, strict=True) + self.assertRaises(exceptions.InvalidInput, + deferred_br.apply_flows) def test_dump_flows(self): table = 23 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 60c34436966..9f88560a522 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 @@ -83,10 +83,12 @@ class TestOVSCookieBridgeOFCtl(ovs_test_base.OVSOFCtlTestBase): self.assertIn(self.tested_bridge._default_cookie, self.bridge.reserved_cookies) - def assert_mock_build_flow_expr_str_call(self, action, kwargs_list): + def assert_mock_build_flow_expr_str_call(self, action, kwargs_list, + strict=False): self.mock_build_flow_expr_str.assert_called_once_with( kwargs_list[0], - action + action, + strict ) def test_add_flow_without_cookie(self):