From c0d101aa81cff200e1db2a0746598b72e26748e4 Mon Sep 17 00:00:00 2001 From: Sean Mooney Date: Tue, 9 May 2023 19:52:48 +0100 Subject: [PATCH] set default qos policy This change modifies the os-vif ovs plugin to set a default tc qdisc on ovs interface when the host os is not windows and the system datapath is used. This change fixes a "silent" bug in the functional test code due to a change in an ovsdbapp function signiture to accpet a new paramater. Closes-Bug: #2017868 Change-Id: Id9ef7074634a0f23d67a4401fa8fca363b51bb43 --- .../internal/command/ip/test_impl_pyroute2.py | 7 + ...t-qos-policy-for-ovs-26f8806046a59c82.yaml | 33 ++++ vif_plug_ovs/ovs.py | 68 ++++++- vif_plug_ovs/ovsdb/impl_idl.py | 3 +- vif_plug_ovs/ovsdb/ovsdb_lib.py | 53 ++++- vif_plug_ovs/tests/functional/base.py | 16 ++ .../tests/functional/ovsdb/test_ovsdb_lib.py | 182 ++++++++++++++++-- vif_plug_ovs/tests/functional/test_plugin.py | 85 ++++++++ vif_plug_ovs/tests/unit/test_plugin.py | 23 ++- 9 files changed, 442 insertions(+), 28 deletions(-) create mode 100644 releasenotes/notes/default-qos-policy-for-ovs-26f8806046a59c82.yaml diff --git a/os_vif/tests/functional/internal/command/ip/test_impl_pyroute2.py b/os_vif/tests/functional/internal/command/ip/test_impl_pyroute2.py index fdefc421..8121a177 100644 --- a/os_vif/tests/functional/internal/command/ip/test_impl_pyroute2.py +++ b/os_vif/tests/functional/internal/command/ip/test_impl_pyroute2.py @@ -12,6 +12,7 @@ import os import re +import time from oslo_concurrency import processutils from oslo_utils import excutils @@ -39,6 +40,12 @@ class ShellIpCommands(object): 'peer', 'name', peer) elif 'dummy' == dev_type: _execute_command('ip', 'link', 'add', device, 'type', dev_type) + # ensure that the device exists to prevent racing + # with other ip commands + for _ in range(10): + if self.exist_device(device): + return + time.sleep(0.1) def del_device(self, device): if self.exist_device(device): diff --git a/releasenotes/notes/default-qos-policy-for-ovs-26f8806046a59c82.yaml b/releasenotes/notes/default-qos-policy-for-ovs-26f8806046a59c82.yaml new file mode 100644 index 00000000..3feb2a53 --- /dev/null +++ b/releasenotes/notes/default-qos-policy-for-ovs-26f8806046a59c82.yaml @@ -0,0 +1,33 @@ +--- +upgrade: + - | + A new config option has been added to the OpenvSwitch plugin + ``[os_vif_ovs]default_qos_type``. This option controls + the Default tc qdisc applied to a kernel interface attached to OpenvSwitch + on Linux hosts. As of this release, the default tc qdisc is ``linux-noop`` + other supported values are ``linux-htb``, ``linux-hfsc``, + ``linux-sfq``, ``linux-codel`` and ``linux-fq_codel``. + before this release the default qdisc was undefined. older kernels did not + apply /proc/sys/net/core/default_qdisc to tap devices. newer kernels such + as the one found in rhel 9 do. This can significantly impact performance. + See bug https://bugs.launchpad.net/os-vif/+bug/2017868 for more details. + The default ``linux-noop`` should perform well for all use-cases so no + explicit action is required on upgrade however it should be noted that + the default_qos_type is only set when a port is first created. As such + this fix will not take effect until the next time the vm interface is + recreated. If you change this value for an existing port it will only + take effect after a hard reboot of the VM or a move operation. + +fixes: + - | + A significant performance regression was observed on a subset of Linux + kernels and sysctl configurations resulting in a reduction of throughput + to between 10% of the prior performance for small packets and 50% for + large packets. This has now been resolved by setting a default + qos_type on ovs interfaces when they are first created. To mimic libvirt's + undocumented behavior the ``linux-noop`` type is set on the ovs port when + it is first created. This will be overridden by neutron if a qos policy + is defined for a port and is simply the initial value to use when first + adding a port to OpenvSwitch. The default QoS type applied can be + controlled by the ``[os_vif_ovs]default_qos_type`` config operation. + See bug https://bugs.launchpad.net/os-vif/+bug/2017868 for more details. \ No newline at end of file diff --git a/vif_plug_ovs/ovs.py b/vif_plug_ovs/ovs.py index 4368ba4e..1c43a695 100644 --- a/vif_plug_ovs/ovs.py +++ b/vif_plug_ovs/ovs.py @@ -102,7 +102,26 @@ class OvsPlugin(plugin.PluginBase): 'bridge. This is experimental and controls the plugging ' 'behavior when not using hybrid-plug.' 'This is only used on linux and should be set to false ' - 'in all other cases such as ironic smartnic ports.') + 'in all other cases such as ironic smartnic ports.'), + cfg.StrOpt('default_qos_type', + choices=[ + 'linux-htb', 'linux-hfsc', 'linux-sfq', 'linux-codel', + 'linux-fq_codel', 'linux-noop' + ], + default='linux-noop', + help=""" + The default qos type to apply to ovs ports. + linux-noop is the default. ovs will not modify + the qdisc on the port if linux-noop is specified. + This allows operators to manage QOS out of band + of OVS. For more information see the ovs man pages + https://manpages.debian.org/testing/openvswitch-common/ovs-vswitchd.conf.db.5.en.html#type~4 + + Note: This will only be set when a port is first created + on the ovs bridge to ensure that the qos type can be + managed via neutron if required for bandwidth limiting + and other use-cases. + """), ) def __init__(self, config): @@ -159,6 +178,14 @@ class OvsPlugin(plugin.PluginBase): return vif.network.mtu return self.config.network_device_mtu + def supports_tc_qdisc(self, vif) -> bool: + if self._get_vif_datapath_type(vif) != constants.OVS_DATAPATH_SYSTEM: + return False + if sys.platform == constants.PLATFORM_WIN32: + return False + + return True + def _create_vif_port(self, vif, vif_name, instance_info, **kwargs): mtu = self._get_mtu(vif) # NOTE(sean-k-mooney): As part of a partial fix to bug #1734320 @@ -175,6 +202,19 @@ class OvsPlugin(plugin.PluginBase): # can be enabled automatically in the future. if self.config.isolate_vif: kwargs['tag'] = constants.DEAD_VLAN + qos_type = self._get_qos_type(vif) + if qos_type is not None: + # NOTE(sean-k-mooney): If the port is not already created + # on the bridge we need to set the default qos type to + # ensure that the port is created with the correct qos + # type. This is only needed for the linux kernel datapath + # as the qos type is not managed by neutron for the other + # datapaths. + # This is a mitigation for the performance regression + # introduced by the fix for bug #1734320. See bug #2017868 + # for more details. + if not self.ovsdb.port_exists(vif_name, vif.network.bridge): + kwargs['qos_type'] = qos_type bridge = kwargs.pop('bridge', vif.network.bridge) self.ovsdb.create_ovs_vif_port( bridge, @@ -382,9 +422,18 @@ class OvsPlugin(plugin.PluginBase): linux_net.delete_bridge(linux_bridge_name, v1_name) - self.ovsdb.delete_ovs_vif_port(vif.network.bridge, v2_name) + qos_type = self._get_qos_type(vif) + self.ovsdb.delete_ovs_vif_port( + vif.network.bridge, v2_name, qos_type=qos_type + ) self._delete_bridge_if_trunk(vif) + def _get_qos_type(self, vif): + qos_type = None + if self.supports_tc_qdisc(vif): + qos_type = self.config.default_qos_type + return qos_type + def _unplug_vif_windows(self, vif, instance_info): """Remove port from OVS.""" self.ovsdb.delete_ovs_vif_port(vif.network.bridge, vif.id, @@ -400,7 +449,10 @@ class OvsPlugin(plugin.PluginBase): int_bridge_patch = self.gen_port_name('ibp', vif.id, max_length=64) self.ovsdb.delete_ovs_vif_port(vif.network.bridge, int_bridge_patch) self.ovsdb.delete_ovs_vif_port(port_bridge_name, port_bridge_patch) - self.ovsdb.delete_ovs_vif_port(port_bridge_name, vif.vif_name) + qos_type = self._get_qos_type(vif) + self.ovsdb.delete_ovs_vif_port( + port_bridge_name, vif.vif_name, qos_type=qos_type + ) self.ovsdb.delete_ovs_bridge(port_bridge_name) self._delete_bridge_if_trunk(vif) @@ -409,7 +461,10 @@ class OvsPlugin(plugin.PluginBase): # NOTE(sean-k-mooney): even with the partial revert of change # Iaf15fa7a678ec2624f7c12f634269c465fbad930 this should be correct # so this is not removed. - self.ovsdb.delete_ovs_vif_port(vif.network.bridge, vif.vif_name) + qos_type = self._get_qos_type(vif) + self.ovsdb.delete_ovs_vif_port( + vif.network.bridge, vif.vif_name, qos_type=qos_type + ) self._delete_bridge_if_trunk(vif) def _unplug_vf(self, vif): @@ -428,8 +483,11 @@ class OvsPlugin(plugin.PluginBase): # The representor interface can't be deleted because it bind the # SR-IOV VF, therefore we just need to remove it from the ovs bridge # and set the status to down + qos_type = self._get_qos_type(vif) self.ovsdb.delete_ovs_vif_port( - vif.network.bridge, representor, delete_netdev=False) + vif.network.bridge, representor, delete_netdev=False, + qos_type=qos_type + ) if datapath == constants.OVS_DATAPATH_SYSTEM: linux_net.set_interface_state(representor, 'down') self._delete_bridge_if_trunk(vif) diff --git a/vif_plug_ovs/ovsdb/impl_idl.py b/vif_plug_ovs/ovsdb/impl_idl.py index 32cf0808..668c8c7f 100644 --- a/vif_plug_ovs/ovsdb/impl_idl.py +++ b/vif_plug_ovs/ovsdb/impl_idl.py @@ -23,7 +23,7 @@ from ovsdbapp.schema.open_vswitch import impl_idl from vif_plug_ovs.ovsdb import api -REQUIRED_TABLES = ('Interface', 'Port', 'Bridge', 'Open_vSwitch') +REQUIRED_TABLES = ('Interface', 'Port', 'Bridge', 'Open_vSwitch', 'QoS') def idl_factory(config): @@ -48,6 +48,7 @@ class NeutronOvsdbIdl(impl_idl.OvsdbIdl, api.ImplAPI): This class provides an OVSDB IDL (Open vSwitch Database Interface Definition Language) interface to the OVS back-end. """ + def __init__(self, conn): vlog.use_python_logger() super(NeutronOvsdbIdl, self).__init__(conn) diff --git a/vif_plug_ovs/ovsdb/ovsdb_lib.py b/vif_plug_ovs/ovsdb/ovsdb_lib.py index f2348cd1..f88f5672 100644 --- a/vif_plug_ovs/ovsdb/ovsdb_lib.py +++ b/vif_plug_ovs/ovsdb/ovsdb_lib.py @@ -11,6 +11,7 @@ # under the License. import sys +import uuid from oslo_log import log as logging @@ -20,6 +21,7 @@ from vif_plug_ovs.ovsdb import api as ovsdb_api LOG = logging.getLogger(__name__) +QOS_UUID_NAMESPACE = uuid.UUID("68da264a-847f-42a8-8ab0-5e774aee3d95") class BaseOVS(object): @@ -142,7 +144,8 @@ class BaseOVS(object): def create_ovs_vif_port( self, bridge, dev, iface_id, mac, instance_id, mtu=None, interface_type=None, vhost_server_path=None, - tag=None, pf_pci=None, vf_num=None, set_ids=True, datapath_type=None + tag=None, pf_pci=None, vf_num=None, set_ids=True, datapath_type=None, + qos_type=None ): """Create OVS port @@ -159,6 +162,7 @@ class BaseOVS(object): :param vf_num: VF number of PF for dpdk representor port. :param set_ids: set external ids on port (bool). :param datapath_type: datapath type for port's bridge + :param qos_type: qos type for a port .. note:: create DPDK representor port by setting all three values: `interface_type`, `pf_pci` and `vf_num`. if interface type is @@ -181,6 +185,24 @@ class BaseOVS(object): PF_PCI=pf_pci, VF_NUM=vf_num) col_values.append(('options', {'dpdk-devargs': devargs_string})) + # create qos record if qos type is specified + # and get the qos id. This is done outside of the transaction + # because we need the qos id to set the qos on the port. + # The qos uuid cannot be set when creating the record so we + # have to look it up after the record is created. this means + # we need to create the qos record outside of the transaction + # that creates the port. + qid = None + if qos_type: + self.delete_qos_if_exists(dev, qos_type) + qos_id = uuid.uuid5(QOS_UUID_NAMESPACE, dev) + qos_external_ids = {'id': str(qos_id), '_type': qos_type} + self.ovsdb.db_create( + 'QoS', type=qos_type, external_ids=qos_external_ids + ).execute(check_error=True) + record = self.get_qos(dev, qos_type) + qid = record[0]['_uuid'] + with self.ovsdb.transaction() as txn: if datapath_type: txn.add(self.ovsdb.add_br(bridge, may_exist=True, @@ -188,19 +210,46 @@ class BaseOVS(object): txn.add(self.ovsdb.add_port(bridge, dev)) if tag: txn.add(self.ovsdb.db_set('Port', dev, ('tag', tag))) + if qid: + txn.add(self.ovsdb.db_set('Port', dev, ('qos', qid))) if col_values: txn.add(self.ovsdb.db_set('Interface', dev, *col_values)) self.update_device_mtu( txn, dev, mtu, interface_type=interface_type ) + def port_exists(self, port_name, bridge): + ports = self.ovsdb.list_ports(bridge).execute() + return ports is not None and port_name in ports + + def get_qos(self, dev, qos_type): + qos_id = uuid.uuid5(QOS_UUID_NAMESPACE, dev) + external_ids = {'id': str(qos_id), '_type': qos_type} + return self.ovsdb.db_find( + 'QoS', ('external_ids', '=', external_ids), + colmuns=['_uuid'] + ).execute() + + def delete_qos_if_exists(self, dev, qos_type): + qos_ids = self.get_qos(dev, qos_type) + if qos_ids is not None and len(qos_ids) > 0: + for qos_id in qos_ids: + if '_uuid' in qos_id: + self.ovsdb.db_destroy( + 'QoS', str(qos_id['_uuid']) + ).execute() + def update_ovs_vif_port(self, dev, mtu=None, interface_type=None): with self.ovsdb.transaction() as txn: self.update_device_mtu( txn, dev, mtu, interface_type=interface_type ) - def delete_ovs_vif_port(self, bridge, dev, delete_netdev=True): + def delete_ovs_vif_port( + self, bridge, dev, delete_netdev=True, qos_type=None + ): self.ovsdb.del_port(dev, bridge=bridge, if_exists=True).execute() + if qos_type: + self.delete_qos_if_exists(dev, qos_type) if delete_netdev: linux_net.delete_net_dev(dev) diff --git a/vif_plug_ovs/tests/functional/base.py b/vif_plug_ovs/tests/functional/base.py index 22d8e5ca..59eb5415 100644 --- a/vif_plug_ovs/tests/functional/base.py +++ b/vif_plug_ovs/tests/functional/base.py @@ -25,6 +25,22 @@ class VifPlugOvsBaseFunctionalTestCase(os_vif_base.BaseFunctionalTestCase): def _check_bridge(self, name): return self._ovsdb.br_exists(name).execute() + def _check_port(self, name, bridge): + return self.ovs.port_exists(name, bridge) + + def _check_parameter(self, table, port, parameter, expected_value): + def get_value(): + return self._ovsdb.db_get(table, port, parameter).execute() + + def check_value(): + val = get_value() + return val == expected_value + self.assertTrue( + wait_until_true(check_value, timeout=2, sleep=0.5), + f"Parameter {parameter} of {table} {port} is {get_value()} " + f"not {expected_value}" + ) + def _add_bridge(self, name, may_exist=True, datapath_type=None): self._ovsdb.add_br(name, may_exist=may_exist, datapath_type=datapath_type).execute() diff --git a/vif_plug_ovs/tests/functional/ovsdb/test_ovsdb_lib.py b/vif_plug_ovs/tests/functional/ovsdb/test_ovsdb_lib.py index 64b6d8c8..5b8d8b3d 100644 --- a/vif_plug_ovs/tests/functional/ovsdb/test_ovsdb_lib.py +++ b/vif_plug_ovs/tests/functional/ovsdb/test_ovsdb_lib.py @@ -10,8 +10,10 @@ # License for the specific language governing permissions and limitations # under the License. +import fixtures import random + from unittest import mock import testscenarios @@ -19,7 +21,6 @@ import testscenarios from oslo_concurrency import processutils from oslo_config import cfg from oslo_utils import uuidutils -from ovsdbapp.schema.open_vswitch import impl_idl from vif_plug_ovs import constants from vif_plug_ovs import linux_net @@ -59,17 +60,17 @@ class TestOVSDBLib(testscenarios.WithScenarios, self.interface) # Make sure exceptions pass through by calling do_post_commit directly - mock.patch.object( - impl_idl.OvsVsctlTransaction, 'post_commit', - side_effect=impl_idl.OvsVsctlTransaction.do_post_commit).start() + post_commit = ( + 'ovsdbapp.schema.open_vswitch.impl_idl.' + 'OvsVsctlTransaction.post_commit' + ) + # "this" is the self parmater which is a reference to the + # OvsVsctlTransaction instance on which do_post_commit is defiend. - def _check_parameter(self, table, port, parameter, expected_value): - def check_value(): - return (self._ovsdb.db_get( - table, port, parameter).execute() == expected_value) + def direct_post_commit(this, transaction): + this.do_post_commit(transaction) - self.assertTrue(base.wait_until_true(check_value, timeout=2, - sleep=0.5)) + self.useFixture(fixtures.MonkeyPatch(post_commit, direct_post_commit)) def _add_port(self, bridge, port, may_exist=True): with self._ovsdb.transaction() as txn: @@ -122,11 +123,11 @@ class TestOVSDBLib(testscenarios.WithScenarios, expected_external_ids) self._check_parameter('Interface', port_name, 'type', interface_type) expected_vhost_server_path = {'vhost-server-path': vhost_server_path} - self._check_parameter('Interface', port_name, 'options', - expected_vhost_server_path) - self._check_parameter('Interface', port_name, 'options', - expected_vhost_server_path) + self._check_parameter( + 'Interface', port_name, 'options', expected_vhost_server_path + ) self._check_parameter('Port', port_name, 'tag', 2000) + self._check_parameter('Port', port_name, 'qos', []) @mock.patch.object(linux_net, 'delete_net_dev') def test_delete_ovs_vif_port(self, *mock): @@ -180,3 +181,156 @@ class TestOVSDBLib(testscenarios.WithScenarios, port_opts = {'peer': int_bridge_port} self._check_parameter( 'Interface', port_bridge_port, 'options', port_opts) + + def test_create_ovs_vif_port_with_default_qos(self): + port_name = 'qos-port-' + self.interface + iface_id = 'iface_id' + mac = 'ca:fe:ca:fe:ca:fe' + instance_id = uuidutils.generate_uuid() + mtu = 1500 + interface_type = 'internal' + qos_type = CONF.os_vif_ovs.default_qos_type + + self.addCleanup(self._del_bridge, self.brname) + self._add_bridge(self.brname) + + self.addCleanup( + self.ovs.delete_ovs_vif_port, self.brname, port_name, + delete_netdev=False, qos_type=qos_type + ) + self.ovs.create_ovs_vif_port( + self.brname, port_name, iface_id, mac, + instance_id, mtu=mtu, interface_type=interface_type, + tag=2000, qos_type=qos_type + ) + + # first we assert that the standard parameters are set correctly + expected_external_ids = {'iface-status': 'active', + 'iface-id': iface_id, + 'attached-mac': mac, + 'vm-uuid': instance_id} + self._check_parameter('Interface', port_name, 'external_ids', + expected_external_ids) + self._check_parameter('Interface', port_name, 'type', interface_type) + self._check_parameter('Port', port_name, 'tag', 2000) + + # now we check that the port has a qos policy attached + qos_uuid = self.ovs.get_qos( + port_name, qos_type + )[0]['_uuid'] + self._check_parameter('Port', port_name, 'qos', qos_uuid) + + # finally we check that the qos policy has the correct parameters + self._check_parameter( + 'QoS', str(qos_uuid), 'type', qos_type + ) + + def test_delete_qos_if_exists(self): + port_name = 'del-qos-port-' + self.interface + iface_id = 'iface_id' + mac = 'ca:fe:ca:fe:ca:fe' + instance_id = uuidutils.generate_uuid() + interface_type = 'internal' + qos_type = CONF.os_vif_ovs.default_qos_type + + # setup test by creating a bridge and port, and register + # cleanup funcitons to avoid leaking them. + self.addCleanup(self._del_bridge, self.brname) + self._add_bridge(self.brname) + self.addCleanup( + self.ovs.delete_ovs_vif_port, self.brname, port_name, + delete_netdev=False, qos_type=qos_type + ) + self.ovs.create_ovs_vif_port( + self.brname, port_name, iface_id, mac, + instance_id, interface_type=interface_type, + qos_type=qos_type + ) + + # now we check that the port has a qos policy attached + qos_uuid = self.ovs.get_qos( + port_name, CONF.os_vif_ovs.default_qos_type + )[0]['_uuid'] + self._check_parameter('Port', port_name, 'qos', qos_uuid) + + # finally we check that the qos policy has the correct parameters + self._check_parameter( + 'QoS', str(qos_uuid), 'type', qos_type + ) + + # we need to delete the port directly in the db to remove + # any references to the qos policy + self.ovs.ovsdb.del_port( + port_name, bridge=self.brname, if_exists=True).execute() + # then we can delete the qos policy + self.ovs.delete_qos_if_exists(port_name, qos_type) + self._check_parameter( + 'QoS', str(qos_uuid), 'type', None + ) + # invoking the delete when the policy does not exist + # should not result in an error + self.ovs.delete_qos_if_exists(port_name, qos_type) + self._check_parameter( + 'QoS', str(qos_uuid), 'type', None + ) + + def test_get_qos(self): + port_name = 'get-qos-' + self.interface + iface_id = 'iface_id' + mac = 'ca:fe:ca:fe:ca:fe' + instance_id = uuidutils.generate_uuid() + interface_type = 'internal' + qos_type = CONF.os_vif_ovs.default_qos_type + # initally no qos policy should exist + self.assertEqual(0, len(self.ovs.get_qos(port_name, qos_type))) + + # if we create a port with a qos policy get_qos should + # return the policy + self.addCleanup(self._del_bridge, self.brname) + self._add_bridge(self.brname) + self.addCleanup( + self.ovs.delete_ovs_vif_port, self.brname, port_name, + delete_netdev=False, qos_type=qos_type + ) + self.ovs.create_ovs_vif_port( + self.brname, port_name, iface_id, mac, + instance_id, interface_type=interface_type, + qos_type=qos_type + ) + # result should be a list of lenght 1 containing the + # qos policy created for the port we defied. + result = self.ovs.get_qos(port_name, qos_type) + self.assertEqual(1, len(result)) + self.assertIn('_uuid', result[0]) + self._check_parameter( + 'Port', port_name, 'qos', result[0]['_uuid'] + ) + # if we delete the port and its qos policy get_qos should + # not return it. + self.ovs.delete_ovs_vif_port( + self.brname, port_name, + delete_netdev=False, qos_type=qos_type + ) + self.assertEqual(0, len(self.ovs.get_qos(port_name, qos_type))) + + def test_port_exists(self): + port_name = 'port-exists-' + self.interface + iface_id = 'iface_id' + mac = 'ca:fe:ca:fe:ca:fe' + instance_id = uuidutils.generate_uuid() + interface_type = 'internal' + + self.assertFalse(self.ovs.port_exists(port_name, self.brname)) + + self.addCleanup(self._del_bridge, self.brname) + self._add_bridge(self.brname) + self.addCleanup( + self.ovs.delete_ovs_vif_port, self.brname, port_name, + delete_netdev=False, + ) + self.ovs.create_ovs_vif_port( + self.brname, port_name, iface_id, mac, + instance_id, interface_type=interface_type, + ) + + self.assertTrue(self.ovs.port_exists(port_name, self.brname)) diff --git a/vif_plug_ovs/tests/functional/test_plugin.py b/vif_plug_ovs/tests/functional/test_plugin.py index 6c176556..e40ae49a 100644 --- a/vif_plug_ovs/tests/functional/test_plugin.py +++ b/vif_plug_ovs/tests/functional/test_plugin.py @@ -11,6 +11,7 @@ # under the License. import testscenarios +import time from oslo_concurrency import processutils from oslo_config import cfg @@ -33,6 +34,41 @@ def run_privileged(*full_args): return processutils.execute(*full_args)[0].rstrip() +# derived from test_impl_pyroute2 + +def exist_device(device): + try: + run_privileged('ip', 'link', 'show', device) + return True + except processutils.ProcessExecutionError as e: + if e.exit_code == 1: + return False + raise + + +def add_device(device, dev_type, peer=None, link=None, + vlan_id=None): + if 'vlan' == dev_type: + run_privileged('ip', 'link', 'add', 'link', link, + 'name', device, 'type', dev_type, 'vlan', 'id', + vlan_id) + elif 'veth' == dev_type: + run_privileged('ip', 'link', 'add', device, 'type', dev_type, + 'peer', 'name', peer) + elif 'dummy' == dev_type: + run_privileged('ip', 'link', 'add', device, 'type', dev_type) + # ensure that the device exists to prevent racing with other ip commands + for _ in range(10): + if exist_device(device): + return + time.sleep(0.1) + + +def del_device(device): + if exist_device(device): + run_privileged('ip', 'link', 'del', device) + + class TestOVSPlugin(testscenarios.WithScenarios, base.VifPlugOvsBaseFunctionalTestCase): @@ -82,6 +118,24 @@ class TestOVSPlugin(testscenarios.WithScenarios, mode='client', port_profile=self.profile_ovs) + self.profile_ovs_system = objects.vif.VIFPortProfileOpenVSwitch( + interface_id='e65867e0-9340-4a7f-a256-09af6eb7a3aa', + datapath_type='system', + create_port=True) + + self.network_ovs = objects.network.Network( + id='437c6db5-4e6f-4b43-b64b-ed6a11ee5ba7', + bridge='br-qos-' + self.interface, + subnets=self.subnets, + vlan=99) + + self.vif_ovs_port = objects.vif.VIFOpenVSwitch( + id='b679325f-ca89-4ee0-a8be-6db1409b69ea', + address='ca:fe:de:ad:be:ef', + network=self.network_ovs, + port_profile=self.profile_ovs_system, + vif_name="qos-port-" + self.interface) + self.instance = objects.instance_info.InstanceInfo( name='demo', uuid='f0000000-0000-0000-0000-000000000001') @@ -98,3 +152,34 @@ class TestOVSPlugin(testscenarios.WithScenarios, self.plugin.unplug(self.vif_vhostuser_trunk, self.instance) self.assertTrue(self._check_bridge(other_bridge)) self.assertFalse(self._check_bridge(trunk_bridge)) + + def test_plug_unplug_ovs_port_with_qos(self): + bridge = 'br-qos-' + self.interface + vif_name = "qos-port-" + self.interface + qos_type = CONF.os_vif_ovs.default_qos_type + self.addCleanup(self._del_bridge, bridge) + self.addCleanup( + self.ovs.delete_ovs_vif_port, bridge, vif_name, + delete_netdev=False, qos_type=qos_type + ) + self.addCleanup(del_device, vif_name) + add_device(vif_name, 'dummy') + # pluging a vif will create the port and bridge + # if either does not exist + self.plugin.plug(self.vif_ovs_port, self.instance) + self.assertTrue(self._check_bridge(bridge)) + self.assertTrue(self._check_port(vif_name, bridge)) + qos_uuid = self.ovs.get_qos( + vif_name, qos_type + )[0]['_uuid'] + self._check_parameter('Port', vif_name, 'qos', qos_uuid) + self._check_parameter( + 'QoS', str(qos_uuid), 'type', qos_type + ) + # unpluging a port will not delete the bridge. + self.plugin.unplug(self.vif_ovs_port, self.instance) + self.assertTrue(self._check_bridge(bridge)) + self.assertFalse(self._check_port(vif_name, bridge)) + self._check_parameter( + 'QoS', str(qos_uuid), 'type', None + ) diff --git a/vif_plug_ovs/tests/unit/test_plugin.py b/vif_plug_ovs/tests/unit/test_plugin.py index d710c6cd..968c67d2 100644 --- a/vif_plug_ovs/tests/unit/test_plugin.py +++ b/vif_plug_ovs/tests/unit/test_plugin.py @@ -362,7 +362,9 @@ class PluginTest(testtools.TestCase): delete_ovs_vif_port, delete_ovs_bridge): calls = { 'delete_bridge': [mock.call('qbrvif-xxx-yyy', 'qvbb679325f-ca')], - 'delete_ovs_vif_port': [mock.call('br0', 'qvob679325f-ca')] + 'delete_ovs_vif_port': [mock.call( + 'br0', 'qvob679325f-ca', qos_type='linux-noop' + )] } mock_sys.platform = 'linux' plugin = ovs.OvsPlugin.load(constants.PLUGIN_NAME) @@ -510,8 +512,12 @@ class PluginTest(testtools.TestCase): 'get_vf_num_by_pci_address': [mock.call('0002:24:12.3')], 'get_representor_port': [mock.call('eth0', '2')], 'set_interface_state': [mock.call('eth0_2', 'down')], - 'delete_ovs_vif_port': [mock.call('br0', 'eth0_2', - delete_netdev=False)] + 'delete_ovs_vif_port': [ + mock.call( + 'br0', 'eth0_2', delete_netdev=False, + qos_type='linux-noop' + ) + ] } get_ifname_by_pci_address.return_value = 'eth0' @@ -602,8 +608,13 @@ class PluginTest(testtools.TestCase): calls = { 'get_dpdk_representor_port_name': [mock.call( self.vif_ovs_vf_dpdk.id)], - 'delete_ovs_vif_port': [mock.call('br0', devname, - delete_netdev=False)]} + 'delete_ovs_vif_port': [ + mock.call( + 'br0', devname, delete_netdev=False, + qos_type=None + ) + ] + } plugin = ovs.OvsPlugin.load(constants.PLUGIN_NAME) plugin.unplug(self.vif_ovs_vf_dpdk, self.instance) get_dpdk_representor_port_name.assert_has_calls( @@ -636,7 +647,7 @@ class PluginTest(testtools.TestCase): mock.call('br0', 'ibpb679325f-ca89-4ee0-a8be-6db1409b69ea'), mock.call( 'pbb679325f-ca8', 'pbpb679325f-ca89-4ee0-a8be-6db1409b69ea'), - mock.call('pbb679325f-ca8', 'tap-xxx-yyy-zzz') + mock.call('pbb679325f-ca8', 'tap-xxx-yyy-zzz', qos_type=None) ] delete_ovs_vif_port.assert_has_calls(calls) delete_ovs_bridge.assert_called_once_with('pbb679325f-ca8')