Allow replacing the QoS policy of bound port

Change-Id: Iebdfd2b303f47ff9f049cf583ea754b35ca26f78
Related-Bug: #1882804
Depends-On: https://review.opendev.org/748279

doc/source/admin/config-qos-min-bw.rst

@@ -61,6 +61,26 @@ Limitations
   effect. That is ports of the QoS policy are not yet used by Nova. Requests
   to change guarantees of in-use policies are rejected.
 
+* Changing the QoS policy of the port with new ``minimum_bandwidth`` rules
+  changes placement ``allocations`` from Victoria release.
+  If the VM was booted with port without QoS policy and ``minimum_bandwidth``
+  rules the port update succeeds but placement allocations will not change.
+  The same is true if the port has no ``binding:profile``, thus no placement
+  allocation record exists for it. But if the VM was booted with a port with
+  QoS policy and ``minimum_bandwidth`` rules the update is possible and the
+  allocations are changed in placement as well.
+
+.. note::
+
+  As it is possible to update a port to remove the QoS policy, updating it
+  back to have QoS policy with ``minimum_bandwidth`` rule will not result in
+  ``placement allocation`` record, only the dataplane enforcement will happen.
+
+.. note::
+
+  updating the ``minimum_bandwidth`` rule of a QoS policy that is attached
+  to a port which is bound to a VM is still not possible.
+
 * The first data-plane-only Guaranteed Minimum Bandwidth implementation
   (for SR-IOV egress traffic) was released in the Newton
   release of Neutron.  Because of the known lack of

neutron/services/qos/qos_plugin.py

@@ -13,6 +13,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+from keystoneauth1 import exceptions as ks_exc
 from neutron_lib.api.definitions import port as port_def
 from neutron_lib.api.definitions import port_resource_request
 from neutron_lib.api.definitions import portbindings
@@ -32,9 +33,12 @@ from neutron_lib.db import api as db_api
 from neutron_lib.db import resource_extend
 from neutron_lib import exceptions as lib_exc
 from neutron_lib.exceptions import qos as qos_exc
+from neutron_lib.placement import client as pl_client
 from neutron_lib.placement import constants as pl_constants
 from neutron_lib.placement import utils as pl_utils
 from neutron_lib.services.qos import constants as qos_consts
+from oslo_config import cfg
+from oslo_log import log as logging
 
 from neutron._i18n import _
 from neutron.db import db_base_plugin_common
@@ -44,10 +48,14 @@ from neutron.objects import network as network_object
 from neutron.objects import ports as ports_object
 from neutron.objects.qos import policy as policy_object
 from neutron.objects.qos import qos_policy_validator as checker
+from neutron.objects.qos import rule as rule_object
 from neutron.objects.qos import rule_type as rule_type_object
 from neutron.services.qos.drivers import manager
 
 
