sonic-net / sonic-py-swsssdk

Python SONiC switch state service sdk
Other
13 stars 86 forks source link

[port_util] Fix issue: port_util.get_vlan_interface_oid_map should not raise exception when DB has not RIF data #117

Closed Junchao-Mellanox closed 2 years ago

Junchao-Mellanox commented 2 years ago

Why I did this?

port_util.get_vlan_interface_oid_map would raise exception when there is no RIF configured, and it would causes snmpagent not work. This PR is to fix it.

#012Traceback (most recent call last):
#012File "/usr/local/lib/python3.7/dist-packages/ax_interface/mib.py", line 37, in start
#012    self.reinit_data()#012  File "/usr/local/lib/python3.7/dist-packages/sonic_ax_impl/mibs/ietf/rfc1213.py", line 233, in reinit_data
#012    self.vlan_oid_name_map = Namespace.get_sync_d_from_all_namespace(mibs.init_sync_d_vlan_tables, self.db_conn)
#012  File "/usr/local/lib/python3.7/dist-packages/sonic_ax_impl/mibs/__init__.py", line 651, in get_sync_d_from_all_namespace
#012    ns_tuple = per_namespace_func(db_conn)
#012  File "/usr/local/lib/python3.7/dist-packages/sonic_ax_impl/mibs/__init__.py", line 341, in init_sync_d_vlan_tables
#012    vlan_name_map = port_util.get_vlan_interface_oid_map(db_conn)
#012  File "/usr/local/lib/python3.7/dist-packages/swsssdk/port_util.py", line 167, in get_vlan_interface_oid_map
#012    rif_name_map = db.get_all('COUNTERS_DB', 'COUNTERS_RIF_NAME_MAP', blocking=True)
#012  File "/usr/lib/python3/dist-packages/swsscommon/swsscommon.py", line 1751, in get_all
#012    return dict(super(SonicV2Connector, self).get_all(db_name, _hash, blocking))
#012  File "/usr/lib/python3/dist-packages/swsscommon/swsscommon.py", line 1708, in get_all
#012    return _swsscommon.SonicV2Connector_Native_get_all(self, db_name, _hash, blocking)
#012RuntimeError: Key '{COUNTERS_RIF_NAME_MAP}' unavailable in database '{COUNTERS_DB}'

How I did this: check redis key exists first before calling get_all.

liat-grozovik commented 2 years ago

@qiluo-msft could you please help to signoff? Due to permission issue we cannot add label. Can you please help to add required for 202111 label?

Junchao-Mellanox commented 2 years ago

Hi @qiluo-msft , could you please review and sign-off?

dprital commented 2 years ago

Hi @qiluo-msft - Can you please direct this PR to the relevant maintainer ?

liat-grozovik commented 2 years ago

@qiluo-msft due to limited permission please add a label of bug as well as requested for 202111

qiluo-msft commented 2 years ago

def get_vlan_interface_oid_map(db):

you can add a new parameter blocking with default value True.

Change the internal blocking accordingly. No need to check exists.

The benefit is to keep old behavior, and allow client code to change behavior case by case.


In reply to: 1067627217


Refers to: src/swsssdk/port_util.py:162 in 9beb381. [](commit_id = 9beb381db7c2910b407252287bd2820f4f4b5e47, deletion_comment = False)

Junchao-Mellanox commented 2 years ago

def get_vlan_interface_oid_map(db):

you can add a new parameter blocking with default value True.

Change the internal blocking accordingly. No need to check exists.

The benefit is to keep old behavior, and allow client code to change behavior case by case.

Refers to: src/swsssdk/port_util.py:162 in 9beb381. [](commit_id = 9beb381, deletion_comment = False)

Hi @qiluo-msft , I would like to confirm your comment here:

  1. add a parameter to get_vlan_interface_oid_map
    def get_vlan_interface_oid_map(db, blocking=True):
    ...
  2. and change snmpagent code to call it like:
    get_vlan_interface_oid_map(db, blocking=False)

Correct?

SuvarnaMeenakshi commented 2 years ago

def get_vlan_interface_oid_map(db):

you can add a new parameter blocking with default value True. Change the internal blocking accordingly. No need to check exists. The benefit is to keep old behavior, and allow client code to change behavior case by case. Refers to: src/swsssdk/port_util.py:162 in 9beb381. [](commit_id = 9beb381, deletion_comment = False)

Hi @qiluo-msft , I would like to confirm your comment here:

  1. add a parameter to get_vlan_interface_oid_map
def get_vlan_interface_oid_map(db, blocking=True):
    ...
  1. and change snmpagent code to call it like:
get_vlan_interface_oid_map(db, blocking=False)

Correct?

--- a/src/swsssdk/port_util.py
+++ b/src/swsssdk/port_util.py
@@ -164,13 +164,13 @@ def get_rif_port_map(db):

     return rif_port_oid_map

-def get_vlan_interface_oid_map(db):
+def get_vlan_interface_oid_map(db, blocking=True):
     """
         Get Vlan Interface names and sai oids
     """
     db.connect('COUNTERS_DB')
-    rif_name_map = db.get_all('COUNTERS_DB', 'COUNTERS_RIF_NAME_MAP', blocking=True)
-    rif_type_name_map = db.get_all('COUNTERS_DB', 'COUNTERS_RIF_TYPE_MAP', blocking=True)
+    rif_name_map = db.get_all('COUNTERS_DB', 'COUNTERS_RIF_NAME_MAP', blocking=blocking)
+    rif_type_name_map = db.get_all('COUNTERS_DB', 'COUNTERS_RIF_TYPE_MAP', blocking=blocking)
SuvarnaMeenakshi commented 2 years ago

@mlok-nokia - fyi

SuvarnaMeenakshi commented 2 years ago

@Junchao-Mellanox created snmp PR https://github.com/Azure/sonic-snmpagent/pull/246, depends on this PR.

qiluo-msft commented 2 years ago

Yes.


In reply to: 1068646058

Junchao-Mellanox commented 2 years ago

I removed previous added unit test case as this change is hard to be covered by unit test.

Junchao-Mellanox commented 2 years ago

@qiluo-msft could you please review and sign off?

Junchao-Mellanox commented 2 years ago

/azpw run Azure.sonic-py-swsssdk

mssonicbld commented 2 years ago

/AzurePipelines run Azure.sonic-py-swsssdk

azure-pipelines[bot] commented 2 years ago
Commenter does not have sufficient privileges for PR 117 in repo Azure/sonic-py-swsssdk
Junchao-Mellanox commented 2 years ago

Hi @qiluo-msft , I run the build locally and cannot reproduce the build failure. Could you please help take a look?

SuvarnaMeenakshi commented 2 years ago

/azp run

azure-pipelines[bot] commented 2 years ago
Azure Pipelines successfully started running 1 pipeline(s).
qiluo-msft commented 2 years ago

@Junchao-Mellanox We recently add coverage >= 50% enforcement to most of the repos. Could you add a unit test to coverage the missing lines? https://dev.azure.com/mssonic/build/_build/results?buildId=82325&view=codecoverage-tab

Junchao-Mellanox commented 2 years ago

Hi @qiluo-msft , could you please review and sign off?

dprital commented 2 years ago

@qiluo-msft - There is no 202111 branch for this repository. Is there a plan to open one ?

SuvarnaMeenakshi commented 2 years ago

@vperumal @anamehra @VenkatCisco fyi