Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Namespace]: Fix SAI_ID key used in cpfcIfTable and csqIfQosGroupStatsTable implementation #138

Merged
merged 11 commits into from
Aug 13, 2020
27 changes: 11 additions & 16 deletions src/sonic_ax_impl/mibs/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -245,12 +245,6 @@ def init_sync_d_interface_tables(db_conn):
logger.debug("Port name map:\n" + pprint.pformat(if_name_map, indent=2))
logger.debug("Interface name map:\n" + pprint.pformat(if_id_map, indent=2))

# { OID -> sai_id }
oid_sai_map = {get_index(if_name): sai_id for if_name, sai_id in if_name_map.items()
# only map the interface if it's a style understood to be a SONiC interface.
if get_index(if_name) is not None}
logger.debug("OID sai map:\n" + pprint.pformat(oid_sai_map, indent=2))

# { OID -> if_name (SONiC) }
oid_name_map = {get_index(if_name): if_name for if_name in if_name_map
# only map the interface if it's a style understood to be a SONiC interface.
Expand All @@ -259,14 +253,14 @@ def init_sync_d_interface_tables(db_conn):
logger.debug("OID name map:\n" + pprint.pformat(oid_name_map, indent=2))

# SyncD consistency checks.
if not oid_sai_map:
if not oid_name_map:
# In the event no interface exists that follows the SONiC pattern, no OIDs are able to be registered.
# A RuntimeError here will prevent the 'main' module from loading. (This is desirable.)
message = "No interfaces found matching pattern '{}'. SyncD database is incoherent." \
.format(port_util.SONIC_ETHERNET_RE_PATTERN)
logger.error(message)
raise RuntimeError(message)
elif len(if_id_map) < len(if_name_map) or len(oid_sai_map) < len(if_name_map):
elif len(if_id_map) < len(if_name_map) or len(oid_name_map) < len(if_name_map):
# a length mismatch indicates a bad interface name
logger.warning("SyncD database contains incoherent interface names. Interfaces must match pattern '{}'"
.format(port_util.SONIC_ETHERNET_RE_PATTERN))
Expand All @@ -281,7 +275,7 @@ def init_sync_d_interface_tables(db_conn):

logger.debug("Chassis name map:\n" + pprint.pformat(if_alias_map, indent=2))

return if_name_map, if_alias_map, if_id_map, oid_sai_map, oid_name_map
return if_name_map, if_alias_map, if_id_map, oid_name_map

def init_sync_d_lag_tables(db_conn):
"""
Expand Down Expand Up @@ -333,15 +327,15 @@ def init_sync_d_queue_tables(db_conn):
:return: tuple(port_queues_map, queue_stat_map)
"""

# { Port index : Queue index (SONiC) -> sai_id }
# ex: { "1:2" : "1000000000023" }
# { Port name : Queue index (SONiC) -> sai_id }
# ex: { "Ethernet0:2" : "1000000000023" }
queue_name_map = db_conn.get_all(COUNTERS_DB, COUNTERS_QUEUE_NAME_MAP, blocking=True)
logger.debug("Queue name map:\n" + pprint.pformat(queue_name_map, indent=2))

# Parse the queue_name_map and create the following maps:
# port_queues_map -> {"if_index : queue_index" : sai_oid}
# queue_stat_map -> {queue stat table name : {counter name : value}}
# port_queue_list_map -> {if_index: [sorted queue list]}
# port_queues_map -> {"port_index : queue_index" : sai_oid}
# queue_stat_map -> {"port_index : queue stat table name" : {counter name : value}}
# port_queue_list_map -> {port_index: [sorted queue list]}
port_queues_map = {}
queue_stat_map = {}
port_queue_list_map = {}
Expand All @@ -356,7 +350,8 @@ def init_sync_d_queue_tables(db_conn):
queue_stat_name = queue_table(sai_id)
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

queue_stat_name [](start = 8, length = 15)

SAI Object ID is unique in this db_conn. So the original implementation of init_sync_d_queue_tables makes sense.

Let's block this PR and continue exploration the dbs dict strategy, as also discussed in another PR (#137) #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the data will eventually merged together , we have to make sure that the keys are unique in these dictionaries.
So in maps which use SAI ID as key, modified it to use a combination of if_idx and SAI ID.

queue_stat = db_conn.get_all(COUNTERS_DB, queue_stat_name, blocking=False)
if queue_stat is not None:
queue_stat_map[queue_stat_name] = queue_stat
queue_stat_key = queue_key(port_index, queue_stat_name)
queue_stat_map[queue_stat_key] = queue_stat

if not port_queue_list_map.get(int(port_index)):
port_queue_list_map[int(port_index)] = [int(queue_index)]
Expand Down Expand Up @@ -577,7 +572,7 @@ def get_non_host_dbs(dbs):
return dbs
else:
return dbs[1:]

@staticmethod
def get_sync_d_from_all_namespace(per_namespace_func, dbs):
# return merged tuple of dictionaries retrieved from per
Expand Down
6 changes: 1 addition & 5 deletions src/sonic_ax_impl/mibs/ieee802_1ab.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ def __init__(self):
self.if_name_map = {}
self.if_alias_map = {}
self.if_id_map = {}
self.oid_sai_map = {}
self.oid_name_map = {}

self.mgmt_oid_name_map = {}
Expand All @@ -165,7 +164,6 @@ def reinit_data(self):
self.if_name_map, \
self.if_alias_map, \
self.if_id_map, \
self.oid_sai_map, \
self.oid_name_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_interface_tables, self.db_conn)

