From 0f78c111e717e62622afd192d37e271ee6acd6d2 Mon Sep 17 00:00:00 2001 From: Nurmatov Mamatisa Date: Mon, 15 Feb 2021 15:42:10 +0300 Subject: [PATCH] Add minimum bw qos rule validation for network Minimum bandwidth QoS rule is only applicable for the network which is backed by physical networks. It will raise exception when you want to set Minimum bandwidth QoS rule or update rule on network without ports. Closes-Bug: #1913180 Conflicts: neutron/services/qos/drivers/manager.py neutron/services/qos/qos_plugin.py Change-Id: I6ab945086b13730ad60957760bbc2eb5c321aca2 (cherry picked from commit 26f3de0f781b8cc48c4294d0cb36596c26777f70) --- neutron/services/qos/drivers/manager.py | 12 +++++ .../qos/drivers/openvswitch/driver.py | 5 +- neutron/services/qos/qos_plugin.py | 33 +++++++++++++ neutron/tests/fullstack/resources/client.py | 4 +- neutron/tests/fullstack/test_qos.py | 25 ++++++++++ .../qos/drivers/openvswitch/test_driver.py | 2 + .../unit/services/qos/drivers/test_manager.py | 23 ++++++++++ .../unit/services/qos/test_qos_plugin.py | 46 +++++++++++++++++++ 8 files changed, 148 insertions(+), 2 deletions(-) diff --git a/neutron/services/qos/drivers/manager.py b/neutron/services/qos/drivers/manager.py index e93fd557b59..8a0001fe57d 100644 --- a/neutron/services/qos/drivers/manager.py +++ b/neutron/services/qos/drivers/manager.py @@ -154,6 +154,18 @@ class QosServiceDriverManager(object): return False + def validate_rule_for_network(self, context, rule, network_id): + for driver in self._drivers: + if driver.is_rule_supported(rule): + # https://review.opendev.org/c/openstack/neutron-lib/+/774083 + # is not present, in this release, in neutron-lib. + if hasattr(driver, 'validate_rule_for_network'): + return driver.validate_rule_for_network(context, rule, + network_id) + return True + + return False + @property def supported_rule_types(self): if not self._drivers: diff --git a/neutron/services/qos/drivers/openvswitch/driver.py b/neutron/services/qos/drivers/openvswitch/driver.py index b1e599749f5..b12a0d75ed7 100644 --- a/neutron/services/qos/drivers/openvswitch/driver.py +++ b/neutron/services/qos/drivers/openvswitch/driver.py @@ -60,11 +60,14 @@ class OVSDriver(base.DriverBase): requires_rpc_notifications=True) def validate_rule_for_port(self, context, rule, port): + return self.validate_rule_for_network(context, rule, port.network_id) + + def validate_rule_for_network(self, context, rule, network_id): # Minimum-bandwidth rule is only supported on networks whose # first segment is backed by a physnet. if rule.rule_type == qos_consts.RULE_TYPE_MINIMUM_BANDWIDTH: net = network_object.Network.get_object( - context, id=port.network_id) + context, id=network_id) physnet = net.segments[0].physical_network if physnet is None: return False diff --git a/neutron/services/qos/qos_plugin.py b/neutron/services/qos/qos_plugin.py index c8e18dd1489..cfd3950a8fd 100644 --- a/neutron/services/qos/qos_plugin.py +++ b/neutron/services/qos/qos_plugin.py @@ -49,6 +49,11 @@ from neutron.objects.qos import rule_type as rule_type_object from neutron.services.qos.drivers import manager +class QosRuleNotSupportedByNetwork(lib_exc.Conflict): + message = _("Rule %(rule_type)s is not supported " + "by network %(network_id)s") + + @resource_extend.has_resource_extenders class QoSPlugin(qos.QoSPluginBase): """Implementation of the Neutron QoS Service Plugin. @@ -86,6 +91,10 @@ class QoSPlugin(qos.QoSPluginBase): self._validate_update_network_callback, callbacks_resources.NETWORK, callbacks_events.PRECOMMIT_UPDATE) + callbacks_registry.subscribe( + self._validate_create_network_callback, + callbacks_resources.NETWORK, + callbacks_events.PRECOMMIT_CREATE) @staticmethod @resource_extend.extends([port_def.COLLECTION_NAME]) @@ -252,6 +261,20 @@ class QoSPlugin(qos.QoSPluginBase): self.validate_policy_for_port(context, policy, updated_port) + def _validate_create_network_callback(self, resource, event, trigger, + **kwargs): + context = kwargs['context'] + network_id = kwargs['network']['id'] + network = network_object.Network.get_object(context, id=network_id) + + policy_id = network.qos_policy_id + if policy_id is None: + return + + policy = policy_object.QosPolicy.get_object( + context.elevated(), id=policy_id) + self.validate_policy_for_network(context, policy, network_id) + def _validate_update_network_callback(self, resource, event, trigger, payload=None): context = payload.context @@ -266,6 +289,9 @@ class QoSPlugin(qos.QoSPluginBase): policy = policy_object.QosPolicy.get_object( context.elevated(), id=policy_id) + self.validate_policy_for_network( + context, policy, network_id=updated_network['id']) + ports = ports_object.Port.get_objects( context, network_id=updated_network['id']) # Filter only this ports which don't have overwritten policy @@ -289,6 +315,13 @@ class QoSPlugin(qos.QoSPluginBase): raise qos_exc.QosRuleNotSupported(rule_type=rule.rule_type, port_id=port['id']) + def validate_policy_for_network(self, context, policy, network_id): + for rule in policy.rules: + if not self.driver_manager.validate_rule_for_network( + context, rule, network_id): + raise QosRuleNotSupportedByNetwork( + rule_type=rule.rule_type, network_id=network_id) + def reject_min_bw_rule_updates(self, context, policy): ports = self._get_ports_with_policy(context, policy) for port in ports: diff --git a/neutron/tests/fullstack/resources/client.py b/neutron/tests/fullstack/resources/client.py index 9d14e5d00a5..42ae4427b74 100644 --- a/neutron/tests/fullstack/resources/client.py +++ b/neutron/tests/fullstack/resources/client.py @@ -81,7 +81,7 @@ class ClientFixture(fixtures.Fixture): def create_network(self, tenant_id, name=None, external=False, network_type=None, segmentation_id=None, - physical_network=None, mtu=None): + physical_network=None, mtu=None, qos_policy_id=None): resource_type = 'network' name = name or utils.get_rand_name(prefix=resource_type) @@ -96,6 +96,8 @@ class ClientFixture(fixtures.Fixture): spec['provider:physical_network'] = physical_network if mtu is not None: spec['mtu'] = mtu + if qos_policy_id is not None: + spec['qos_policy_id'] = qos_policy_id return self._create_resource(resource_type, spec) diff --git a/neutron/tests/fullstack/test_qos.py b/neutron/tests/fullstack/test_qos.py index 7f0e25d4c91..3aa628acdc5 100644 --- a/neutron/tests/fullstack/test_qos.py +++ b/neutron/tests/fullstack/test_qos.py @@ -707,6 +707,31 @@ class TestMinBwQoSOvs(_TestMinBwQoS, base.BaseFullStackTestCase): queues = '\nList of OVS Queue registers:\n%s' % '\n'.join(queues) self.fail(queuenum + qoses + queues) + def test_min_bw_qos_create_network_vxlan_not_supported(self): + qos_policy = self._create_qos_policy() + qos_policy_id = qos_policy['id'] + self.safe_client.create_minimum_bandwidth_rule( + self.tenant_id, qos_policy_id, MIN_BANDWIDTH, self.direction) + network_args = {'network_type': 'vxlan', + 'qos_policy_id': qos_policy_id} + self.assertRaises( + exceptions.Conflict, + self.safe_client.create_network, + self.tenant_id, name='network-test', **network_args) + + def test_min_bw_qos_update_network_vxlan_not_supported(self): + network_args = {'network_type': 'vxlan'} + network = self.safe_client.create_network( + self.tenant_id, name='network-test', **network_args) + qos_policy = self._create_qos_policy() + qos_policy_id = qos_policy['id'] + self.safe_client.create_minimum_bandwidth_rule( + self.tenant_id, qos_policy_id, MIN_BANDWIDTH, self.direction) + self.assertRaises( + exceptions.Conflict, + self.client.update_network, network['id'], + body={'network': {'qos_policy_id': qos_policy_id}}) + def test_min_bw_qos_port_removed(self): """Test if min BW limit config is properly removed when port removed. diff --git a/neutron/tests/unit/services/qos/drivers/openvswitch/test_driver.py b/neutron/tests/unit/services/qos/drivers/openvswitch/test_driver.py index 5378dbbd926..6345cb82608 100644 --- a/neutron/tests/unit/services/qos/drivers/openvswitch/test_driver.py +++ b/neutron/tests/unit/services/qos/drivers/openvswitch/test_driver.py @@ -43,3 +43,5 @@ class TestOVSDriver(base.BaseQosTestCase): return_value=net): test_method(self.driver.validate_rule_for_port( mock.Mock(), rule, port)) + test_method(self.driver.validate_rule_for_network( + mock.Mock(), rule, network_id=mock.Mock())) diff --git a/neutron/tests/unit/services/qos/drivers/test_manager.py b/neutron/tests/unit/services/qos/drivers/test_manager.py index 0a55c7feb96..b4c2b8d7986 100644 --- a/neutron/tests/unit/services/qos/drivers/test_manager.py +++ b/neutron/tests/unit/services/qos/drivers/test_manager.py @@ -119,6 +119,29 @@ class TestQoSDriversRulesValidations(TestQosDriversManagerBase): else: is_rule_supported_mock.assert_not_called() + def test_validate_rule_for_network(self): + driver_manager = self._create_manager_with_drivers({ + 'driver-A': { + 'is_loaded': True, + 'rules': { + qos_consts.RULE_TYPE_MINIMUM_BANDWIDTH: { + "min_kbps": {'type:values': None}, + 'direction': { + 'type:values': lib_consts.VALID_DIRECTIONS} + } + } + } + }) + rule = rule_object.QosMinimumBandwidthRule( + self.ctxt, id=uuidutils.generate_uuid()) + + is_rule_supported_mock = mock.Mock() + is_rule_supported_mock.return_value = True + driver_manager._drivers[0].is_rule_supported = is_rule_supported_mock + self.assertTrue(driver_manager.validate_rule_for_network( + mock.Mock(), rule, mock.Mock())) + is_rule_supported_mock.assert_called_once_with(rule) + def test_validate_rule_for_port_rule_vif_type_supported(self): port = self._get_port( portbindings.VIF_TYPE_OVS, portbindings.VNIC_NORMAL) diff --git a/neutron/tests/unit/services/qos/test_qos_plugin.py b/neutron/tests/unit/services/qos/test_qos_plugin.py index b5bfce23314..bca92987a59 100644 --- a/neutron/tests/unit/services/qos/test_qos_plugin.py +++ b/neutron/tests/unit/services/qos/test_qos_plugin.py @@ -326,6 +326,8 @@ class TestQosPlugin(base.BaseQosTestCase): 'neutron.objects.qos.policy.QosPolicy.get_object', return_value=policy_mock ) as get_policy, mock.patch.object( + self.qos_plugin, "validate_policy_for_network" + ) as validate_policy_for_network, mock.patch.object( self.qos_plugin, "validate_policy_for_ports" ) as validate_policy_for_ports, mock.patch.object( self.ctxt, "elevated", return_value=admin_ctxt @@ -337,6 +339,7 @@ class TestQosPlugin(base.BaseQosTestCase): states=(kwargs['original_network'],))) if policy_id is None or policy_id == original_policy_id: get_policy.assert_not_called() + validate_policy_for_network.assert_not_called() get_ports.assert_not_called() validate_policy_for_ports.assert_not_called() else: @@ -384,6 +387,20 @@ class TestQosPlugin(base.BaseQosTestCase): except qos_exc.QosRuleNotSupported: self.fail("QosRuleNotSupported exception unexpectedly raised") + def test_validate_policy_for_network(self): + network = uuidutils.generate_uuid() + with mock.patch.object( + self.qos_plugin.driver_manager, "validate_rule_for_network", + return_value=True + ): + self.policy.rules = [self.rule] + try: + self.qos_plugin.validate_policy_for_network( + self.ctxt, self.policy, network_id=network) + except qos_exc.QosRuleNotSupportedByNetwork: + self.fail("QosRuleNotSupportedByNetwork " + "exception unexpectedly raised") + def test_create_min_bw_rule_on_bound_port(self): policy = self._get_policy() policy.rules = [self.min_rule] @@ -1236,6 +1253,35 @@ class TestQosPluginDB(base.BaseQosTestCase): network.create() return network + def _test_validate_create_network_callback(self, network_qos=False): + net_qos_obj = self._make_qos_policy() + net_qos_id = net_qos_obj.id if network_qos else None + network = self._make_network(qos_policy_id=net_qos_id) + kwargs = {"context": self.context, + "network": network} + + with mock.patch.object(self.qos_plugin, + 'validate_policy_for_network') \ + as mock_validate_policy: + self.qos_plugin._validate_create_network_callback( + 'NETWORK', 'precommit_create', 'test_plugin', **kwargs) + + qos_policy = None + if network_qos: + qos_policy = net_qos_obj + + if qos_policy: + mock_validate_policy.assert_called_once_with( + self.context, qos_policy, network.id) + else: + mock_validate_policy.assert_not_called() + + def test_validate_create_network_callback(self): + self._test_validate_create_network_callback(network_qos=True) + + def test_validate_create_network_callback_no_qos(self): + self._test_validate_create_network_callback(network_qos=False) + def _test_validate_create_port_callback(self, port_qos=False, network_qos=False): net_qos_obj = self._make_qos_policy()