[Namespace] Fix interfaces counters in InterfacesMIB RFC 2863 #141

SuvarnaMeenakshi · 2020-07-10T23:06:12Z

- 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 fix InterfacesMIB rfc2863 implementation to make sure that interfaces counters is keyed based on interface index and not SAI OID.

- How I did it

Add namespace parameter in key ; instead of using SAI OID as key , the key will be namespace:sai_oid for multi-asic platform.
In InterfacesMIB ; update code to use interface index as key in interface counter dict.
Skip the unit-test cases for other MIBs where SAI OID is used, until the implementation is updated for multiple namespaces.
FDB/PFC and InterfacesMIB in rfc1213.

- How to verify it
Tested InterfacesMIB snmp query on multi-asic platform.

- Description for the changelog

multi-asic platforms. In multi-asic platforms SAI oid is not unique across namespace/asic. Use interface index as the key instead of using SAI oid. Signed-off-by: SuvarnaMeenakshi <[email protected]>

to reduce time taken. Signed-off-by: SuvarnaMeenakshi <[email protected]> (cherry picked from commit 16f36d8)

platform. Signed-off-by: SuvarnaMeenakshi <[email protected]>

Do not run namespace unit-test for MIBs using SAI id as key. Signed-off-by: SuvarnaMeenakshi <[email protected]>

Signed-off-by: SuvarnaMeenakshi <[email protected]>

qiluo-msft · 2020-07-11T01:32:09Z

src/sonic_ax_impl/mibs/ietf/rfc2863.py

@@ -4,6 +4,7 @@
 from sonic_ax_impl import mibs
 from ax_interface.mib import MIBMeta, MIBUpdater, ValueType, SubtreeMIBEntry, OverlayAdpaterMIBEntry, OidMIBEntry
 from sonic_ax_impl.mibs import Namespace
+from swsssdk.port_util import  get_index_from_str


[](start = 30, length = 1)

One extra blank #Closed

qiluo-msft · 2020-07-11T01:37:44Z

src/sonic_ax_impl/mibs/__init__.py

@@ -160,6 +160,22 @@ def mgmt_if_entry_table_state_db(if_name):

    return b'MGMT_PORT_TABLE|' + if_name

+def get_sai_id_key(namespace, sai_id):
+    sai_id = sai_id.decode()


decode [](start = 20, length = 6)

decode and encode will applied to single ASIC and this could be optimized. #Closed

qiluo-msft · 2020-07-11T01:38:12Z

src/sonic_ax_impl/mibs/__init__.py

+def get_sai_id_key(namespace, sai_id):
+    sai_id = sai_id.decode()
+    if namespace != '':
+        key = namespace + ':' + sai_id


namespace + ':' + sai_id [](start = 14, length = 24)

why not just concat b':' without decoding? #Closed

Modified as per comment.

qiluo-msft · 2020-07-11T01:39:20Z

src/sonic_ax_impl/mibs/__init__.py

+def split_sai_id_key(sai_id_key):
+    sai_id_key = sai_id_key.decode()
+    if ':' in sai_id_key:
+        namespace, sai_id = sai_id_key.split(':')


split(':') [](start = 39, length = 10)

Why not just split(b':') without decoding? #Closed

Modified as per comment.

qiluo-msft · 2020-07-11T01:40:59Z

src/sonic_ax_impl/mibs/__init__.py

@@ -221,7 +237,8 @@ def init_sync_d_interface_tables(db_conn):
    if_name_map = {if_name: sai_id for if_name, sai_id in if_name_map.items() if \
                   (re.match(port_util.SONIC_ETHERNET_RE_PATTERN, if_name.decode()) or \
                    re.match(port_util.SONIC_ETHERNET_BP_RE_PATTERN, if_name.decode()))}