self.mgmt_oid_name_map, \
Expand Down Expand Up @@ -382,7 +380,6 @@ def __init__(self):
self.if_name_map = {}
self.if_alias_map = {}
self.if_id_map = {}
self.oid_sai_map = {}
self.oid_name_map = {}

self.mgmt_oid_name_map = {}
Expand All @@ -400,7 +397,6 @@ def reinit_data(self):
self.if_name_map, \
self.if_alias_map, \
self.if_id_map, \
self.oid_sai_map, \
self.oid_name_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_interface_tables, self.db_conn)

self.mgmt_oid_name_map, _ = mibs.init_mgmt_interface_tables(self.db_conn[0])
Expand Down Expand Up @@ -566,7 +562,7 @@ def reinit_data(self):
"""
Subclass reinit data routine.
"""
_, _, _, _, self.oid_name_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_interface_tables, self.db_conn)
_, _, _, self.oid_name_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_interface_tables, self.db_conn)

self.mgmt_oid_name_map, _ = mibs.init_mgmt_interface_tables(self.db_conn[0])

Expand Down
4 changes: 1 addition & 3 deletions src/sonic_ax_impl/mibs/ietf/rfc1213.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,6 @@ def __init__(self):
self.if_name_map = {}
self.if_alias_map = {}
self.if_id_map = {}
self.oid_sai_map = {}
self.oid_name_map = {}
self.namespace_db_map = Namespace.get_namespace_db_map(self.db_conn)

Expand All @@ -211,7 +210,6 @@ def reinit_data(self):
self.if_name_map, \
self.if_alias_map, \
self.if_id_map, \
self.oid_sai_map, \
self.oid_name_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_interface_tables, self.db_conn)
"""
db_conn - will have db_conn to all namespace DBs and
Expand All @@ -236,7 +234,7 @@ def update_data(self):
self.if_name_lag_name_map, \
self.oid_lag_name_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_lag_tables, self.db_conn)

self.if_range = sorted(list(self.oid_sai_map.keys()) +
self.if_range = sorted(list(self.oid_name_map.keys()) +
list(self.oid_lag_name_map.keys()) +
list(self.mgmt_oid_name_map.keys()))
self.if_range = [(i,) for i in self.if_range]
Expand Down
2 changes: 1 addition & 1 deletion src/sonic_ax_impl/mibs/ietf/rfc2737.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ def reinit_data(self):
self.physical_model_name_map = {}

# update interface maps
_, self.if_alias_map, _, _, _ = \
_, self.if_alias_map, _, _ = \
Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_interface_tables, Namespace.init_namespace_dbs())

device_metadata = mibs.get_device_metadata(self.statedb[0])
Expand Down
4 changes: 1 addition & 3 deletions src/sonic_ax_impl/mibs/ietf/rfc2863.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ def __init__(self):
self.if_name_map = {}
self.if_alias_map = {}
self.if_id_map = {}
self.oid_sai_map = {}
self.oid_name_map = {}
self.lag_name_if_name_map = {}
self.if_name_lag_name_map = {}
Expand All @@ -76,7 +75,6 @@ def reinit_data(self):
self.if_name_map, \
self.if_alias_map, \
self.if_id_map, \
self.oid_sai_map, \
self.oid_name_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_interface_tables, self.db_conn)

self.lag_name_if_name_map, \
Expand All @@ -90,7 +88,7 @@ def reinit_data(self):
self.mgmt_oid_name_map, \
self.mgmt_alias_map = mibs.init_mgmt_interface_tables(self.db_conn[0])

self.if_range = sorted(list(self.oid_sai_map.keys()) +
self.if_range = sorted(list(self.oid_name_map.keys()) +
list(self.oid_lag_name_map.keys()) +
list(self.mgmt_oid_name_map.keys()))
self.if_range = [(i,) for i in self.if_range]
Expand Down
2 changes: 0 additions & 2 deletions src/sonic_ax_impl/mibs/ietf/rfc4363.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ def __init__(self):
self.if_name_map = {}
self.if_alias_map = {}
self.if_id_map = {}
self.oid_sai_map = {}
self.oid_name_map = {}
self.vlanmac_ifindex_map = {}
self.vlanmac_ifindex_list = []
Expand All @@ -40,7 +39,6 @@ def reinit_data(self):
self.if_name_map, \
self.if_alias_map, \
self.if_id_map, \
self.oid_sai_map, \
self.oid_name_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_interface_tables, self.db_conn)

self.if_bpid_map = Namespace.dbs_get_bridge_port_map(self.db_conn, mibs.ASIC_DB)
Expand Down
16 changes: 8 additions & 8 deletions src/sonic_ax_impl/mibs/vendor/cisco/ciscoPfcExtMIB.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ def __init__(self):
self.if_name_map = {}
self.if_alias_map = {}
self.if_id_map = {}
self.oid_sai_map = {}
self.oid_name_map = {}

self.lag_name_if_name_map = {}
Expand All @@ -27,6 +26,7 @@ def __init__(self):
# cache of interface counters
self.if_counters = {}
self.if_range = []
self.namespace_db_map = Namespace.get_namespace_db_map(self.db_conn)

def reinit_data(self):
"""
Expand All @@ -35,7 +35,6 @@ def reinit_data(self):
self.if_name_map, \
self.if_alias_map, \
self.if_id_map, \
self.oid_sai_map, \
self.oid_name_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_interface_tables, self.db_conn)

