From c3ebefa5f73e8cb89c3c9c12b385b21e8749299f Mon Sep 17 00:00:00 2001 From: LIU Yulong Date: Fri, 2 Sep 2022 10:13:20 +0800 Subject: [PATCH] Refactor for meter ID Generator Add a Singleton meter ID Generator for both bandwidth limit and packet rate limit, because for one bridge the meter ID is a sharing range. Closes-Bug: #1964342 Change-Id: Ibb9762d57913ea701dcf2746a0e0db74c6a7ca01 --- neutron/plugins/ml2/common/constants.py | 3 + .../agent/extension_drivers/qos_driver.py | 106 +++++++++++------- neutron/tests/common/agents/l2_extensions.py | 6 +- .../extension_drivers/test_qos_driver.py | 85 ++++++++++++++ 4 files changed, 158 insertions(+), 42 deletions(-) diff --git a/neutron/plugins/ml2/common/constants.py b/neutron/plugins/ml2/common/constants.py index db11cd99c6f..f9e627ff59e 100644 --- a/neutron/plugins/ml2/common/constants.py +++ b/neutron/plugins/ml2/common/constants.py @@ -28,3 +28,6 @@ NO_PBLOCKS_TYPES = [ constants.DEVICE_OWNER_ROUTER_HA_INTF, constants.DEVICE_OWNER_FLOATINGIP, ] + +METER_FLAG_BPS = "bps" +METER_FLAG_PPS = "pps" diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/extension_drivers/qos_driver.py b/neutron/plugins/ml2/drivers/openvswitch/agent/extension_drivers/qos_driver.py index 4051f9a17c1..bc7815e74c0 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/extension_drivers/qos_driver.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/extension_drivers/qos_driver.py @@ -22,6 +22,7 @@ from oslo_config import cfg from oslo_log import log as logging from neutron.agent.l2.extensions import qos_linux as qos +from neutron.plugins.ml2.common import constants as comm_consts from neutron.services.qos.drivers.openvswitch import driver @@ -30,27 +31,71 @@ LOG = logging.getLogger(__name__) MAX_RETIES = 1000 -class MeterRuleManager(object): +class MeterIDGenerator(object): # This cache will be: # PORT_METER_ID = {"port_id_1_ingress": 1, # "port_id_1_egress: 2, # "port_id_2_ingress": 3, # "port_id_2_egress: 4} - PORT_METER_ID = {} - # This will be: - # PORT_INFO_INGRESS = {"port_id_1": (of_port_name, mac_1, local_vlan_1), - # "port_id_2": (of_port_name, mac_2, local_vlan_2), - PORT_INFO_INGRESS = {} - # PORT_INFO_EGRESS = {"port_id_1": (of_port_name, mac_1, of_port_1), - # "port_id_2": (of_port_name, mac_2, of_port_2), - PORT_INFO_EGRESS = {} - def __init__(self, br_int): + def __new__(cls, *args, **kwargs): + # make it a singleton + if not hasattr(cls, '_instance'): + cls._instance = super(MeterIDGenerator, cls).__new__(cls) + cls.PORT_METER_ID = {} + return cls._instance + + def __init__(self, max_meter): + self.max_meter = max_meter + + def _generate_meter_id(self): + if self.max_meter <= 0: + return + used_meter_ids = self.PORT_METER_ID.values() + cid = None + times = 0 + while not cid or cid in used_meter_ids: + cid = random.randint(1, self.max_meter) + times += 1 + if times >= MAX_RETIES: + return + return cid + + def allocate_meter_id(self, key): + meter_id = self._generate_meter_id() + if not meter_id: + return + self.set_meter_id(key, meter_id) + return meter_id + + def remove_port_meter_id(self, key): + return self.PORT_METER_ID.pop(key, None) + + def set_meter_id(self, key, meter_id): + self.PORT_METER_ID[key] = meter_id + + +class MeterRuleManager(object): + + def __init__(self, br_int, type_=comm_consts.METER_FLAG_PPS): self.br_int = br_int + self.max_meter = 0 self._init_max_meter_id() + self.rule_type = type_ + self.generator = MeterIDGenerator(self.max_meter) + # This will be: + # PORT_INFO_INGRESS = {"port_id_1": (mac_1, 1), + # "port_id_2": (mac_2, 2), + # "port_id_3": (mac_3, 3), + # "port_id_4": (mac_4, 4)} + self.PORT_INFO_INGRESS = {} + # PORT_INFO_EGRESS = {"port_id_1": (mac_1, 1), + # "port_id_2": (mac_2, 1), + # "port_id_3": (mac_3, 1), + # "port_id_4": (mac_4, 1)} + self.PORT_INFO_EGRESS = {} def _init_max_meter_id(self): - self.max_meter = 0 features = self.br_int.list_meter_features() for f in features: if f["max_meter"] > 0: @@ -58,20 +103,20 @@ class MeterRuleManager(object): break def get_data_key(self, port_id, direction): - return "%s_%s" % (port_id, direction) + return "%s_%s_%s" % (self.rule_type, port_id, direction) def load_port_meter_id(self, port_name, port_id, direction): key = self.get_data_key(port_id, direction) - meter_id = self.br_int.get_value_from_other_config( - port_name, key, value_type=int) - if meter_id: - self.PORT_METER_ID[key] = meter_id - else: - LOG.warning("Failed to load port %(port)s meter id in " + try: + meter_id = self.br_int.get_value_from_other_config( + port_name, key, value_type=int) + self.generator.set_meter_id(key, meter_id) + return meter_id + except Exception: + LOG.warning("Failed to load port $(port)s meter id in " "direction %(direction)s", {"direction": direction, "port": port_id}) - return meter_id def store_port_meter_id_to_ovsdb(self, port_name, port_id, direction, meter_id): @@ -84,32 +129,13 @@ class MeterRuleManager(object): self.br_int.remove_value_from_other_config( port_name, key) - def generate_meter_id(self): - if self.max_meter <= 0: - return - used_meter_ids = self.PORT_METER_ID.values() - cid = None - times = 0 - while not cid or cid in used_meter_ids: - cid = random.randint(1, self.max_meter) - times += 1 - if times >= MAX_RETIES: - LOG.warning("Failed to allocate meter " - "id after %d retries", times) - return - return cid - def allocate_meter_id(self, port_id, direction): - meter_id = self.generate_meter_id() - if not meter_id: - return key = self.get_data_key(port_id, direction) - self.PORT_METER_ID[key] = meter_id - return meter_id + return self.generator.allocate_meter_id(key) def remove_port_meter_id(self, port_id, direction): key = self.get_data_key(port_id, direction) - return self.PORT_METER_ID.pop(key, None) + return self.generator.remove_port_meter_id(key) def set_port_info_ingress(self, port_id, port_name, mac, vlan): self.PORT_INFO_INGRESS[port_id] = (port_name, mac, vlan) diff --git a/neutron/tests/common/agents/l2_extensions.py b/neutron/tests/common/agents/l2_extensions.py index 9b14955036a..55077c66255 100644 --- a/neutron/tests/common/agents/l2_extensions.py +++ b/neutron/tests/common/agents/l2_extensions.py @@ -21,6 +21,7 @@ from oslo_log import log as logging from neutron.agent.common import async_process from neutron.agent.linux import iptables_manager from neutron.common import utils as common_utils +from neutron.plugins.ml2.common import constants as comm_consts LOG = logging.getLogger(__name__) @@ -87,11 +88,12 @@ def wait_until_dscp_marking_rule_applied_ovs(bridge, port_vif, rule): def wait_until_pkt_meter_rule_applied_ovs(bridge, port_vif, port_id, - direction, mac=None): + direction, mac=None, + type_=comm_consts.METER_FLAG_PPS): def _pkt_rate_limit_rule_applied(): port_num = bridge.get_port_ofport(port_vif) port_vlan = bridge.get_port_tag_by_name(port_vif) - key = "%s_%s" % (port_id, direction) + key = "%s_%s_%s" % (type_, port_id, direction) meter_id = bridge.get_value_from_other_config( port_vif, key, value_type=int) 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 e2bef51f1fc..15988b45a0d 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 @@ -20,6 +20,7 @@ from oslo_utils import uuidutils from neutron.objects.qos import policy from neutron.objects.qos import rule +from neutron.plugins.ml2.common import constants as comm_consts from neutron.plugins.ml2.drivers.openvswitch.agent import ( ovs_agent_extension_api as ovs_ext_api) from neutron.plugins.ml2.drivers.openvswitch.agent.extension_drivers import ( @@ -243,6 +244,90 @@ class QosOVSAgentDriverTestCase(ovs_test_base.OVSAgentConfigTestBase): self.delete_meter.assert_not_called() self.remove_meter_from_port.assert_not_called() + def test_meter_manager_allocate_meter_id(self): + meter_cache_pps = qos_driver.MeterRuleManager(mock.Mock()) + meter_cache_pps.generator.max_meter = 10000 + meter_cache_bps = qos_driver.MeterRuleManager( + mock.Mock(), type_=comm_consts.METER_FLAG_BPS) + meter_cache_bps.generator.max_meter = 10000 + meter_cache_pps.allocate_meter_id("1", "ingress") + meter_cache_pps.allocate_meter_id("1", "egress") + meter_cache_bps.allocate_meter_id("1", "ingress") + meter_cache_bps.allocate_meter_id("1", "egress") + meter_cache_pps.allocate_meter_id("2", "ingress") + meter_cache_pps.allocate_meter_id("2", "egress") + meter_cache_bps.allocate_meter_id("2", "ingress") + meter_cache_bps.allocate_meter_id("2", "egress") + self.assertEqual( + meter_cache_pps.generator.PORT_METER_ID, + meter_cache_bps.generator.PORT_METER_ID) + pps_keys = meter_cache_pps.generator.PORT_METER_ID.keys() + bps_keys = meter_cache_bps.generator.PORT_METER_ID.keys() + self.assertEqual( + 2, len([k for k in pps_keys if k.startswith('pps_1')])) + self.assertEqual( + 2, len([k for k in bps_keys if k.startswith('bps_1')])) + self.assertEqual( + 2, len([k for k in pps_keys if k.startswith('pps_2')])) + self.assertEqual( + 2, len([k for k in bps_keys if k.startswith('bps_2')])) + self.assertEqual( + meter_cache_pps.generator.PORT_METER_ID.keys(), + meter_cache_bps.generator.PORT_METER_ID.keys()) + pps_values = list(meter_cache_pps.generator.PORT_METER_ID.values()) + bps_values = list(meter_cache_bps.generator.PORT_METER_ID.values()) + self.assertEqual(pps_values, bps_values) + except_keys = ["pps_1_ingress", "pps_1_egress", + "bps_1_ingress", "bps_1_egress", + "pps_2_ingress", "pps_2_egress", + "bps_2_ingress", "bps_2_egress"] + except_values = [] + for key in except_keys: + value = meter_cache_bps.generator.PORT_METER_ID.get(key) + if value: + except_values.append(value) + self.assertEqual(8, len(set(except_values))) + + def test_meter_manager_remove_port_meter_id(self): + meter_cache_pps = qos_driver.MeterRuleManager(mock.Mock()) + meter_cache_pps.generator.max_meter = 10000 + meter_cache_bps = qos_driver.MeterRuleManager( + mock.Mock(), type_=comm_consts.METER_FLAG_BPS) + meter_cache_bps.generator.max_meter = 10000 + meter_cache_pps.allocate_meter_id("1", "ingress") + meter_cache_pps.allocate_meter_id("1", "egress") + meter_cache_bps.allocate_meter_id("1", "ingress") + meter_cache_bps.allocate_meter_id("1", "egress") + meter_cache_pps.allocate_meter_id("2", "ingress") + meter_cache_pps.allocate_meter_id("2", "egress") + meter_cache_bps.allocate_meter_id("2", "ingress") + meter_cache_bps.allocate_meter_id("2", "egress") + self.assertEqual( + meter_cache_pps.generator.PORT_METER_ID, + meter_cache_bps.generator.PORT_METER_ID) + + meter_cache_bps.remove_port_meter_id("2", "ingress") + meter_cache_pps.remove_port_meter_id("1", "egress") + + self.assertNotIn( + "pps_1_egress", meter_cache_pps.generator.PORT_METER_ID.keys()) + self.assertNotIn( + "bps_2_ingress", meter_cache_pps.generator.PORT_METER_ID.keys()) + + pps_values = list(meter_cache_pps.generator.PORT_METER_ID.values()) + bps_values = list(meter_cache_bps.generator.PORT_METER_ID.values()) + self.assertEqual(pps_values, bps_values) + except_keys = ["pps_1_ingress", + "bps_1_ingress", "bps_1_egress", + "pps_2_ingress", "pps_2_egress", + "bps_2_egress"] + except_values = [] + for key in except_keys: + value = meter_cache_bps.generator.PORT_METER_ID.get(key) + if value: + except_values.append(value) + self.assertEqual(6, len(set(except_values))) + def _assert_rules_create_updated(self): self.create_egress.assert_called_once_with( self.port_name, self.rules[0].max_kbps,