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 interfaces counters in InterfacesMIB RFC 2863 #141

Merged
merged 12 commits into from
Jul 11, 2020
31 changes: 30 additions & 1 deletion src/sonic_ax_impl/mibs/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,31 @@ def mgmt_if_entry_table_state_db(if_name):

return b'MGMT_PORT_TABLE|' + if_name

def get_sai_id_key(namespace, sai_id):
"""
inputs:
namespace - string
sai id - bytes
Return type:
bytes
Return value: namespace:sai id or sai id
"""
if namespace != '':
sai_id_key = namespace + ':' + sai_id.decode()
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 11, 2020

Choose a reason for hiding this comment

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

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

return namespace.encode() + b':' + sai_id
This will be easier and shorter. #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.

updated as per comment.

return sai_id_key.encode()
else:
return sai_id

def split_sai_id_key(sai_id_key):
"""
Input - bytes
Return namespace string and sai id in byte string.
"""
if b':' in sai_id_key:
namespace, sai_id = sai_id_key.split(b':')
return namespace.decode(), sai_id
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 11, 2020

Choose a reason for hiding this comment

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

you can split first and check result len().
This will scan the string only once. #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.

Updated as per comment.

else:
return '', sai_id

def config(**kwargs):
global redis_kwargs
Expand Down Expand Up @@ -221,7 +246,11 @@ 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 \
# As sai_id is not unique in multi-asic platform, concatenate it with
# namespace to get a unique key. Assuming that ':' is not present in namespace
# string or in sai id.
# sai_id_key = namespace : sai_id
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 11, 2020

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment.

if_id_map = {get_sai_id_key(db_conn.namespace, sai_id): if_name for sai_id, if_name in if_id_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()))}
logger.debug("Port name map:\n" + pprint.pformat(if_name_map, indent=2))
Expand Down
14 changes: 8 additions & 6 deletions src/sonic_ax_impl/mibs/ietf/rfc2863.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

@unique
class DbTables32(int, Enum):
Expand Down Expand Up @@ -98,10 +99,12 @@ 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 = get_index_from_str(self.if_id_map[sai_id_key].decode())
for db in self.db_conn:
if db.namespace == namespace:
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 11, 2020

Choose a reason for hiding this comment

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

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

Propose 2 alternative solutions:

  1. create a map (namespace -> db)
  2. use db_index instread of namespace in get_sai_id_key #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.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

self.if_counters[if_idx] = db.get_all(mibs.COUNTERS_DB, mibs.counter_table(sai_id), blocking=True)
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 11, 2020

Choose a reason for hiding this comment

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

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


def get_next(self, sub_id):
"""
Expand Down Expand Up @@ -186,11 +189,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
1 change: 1 addition & 0 deletions tests/namespace/test_fdb.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
class TestSonicMIB(TestCase):
@classmethod
def setUpClass(cls):
cls.skipTest(cls, "Namespace not implemented")
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 11, 2020

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated comment with TODO

tests.mock_tables.dbconnector.load_namespace_config()
importlib.reload(rfc4363)

Expand Down
1 change: 1 addition & 0 deletions tests/namespace/test_interfaces.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
class TestGetNextPDU(TestCase):
@classmethod
def setUpClass(cls):
cls.skipTest(cls, "Namespace not implemented")
tests.mock_tables.dbconnector.load_namespace_config()
importlib.reload(rfc1213)
cls.lut = MIBTable(rfc1213.InterfacesMIB)
Expand Down
1 change: 1 addition & 0 deletions tests/namespace/test_pfc.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
class TestPfcPortCounters(TestCase):
@classmethod
def setUpClass(cls):
cls.skipTest(cls, "Namespace not implemented")
tests.mock_tables.dbconnector.load_namespace_config()
importlib.reload(ciscoPfcExtMIB)

Expand Down