self.update_data()
Expand All @@ -45,15 +44,17 @@ def update_data(self):
Update redis (caches config)
Pulls the table references for each interface.
"""
self.if_counters = \
{sai_id: Namespace.dbs_get_all(self.db_conn, mibs.COUNTERS_DB, mibs.counter_table(sai_id), blocking=True)
for sai_id in self.if_id_map}
for sai_id_key in self.if_id_map:
namespace, sai_id = mibs.split_sai_id_key(sai_id_key)
if_idx = mibs.get_index_from_str(self.if_id_map[sai_id_key])
self.if_counters[if_idx] = self.namespace_db_map[namespace].get_all(mibs.COUNTERS_DB, \
mibs.counter_table(sai_id), blocking=True)

self.lag_name_if_name_map, \
self.if_name_lag_name_map, \
self.oid_lag_name_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_lag_tables, self.db_conn)

self.if_range = sorted(list(self.oid_sai_map.keys()) + list(self.oid_lag_name_map.keys()))
self.if_range = sorted(list(self.oid_name_map.keys()) + list(self.oid_lag_name_map.keys()))
self.if_range = [(i,) for i in self.if_range]

def get_next(self, sub_id):
Expand Down Expand Up @@ -88,13 +89,12 @@ def _get_counter(self, oid, counter_name):
:param counter_name: the redis table (either IntEnum or string literal) to query.
:return: the counter for the respective sub_id/table.
"""
sai_id = self.oid_sai_map[oid]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is oid_sai_map is being used or this can be removed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed oid_sai_map as it is not required. The required information can be retrieved from oid_name_map.


# Enum.name or counter_name = 'name_of_the_table'
_counter_name = bytes(getattr(counter_name, 'name', counter_name), 'utf-8')

try:
counter_value = self.if_counters[sai_id][_counter_name]
counter_value = self.if_counters[oid][_counter_name]
counter_value = int(counter_value) & 0xffffffffffffffff
# done!
return counter_value
Expand Down
32 changes: 23 additions & 9 deletions src/sonic_ax_impl/mibs/vendor/cisco/ciscoSwitchQosMIB.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ def __init__(self):
self.if_name_map = {}
self.if_alias_map = {}
self.if_id_map = {}
self.oid_sai_map = {}
self.oid_name_map = {}

self.port_queues_map = {}
Expand All @@ -65,6 +64,8 @@ def __init__(self):
self.mib_oid_list = []

self.queue_type_map = {}
self.port_index_namespace = {}
self.namespace_db_map = Namespace.get_namespace_db_map(self.db_conn)

