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

SuvarnaMeenakshi · 2020-07-04T22:08:48Z

Signed-off-by: SuvarnaMeenakshi sumeenak@microsoft.com

- What I did
In multi-asic platform, SAI OID is not unique for the whole device. It is unique for an asic and within a single namespace.
This PR is to make sure that data structures that use SAI Object ID as a key to retrieve data from COUNTERS_DB updated so that a combination of SAI Object ID and port index is used as a key.
- How I did it

Update data structure to use combination SAI Object ID and port index as a key to retrieve data from COUNTERS_DB.
Update Unit-test mock DB in namespaces, to reflect the scenario where SAI Object is same across different namespaces.
Updates done to MIB implementation for the below MIB tables to get data from the right database instances:
- cpfcIfTable
- csqIfQosGroupStatsTable

- How to verify it

- Description for the changelog

unique across the device. The SAI object ID is unique for asic or within a namespace. Modify the existing code to make sure that data structures using SAI OID, it is not used as a key directly. Modify implementation for MIB tables using COUNTER_DB and ASIC_DB so that SAI OID is used to retrieve data only from a single namespace. Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

lgtm-com · 2020-07-04T22:15:48Z

This pull request introduces 4 alerts when merging 7a02ecc into eba1408 - view on LGTM.com

new alerts:

2 for Syntax error
1 for Mismatch in multiple assignment
1 for Wrong number of arguments in a call

abdosi · 2020-07-08T18:10:30Z

src/sonic_ax_impl/mibs/ietf/rfc1213.py

- {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)


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

Fixed the loop.

abdosi · 2020-07-08T18:11:11Z

tests/mock_tables/asic1/asic_db.json

@@ -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",


what trigger this changes ?

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

qiluo-msft · 2020-07-08T18:30:18Z

src/sonic_ax_impl/mibs/__init__.py

@@ -320,7 +320,8 @@ def init_sync_d_queue_tables(db_conn):
 queue_stat_name = queue_table(sai_id)


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

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.

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

Revert FDB MIB namespace related changes. Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

lgtm-com · 2020-08-04T00:10:09Z

This pull request introduces 2 alerts when merging 7657c72 into 90e9f2e - view on LGTM.com

new alerts:

1 for First parameter of a method is not named 'self'
1 for Non-callable called

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

qiluo-msft · 2020-08-05T04:27:32Z

# { Port index : Queue index (SONiC) -> sai_id }

Sorry this is not your fault. The correct comment should be "Port name". #Closed

Refers to: src/sonic_ax_impl/mibs/init.py:336 in ce764d1. [](commit_id = ce764d1, deletion_comment = False)

qiluo-msft · 2020-08-05T04:36:35Z

src/sonic_ax_impl/mibs/__init__.py

@@ -339,8 +339,8 @@ def init_sync_d_queue_tables(db_conn):
 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_queues_map -> {"if_name : queue_index" : sai_oid}


if_name [](start = 27, length = 7)

It should be "port_index"? #Closed

Fixed this, it should be port_index and not port_name.

qiluo-msft · 2020-08-05T04:41:07Z

# ex: { "1:2" : "1000000000023" }

Ethernet0:2 #Closed

Refers to: src/sonic_ax_impl/mibs/init.py:337 in 661ffe9. [](commit_id = 661ffe9, deletion_comment = False)

qiluo-msft · 2020-08-05T05:23:37Z

src/sonic_ax_impl/mibs/vendor/cisco/ciscoSwitchQosMIB.py

@@ -65,6 +65,7 @@ def __init__(self):
 self.mib_oid_list = []

 self.queue_type_map = {}
+ self.if_id_namespace = {}


if_ [](start = 13, length = 3)

The concept in this MIB is port, not interface. Suggest rename if_* to port_* #Closed

I have modified the naming in this file.

Fix variable name used in ciscoSwitchQosMIB and queue related init function. Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

SuvarnaMeenakshi · 2020-08-05T17:57:06Z

# ex: { "1:2" : "1000000000023" }
Ethernet0:2

Refers to: src/sonic_ax_impl/mibs/init.py:337 in 661ffe9. [](commit_id = 661ffe9, deletion_comment = False)

