diff --git a/neutron/agent/common/ovs_lib.py b/neutron/agent/common/ovs_lib.py index 30e3041e058..249a8ba64e1 100644 --- a/neutron/agent/common/ovs_lib.py +++ b/neutron/agent/common/ovs_lib.py @@ -47,6 +47,9 @@ UNASSIGNED_OFPORT = [] FAILMODE_SECURE = 'secure' FAILMODE_STANDALONE = 'standalone' +# special values for cookies +COOKIE_ANY = object() + ovs_conf.register_ovs_agent_opts() LOG = logging.getLogger(__name__) @@ -335,15 +338,30 @@ class OVSBridge(BaseOVS): self.br_name, 'datapath_id') def do_action_flows(self, action, kwargs_list): + for kw in kwargs_list: + if action is 'del': + if kw.get('cookie') == COOKIE_ANY: + # special value COOKIE_ANY was provided, unset + # cookie to match flows whatever their cookie is + kw.pop('cookie') + if kw.get('cookie_mask'): # non-zero cookie mask + LOG.error(_LE("cookie=COOKIE_ANY but cookie_mask set " + "to %s"), kw.get('cookie_mask')) + elif 'cookie' in kw: + # a cookie was specified, use it + kw['cookie'] = check_cookie_mask(kw['cookie']) + else: + # nothing was specified about cookies, use default + kw['cookie'] = "%d/-1" % self._default_cookie + else: + if 'cookie' not in kw: + kw['cookie'] = self._default_cookie + 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: - if action != 'del': - for kw in kwargs_list: - if 'cookie' not in kw: - kw['cookie'] = self._default_cookie flow_strs = [_build_flow_expr_str(kw, action) for kw in kwargs_list] self.run_ofctl('%s-flows' % action, ['-'], '\n'.join(flow_strs)) @@ -755,6 +773,7 @@ def generate_random_cookie(): def check_cookie_mask(cookie): + cookie = str(cookie) if '/' not in cookie: return cookie + '/-1' else: 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 d43e1eff8a3..fb21810bee3 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,7 +123,10 @@ class QosOVSAgentDriver(qos.QosLinuxAgentDriver): RULE_TYPE_DSCP_MARKING, 0) if dscp_port: port_num = dscp_port['vif_port'].ofport - self.br_int.uninstall_flows(in_port=port_num, table_id=0, reg2=0) + # 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) 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/native/ofswitch.py b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/ofswitch.py index e1c1e1521b1..1531ae3bd2a 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/ofswitch.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/ofswitch.py @@ -23,10 +23,13 @@ from oslo_utils import timeutils import ryu.app.ofctl.api as ofctl_api import ryu.exception as ryu_exc -from neutron._i18n import _, _LW +from neutron._i18n import _, _LW, _LE +from neutron.agent.common import ovs_lib LOG = logging.getLogger(__name__) +COOKIE_DEFAULT = object() + class OpenFlowSwitchMixin(object): """Mixin to provide common convenient routines for an openflow switch. @@ -100,11 +103,21 @@ class OpenFlowSwitchMixin(object): return ofpp.OFPMatch(**match_kwargs) def uninstall_flows(self, table_id=None, strict=False, priority=0, - cookie=0, cookie_mask=0, + cookie=COOKIE_DEFAULT, cookie_mask=0, match=None, **match_kwargs): (dp, ofp, ofpp) = self._get_dp() if table_id is None: table_id = ofp.OFPTT_ALL + + if cookie == ovs_lib.COOKIE_ANY: + cookie = 0 + if cookie_mask != 0: + LOG.error(_LE("cookie=COOKIE_ANY but cookie_mask set to %s"), + cookie_mask) + elif cookie == COOKIE_DEFAULT: + cookie = self._default_cookie + cookie_mask = ovs_lib.UINT64_BITMASK + match = self._match(ofp, ofpp, match, **match_kwargs) if strict: cmd = ofp.OFPFC_DELETE_STRICT @@ -140,7 +153,7 @@ class OpenFlowSwitchMixin(object): for c in cookies: LOG.warning(_LW("Deleting flow with cookie 0x%(cookie)x"), {'cookie': c}) - self.uninstall_flows(cookie=c, cookie_mask=((1 << 64) - 1)) + self.uninstall_flows(cookie=c, cookie_mask=ovs_lib.UINT64_BITMASK) def install_goto_next(self, table_id): self.install_goto(table_id=table_id, dest_table_id=table_id + 1) 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 16ce7d6abc4..60e5b8a27de 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 @@ -23,6 +23,7 @@ from oslo_utils import excutils from osprofiler import profiler from neutron._i18n import _LE, _LI, _LW +from neutron.agent.common import ovs_lib from neutron.common import utils as n_utils from neutron.plugins.common import constants as p_const from neutron.plugins.ml2.drivers.openvswitch.agent.common import constants @@ -209,7 +210,7 @@ class OVSDVRNeutronAgent(object): self.dvr_mac_address) # Remove existing flows in integration bridge if self.conf.AGENT.drop_flows_on_start: - self.int_br.uninstall_flows() + self.int_br.uninstall_flows(cookie=ovs_lib.COOKIE_ANY) # Add a canary flow to int_br to track OVS restarts self.int_br.setup_canary_table() 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 825013816f7..0955bdca061 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py @@ -996,7 +996,7 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, # the flows on br-int, so that traffic doesn't get flooded over # while flows are missing. self.int_br.delete_port(self.conf.OVS.int_peer_patch_port) - self.int_br.uninstall_flows() + self.int_br.uninstall_flows(cookie=ovs_lib.COOKIE_ANY) self.int_br.setup_default_table() def setup_ancillary_bridges(self, integ_br, tun_br): @@ -1058,7 +1058,7 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, "ports. Agent terminated!")) sys.exit(1) if self.conf.AGENT.drop_flows_on_start: - self.tun_br.uninstall_flows() + self.tun_br.uninstall_flows(cookie=ovs_lib.COOKIE_ANY) def setup_tunnel_br_flows(self): '''Setup the tunnel bridge. @@ -1102,7 +1102,7 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, br.set_secure_mode() br.setup_controllers(self.conf) if cfg.CONF.AGENT.drop_flows_on_start: - br.uninstall_flows() + br.uninstall_flows(cookie=ovs_lib.COOKIE_ANY) br.setup_default_table() self.phys_brs[physical_network] = br diff --git a/neutron/tests/functional/agent/test_ovs_flows.py b/neutron/tests/functional/agent/test_ovs_flows.py index c55f521b838..c4cd8b993bb 100644 --- a/neutron/tests/functional/agent/test_ovs_flows.py +++ b/neutron/tests/functional/agent/test_ovs_flows.py @@ -24,6 +24,7 @@ from oslo_serialization import jsonutils from oslo_utils import importutils from testtools.content import text_content +from neutron.agent.common import ovs_lib from neutron.agent.common import utils from neutron.agent.linux import ip_lib from neutron.cmd.sanity import checks @@ -311,7 +312,7 @@ class ARPSpoofTestCase(OVSAgentTestBase): class CanaryTableTestCase(OVSAgentTestBase): def test_canary_table(self): - self.br_int.uninstall_flows() + self.br_int.uninstall_flows(cookie=ovs_lib.COOKIE_ANY) self.assertEqual(constants.OVS_RESTARTED, self.br_int.check_canary_table()) self.br_int.setup_canary_table() @@ -319,6 +320,42 @@ class CanaryTableTestCase(OVSAgentTestBase): self.br_int.check_canary_table()) +class DeleteFlowsTestCase(OVSAgentTestBase): + + def test_delete_flows_bridge_cookie_only(self): + PORT = 1 + + self.br_int.add_flow(in_port=PORT, ip=True, nw_dst="1.1.1.1", + actions="output:11") + self.br_int.add_flow(in_port=PORT, ip=True, nw_dst="2.2.2.2", + cookie=42, actions="output:42") + + # delete (should only delete flows with the bridge cookie) + self.br_int.delete_flows(in_port=PORT) + flows = self.br_int.dump_flows_for(in_port=PORT, + cookie=self.br_int._default_cookie) + flows42 = self.br_int.dump_flows_for(in_port=PORT, cookie=42) + + # check that only flows with cookie 42 remain + self.assertFalse(flows) + self.assertTrue(flows42) + + def test_delete_flows_all(self): + PORT = 1 + + self.br_int.add_flow(in_port=PORT, ip=True, nw_dst="1.1.1.1", + actions="output:11") + self.br_int.add_flow(in_port=PORT, ip=True, nw_dst="2.2.2.2", + cookie=42, actions="output:42") + + # delete both flows + self.br_int.delete_flows(in_port=PORT, cookie=ovs_lib.COOKIE_ANY) + + # check that no flow remains + flows = self.br_int.dump_flows_for(in_port=PORT) + self.assertFalse(flows) + + class OVSFlowTestCase(OVSAgentTestBase): """Tests defined in this class use ovs-appctl ofproto/trace commands, which simulate processing of imaginary packets, to check desired actions diff --git a/neutron/tests/functional/agent/test_ovs_lib.py b/neutron/tests/functional/agent/test_ovs_lib.py index b0aad203a32..ab4a7d73a3d 100644 --- a/neutron/tests/functional/agent/test_ovs_lib.py +++ b/neutron/tests/functional/agent/test_ovs_lib.py @@ -469,9 +469,9 @@ class OVSBridgeTestCase(OVSBridgeTestBase): self.assertRaises((RuntimeError, idlutils.RowNotFound), del_port_mod_iface) - def test_delete_flows_no_args(self): + def test_delete_flows_all(self): self.br.add_flow(in_port=1, actions="output:2") - self.br.delete_flows() + self.br.delete_flows(cookie=ovs_lib.COOKIE_ANY) self.assertEqual([], self.br.dump_all_flows()) diff --git a/neutron/tests/unit/agent/common/test_ovs_lib.py b/neutron/tests/unit/agent/common/test_ovs_lib.py index 729cdc5a814..22483a164a0 100644 --- a/neutron/tests/unit/agent/common/test_ovs_lib.py +++ b/neutron/tests/unit/agent/common/test_ovs_lib.py @@ -61,6 +61,29 @@ class OFCTLParamListMatcher(object): __repr__ = __str__ +class StringSetMatcher(object): + """A helper object for unordered CSV strings + + Will compare equal if both strings, when read as a comma-separated set + of values, represent the same set. + + Example: "a,b,45" == "b,45,a" + """ + def __init__(self, string, separator=','): + self.separator = separator + self.set = set(string.split(self.separator)) + + def __eq__(self, other): + return self.set == set(other.split(self.separator)) + + def __ne__(self, other): + return self.set != set(other.split(self.separator)) + + def __repr__(self): + sep = '' if self.separator == ',' else " on %s" % self.separator + return '' % (self.set, sep) + + def vsctl_only(f): # NOTE(ivasilevskaya) as long as some tests rely heavily on mocking # direct vsctl commands, need to ensure that ovsdb_interface = 'vsctl' @@ -298,38 +321,54 @@ class OVS_Lib_Test(base.BaseTestCase): self._verify_ofctl_mock("dump-flows", self.BR_NAME, process_input=None) def test_delete_flow(self): - ofport = "5" + ofport = 5 lsw_id = 40 vid = 39 self.br.delete_flows(in_port=ofport) self.br.delete_flows(tun_id=lsw_id) self.br.delete_flows(dl_vlan=vid) self.br.delete_flows() + cookie_spec = "cookie=%s/-1" % self.br._default_cookie expected_calls = [ self._ofctl_mock("del-flows", self.BR_NAME, '-', - process_input="in_port=" + ofport), + process_input=StringSetMatcher( + "%s,in_port=%d" % (cookie_spec, ofport))), self._ofctl_mock("del-flows", self.BR_NAME, '-', - process_input="tun_id=%s" % lsw_id), + process_input=StringSetMatcher( + "%s,tun_id=%s" % (cookie_spec, lsw_id))), self._ofctl_mock("del-flows", self.BR_NAME, '-', - process_input="dl_vlan=%s" % vid), - self._ofctl_mock("del-flows", self.BR_NAME, - process_input=None), + process_input=StringSetMatcher( + "%s,dl_vlan=%s" % (cookie_spec, vid))), + self._ofctl_mock("del-flows", self.BR_NAME, '-', + process_input="%s" % cookie_spec), ] self.execute.assert_has_calls(expected_calls) + def test_delete_flows_cookie_nomask(self): + self.br.delete_flows(cookie=42) + self.execute.assert_has_calls([ + self._ofctl_mock("del-flows", self.BR_NAME, '-', + process_input="cookie=42/-1"), + ]) + def test_do_action_flows_delete_flows(self): - # test what the deffered bridge implementation calls in the case of a - # delete_flows() among calls to delete_flows(foo=bar) - self.br.do_action_flows('del', [{'in_port': 5}, {}]) + # test what the deferred bridge implementation calls, in the case of a + # delete_flows(cookie=ovs_lib.COOKIE_ANY) among calls to + # delete_flows(foo=bar) + self.br.do_action_flows('del', [{'in_port': 5}, + {'cookie': ovs_lib.COOKIE_ANY}]) expected_calls = [ self._ofctl_mock("del-flows", self.BR_NAME, process_input=None), ] self.execute.assert_has_calls(expected_calls) - def test_do_action_flows_delete_flows_empty(self): - self.br.delete_flows() + def test_delete_flows_any_cookie(self): + self.br.delete_flows(in_port=5, cookie=ovs_lib.COOKIE_ANY) + self.br.delete_flows(cookie=ovs_lib.COOKIE_ANY) expected_calls = [ + self._ofctl_mock("del-flows", self.BR_NAME, '-', + process_input="in_port=5"), self._ofctl_mock("del-flows", self.BR_NAME, process_input=None), ]