From fe236bdaadb949661a0bfb9b62ddbe432b4cf5f1 Mon Sep 17 00:00:00 2001 From: Miguel Angel Ajo Date: Thu, 3 Sep 2015 15:40:12 +0200 Subject: [PATCH] No network devices on network attached qos policies Network devices, like internal router legs, or dhcp ports should not be affected by bandwidth limiting rules. This patch disables application of network attached policies to network/neutron owned ports. Closes-bug: #1486039 DocImpact Change-Id: I75d80227f1e6c4b3f5fa7762b8dc3b0c0f1abd46 --- doc/source/devref/quality_of_service.rst | 10 ++++- neutron/agent/l2/extensions/qos.py | 14 ++++-- neutron/common/constants.py | 2 + neutron/objects/qos/rule.py | 17 +++++++ neutron/plugins/ml2/rpc.py | 8 ++-- .../test_ovs_agent_qos_extension.py | 3 ++ .../unit/agent/l2/extensions/test_qos.py | 17 ++++++- neutron/tests/unit/objects/qos/test_rule.py | 44 +++++++++++++++++++ .../extension_drivers/test_qos_driver.py | 13 ++++-- .../extension_drivers/test_qos_driver.py | 12 +++-- neutron/tests/unit/plugins/ml2/test_rpc.py | 6 +-- 11 files changed, 125 insertions(+), 21 deletions(-) diff --git a/doc/source/devref/quality_of_service.rst b/doc/source/devref/quality_of_service.rst index bd4a8c716..ac7347db6 100644 --- a/doc/source/devref/quality_of_service.rst +++ b/doc/source/devref/quality_of_service.rst @@ -84,8 +84,14 @@ for a port or a network: Each QoS policy contains zero or more QoS rules. A policy is then applied to a network or a port, making all rules of the policy applied to the corresponding -Neutron resource (for a network, applying a policy means that the policy will -be applied to all ports that belong to it). +Neutron resource. + +When applied through a network association, policy rules could apply or not +to neutron internal ports (like router, dhcp, load balancer, etc..). The QosRule +base object provides a default should_apply_to_port method which could be +overridden. In the future we may want to have a flag in QoSNetworkPolicyBinding +or QosRule to enforce such type of application (for example when limiting all +the ingress of routers devices on an external network automatically). From database point of view, following objects are defined in schema: diff --git a/neutron/agent/l2/extensions/qos.py b/neutron/agent/l2/extensions/qos.py index 06d47161d..7cb9111e2 100644 --- a/neutron/agent/l2/extensions/qos.py +++ b/neutron/agent/l2/extensions/qos.py @@ -104,9 +104,13 @@ class QosAgentDriver(object): def _handle_update_create_rules(self, action, port, qos_policy): for rule in self._iterate_rules(qos_policy.rules): - handler_name = "".join((action, "_", rule.rule_type)) - handler = getattr(self, handler_name) - handler(port, rule) + if rule.should_apply_to_port(port): + handler_name = "".join((action, "_", rule.rule_type)) + handler = getattr(self, handler_name) + handler(port, rule) + else: + LOG.debug("Port %(port)s excluded from QoS rule %(rule)s", + {'port': port, 'rule': rule.id}) class PortPolicyMap(object): @@ -204,7 +208,9 @@ class QosAgentExtension(agent_extension.AgentCoreResourceExtension): Update events are handled in _handle_notification. """ port_id = port['port_id'] - qos_policy_id = port.get('qos_policy_id') + port_qos_policy_id = port.get('qos_policy_id') + network_qos_policy_id = port.get('network_qos_policy_id') + qos_policy_id = port_qos_policy_id or network_qos_policy_id if qos_policy_id is None: self._process_reset_port(port) return diff --git a/neutron/common/constants.py b/neutron/common/constants.py index 9a4ada150..315f58c3d 100644 --- a/neutron/common/constants.py +++ b/neutron/common/constants.py @@ -41,6 +41,8 @@ DEVICE_OWNER_ROUTER_SNAT = "network:router_centralized_snat" DEVICE_OWNER_LOADBALANCER = "neutron:LOADBALANCER" DEVICE_OWNER_LOADBALANCERV2 = "neutron:LOADBALANCERV2" +DEVICE_OWNER_PREFIXES = ["network:", "neutron:"] + # Collection used to identify devices owned by router interfaces. # DEVICE_OWNER_ROUTER_HA_INTF is a special case and so is not included. ROUTER_INTERFACE_OWNERS = (DEVICE_OWNER_ROUTER_INTF, diff --git a/neutron/objects/qos/rule.py b/neutron/objects/qos/rule.py index 4398c7004..2362fb7be 100644 --- a/neutron/objects/qos/rule.py +++ b/neutron/objects/qos/rule.py @@ -20,6 +20,7 @@ from oslo_versionedobjects import base as obj_base from oslo_versionedobjects import fields as obj_fields import six +from neutron.common import constants from neutron.common import utils from neutron.db import api as db_api from neutron.db.qos import models as qos_db_model @@ -57,6 +58,22 @@ class QosRule(base.NeutronDbObject): dict_['type'] = self.rule_type return dict_ + def should_apply_to_port(self, port): + """Check whether a rule can be applied to a specific port. + + This function has the logic to decide whether a rule should + be applied to a port or not, depending on the source of the + policy (network, or port). Eventually rules could override + this method, or we could make it abstract to allow different + rule behaviour. + """ + is_network_rule = self.qos_policy_id != port[qos_consts.QOS_POLICY_ID] + is_network_device_port = any(port['device_owner'].startswith(prefix) + for prefix + in constants.DEVICE_OWNER_PREFIXES) + + return not (is_network_rule and is_network_device_port) + @obj_base.VersionedObjectRegistry.register class QosBandwidthLimitRule(QosRule): diff --git a/neutron/plugins/ml2/rpc.py b/neutron/plugins/ml2/rpc.py index b402c58c1..cf09ee349 100644 --- a/neutron/plugins/ml2/rpc.py +++ b/neutron/plugins/ml2/rpc.py @@ -109,9 +109,8 @@ class RpcCallbacks(type_tunnel.TunnelRpcCallbackMixin): host, port_context.network.current) - qos_policy_id = (port.get(qos_consts.QOS_POLICY_ID) or - port_context.network._network.get( - qos_consts.QOS_POLICY_ID)) + network_qos_policy_id = port_context.network._network.get( + qos_consts.QOS_POLICY_ID) entry = {'device': device, 'network_id': port['network_id'], 'port_id': port['id'], @@ -124,7 +123,8 @@ class RpcCallbacks(type_tunnel.TunnelRpcCallbackMixin): 'device_owner': port['device_owner'], 'allowed_address_pairs': port['allowed_address_pairs'], 'port_security_enabled': port.get(psec.PORTSECURITY, True), - 'qos_policy_id': qos_policy_id, + 'qos_policy_id': port.get(qos_consts.QOS_POLICY_ID), + 'network_qos_policy_id': network_qos_policy_id, 'profile': port[portbindings.PROFILE]} if 'security_groups' in port: entry['security_groups'] = port['security_groups'] diff --git a/neutron/tests/functional/agent/l2/extensions/test_ovs_agent_qos_extension.py b/neutron/tests/functional/agent/l2/extensions/test_ovs_agent_qos_extension.py index ad6e38b21..9b73e7336 100644 --- a/neutron/tests/functional/agent/l2/extensions/test_ovs_agent_qos_extension.py +++ b/neutron/tests/functional/agent/l2/extensions/test_ovs_agent_qos_extension.py @@ -31,11 +31,13 @@ TEST_POLICY_ID1 = "a2d72369-4246-4f19-bd3c-af51ec8d70cd" TEST_POLICY_ID2 = "46ebaec0-0570-43ac-82f6-60d2b03168c5" TEST_BW_LIMIT_RULE_1 = rule.QosBandwidthLimitRule( context=None, + qos_policy_id=TEST_POLICY_ID1, id="5f126d84-551a-4dcf-bb01-0e9c0df0c793", max_kbps=1000, max_burst_kbps=10) TEST_BW_LIMIT_RULE_2 = rule.QosBandwidthLimitRule( context=None, + qos_policy_id=TEST_POLICY_ID2, id="fa9128d9-44af-49b2-99bb-96548378ad42", max_kbps=900, max_burst_kbps=9) @@ -80,6 +82,7 @@ class OVSAgentQoSExtensionTestFramework(base.OVSAgentTestFramework): port_dict = super(OVSAgentQoSExtensionTestFramework, self)._create_test_port_dict() port_dict['qos_policy_id'] = policy_id + port_dict['network_qos_policy_id'] = None return port_dict def _get_device_details(self, port, network): diff --git a/neutron/tests/unit/agent/l2/extensions/test_qos.py b/neutron/tests/unit/agent/l2/extensions/test_qos.py index 47d42fc16..99d31c5d0 100755 --- a/neutron/tests/unit/agent/l2/extensions/test_qos.py +++ b/neutron/tests/unit/agent/l2/extensions/test_qos.py @@ -69,10 +69,14 @@ class QosAgentDriverTestCase(base.BaseTestCase): self.policy = TEST_POLICY self.rule = ( rule.QosBandwidthLimitRule(context=None, id='fake_rule_id', + qos_policy_id=self.policy.id, max_kbps=100, max_burst_kbps=200)) self.policy.rules = [self.rule] - self.port = object() - self.fake_rule = QosFakeRule(context=None, id='really_fake_rule_id') + self.port = {'qos_policy_id': None, 'network_qos_policy_id': None, + 'device_owner': 'random-device-owner'} + + self.fake_rule = QosFakeRule(context=None, id='really_fake_rule_id', + qos_policy_id=self.policy.id) def test_create(self): self.driver.create(self.port, self.policy) @@ -98,6 +102,15 @@ class QosAgentDriverTestCase(base.BaseTestCase): self.assertEqual(1, len(rules)) self.assertIsInstance(rules[0], rule.QosBandwidthLimitRule) + def test__handle_update_create_rules_checks_should_apply_to_port(self): + self.rule.should_apply_to_port = mock.Mock(return_value=False) + self.driver.create(self.port, self.policy) + self.assertFalse(self.driver.create_bandwidth_limit.called) + + self.rule.should_apply_to_port = mock.Mock(return_value=True) + self.driver.create(self.port, self.policy) + self.assertTrue(self.driver.create_bandwidth_limit.called) + class QosExtensionBaseTestCase(base.BaseTestCase): diff --git a/neutron/tests/unit/objects/qos/test_rule.py b/neutron/tests/unit/objects/qos/test_rule.py index 5edc81216..f4e14e703 100644 --- a/neutron/tests/unit/objects/qos/test_rule.py +++ b/neutron/tests/unit/objects/qos/test_rule.py @@ -10,12 +10,56 @@ # License for the specific language governing permissions and limitations # under the License. +from neutron.common import constants from neutron.objects.qos import policy from neutron.objects.qos import rule from neutron.services.qos import qos_consts +from neutron.tests import base as neutron_test_base from neutron.tests.unit.objects import test_base from neutron.tests.unit import testlib_api +POLICY_ID_A = 'policy-id-a' +POLICY_ID_B = 'policy-id-b' +DEVICE_OWNER_COMPUTE = 'compute:None' + + +class QosRuleObjectTestCase(neutron_test_base.BaseTestCase): + + def _test_should_apply_to_port(self, rule_policy_id, port_policy_id, + device_owner, expected_result): + test_rule = rule.QosRule(qos_policy_id=rule_policy_id) + port = {qos_consts.QOS_POLICY_ID: port_policy_id, + 'device_owner': device_owner} + self.assertEqual(expected_result, test_rule.should_apply_to_port(port)) + + def test_should_apply_to_port_with_network_port_and_net_policy(self): + self._test_should_apply_to_port( + rule_policy_id=POLICY_ID_B, + port_policy_id=POLICY_ID_A, + device_owner=constants.DEVICE_OWNER_ROUTER_INTF, + expected_result=False) + + def test_should_apply_to_port_with_network_port_and_port_policy(self): + self._test_should_apply_to_port( + rule_policy_id=POLICY_ID_A, + port_policy_id=POLICY_ID_A, + device_owner=constants.DEVICE_OWNER_ROUTER_INTF, + expected_result=True) + + def test_should_apply_to_port_with_compute_port_and_net_policy(self): + self._test_should_apply_to_port( + rule_policy_id=POLICY_ID_B, + port_policy_id=POLICY_ID_A, + device_owner=DEVICE_OWNER_COMPUTE, + expected_result=True) + + def test_should_apply_to_port_with_compute_port_and_port_policy(self): + self._test_should_apply_to_port( + rule_policy_id=POLICY_ID_A, + port_policy_id=POLICY_ID_A, + device_owner=DEVICE_OWNER_COMPUTE, + expected_result=True) + class QosBandwidthLimitRuleObjectTestCase(test_base.BaseObjectIfaceTestCase): diff --git a/neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/extension_drivers/test_qos_driver.py b/neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/extension_drivers/test_qos_driver.py index 1541a299f..60a5ef208 100755 --- a/neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/extension_drivers/test_qos_driver.py +++ b/neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/extension_drivers/test_qos_driver.py @@ -22,6 +22,7 @@ from neutron.objects.qos import rule from neutron.plugins.ml2.drivers.mech_sriov.agent.common import exceptions from neutron.plugins.ml2.drivers.mech_sriov.agent.extension_drivers import ( qos_driver) +from neutron.services.qos import qos_consts from neutron.tests import base @@ -42,7 +43,7 @@ class QosSRIOVAgentDriverTestCase(base.BaseTestCase): self.clear_max_rate_mock = self.qos_driver.eswitch_mgr.clear_max_rate self.rule = self._create_bw_limit_rule_obj() self.qos_policy = self._create_qos_policy_obj([self.rule]) - self.port = self._create_fake_port() + self.port = self._create_fake_port(self.qos_policy.id) def _create_bw_limit_rule_obj(self): rule_obj = rule.QosBandwidthLimitRule() @@ -61,12 +62,18 @@ class QosSRIOVAgentDriverTestCase(base.BaseTestCase): 'rules': rules} policy_obj = policy.QosPolicy(self.context, **policy_dict) policy_obj.obj_reset_changes() + for policy_rule in policy_obj.rules: + policy_rule.qos_policy_id = policy_obj.id + policy_rule.obj_reset_changes() + return policy_obj - def _create_fake_port(self): + def _create_fake_port(self, qos_policy_id): return {'port_id': uuidutils.generate_uuid(), 'profile': {'pci_slot': self.PCI_SLOT}, - 'device': self.ASSIGNED_MAC} + 'device': self.ASSIGNED_MAC, + qos_consts.QOS_POLICY_ID: qos_policy_id, + 'device_owner': uuidutils.generate_uuid()} def test_create_rule(self): self.qos_driver.create(self.port, self.qos_policy) diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/extension_drivers/test_qos_driver.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/extension_drivers/test_qos_driver.py index c9e276c72..5e19c0f36 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/extension_drivers/test_qos_driver.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/extension_drivers/test_qos_driver.py @@ -39,7 +39,7 @@ class QosOVSAgentDriverTestCase(ovs_test_base.OVSAgentConfigTestBase): self.create = self.qos_driver.br_int.create_egress_bw_limit_for_port self.rule = self._create_bw_limit_rule_obj() self.qos_policy = self._create_qos_policy_obj([self.rule]) - self.port = self._create_fake_port() + self.port = self._create_fake_port(self.qos_policy.id) def _create_bw_limit_rule_obj(self): rule_obj = rule.QosBandwidthLimitRule() @@ -58,15 +58,21 @@ class QosOVSAgentDriverTestCase(ovs_test_base.OVSAgentConfigTestBase): 'rules': rules} policy_obj = policy.QosPolicy(self.context, **policy_dict) policy_obj.obj_reset_changes() + for policy_rule in policy_obj.rules: + policy_rule.qos_policy_id = policy_obj.id + policy_rule.obj_reset_changes() return policy_obj - def _create_fake_port(self): + def _create_fake_port(self, policy_id): self.port_name = 'fakeport' class FakeVifPort(object): port_name = self.port_name - return {'vif_port': FakeVifPort()} + return {'vif_port': FakeVifPort(), + 'qos_policy_id': policy_id, + 'network_qos_policy_id': None, + 'device_owner': uuidutils.generate_uuid()} def test_create_new_rule(self): self.qos_driver.br_int.get_egress_bw_limit_for_port = mock.Mock( diff --git a/neutron/tests/unit/plugins/ml2/test_rpc.py b/neutron/tests/unit/plugins/ml2/test_rpc.py index 5e79eb761..4d36c886f 100644 --- a/neutron/tests/unit/plugins/ml2/test_rpc.py +++ b/neutron/tests/unit/plugins/ml2/test_rpc.py @@ -144,16 +144,16 @@ class RpcCallbacksTestCase(base.BaseTestCase): res = self.callbacks.get_device_details(mock.Mock(), host='fake') self.assertIsNone(res['qos_policy_id']) - def test_get_device_details_qos_policy_id_inherited_from_network(self): + def test_get_device_details_network_qos_policy_id(self): port = collections.defaultdict(lambda: 'fake_port') self.plugin.get_bound_port_context().current = port self.plugin.get_bound_port_context().network._network = ( {"id": "fake_network", qos_consts.QOS_POLICY_ID: 'test-policy-id'}) res = self.callbacks.get_device_details(mock.Mock(), host='fake') - self.assertEqual('test-policy-id', res['qos_policy_id']) + self.assertEqual('test-policy-id', res['network_qos_policy_id']) - def test_get_device_details_qos_policy_id_taken_from_port(self): + def test_get_device_details_qos_policy_id_from_port(self): port = collections.defaultdict( lambda: 'fake_port', {qos_consts.QOS_POLICY_ID: 'test-port-policy-id'}) -- 2.45.2