def reinit_data(self):
"""
Expand All @@ -73,14 +74,19 @@ def reinit_data(self):
self.if_name_map, \
self.if_alias_map, \
self.if_id_map, \
self.oid_sai_map, \
self.oid_name_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_interface_tables, self.db_conn)

for sai_id_key in self.if_id_map:
namespace, sai_id = mibs.split_sai_id_key(sai_id_key)
if_idx = mibs.get_index_from_str(self.if_id_map[sai_id_key])
self.port_index_namespace[if_idx] = namespace

self.port_queues_map, self.queue_stat_map, self.port_queue_list_map = \
Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_queue_tables, self.db_conn)

self.queue_type_map = Namespace.dbs_get_all(self.db_conn, mibs.COUNTERS_DB, "COUNTERS_QUEUE_TYPE_MAP", blocking=False)

for db_conn in Namespace.get_non_host_dbs(self.db_conn):
self.queue_type_map[db_conn.namespace] = db_conn.get_all(mibs.COUNTERS_DB, "COUNTERS_QUEUE_TYPE_MAP", blocking=False)

self.update_data()

def update_data(self):
Expand All @@ -90,9 +96,15 @@ def update_data(self):
"""
for queue_key, sai_id in self.port_queues_map.items():
queue_stat_name = mibs.queue_table(sai_id)
queue_stat = Namespace.dbs_get_all(self.db_conn, mibs.COUNTERS_DB, queue_stat_name, blocking=False)
port_index, _ = queue_key.split(':')
queue_stat_idx = mibs.queue_key(port_index, queue_stat_name)
namespace = self.port_index_namespace[int(port_index)]
queue_stat = self.namespace_db_map[namespace].get_all( \
mibs.COUNTERS_DB, queue_stat_name, blocking=False)
if queue_stat is not None:
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if queue_stat is not None [](start = 12, length = 25)

Otherwise, del the index from self.queue_stat_map #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if queue_stat is not None [](start = 12, length = 25)

Otherwise, del the index from self.queue_stat_map

Added as per comment.

self.queue_stat_map[queue_stat_name] = queue_stat
self.queue_stat_map[queue_stat_idx] = queue_stat
else:
del self.queue_stat_map[queue_stat_idx]

self.update_stats()

Expand All @@ -109,13 +121,14 @@ def update_stats(self):
self.mib_oid_list = []

# Sort the ports to keep the OID order in the MIB
if_range = list(self.oid_sai_map.keys())
if_range = list(self.oid_name_map.keys())
# Update queue counters for port
for if_index in if_range:
if if_index not in self.port_queue_list_map:
# Port does not has a queues, continue..
continue
if_queues = self.port_queue_list_map[if_index]
namespace = self.port_index_namespace[if_index]

# The first half of queue id is for ucast, and second half is for mcast
# To simulate vendor OID, we wrap queues by half distance
Expand All @@ -125,8 +138,9 @@ def update_stats(self):
# Get queue type and statistics
queue_sai_oid = self.port_queues_map[mibs.queue_key(if_index, queue)]
queue_stat_table_name = mibs.queue_table(queue_sai_oid)
queue_type = self.queue_type_map.get(queue_sai_oid)
queue_stat = self.queue_stat_map.get(queue_stat_table_name, {})
queue_stat_key = mibs.queue_key(if_index, queue_stat_table_name)
queue_type = self.queue_type_map[namespace].get(queue_sai_oid)
queue_stat = self.queue_stat_map.get(queue_stat_key, {})

# Add supported counters to MIBs list and store counters values
for (counter, counter_type), counter_mib_id in CounterMap.items():
Expand Down
6 changes: 3 additions & 3 deletions tests/mock_tables/asic1/asic_db.json
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
{
"ASIC_STATE:SAI_OBJECT_TYPE_FDB_ENTRY:{\"bridge_id\":\"oid:0x0\",\"bridge_type\":\"SAI_FDB_ENTRY_BRIDGE_TYPE_1Q\",\"mac\":\"7C:FE:90:80:9F:10\",\"switch_id\":\"oid:0x21000000000000\",\"vlan\":\"1000\"}": {
"SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID": "oid:0x3a0000000006208",
"SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID": "oid:0x3a000000000616",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what trigger this changes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified namespace mock dbs to reflect the idea that SAI ID will be repeated across namespaces.

"SAI_FDB_ENTRY_ATTR_TYPE": "SAI_FDB_ENTRY_TYPE_DYNAMIC"
},
"ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT:oid:0x3a000000000620": {
"ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT:oid:0x3a000000000616": {
"SAI_BRIDGE_PORT_ATTR_TYPE": "SAI_BRIDGE_PORT_TYPE_PORT",
"SAI_BRIDGE_PORT_ATTR_PORT_ID": "oid:0x1000000000010",
"SAI_BRIDGE_PORT_ATTR_PORT_ID": "oid:0x1000000000005",
"SAI_BRIDGE_PORT_ATTR_ADMIN_STATE": "true"
}
}
Loading