-    if_id_map = {sai_id: if_name for sai_id, if_name in if_id_map.items() if \
+    # sai_id_key = namespace : sai_id


: [](start = 29, length = 1)

This is a new assumption that ':' will not appear in name space or sai_id. Please add code comment. #Closed

How about using '|' which is widely used in Redis keys

In reply to: 453139692 [](ancestors = 453139692)

Added comment.

qiluo-msft · 2020-07-11T01:43:58Z

src/sonic_ax_impl/mibs/ietf/rfc2863.py

+            if_idx = get_index_from_str(self.if_id_map[sai_id_key].decode())
+            for db in self.db_conn:
+                if db.namespace == namespace:
+                    self.if_counters[if_idx] = db.get_all(mibs.COUNTERS_DB, mibs.counter_table(sai_id), blocking=True)


self.if_counters[if_idx] = db.get_all(mibs.COUNTERS_DB, mibs.counter_table(sai_id), blocking=True) [](start = 20, length = 98)

break after this line? #Closed

qiluo-msft · 2020-07-11T01:46:58Z

    db_conn= []

Add one blank before = #Closed

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

qiluo-msft · 2020-07-11T01:51:02Z

src/sonic_ax_impl/mibs/ietf/rfc2863.py

+            namespace, sai_id = mibs.split_sai_id_key(sai_id_key)
+            if_idx = get_index_from_str(self.if_id_map[sai_id_key].decode())
+            for db in self.db_conn:
+                if db.namespace == namespace:


This is not efficient, because you search self.db_conn too many times.

Propose 2 alternative solutions:

create a map (namespace -> db)

use db_index instread of namespace in get_sai_id_key #Closed

Added a map, db_index cannot be obtained directly in this function and will have to be passed as an additional argument.

@qiluo-msft thanks for pointing out.
we discussed to have map but to avoid new map went with this approach of checking namespace and break considering loop is small.

qiluo-msft · 2020-07-11T01:52:00Z

tests/namespace/test_fdb.py

@@ -23,6 +23,7 @@
 class TestSonicMIB(TestCase):
    @classmethod
    def setUpClass(cls):
+        cls.skipTest(cls, "Namespace not implemented")


skipTest [](start = 12, length = 8)

What is the meaning of "Namespace not implemented"?
Why you skip test? #Pending

For multi-asic platform, the change is made to if_id_map key to store namespace and sai id together.
with this change, MIBS using this key have to be updated to use the new type of key.
Instead of making this change on all MIBS together in same PR, will make change in other PRs.
So, for multi-asic platform, the unit-tests for these MIB implementations are skipped.

@SuvarnaMeenakshi better we should add similar comment in test case itself as TODO so that is easy to understand and track

Updated comment with TODO

Signed-off-by: SuvarnaMeenakshi <[email protected]>

qiluo-msft · 2020-07-11T02:24:33Z

src/sonic_ax_impl/mibs/__init__.py

+    Return value: namespace:sai id or sai id
+    """
+    if namespace != '':
+        sai_id_key =  namespace + ':' + sai_id.decode()


namespace + ':' + sai_id.decode() [](start = 22, length = 33)

return namespace.encode() + b':' + sai_id
This will be easier and shorter. #Closed

updated as per comment.

Add a new dict of namespace:db connector for easier access. Signed-off-by: SuvarnaMeenakshi <[email protected]>

Signed-off-by: SuvarnaMeenakshi <[email protected]>

qiluo-msft · 2020-07-11T05:30:26Z

src/sonic_ax_impl/mibs/__init__.py

+    """
+    if b':' in sai_id_key:
+        namespace, sai_id = sai_id_key.split(b':')
+        return namespace.decode(), sai_id


you can split first and check result len().
This will scan the string only once. #Closed

Updated as per comment.

Signed-off-by: SuvarnaMeenakshi <[email protected]>

lgtm-com · 2020-07-11T16:44:57Z

This pull request introduces 2 alerts when merging 5a54bdb into 1d210d9 - view on LGTM.com

new alerts:

1 for First parameter of a method is not named 'self'
1 for Non-iterable used in for loop

qiluo-msft · 2020-07-11T17:05:15Z

@SuvarnaMeenakshi Please fix 2 new alerts

* [InterfacesMIB]: Fix counters in InterfacesMIB for multi-asic platforms. In multi-asic platforms SAI oid is not unique across namespace/asic. Use interface index as the key instead of using SAI oid. Signed-off-by: SuvarnaMeenakshi <[email protected]> * [Namespace]: Remove key exists check in dbs_get_all to reduce time taken. Signed-off-by: SuvarnaMeenakshi <[email protected]> (cherry picked from commit 16f36d8) * Change the key used in if_id_map to support multi-asic platform. Signed-off-by: SuvarnaMeenakshi <[email protected]> * Fix interface counters only for rfc2863. Do not run namespace unit-test for MIBs using SAI id as key. Signed-off-by: SuvarnaMeenakshi <[email protected]> * Fix getting interface index in InterfacesMIB Signed-off-by: SuvarnaMeenakshi <[email protected]> * Remove debug print. Signed-off-by: SuvarnaMeenakshi <[email protected]> * Remove db connect that is not required. Signed-off-by: SuvarnaMeenakshi <[email protected]> * Modify as per review comments. Signed-off-by: SuvarnaMeenakshi <[email protected]> * Fix as per review comment. Add a new dict of namespace:db connector for easier access. Signed-off-by: SuvarnaMeenakshi <[email protected]> * Fix as per review comment. Signed-off-by: SuvarnaMeenakshi <[email protected]> * Modify based on review comment. Signed-off-by: SuvarnaMeenakshi <[email protected]> Co-authored-by: abdosi <[email protected]>

SuvarnaMeenakshi added 7 commits July 9, 2020 23:55

[InterfacesMIB]: Fix counters in InterfacesMIB for

815fc67

multi-asic platforms. In multi-asic platforms SAI oid is not unique across namespace/asic. Use interface index as the key instead of using SAI oid. Signed-off-by: SuvarnaMeenakshi <[email protected]>

[Namespace]: Remove key exists check in dbs_get_all

216ef86

to reduce time taken. Signed-off-by: SuvarnaMeenakshi <[email protected]> (cherry picked from commit 16f36d8)

Change the key used in if_id_map to support multi-asic

7baa245

platform. Signed-off-by: SuvarnaMeenakshi <[email protected]>

Fix interface counters only for rfc2863.

964a353

Do not run namespace unit-test for MIBs using SAI id as key. Signed-off-by: SuvarnaMeenakshi <[email protected]>

Fix getting interface index in InterfacesMIB

f8c6ade

Signed-off-by: SuvarnaMeenakshi <[email protected]>

Remove debug print.

6772fa1

Signed-off-by: SuvarnaMeenakshi <[email protected]>

Remove db connect that is not required.

14353e2

Signed-off-by: SuvarnaMeenakshi <[email protected]>

SuvarnaMeenakshi requested review from qiluo-msft and abdosi July 10, 2020 23:07

abdosi previously approved these changes Jul 10, 2020

View reviewed changes

qiluo-msft reviewed Jul 11, 2020

View reviewed changes

Modify as per review comments.

5f49791

Signed-off-by: SuvarnaMeenakshi <[email protected]>

SuvarnaMeenakshi dismissed abdosi’s stale review via 5f49791 July 11, 2020 02:11

qiluo-msft reviewed Jul 11, 2020

View reviewed changes

SuvarnaMeenakshi added 2 commits July 10, 2020 19:30

Fix as per review comment.

4ed96fd

Add a new dict of namespace:db connector for easier access. Signed-off-by: SuvarnaMeenakshi <[email protected]>

Fix as per review comment.

b91863b

Signed-off-by: SuvarnaMeenakshi <[email protected]>

qiluo-msft reviewed Jul 11, 2020

View reviewed changes

Modify based on review comment.

ff5d7b8

Signed-off-by: SuvarnaMeenakshi <[email protected]>

qiluo-msft previously approved these changes Jul 11, 2020

View reviewed changes

Merge branch 'master' into rfc2863_counters

5a54bdb

abdosi dismissed qiluo-msft’s stale review via 5a54bdb July 11, 2020 16:36

abdosi approved these changes Jul 11, 2020

View reviewed changes

abdosi merged commit a26b556 into sonic-net:master Jul 11, 2020

abdosi mentioned this pull request Jul 11, 2020

Fix LGTM reported alert of PR#141 #142

Merged

abdosi added Request for 201911 Branch Included in 201911 Branch labels Jul 11, 2020

abdosi mentioned this pull request Jul 11, 2020

[201911]Cherry-pick Master PR's to 201911 #143

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Namespace] Fix interfaces counters in InterfacesMIB RFC 2863 #141

