From 7c3921585e404c31272c4d31ed326f7323c062d8 Mon Sep 17 00:00:00 2001 From: Cedric Brandily Date: Wed, 26 Nov 2014 00:31:22 +0000 Subject: [PATCH] Remove ovs-vsctl call from OVSInterfaceDriver ovs_lib module is responsible for the interaction with OVS but OVSInterfaceDriver._ovs_add_port() method calls directly ovs-vsctl which implies inconsistencies: ovs_lib calls ovs-vsctl with a timeout but not OVSInterfaceDriver._ovs_add_port(). This change moves ovs-vsctl call in OVSInterfaceDriver._ovs_add_port to ovs_lib. Closes-Bug: #1396489 Change-Id: I4d03f8beef2c5f2366ad2d1cf5f0b18b4f2857cd --- neutron/agent/linux/interface.py | 17 ++++----- neutron/agent/linux/ovs_lib.py | 9 +++++ .../tests/unit/agent/linux/test_ovs_lib.py | 21 +++++++++++ neutron/tests/unit/test_linux_interface.py | 36 ++++++++----------- 4 files changed, 51 insertions(+), 32 deletions(-) diff --git a/neutron/agent/linux/interface.py b/neutron/agent/linux/interface.py index fdc1d8e3f..0d7ed71c3 100644 --- a/neutron/agent/linux/interface.py +++ b/neutron/agent/linux/interface.py @@ -207,17 +207,14 @@ class OVSInterfaceDriver(LinuxInterfaceDriver): def _ovs_add_port(self, bridge, device_name, port_id, mac_address, internal=True): - cmd = ['ovs-vsctl', '--', '--if-exists', 'del-port', device_name, '--', - 'add-port', bridge, device_name] + attrs = [('external-ids:iface-id', port_id), + ('external-ids:iface-status', 'active'), + ('external-ids:attached-mac', mac_address)] if internal: - cmd += ['--', 'set', 'Interface', device_name, 'type=internal'] - cmd += ['--', 'set', 'Interface', device_name, - 'external-ids:iface-id=%s' % port_id, - '--', 'set', 'Interface', device_name, - 'external-ids:iface-status=active', - '--', 'set', 'Interface', device_name, - 'external-ids:attached-mac=%s' % mac_address] - utils.execute(cmd, self.root_helper) + attrs.insert(0, ('type', 'internal')) + + ovs = ovs_lib.OVSBridge(bridge, self.root_helper) + ovs.replace_port(device_name, *attrs) def plug(self, network_id, port_id, device_name, mac_address, bridge=None, namespace=None, prefix=None): diff --git a/neutron/agent/linux/ovs_lib.py b/neutron/agent/linux/ovs_lib.py index a2a8d4b3a..2a61187b9 100644 --- a/neutron/agent/linux/ovs_lib.py +++ b/neutron/agent/linux/ovs_lib.py @@ -150,6 +150,15 @@ class OVSBridge(BaseOVS): port_name]) return self.get_port_ofport(port_name) + def replace_port(self, port_name, *interface_attr_tuples): + """Replace existing port or create it, and configure port interface.""" + cmd = ['--', '--if-exists', 'del-port', port_name, + '--', 'add-port', self.br_name, port_name] + if interface_attr_tuples: + cmd += ['--', 'set', 'Interface', port_name] + cmd += ['%s=%s' % kv for kv in interface_attr_tuples] + self.run_vsctl(cmd) + def delete_port(self, port_name): self.run_vsctl(["--", "--if-exists", "del-port", self.br_name, port_name]) diff --git a/neutron/tests/unit/agent/linux/test_ovs_lib.py b/neutron/tests/unit/agent/linux/test_ovs_lib.py index 3d2611e2e..dfe39b52a 100644 --- a/neutron/tests/unit/agent/linux/test_ovs_lib.py +++ b/neutron/tests/unit/agent/linux/test_ovs_lib.py @@ -218,6 +218,27 @@ class OVS_Lib_Test(base.BaseTestCase): def _build_timeout_opt(self, exp_timeout): return "--timeout=%d" % exp_timeout if exp_timeout else self.TO + def test_replace_port(self): + pname = "tap5" + self.br.replace_port(pname) + self.execute.assert_called_once_with( + ["ovs-vsctl", self.TO, + "--", "--if-exists", "del-port", pname, + "--", "add-port", self.BR_NAME, pname], + root_helper=self.root_helper) + + def test_replace_port_with_attrs(self): + pname = "tap5" + self.br.replace_port(pname, ('type', 'internal'), + ('external-ids:iface-status', 'active')) + self.execute.assert_called_once_with( + ["ovs-vsctl", self.TO, + "--", "--if-exists", "del-port", pname, + "--", "add-port", self.BR_NAME, pname, + "--", "set", "Interface", pname, + "type=internal", "external-ids:iface-status=active"], + root_helper=self.root_helper) + def _test_delete_port(self, exp_timeout=None): exp_timeout_str = self._build_timeout_opt(exp_timeout) pname = "tap5" diff --git a/neutron/tests/unit/test_linux_interface.py b/neutron/tests/unit/test_linux_interface.py index 431687c9e..0136bd240 100644 --- a/neutron/tests/unit/test_linux_interface.py +++ b/neutron/tests/unit/test_linux_interface.py @@ -19,6 +19,7 @@ from neutron.agent.common import config from neutron.agent.linux import dhcp from neutron.agent.linux import interface from neutron.agent.linux import ip_lib +from neutron.agent.linux import ovs_lib from neutron.agent.linux import utils from neutron.extensions import flavor from neutron.openstack.common import uuidutils @@ -212,17 +213,7 @@ class TestOVSInterfaceDriver(TestBase): def device_exists(dev, root_helper=None, namespace=None): return dev == bridge - vsctl_cmd = ['ovs-vsctl', '--', '--if-exists', 'del-port', - 'tap0', '--', 'add-port', - bridge, 'tap0', '--', 'set', 'Interface', 'tap0', - 'type=internal', '--', 'set', 'Interface', 'tap0', - 'external-ids:iface-id=port-1234', '--', 'set', - 'Interface', 'tap0', - 'external-ids:iface-status=active', '--', 'set', - 'Interface', 'tap0', - 'external-ids:attached-mac=aa:bb:cc:dd:ee:ff'] - - with mock.patch.object(utils, 'execute') as execute: + with mock.patch.object(ovs_lib.OVSBridge, 'replace_port') as replace: ovs = interface.OVSInterfaceDriver(self.conf) self.device_exists.side_effect = device_exists ovs.plug('01234567-1234-1234-99', @@ -231,7 +222,12 @@ class TestOVSInterfaceDriver(TestBase): 'aa:bb:cc:dd:ee:ff', bridge=bridge, namespace=namespace) - execute.assert_called_once_with(vsctl_cmd, 'sudo') + replace.assert_called_once_with( + 'tap0', + ('type', 'internal'), + ('external-ids:iface-id', 'port-1234'), + ('external-ids:iface-status', 'active'), + ('external-ids:attached-mac', 'aa:bb:cc:dd:ee:ff')) expected = [mock.call('sudo'), mock.call().device('tap0'), @@ -300,15 +296,7 @@ class TestOVSInterfaceDriverWithVeth(TestOVSInterfaceDriver): mock.call().add_veth('tap0', devname, namespace2=namespace)] - vsctl_cmd = ['ovs-vsctl', '--', '--if-exists', 'del-port', - 'tap0', '--', 'add-port', - bridge, 'tap0', '--', 'set', 'Interface', 'tap0', - 'external-ids:iface-id=port-1234', '--', 'set', - 'Interface', 'tap0', - 'external-ids:iface-status=active', '--', 'set', - 'Interface', 'tap0', - 'external-ids:attached-mac=aa:bb:cc:dd:ee:ff'] - with mock.patch.object(utils, 'execute') as execute: + with mock.patch.object(ovs_lib.OVSBridge, 'replace_port') as replace: ovs.plug('01234567-1234-1234-99', 'port-1234', devname, @@ -316,7 +304,11 @@ class TestOVSInterfaceDriverWithVeth(TestOVSInterfaceDriver): bridge=bridge, namespace=namespace, prefix=prefix) - execute.assert_called_once_with(vsctl_cmd, 'sudo') + replace.assert_called_once_with( + 'tap0', + ('external-ids:iface-id', 'port-1234'), + ('external-ids:iface-status', 'active'), + ('external-ids:attached-mac', 'aa:bb:cc:dd:ee:ff')) ns_dev.assert_has_calls( [mock.call.link.set_address('aa:bb:cc:dd:ee:ff')]) -- 2.45.2