This is port_index, so existing comment is good.

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

SuvarnaMeenakshi · 2020-08-05T19:24:55Z

# { Port index : Queue index (SONiC) -> sai_id }
Sorry this is not your fault. The correct comment should be "Port name".

Refers to: src/sonic_ax_impl/mibs/init.py:336 in ce764d1. [](commit_id = ce764d1, deletion_comment = False)

Fixed as suggested.

SuvarnaMeenakshi · 2020-08-05T19:25:16Z

# ex: { "1:2" : "1000000000023" }
Ethernet0:2
Refers to: src/sonic_ax_impl/mibs/init.py:337 in 661ffe9. [](commit_id = 661ffe9, deletion_comment = False)
This is port_index, so existing comment is good.

The correct comment is port_name, Fixed as suggested.

abdosi · 2020-08-07T22:52:53Z

src/sonic_ax_impl/mibs/vendor/cisco/ciscoSwitchQosMIB.py

- queue_stat = Namespace.dbs_get_all(self.db_conn, mibs.COUNTERS_DB, queue_stat_name, blocking=False)
+ if_index, _ = queue_key.split(':')
+ queue_stat_idx = mibs.queue_key(if_index, queue_stat_name)
+ queue_stat = self.queue_stat_map.get(queue_stat_idx, {})


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

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.

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

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.

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

qiluo-msft · 2020-08-08T04:36:38Z

src/sonic_ax_impl/mibs/vendor/cisco/ciscoSwitchQosMIB.py

+ 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:


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

Otherwise, del the index from self.queue_stat_map #Closed

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

Otherwise, del the index from self.queue_stat_map

Added as per comment.

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

abdosi · 2020-08-11T22:41:35Z

src/sonic_ax_impl/mibs/vendor/cisco/ciscoPfcExtMIB.py

@@ -88,13 +91,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]


is oid_sai_map is being used or this can be removed ?

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

The required data can be retrieved from oid_name_map data structure. Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

qiluo-msft · 2020-08-12T22:06:42Z

Retest this please

