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
23 changes: 9 additions & 14 deletions src/sonic_ax_impl/mibs/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ def init_sync_d_queue_tables(db_conn):

# 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}}
# queue_stat_map -> {"if_index : queue stat table name" : {counter name : value}}
# port_queue_list_map -> {if_index: [sorted queue list]}
port_queues_map = {}
queue_stat_map = {}
Expand All @@ -320,7 +320,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 @@ -519,6 +520,7 @@ def init_namespace_sync_d_interface_tables(dbs):
global db. First db in the list is global db.
Ignore first global db to get interface tables if there
are multiple namespaces.
if_id_map = {db_index: {sai_oid : if_name}}
"""
for db_conn in Namespace.get_non_host_dbs(dbs):
if_name_map_ns, \
Expand All @@ -528,7 +530,7 @@ def init_namespace_sync_d_interface_tables(dbs):
oid_name_map_ns = init_sync_d_interface_tables(db_conn)
if_name_map.update(if_name_map_ns)
if_alias_map.update(if_alias_map_ns)
if_id_map.update(if_id_map_ns)
if_id_map[inst] = if_id_map_ns
oid_sai_map.update(oid_sai_map_ns)
oid_name_map.update(oid_name_map_ns)

Expand Down Expand Up @@ -580,20 +582,13 @@ def init_namespace_sync_d_queue_tables(dbs):
return port_queues_map, queue_stat_map, port_queue_list_map

@staticmethod
def dbs_get_bridge_port_map(dbs, db_name):
def dbs_get_bridge_port_map(dbs):
"""
get_bridge_port_map from all namespace DBs
"""
if_br_oid_map = {}
for db_conn in Namespace.get_non_host_dbs(dbs):
if_br_oid_map_ns = port_util.get_bridge_port_map(db_conn)
if_br_oid_map.update(if_br_oid_map_ns)
for inst in range(Namespace.get_start_idx_of_non_host(dbs), len(dbs)):
if_br_oid_map_ns = port_util.get_bridge_port_map(dbs[inst])
if_br_oid_map[inst] = if_br_oid_map_ns
return if_br_oid_map

@staticmethod
def dbs_get_vlan_id_from_bvid(dbs, bvid):
for db_conn in Namespace.get_non_host_dbs(dbs):
db_conn.connect('ASIC_DB')
vlan_obj = db_conn.keys('ASIC_DB', "ASIC_STATE:SAI_OBJECT_TYPE_VLAN:" + bvid)
if vlan_obj is not None:
return port_util.get_vlan_id_from_bvid(db_conn, bvid)
7 changes: 3 additions & 4 deletions src/sonic_ax_impl/mibs/ietf/rfc1213.py
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,8 @@ def update_data(self):
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}
{if_idx: self.db_conn[self.if_oid_namespace[if_idx]].get_all(
mibs.COUNTERS_DB, mibs.counter_table(self.oid_sai_map[if_idx]), blocking=True)
Copy link
Contributor

@abdosi abdosi Jul 8, 2020

Choose a reason for hiding this comment

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

Missing for loop of retrieving if_idx. Apply for all below files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the loop.


self.lag_name_if_name_map, \
self.if_name_lag_name_map, \
Expand Down Expand Up @@ -254,12 +254,11 @@ def _get_counter(self, oid, table_name):
:param table_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]
# Enum.name or table_name = 'name_of_the_table'
_table_name = bytes(getattr(table_name, 'name', table_name), 'utf-8')

try:
counter_value = self.if_counters[sai_id][_table_name]
counter_value = self.if_counters[oid][_table_name]
# truncate to 32-bit counter (database implements 64-bit counters)
counter_value = int(counter_value) & 0x00000000ffffffff
# done!
Expand Down
10 changes: 4 additions & 6 deletions src/sonic_ax_impl/mibs/ietf/rfc2863.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,9 @@ 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}

self.if_counters = \
{if_idx: self.db_conn[self.if_oid_namespace[if_idx]].get_all(
mibs.COUNTERS_DB, mibs.counter_table(self.oid_sai_map[if_idx]), blocking=True)

def get_next(self, sub_id):
"""
Expand Down Expand Up @@ -186,11 +185,10 @@ def _get_counter(self, oid, table_name, mask):

return counter_value & mask

sai_id = self.oid_sai_map[oid]
# Enum.name or table_name = 'name_of_the_table'
_table_name = bytes(getattr(table_name, 'name', table_name), 'utf-8')
try:
counter_value = self.if_counters[sai_id][_table_name]
counter_value = self.if_counters[oid][_table_name]
# truncate to 32-bit counter (database implements 64-bit counters)
counter_value = int(counter_value) & mask
# done!
Expand Down
57 changes: 31 additions & 26 deletions src/sonic_ax_impl/mibs/ietf/rfc4363.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,17 @@ def __init__(self):
self.if_bpid_map = {}
self.bvid_vlan_map = {}

def fdb_vlanmac(self, fdb):
def fdb_vlanmac(self, fdb, db_index):
if 'vlan' in fdb:
vlan_id = fdb["vlan"]
elif 'bvid' in fdb:
if fdb["bvid"] in self.bvid_vlan_map:
vlan_id = self.bvid_vlan_map[fdb["bvid"]]
else:
vlan_id = Namespace.dbs_get_vlan_id_from_bvid(self.db_conn, fdb["bvid"])
vlan_id = port_util.get_vlan_id_from_bvid(self.db_conn[db_index], fdb["bvid"])
self.bvid_vlan_map[fdb["bvid"]] = vlan_id
return (int(vlan_id),) + mac_decimals(fdb["mac"])

