Merge "set default qos policy"

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

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.

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)

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)

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)

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

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(
+        post_commit = (
-            impl_idl.OvsVsctlTransaction, 'post_commit',
+            'ovsdbapp.schema.open_vswitch.impl_idl.'
-            side_effect=impl_idl.OvsVsctlTransaction.do_post_commit).start()
+            '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 direct_post_commit(this, transaction):
-        def check_value():
+            this.do_post_commit(transaction)
-            return (self._ovsdb.db_get(
-                table, port, parameter).execute() == expected_value)
 
-        self.assertTrue(base.wait_until_true(check_value, timeout=2,
+        self.useFixture(fixtures.MonkeyPatch(post_commit, direct_post_commit))
-                                             sleep=0.5))
 
     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',
+        self._check_parameter(
-                              expected_vhost_server_path)
+            '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))

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

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_ovs_vif_port': [
-                                              delete_netdev=False)]
+                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_ovs_vif_port': [
-                                              delete_netdev=False)]}
+                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')