…sTable implementation (#138) In multi-asic platform, SAI OID is not unique for the whole device. It is unique for an asic and within a single namespace. This PR is to make sure that data structures that use SAI Object ID as a key to retrieve data from COUNTERS_DB updated so that a combination of SAI Object ID and port index is used as a key. Update data structure to use combination SAI Object ID and port index as a key to retrieve data from COUNTERS_DB. Update Unit-test mock DB in namespaces, to reflect the scenario where SAI Object is same across different namespaces. Updates done to MIB implementation for the below MIB tables to get data from the right database instances: cpfcIfTable csqIfQosGroupStatsTable * Remove usage of oid_sai_map data structure as it is not being used. The required data can be retrieved from oid_name_map data structure. Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

rlhui requested review from abdosi, qiluo-msft and zhenggen-xu and removed request for zhenggen-xu July 7, 2020 19:25

abdosi reviewed Jul 8, 2020

View reviewed changes

qiluo-msft reviewed Jul 8, 2020

View reviewed changes

SuvarnaMeenakshi mentioned this pull request Jul 20, 2020

[Namespace]: Fix to reduce time and fix counters related MIB #139

Closed

SuvarnaMeenakshi added 3 commits July 24, 2020 16:33

Merge remote-tracking branch 'azure/master' into counters

9edee89

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

Modify pfc and qos counters MIB for Namespace related changes.

4cf3c25

Revert FDB MIB namespace related changes. Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

Merge remote-tracking branch 'azure/master' into counters

7657c72

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

SuvarnaMeenakshi added 2 commits August 3, 2020 17:12

Fix LGTM error.

ce764d1

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

Fix code comment.

661ffe9

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

qiluo-msft reviewed Aug 5, 2020

View reviewed changes

Fix code comment.

0246c33

Fix variable name used in ciscoSwitchQosMIB and queue related init function. Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

Fix code comment as per review feedback.

8c16629

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

qiluo-msft previously approved these changes Aug 5, 2020

View reviewed changes

SuvarnaMeenakshi requested a review from abdosi August 6, 2020 17:47

SuvarnaMeenakshi changed the title ~~[Namespace]: Fix code to avoid using SAI object ID as key~~ [Namespace]: Fix SAI_ID key used in cpfcIfTable and csqIfQosGroupStatsTable implementation Aug 7, 2020

abdosi reviewed Aug 7, 2020

View reviewed changes

Fix update_data function.

2d693a8

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

SuvarnaMeenakshi dismissed qiluo-msft’s stale review via 2d693a8 August 8, 2020 01:17

qiluo-msft reviewed Aug 8, 2020

View reviewed changes

Delete queue stats if stats is not present.

b0b7d02

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

qiluo-msft previously approved these changes Aug 8, 2020

View reviewed changes

SuvarnaMeenakshi requested a review from abdosi August 10, 2020 23:22

abdosi reviewed Aug 11, 2020

View reviewed changes

Remove usage of oid_sai_map data structure as it is not being used.

4d8cae2

The required data can be retrieved from oid_name_map data structure. Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

SuvarnaMeenakshi dismissed qiluo-msft’s stale review via 4d8cae2 August 12, 2020 00:18

abdosi approved these changes Aug 12, 2020

View reviewed changes

qiluo-msft approved these changes Aug 12, 2020

View reviewed changes

SuvarnaMeenakshi merged commit 1a2b62a into sonic-net:master Aug 13, 2020

SuvarnaMeenakshi added the Request for 201911 Branch label Aug 18, 2020

abdosi added the Included in 201911 Branch label Sep 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SuvarnaMeenakshi commented Jul 4, 2020 •

edited

Loading

lgtm-com bot commented Jul 4, 2020

abdosi Jul 8, 2020 •

edited

Loading

SuvarnaMeenakshi Jul 31, 2020

abdosi Jul 8, 2020

SuvarnaMeenakshi Jul 31, 2020

qiluo-msft Jul 8, 2020 •

edited

Loading

SuvarnaMeenakshi Jul 31, 2020

lgtm-com bot commented Aug 4, 2020

qiluo-msft commented Aug 5, 2020 •

edited

Loading

qiluo-msft Aug 5, 2020 •

edited

Loading

SuvarnaMeenakshi Aug 5, 2020

qiluo-msft commented Aug 5, 2020 •

edited

Loading

qiluo-msft Aug 5, 2020 •

edited

Loading

SuvarnaMeenakshi Aug 5, 2020

SuvarnaMeenakshi commented Aug 5, 2020

SuvarnaMeenakshi commented Aug 5, 2020

SuvarnaMeenakshi commented Aug 5, 2020

abdosi Aug 7, 2020

SuvarnaMeenakshi Aug 7, 2020

abdosi Aug 7, 2020

SuvarnaMeenakshi Aug 8, 2020

qiluo-msft Aug 8, 2020 •

edited

Loading

SuvarnaMeenakshi Aug 8, 2020

abdosi Aug 11, 2020

SuvarnaMeenakshi Aug 12, 2020

qiluo-msft commented Aug 12, 2020

		@@ -320,7 +320,8 @@ def init_sync_d_queue_tables(db_conn):
		queue_stat_name = queue_table(sai_id)

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

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

Conversation

SuvarnaMeenakshi commented Jul 4, 2020 • edited Loading

- How to verify it

lgtm-com bot commented Jul 4, 2020

abdosi Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Aug 4, 2020

qiluo-msft commented Aug 5, 2020 • edited Loading

qiluo-msft Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft commented Aug 5, 2020 • edited Loading

qiluo-msft Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuvarnaMeenakshi commented Aug 5, 2020

SuvarnaMeenakshi commented Aug 5, 2020

SuvarnaMeenakshi commented Aug 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Aug 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft commented Aug 12, 2020

SuvarnaMeenakshi commented Jul 4, 2020 •

edited

Loading

abdosi Jul 8, 2020 •

edited

Loading

qiluo-msft Jul 8, 2020 •

edited

Loading

qiluo-msft commented Aug 5, 2020 •

edited

Loading

qiluo-msft Aug 5, 2020 •

edited

Loading

qiluo-msft commented Aug 5, 2020 •

edited

Loading

qiluo-msft Aug 5, 2020 •

edited

Loading

qiluo-msft Aug 8, 2020 •

edited

Loading