+LOG = logging.getLogger(__name__)
+
+
 @resource_extend.has_resource_extenders
 class QoSPlugin(qos.QoSPluginBase):
     """Implementation of the Neutron QoS Service Plugin.
@@ -72,11 +80,19 @@ class QoSPlugin(qos.QoSPluginBase):
     def __init__(self):
         super(QoSPlugin, self).__init__()
         self.driver_manager = manager.QosServiceDriverManager()
+        self._placement_client = pl_client.PlacementAPIClient(cfg.CONF)
 
         callbacks_registry.subscribe(
             self._validate_create_port_callback,
             callbacks_resources.PORT,
             callbacks_events.PRECOMMIT_CREATE)
+        # TODO(lajoskatona): PORT BEFORE_UPDATE is a notify, so
+        # "old style" kwargs instead of payload object, let's change it
+        # to notify and payload.
+        callbacks_registry.subscribe(
+            self._check_port_for_placement_allocation_change,
+            callbacks_resources.PORT,
+            callbacks_events.BEFORE_UPDATE)
         callbacks_registry.subscribe(
             self._validate_update_port_callback,
             callbacks_resources.PORT,
@@ -172,6 +188,81 @@ class QoSPlugin(qos.QoSPluginBase):
             context.elevated(), id=policy_id)
         self.validate_policy_for_port(context, policy, port)
 
+    def _check_port_for_placement_allocation_change(self, resource, event,
+                                                    trigger, **kwargs):
+        context = kwargs['context']
+        orig_port = kwargs['original_port']
+        original_policy_id = orig_port.get(qos_consts.QOS_POLICY_ID)
+        policy_id = kwargs['port'].get(qos_consts.QOS_POLICY_ID)
+
+        if policy_id == original_policy_id:
+            return
+
+        # Do this only for compute bound ports
+        if (nl_constants.DEVICE_OWNER_COMPUTE_PREFIX in
+                orig_port['device_owner']):
+            original_policy = policy_object.QosPolicy.get_object(
+                context.elevated(), id=original_policy_id)
+            policy = policy_object.QosPolicy.get_object(
+                context.elevated(), id=policy_id)
+            self._change_placement_allocation(original_policy, policy,
+                                              orig_port)
+
+    def _prepare_allocation_needs(self, original_rule, desired_rule):
+        if (isinstance(desired_rule, rule_object.QosMinimumBandwidthRule) or
+                isinstance(desired_rule, dict)):
+            o_dir = original_rule.get('direction')
+            o_minkbps = original_rule.get('min_kbps')
+            d_minkbps = desired_rule.get('min_kbps')
+            d_dir = desired_rule.get('direction')
+            if o_dir == d_dir and o_minkbps != d_minkbps:
+                diff = d_minkbps - o_minkbps
+                # TODO(lajoskatona): move this to neutron-lib, see similar
+                # dict @l125.
+                if d_dir == 'egress':
+                    drctn = pl_constants.CLASS_NET_BW_EGRESS_KBPS
+                else:
+                    drctn = pl_constants.CLASS_NET_BW_INGRESS_KBPS
+                return {drctn: diff}
+        return {}
+
+    def _change_placement_allocation(self, original_policy, desired_policy,
+                                     orig_port):
+        alloc_diff = {}
+        original_rules = []
+        rp_uuid = orig_port['binding:profile'].get('allocation')
+        device_id = orig_port['device_id']
+        if original_policy:
+            original_rules = original_policy.get('rules')
+        if desired_policy:
+            desired_rules = desired_policy.get('rules')
+        else:
+            desired_rules = [{'direction': 'egress', 'min_kbps': 0},
+                             {'direction': 'ingress', 'min_kbps': 0}]
+
+        any_rules_minbw = any(
+            [isinstance(r, rule_object.QosMinimumBandwidthRule)
+             for r in desired_rules])
+        if (not original_rules and any_rules_minbw) or not rp_uuid:
+            LOG.warning("There was no QoS policy with minimum_bandwidth rule "
+                        "attached to the port %s, there is no allocation "
+                        "record in placement for it, only the dataplane "
+                        "enforcement will happen!", orig_port['id'])
+            return
+        for o_rule in original_rules:
+            if isinstance(o_rule, rule_object.QosMinimumBandwidthRule):
+                for d_rule in desired_rules:
+                    alloc_diff.update(
+                        self._prepare_allocation_needs(o_rule, d_rule))
+        if alloc_diff:
+            try:
+                self._placement_client.update_qos_minbw_allocation(
+                    consumer_uuid=device_id, minbw_alloc_diff=alloc_diff,
+                    rp_uuid=rp_uuid)
+            except ks_exc.Conflict:
+                raise qos_exc.QosPlacementAllocationConflict(
+                    consumer=device_id, rp=rp_uuid)
+
     def _validate_update_port_callback(self, resource, event, trigger,
                                        payload=None):
         context = payload.context

neutron/tests/unit/services/qos/test_qos_plugin.py

@@ -13,12 +13,14 @@
 import copy
 from unittest import mock
 
+from keystoneauth1 import exceptions as ks_exc
 import netaddr
 from neutron_lib.api.definitions import qos
 from neutron_lib.callbacks import events
 from neutron_lib import constants as lib_constants
 from neutron_lib import context
 from neutron_lib import exceptions as lib_exc
+from neutron_lib.exceptions import placement as pl_exc
 from neutron_lib.exceptions import qos as qos_exc
 from neutron_lib.objects import utils as obj_utils
 from neutron_lib.placement import constants as pl_constants
@@ -1221,6 +1223,14 @@ class TestQosPluginDB(base.BaseQosTestCase):
         qos_policy.create()
         return qos_policy
 
+    def _make_qos_minbw_rule(self, policy_id, direction='ingress',
+                             min_kbps=1000):
+        qos_rule = rule_object.QosMinimumBandwidthRule(
+            self.context, project_id=self.project_id,
+            qos_policy_id=policy_id, direction=direction, min_kbps=min_kbps)
+        qos_rule.create()
+        return qos_rule
+
     def _make_port(self, network_id, qos_policy_id=None):
         base_mac = ['aa', 'bb', 'cc', 'dd', 'ee', 'ff']
         mac = netaddr.EUI(next(net_utils.random_mac_generator(base_mac)))
@@ -1277,3 +1287,204 @@ class TestQosPluginDB(base.BaseQosTestCase):
 
     def test_validate_create_port_callback_no_policy(self):
         self._test_validate_create_port_callback()
+
+    def _prepare_for_port_placement_allocation_change(self, qos1, qos2):
+        qos1_id = qos1.id if qos1 else None
+        qos2_id = qos2.id if qos2 else None
+
+        network = self._make_network()
+        port = self._make_port(network.id, qos_policy_id=qos1_id)
+
+        return {"context": self.context,
+                "original_port": {
+                      "id": port.id,
+                      "device_owner": "compute:uu:id",
+                      "qos_policy_id": qos1_id},
+                "port": {"id": port.id, "qos_policy_id": qos2_id}}
+
+    def test_check_port_for_placement_allocation_change_no_qos_change(self):
+        qos1_obj = self._make_qos_policy()
+        kwargs = self._prepare_for_port_placement_allocation_change(
+            qos1=qos1_obj, qos2=qos1_obj)
+        with mock.patch.object(
+                self.qos_plugin,
+                '_change_placement_allocation') as mock_alloc_change:
+            self.qos_plugin._check_port_for_placement_allocation_change(
+                'PORT', 'before_update', 'test_plugin', **kwargs)
+        mock_alloc_change.assert_not_called()
+
+    def test_check_port_for_placement_allocation_change(self):
+        qos1_obj = self._make_qos_policy()
+        qos2_obj = self._make_qos_policy()
+        kwargs = self._prepare_for_port_placement_allocation_change(
+            qos1=qos1_obj, qos2=qos2_obj)
+
+        with mock.patch.object(
+                self.qos_plugin,
+                '_change_placement_allocation') as mock_alloc_change:
+            self.qos_plugin._check_port_for_placement_allocation_change(
+                'PORT', 'before_update', 'test_plugin', **kwargs)
+        mock_alloc_change.assert_called_once_with(
+            qos1_obj, qos2_obj, kwargs['original_port'])
+
+    def test_check_port_for_placement_allocation_change_no_new_policy(self):
+        qos1_obj = self._make_qos_policy()
+        kwargs = self._prepare_for_port_placement_allocation_change(
+            qos1=qos1_obj, qos2=None)
+
+        with mock.patch.object(
+                self.qos_plugin,
+                '_change_placement_allocation') as mock_alloc_change:
+            self.qos_plugin._check_port_for_placement_allocation_change(
+                'PORT', 'before_update', 'test_plugin', **kwargs)
+        mock_alloc_change.assert_called_once_with(
+            qos1_obj, None, kwargs['original_port'])
+
+    def _prepare_port_for_placement_allocation(self, qos1, qos2=None,
+                                               min_kbps1=1000, min_kbps2=2000):
+        rule1_obj = self._make_qos_minbw_rule(qos1.id, min_kbps=min_kbps1)
+        qos1.rules = [rule1_obj]
+        if qos2:
+            rule2_obj = self._make_qos_minbw_rule(qos2.id, min_kbps=min_kbps2)
+            qos2.rules = [rule2_obj]
+        orig_port = {'binding:profile': {'allocation': 'rp:uu:id'},
+                     'device_id': 'uu:id'}
+        return orig_port
+
+    def test_change_placement_allocation_increase(self):
+        qos1 = self._make_qos_policy()
+        qos2 = self._make_qos_policy()
+        port = self._prepare_port_for_placement_allocation(qos1, qos2)
+        with mock.patch.object(self.qos_plugin._placement_client,
+                'update_qos_minbw_allocation') as mock_update_qos_alloc:
+            self.qos_plugin._change_placement_allocation(qos1, qos2, port)
+        mock_update_qos_alloc.assert_called_once_with(
+            consumer_uuid='uu:id',
+            minbw_alloc_diff={'NET_BW_IGR_KILOBIT_PER_SEC': 1000},
+            rp_uuid='rp:uu:id')
+
+    def test_test_change_placement_allocation_decrease(self):
+        qos1 = self._make_qos_policy()
+        qos2 = self._make_qos_policy()
+        port = self._prepare_port_for_placement_allocation(qos2, qos1)
+        with mock.patch.object(self.qos_plugin._placement_client,
+                'update_qos_minbw_allocation') as mock_update_qos_alloc:
+            self.qos_plugin._change_placement_allocation(qos1, qos2, port)
+        mock_update_qos_alloc.assert_called_once_with(
+            consumer_uuid='uu:id',
+            minbw_alloc_diff={'NET_BW_IGR_KILOBIT_PER_SEC': -1000},
+            rp_uuid='rp:uu:id')
+
+    def test_change_placement_allocation_no_original_qos(self):
+        qos1 = None
+        qos2 = self._make_qos_policy()
+        rule2_obj = self._make_qos_minbw_rule(qos2.id, min_kbps=1000)
+        qos2.rules = [rule2_obj]
+        orig_port = {'id': 'u:u', 'device_id': 'i:d', 'binding:profile': {}}
+        with mock.patch.object(self.qos_plugin._placement_client,
+                'update_qos_minbw_allocation') as mock_update_qos_alloc:
+            self.qos_plugin._change_placement_allocation(
+                qos1, qos2, orig_port)
+        mock_update_qos_alloc.assert_not_called()
+
+    def test_change_placement_allocation_no_original_allocation(self):
+        qos1 = self._make_qos_policy()
+        rule1_obj = self._make_qos_minbw_rule(qos1.id, min_kbps=500)
+        qos1.rules = [rule1_obj]
+        qos2 = self._make_qos_policy()
+        rule2_obj = self._make_qos_minbw_rule(qos2.id, min_kbps=1000)
+        qos2.rules = [rule2_obj]
+        orig_port = {'id': 'u:u', 'device_id': 'i:d', 'binding:profile': {}}
+        with mock.patch.object(self.qos_plugin._placement_client,
+                'update_qos_minbw_allocation') as mock_update_qos_alloc:
+            self.qos_plugin._change_placement_allocation(
+                qos1, qos2, orig_port)
+        mock_update_qos_alloc.assert_not_called()
+
+    def test_change_placement_allocation_new_policy_empty(self):
+        qos1 = self._make_qos_policy()
+        port = self._prepare_port_for_placement_allocation(qos1)
+        with mock.patch.object(self.qos_plugin._placement_client,
+                'update_qos_minbw_allocation') as mock_update_qos_alloc:
+            self.qos_plugin._change_placement_allocation(qos1, None, port)
+        mock_update_qos_alloc.assert_called_once_with(
+            consumer_uuid='uu:id',
+            minbw_alloc_diff={'NET_BW_IGR_KILOBIT_PER_SEC': -1000},
+            rp_uuid='rp:uu:id')
+
+    def test_change_placement_allocation_no_min_bw(self):
+        qos1 = self._make_qos_policy()
+        qos2 = self._make_qos_policy()
+        bw_limit_rule1 = rule_object.QosDscpMarkingRule(dscp_mark=16)
+        bw_limit_rule2 = rule_object.QosDscpMarkingRule(dscp_mark=18)
+        qos1.rules = [bw_limit_rule1]
+        qos2.rules = [bw_limit_rule2]
+        port = {'binding:profile': {'allocation': 'rp:uu:id'},
+                'device_id': 'uu:id'}
+
+        with mock.patch.object(self.qos_plugin._placement_client,
+                'update_qos_minbw_allocation') as mock_update_qos_alloc:
+            self.qos_plugin._change_placement_allocation(qos1, None, port)
+        mock_update_qos_alloc.assert_not_called()
+
+    def test_change_placement_allocation_old_rule_not_min_bw(self):
+        qos1 = self._make_qos_policy()
+        qos2 = self._make_qos_policy()
+        bw_limit_rule = rule_object.QosDscpMarkingRule(dscp_mark=16)
+        port = self._prepare_port_for_placement_allocation(qos1, qos2)
+        qos1.rules = [bw_limit_rule]
+
+        with mock.patch.object(self.qos_plugin._placement_client,
+                'update_qos_minbw_allocation') as mock_update_qos_alloc:
+            self.qos_plugin._change_placement_allocation(qos1, qos2, port)
+        mock_update_qos_alloc.assert_not_called()
+
+    def test_change_placement_allocation_new_rule_not_min_bw(self):
+        qos1 = self._make_qos_policy()
+        qos2 = self._make_qos_policy()
+        bw_limit_rule = rule_object.QosDscpMarkingRule(dscp_mark=16)
+        qos2.rules = [bw_limit_rule]
+        port = self._prepare_port_for_placement_allocation(qos1)
+
+        with mock.patch.object(self.qos_plugin._placement_client,
+                'update_qos_minbw_allocation') as mock_update_qos_alloc:
+            self.qos_plugin._change_placement_allocation(qos1, qos2, port)
+        mock_update_qos_alloc.assert_not_called()
+
+    def test_change_placement_allocation_equal_minkbps(self):
+        qos1 = self._make_qos_policy()
+        qos2 = self._make_qos_policy()
+        port = self._prepare_port_for_placement_allocation(qos1, qos2, 1000,
+                                                           1000)
+        with mock.patch.object(self.qos_plugin._placement_client,
+                'update_qos_minbw_allocation') as mock_update_qos_alloc:
+            self.qos_plugin._change_placement_allocation(qos1, qos2, port)
+        mock_update_qos_alloc.assert_not_called()
+
+    def test_change_placement_allocation_update_conflict(self):
+        qos1 = self._make_qos_policy()
+        qos2 = self._make_qos_policy()
+        port = self._prepare_port_for_placement_allocation(qos1, qos2)
+        with mock.patch.object(self.qos_plugin._placement_client,
+                'update_qos_minbw_allocation') as mock_update_qos_alloc:
+            mock_update_qos_alloc.side_effect = ks_exc.Conflict(
+                response={'errors': [{'code': 'placement.concurrent_update'}]}
+            )
+            self.assertRaises(
+                qos_exc.QosPlacementAllocationConflict,
+                self.qos_plugin._change_placement_allocation,
+                qos1, qos2, port)
+
+    def test_change_placement_allocation_update_generation_conflict(self):
+        qos1 = self._make_qos_policy()
+        qos2 = self._make_qos_policy()
+        port = self._prepare_port_for_placement_allocation(qos1, qos2)
+        with mock.patch.object(self.qos_plugin._placement_client,
+                'update_qos_minbw_allocation') as mock_update_qos_alloc:
+            mock_update_qos_alloc.side_effect = (
+                pl_exc.PlacementAllocationGenerationConflict(
+                    consumer='rp:uu:id'))
+            self.assertRaises(
+                pl_exc.PlacementAllocationGenerationConflict,
+                self.qos_plugin._change_placement_allocation,
+                qos1, qos2, port)

releasenotes/notes/update_qos_allocation_for_bound_port-5358620322b66ae9.yaml

@@ -0,0 +1,8 @@
+---
+features:
+  - |
+    Update of an ``already bound port`` with ``QoS minimum_bandwidth`` rule
+    with new ``QoS policy`` with ``minimum_bandwidth`` rule changes the
+    allocations in placement as well. ``NOTE``: updating the
+    ``minimum_bandwidth`` rule of a QoS policy that is attached to a port
+    which is bound to a VM is still not possible.