openstack
/
neutron
Code Issues Proposed changes

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
This commit is contained in:
Thomas Morin 2017-01-26 15:19:54 +01:00
parent d761d26225
commit 106f6507db
4 changed files with 109 additions and 127 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

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

8
neutron/plugins/ml2/drivers/openvswitch/agent/openflow/br_cookie.py
View File

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

61
neutron/plugins/ml2/drivers/openvswitch/agent/ovs_agent_extension_api.py
View File

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

162
neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_agent_extension_api.py
View File

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