[Namespace] Fix interfaces counters in InterfacesMIB RFC 2863 #141

SuvarnaMeenakshi commented Jul 10, 2020

qiluo-msft Jul 11, 2020 •

edited

Loading

SuvarnaMeenakshi Jul 11, 2020

qiluo-msft Jul 11, 2020 •

edited

Loading

SuvarnaMeenakshi Jul 11, 2020

qiluo-msft Jul 11, 2020 •

edited

Loading

SuvarnaMeenakshi Jul 11, 2020

qiluo-msft Jul 11, 2020 •

edited

Loading

SuvarnaMeenakshi Jul 11, 2020

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020

SuvarnaMeenakshi Jul 11, 2020

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft commented Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

SuvarnaMeenakshi Jul 11, 2020

abdosi Jul 11, 2020

qiluo-msft Jul 11, 2020 •

edited

Loading

SuvarnaMeenakshi Jul 11, 2020

abdosi Jul 11, 2020

SuvarnaMeenakshi Jul 11, 2020

qiluo-msft Jul 11, 2020 •

edited

Loading

SuvarnaMeenakshi Jul 11, 2020

qiluo-msft Jul 11, 2020 •

edited

Loading

SuvarnaMeenakshi Jul 11, 2020

lgtm-com bot commented Jul 11, 2020

qiluo-msft commented Jul 11, 2020

[Namespace] Fix interfaces counters in InterfacesMIB RFC 2863 #141

[Namespace] Fix interfaces counters in InterfacesMIB RFC 2863 #141

Conversation

SuvarnaMeenakshi commented Jul 10, 2020

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

qiluo-msft commented Jul 11, 2020 • edited Loading

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 11, 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 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jul 11, 2020

qiluo-msft commented Jul 11, 2020

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft commented Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading