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

Fix undefined variable and warning message #134

Merged
merged 8 commits into from
Jun 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions src/sonic_ax_impl/mibs/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ def init_db():
"""
# SyncD database connector. THIS MUST BE INITIALIZED ON A PER-THREAD BASIS.
# Redis PubSub objects (such as those within swsssdk) are NOT thread-safe.
db_conn = SonicV2Connector(**redis_kwargs)
db_conn = SonicV2Connector(**redis_kwargs)

return db_conn

Expand Down Expand Up @@ -389,6 +389,13 @@ def get_transceiver_sensor_sub_id(ifindex, sensor):
transceiver_oid, = get_transceiver_sub_id(ifindex)
return (transceiver_oid + SENSOR_PART_ID_MAP[sensor], )

def get_redis_pubsub(db_conn, db_name, pattern):
redis_client = db_conn.get_redis_client(db_name)
db = db_conn.get_dbid(db_name)
pubsub = redis_client.pubsub()
pubsub.psubscribe("__keyspace@{}__:{}".format(db, pattern))
return pubsub

class RedisOidTreeUpdater(MIBUpdater):
def __init__(self, prefix_str):
super().__init__()
Expand Down Expand Up @@ -419,7 +426,7 @@ def update_data(self):
self.oid_list = []
self.oid_map = {}

keys = Namespace.dbs_keys(self.db_conn, SNMP_OVERLAY_DB, self.prefix_str + '*')
keys = Namespace.dbs_keys(self.db_conn, SNMP_OVERLAY_DB, self.prefix_str + '*')
# TODO: fix db_conn.keys to return empty list instead of None if there is no match
if keys is None:
keys = []
Expand All @@ -428,7 +435,7 @@ def update_data(self):
key = key.decode()
oid = oid2tuple(key, dot_prefix=False)
self.oid_list.append(oid)
value = Namespace.dbs_get_all(self.db_conn, SNMP_OVERLAY_DB, key)
value = Namespace.dbs_get_all(self.db_conn, SNMP_OVERLAY_DB, key)
if value[b'type'] in [b'COUNTER_32', b'COUNTER_64']:
self.oid_map[oid] = int(value[b'data'])
else:
Expand Down Expand Up @@ -587,6 +594,6 @@ def dbs_get_bridge_port_map(dbs, db_name):
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.keys('ASIC_DB', "ASIC_STATE:SAI_OBJECT_TYPE_VLAN:" + bvid)
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)
33 changes: 13 additions & 20 deletions src/sonic_ax_impl/mibs/ieee802_1ab.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ class ManAddrConst:
"""
man_addr_oid = (1, 3, 6, 1, 2, 1, 2, 2, 1, 1)


def poll_lldp_entry_updates(pubsub):
ret = None, None, None
msg = pubsub.get_message()
Expand Down Expand Up @@ -157,7 +156,7 @@ def __init__(self):
# cache of port data
# { if_name -> { 'key': 'value' } }
self.loc_port_data = {}
self.pubsub = [None] * len(self.db_conn)
self.pubsub = [None] * len(self.db_conn)

def reinit_data(self):
"""
Expand Down Expand Up @@ -222,16 +221,10 @@ def get_next(self, sub_id):
return None
return self.if_range[right]

def _update_per_namespace_data(self, db_conn, pubsub):
def _update_per_namespace_data(self, pubsub):
"""
Listen to updates in APP DB, update local cache
"""
if not pubsub:
redis_client = db_conn.get_redis_client(db_conn.APPL_DB)
db = db_conn.get_dbid(db_conn.APPL_DB)
pubsub = redis_client.pubsub()
pubsub.psubscribe("__keyspace@{}__:{}".format(db, mibs.lldp_entry_table(b'*')))

while True:
data, interface, if_id = poll_lldp_entry_updates(pubsub)

Expand All @@ -243,7 +236,10 @@ def _update_per_namespace_data(self, db_conn, pubsub):

def update_data(self):
for i in range(len(self.db_conn)):
self._update_per_namespace_data(self.db_conn[i], self.pubsub[i])
if not self.pubsub[i]:
pattern = mibs.lldp_entry_table(b'*')
self.pubsub[i] = mibs.get_redis_pubsub(self.db_conn[i], self.db_conn[i].APPL_DB, pattern)
self._update_per_namespace_data(self.pubsub[i])

def local_port_num(self, sub_id):
if len(sub_id) == 0:
Expand Down Expand Up @@ -307,7 +303,7 @@ def reinit_data(self):
self.mgmt_ip_str = None

# establish connection to application database.
self.db_conn.connect(mibs.APPL_DB)
self.db_conn.connect(mibs.APPL_DB)
mgmt_ip_bytes = self.db_conn.get(mibs.APPL_DB, mibs.LOC_CHASSIS_TABLE, b'lldp_loc_man_addr')

if not mgmt_ip_bytes:
Expand Down Expand Up @@ -488,7 +484,7 @@ class LLDPRemManAddrUpdater(MIBUpdater):
def __init__(self):
super().__init__()

self.db_conn = Namespace.init_namespace_dbs()
self.db_conn = Namespace.init_namespace_dbs()
# establish connection to application database.
Namespace.connect_all_dbs(self.db_conn, mibs.APPL_DB)
self.if_range = []
Expand Down Expand Up @@ -536,16 +532,10 @@ def update_rem_if_mgmt(self, if_oid, if_name):
return
self.if_range.sort()

def _update_per_namespace_data(self, db_conn, pubsub):
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 18, 2020

Choose a reason for hiding this comment

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

_update_per_namespace_data [](start = 8, length = 26)

Original code seems better. Why change? #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.

Before namespace changes, self.pubsub was getting updated in update_data(). After namespace change, self.pubsub array was not updated as we pass the instance. By passing the array index, we can ensure that the array is directly getting udpated. So this fix is to make sure the code is as it was before namespace 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.

Updated description with the issue and why this fix is required.

Copy link
Contributor

@qiluo-msft qiluo-msft Jun 25, 2020

Choose a reason for hiding this comment

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

I see the bug in #131.

To make code cleaner, could you try add

db_conn = self.db_conn[inst]
pubsub = self.pubsub[inst]
if not pubsub:
    ...
    pubsub = redis_client.pubsub()
    self.pubsub[inst] = pubsub


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

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 as per comments.

def _update_per_namespace_data(self, pubsub):
"""
Listen to updates in APP DB, update local cache
"""
if not pubsub:
redis_client = db_conn.get_redis_client(db_conn.APPL_DB)
db = db_conn.get_dbid(db_conn.APPL_DB)
pubsub = redis_client.pubsub()
pubsub.psubscribe("__keyspace@{}__:{}".format(db, mibs.lldp_entry_table(b'*')))

while True:
data, interface, if_index = poll_lldp_entry_updates(pubsub)

Expand All @@ -561,7 +551,10 @@ def _update_per_namespace_data(self, db_conn, pubsub):

def update_data(self):
for i in range(len(self.db_conn)):
self._update_per_namespace_data(self.db_conn[i], self.pubsub[i])
if not self.pubsub[i]:
pattern = mibs.lldp_entry_table(b'*')
self.pubsub[i] = mibs.get_redis_pubsub(self.db_conn[i], self.db_conn[i].APPL_DB, pattern)
self._update_per_namespace_data(self.pubsub[i])


def reinit_data(self):
Expand Down
15 changes: 7 additions & 8 deletions src/sonic_ax_impl/mibs/ietf/rfc2737.py
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ def reinit_data(self):
interface = transceiver_entry.split(mibs.TABLE_NAME_SEPARATOR_VBAR)[-1]
self._update_transceiver_cache(interface)

def _update_per_namespace_data(self, statedb, pubsub):
def _update_per_namespace_data(self, pubsub):
"""
Update cache.
Here we listen to changes in STATE_DB TRANSCEIVER_INFO table
Expand All @@ -190,12 +190,6 @@ def _update_per_namespace_data(self, statedb, pubsub):

# This code is not executed in unit test, since mockredis
# does not support pubsub
if not pubsub:
redis_client = statedb.get_redis_client(statedb.STATE_DB)
db = statedb.get_dbid(statedb.STATE_DB)
pubsub = redis_client.pubsub()
pubsub.psubscribe("__keyspace@{}__:{}".format(db, self.TRANSCEIVER_KEY_PATTERN))

while True:
msg = pubsub.get_message()

Expand Down Expand Up @@ -233,8 +227,13 @@ def _update_per_namespace_data(self, statedb, pubsub):
self.physical_entities.remove(sub_id)

def update_data(self):
# This code is not executed in unit test, since mockredis
# does not support pubsub
for i in range(len(self.statedb)):
self._update_per_namespace_data(self.statedb[i], self.pubsub[i])
if not self.pubsub[i]:
pattern = self.TRANSCEIVER_KEY_PATTERN
self.pubsub[i] = mibs.get_redis_pubsub(self.statedb[i], self.statedb[i].STATE_DB, pattern)
self._update_per_namespace_data(self.pubsub[i])

def _update_transceiver_cache(self, interface):
"""
Expand Down
4 changes: 2 additions & 2 deletions src/sonic_ax_impl/mibs/ietf/rfc3433.py
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ def __init__(self):

super().__init__()

self.statedb = Namespace.init_namespace_dbs()
self.statedb = Namespace.init_namespace_dbs()
Namespace.connect_all_dbs(self.statedb, mibs.STATE_DB)

# list of available sub OIDs
Expand Down Expand Up @@ -378,7 +378,7 @@ def get_ent_physical_sensor_precision(self, sub_id):
"""
Get sensor precision value based on sub OID
:param sub_id: sub ID of the sensor
:return: sensor precision in range (-8, 9)
:return: sensor precision in range (-8, 9)
or None if sub_id not in the cache.
"""

Expand Down
2 changes: 1 addition & 1 deletion src/sonic_ax_impl/mibs/vendor/cisco/ciscoPfcExtMIB.py
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ def requests_per_priority(self, sub_id):
"""
:param sub_id: The 0-based sub-identifier query.
:return: the counter for the respective sub_id/table.
"""
"""
port_oid = ''
queue_index = ''
try:
Expand Down
25 changes: 25 additions & 0 deletions tests/mock_tables/asic0/asic_db.json
Original file line number Diff line number Diff line change
@@ -1,2 +1,27 @@
{
"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:04\",\"switch_id\":\"oid:0x21000000000000\",\"vlan\":\"1000\"}": {
"SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID": "oid:0x3a000000000608",
"SAI_FDB_ENTRY_ATTR_TYPE": "SAI_FDB_ENTRY_TYPE_DYNAMIC"
},
"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:06\",\"switch_id\":\"oid:0x21000000000000\",\"vlan\":\"1000\"}": {
"SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID": "oid:0x3a000000000616",
"SAI_FDB_ENTRY_ATTR_TYPE": "SAI_FDB_ENTRY_TYPE_DYNAMIC"
},
"ASIC_STATE:SAI_OBJECT_TYPE_FDB_ENTRY:{\"bvid\":\"oid:0x26000000000016\",\"mac\":\"7C:FE:90:80:9F:08\",\"switch_id\":\"oid:0x21000000000000\"}": {
"SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID": "oid:0x3a000000000620",
"SAI_FDB_ENTRY_ATTR_TYPE": "SAI_FDB_ENTRY_TYPE_DYNAMIC"
},
"ASIC_STATE:SAI_OBJECT_TYPE_VLAN:oid:0x26000000000016": {
"SAI_VLAN_ATTR_VLAN_ID": "1000"
},
"ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT:oid:0x3a000000000608": {
"SAI_BRIDGE_PORT_ATTR_TYPE": "SAI_BRIDGE_PORT_TYPE_PORT",
"SAI_BRIDGE_PORT_ATTR_PORT_ID": "oid:0x1000000000003",
"SAI_BRIDGE_PORT_ATTR_ADMIN_STATE": "true"
},
"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:0x1000000000005",
"SAI_BRIDGE_PORT_ATTR_ADMIN_STATE": "true"
}
}
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 18, 2020

Choose a reason for hiding this comment

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

If not related to the PR subject, could you submit another PR just for test cases? #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.

This test case is to test the code path where the undefined variable is fixed in this PR.

9 changes: 9 additions & 0 deletions tests/mock_tables/asic1/asic_db.json
Original file line number Diff line number Diff line change
@@ -1,2 +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_TYPE": "SAI_FDB_ENTRY_TYPE_DYNAMIC"
},
"ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT:oid:0x3a000000000620": {
"SAI_BRIDGE_PORT_ATTR_TYPE": "SAI_BRIDGE_PORT_TYPE_PORT",
"SAI_BRIDGE_PORT_ATTR_PORT_ID": "oid:0x1000000000010",
"SAI_BRIDGE_PORT_ATTR_ADMIN_STATE": "true"
}
}
2 changes: 2 additions & 0 deletions tests/mock_tables/global_db/asic_db.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{
}
Loading