def reinit_data(self):
"""
Subclass update interface information
Expand All @@ -41,7 +41,7 @@ def reinit_data(self):
self.if_alias_map, \
self.if_id_map, \
self.oid_sai_map, \
self.oid_name_map = Namespace.init_namespace_sync_d_interface_tables(self.db_conn)
self.oid_name_map, _ = Namespace.init_namespace_sync_d_interface_tables(self.db_conn)

self.if_bpid_map = Namespace.dbs_get_bridge_port_map(self.db_conn, mibs.ASIC_DB)
self.bvid_vlan_map.clear()
Expand All @@ -54,28 +54,33 @@ def update_data(self):
self.vlanmac_ifindex_map = {}
self.vlanmac_ifindex_list = []

fdb_strings = Namespace.dbs_keys(self.db_conn, mibs.ASIC_DB, "ASIC_STATE:SAI_OBJECT_TYPE_FDB_ENTRY:*")
if not fdb_strings:
return

for s in fdb_strings:
fdb_str = s.decode()
try:
fdb = json.loads(fdb_str.split(":", maxsplit=2)[-1])
except ValueError as e: # includes simplejson.decoder.JSONDecodeError
mibs.logger.error("SyncD 'ASIC_DB' includes invalid FDB_ENTRY '{}': {}.".format(fdb_str, e))
break

ent = Namespace.dbs_get_all(self.db_conn, mibs.ASIC_DB, s, blocking=True)
# Example output: oid:0x3a000000000608
bridge_port_id = ent[b"SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID"][6:]
if bridge_port_id not in self.if_bpid_map:
continue
port_id = self.if_bpid_map[bridge_port_id]

vlanmac = self.fdb_vlanmac(fdb)
self.vlanmac_ifindex_map[vlanmac] = mibs.get_index(self.if_id_map[port_id])
self.vlanmac_ifindex_list.append(vlanmac)
Namespace.connect_all_dbs(self.db_conn, mibs.ASIC_DB)

for db_index in range(Namespace.get_start_idx_of_non_host(self.db_conn), len(self.db_conn)):
fdb_strings = self.db_conn[db_index].keys(mibs.ASIC_DB, "ASIC_STATE:SAI_OBJECT_TYPE_FDB_ENTRY:*")
if not fdb_strings:
continue
for s in fdb_strings:
fdb_str = s.decode()
try:
fdb = json.loads(fdb_str.split(":", maxsplit=2)[-1])
except ValueError as e: # includes simplejson.decoder.JSONDecodeError
mibs.logger.error("SyncD 'ASIC_DB' includes invalid FDB_ENTRY '{}': {}.".format(fdb_str, e))
break

ent = self.db_conn[db_index].get_all(mibs.ASIC_DB, s, blocking=True)
# Example output: oid:0x3a000000000608
bridge_port_id = ent[b"SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID"][6:]
#if_bpid_map = self.if_bpid_map[db_index]
if db_index not in self.if_bpid_map or db_index not in self.if_id_map:
continue
if bridge_port_id not in self.if_bpid_map[db_index]:
continue
port_id = self.if_bpid_map[db_index][bridge_port_id]

vlanmac = self.fdb_vlanmac(fdb, db_index)
self.vlanmac_ifindex_map[vlanmac] = mibs.get_index(self.if_id_map[db_index][port_id])
self.vlanmac_ifindex_list.append(vlanmac)
self.vlanmac_ifindex_list.sort()

def fdb_ifindex(self, sub_id):
Expand Down
7 changes: 3 additions & 4 deletions src/sonic_ax_impl/mibs/vendor/cisco/ciscoPfcExtMIB.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ def update_data(self):
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}
{if_idx: self.db_conn[self.if_oid_namespace[if_idx]].get_all(
mibs.COUNTERS_DB, mibs.counter_table(self.oid_sai_map[if_idx]), blocking=True)

self.lag_name_if_name_map, \
self.if_name_lag_name_map, \
Expand Down Expand Up @@ -88,13 +88,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
12 changes: 9 additions & 3 deletions src/sonic_ax_impl/mibs/vendor/cisco/ciscoSwitchQosMIB.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,13 @@ def reinit_data(self):
self.port_queues_map, self.queue_stat_map, self.port_queue_list_map = \
Namespace.init_namespace_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)

"""
COUNTERS_QUEUE_TYPE_MAP OID will be the same for all namespaces.
Get OID list from first namespace.
"""
db_start_idx = Namespace.get_start_idx_of_non_host(self.db_conn)
self.queue_type_map = self.db_conn[db_start_idx].get_all(mibs.COUNTERS_DB, "COUNTERS_QUEUE_TYPE_MAP", blocking=False)

self.update_data()

def update_data(self):
Expand All @@ -90,7 +95,8 @@ 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)
queue_stat_idx = mibs.queue_key(if_index, queue_stat_table_name)
queue_stat = self.queue_stat_map.get(queue_stat_idx, {})
Copy link
Contributor

Choose a reason for hiding this comment

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

can queue_stat be None ? Do we have to do this check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can queue_stat be None ? Do we have to do this check

We have a check in the line below to check if queue_stat is not None.

Copy link
Contributor

Choose a reason for hiding this comment

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

my point was can it be ever None ? do we need to put if condition ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake, none check is required the same as in: https://github.com/SuvarnaMeenakshi/sonic-snmpagent/blob/counters_fix/src/sonic_ax_impl/mibs/__init__.py#L358.
I have fixed the code to do a get_all to get the updated stats.
The previous code was providing the correct results as each time reinit_data was invoked, the stats was getting updated.
But update data is called more frequently, and update_data also should do a get_all each time, after which the None check will be required.

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

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