openstack
/
neutron
Code Issues Proposed changes

delete_flows shall only touch flows with the bridge cookie 

With this change delete_flows will only remove flows matching the default
cookie of the bridge.

The uninstall_flows implementation in the native bridge is also modified
to touch only the flows with the bridge cookie.

To still allow deletion of all cookies, cookie=COOKIE_ANY is introduced
as a special value, and used in the agent code in the places where the
intent is indeed to clean all flows whatever their cookie is.

Partial-Bug: #1557620
Change-Id: Idd0531cedda87224531cb8fb6a912ccd0f1554d5
This commit is contained in:
Thomas Morin 2017-02-23 17:44:38 -05:00
parent e21fa7049a
commit d761d26225
8 changed files with 138 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
neutron/agent/common/ovs_lib.py
View File

@ -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:

5
neutron/plugins/ml2/drivers/openvswitch/agent/extension_drivers/qos_driver.py
View File

@ -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",

19
neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/ofswitch.py
View File

@ -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)

3
neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py
View File

@ -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()

6
neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py
View File

@ -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

39
neutron/tests/functional/agent/test_ovs_flows.py
View File

@ -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

4
neutron/tests/functional/agent/test_ovs_lib.py
View File

@ -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())

61
neutron/tests/unit/agent/common/test_ovs_lib.py
View File

@ -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 '<comma-separated string for %s%s>' % (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),
]