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

Improve MIBUpdater to re-connect DBConnector when re-init data. #290

Merged
merged 10 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from 4 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
8 changes: 8 additions & 0 deletions src/ax_interface/mib.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ def __init__(self):
self.frequency = DEFAULT_UPDATE_FREQUENCY
self.reinit_rate = DEFAULT_REINIT_RATE // DEFAULT_UPDATE_FREQUENCY
self.update_counter = self.reinit_rate + 1 # reinit_data when init
self.redis_exception_happen = False

async def start(self):
# Run the update while we are allowed
Expand All @@ -41,6 +42,13 @@ async def start(self):

# run the background update task
self.update_data()
self.redis_exception_happen = False
Copy link
Contributor

@qiluo-msft qiluo-msft Oct 9, 2023

Choose a reason for hiding this comment

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

redis_exception_happen

I am worried about the contract of member redis_exception_happen between multiple classes. How about define a new argument reconnect=False in reinit_data(), and keep redis_exception_happen as a private member only in this class. #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.

Fixed, move redis_exception_happen to local variable. also because the python method override issue, add new method reinit_connection() to MIBUpdater interface.

except RuntimeError:
# Any unexpected exception or error, log it and keep running
logger.exception("MIBUpdater.start() caught an unexpected exception during update_data()")
# When redis server restart, swsscommon will throw swsscommon.RedisError, redis connection need re-initialize in reinit_data()
# TODO: change to swsscommon.RedisError
self.redis_exception_happen = True
except Exception:
# Any unexpected exception or error, log it and keep running
logger.exception("MIBUpdater.start() caught an unexpected exception during update_data()")
Expand Down
7 changes: 7 additions & 0 deletions src/sonic_ax_impl/mibs/ietf/rfc1213.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,10 @@ def __init__(self):
self.nexthop_map = {}
self.route_list = []

def reinit_data(self):
if self.redis_exception_happen:
Namespace.connect_all_dbs(self.db_conn, mibs.APPL_DB)

def update_data(self):
"""
Update redis (caches config)
Expand Down Expand Up @@ -220,6 +224,9 @@ def reinit_data(self):
"""
Subclass update interface information
"""
if self.redis_exception_happen:
Namespace.connect_namespace_dbs(self.db_conn)

self.if_name_map, \
self.if_alias_map, \
self.if_id_map, \
Expand Down
3 changes: 3 additions & 0 deletions src/sonic_ax_impl/mibs/ietf/rfc2737.py
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,9 @@ def reinit_data(self):
self.physical_name_to_oid_map = {}
self.pending_resolve_parent_name_map = {}

if self.redis_exception_happen:
Namespace.connect_all_dbs(self.statedb, mibs.STATE_DB)

device_metadata = mibs.get_device_metadata(self.statedb[0])
chassis_sub_id = (CHASSIS_SUB_ID, )
self.physical_entities = [chassis_sub_id]
Expand Down
3 changes: 3 additions & 0 deletions src/sonic_ax_impl/mibs/ietf/rfc2863.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@ def reinit_data(self):
"""
Subclass update interface information
"""
if self.redis_exception_happen:
Namespace.connect_namespace_dbs(self.db_conn)

self.if_name_map, \
self.if_alias_map, \
self.if_id_map, \
Expand Down
3 changes: 3 additions & 0 deletions src/sonic_ax_impl/mibs/ietf/rfc3433.py
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,9 @@ def reinit_data(self):
self.ent_phy_sensor_value_map = {}
self.ent_phy_sensor_oper_state_map = {}

if self.redis_exception_happen:
Namespace.connect_all_dbs(self.statedb, mibs.STATE_DB)

transceiver_dom_encoded = Namespace.dbs_keys(self.statedb, mibs.STATE_DB, self.TRANSCEIVER_DOM_KEY_PATTERN)
if transceiver_dom_encoded:
self.transceiver_dom = [entry for entry in transceiver_dom_encoded]
Expand Down
3 changes: 3 additions & 0 deletions src/sonic_ax_impl/mibs/ietf/rfc4292.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ def reinit_data(self):
"""
self.loips = {}

if self.redis_exception_happen:
Namespace.connect_all_dbs(self.db_conn, mibs.APPL_DB)

loopbacks = Namespace.dbs_keys(self.db_conn, mibs.APPL_DB, "INTF_TABLE:lo:*")
if not loopbacks:
return
Expand Down
3 changes: 3 additions & 0 deletions src/sonic_ax_impl/mibs/ietf/rfc4363.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ def reinit_data(self):
"""
Subclass update interface information
"""
if self.redis_exception_happen:
Namespace.connect_namespace_dbs(self.db_conn)

(
self.if_name_map,
self.if_alias_map,
Expand Down
3 changes: 3 additions & 0 deletions src/sonic_ax_impl/mibs/vendor/cisco/ciscoPfcExtMIB.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ def reinit_data(self):
"""
Subclass update interface information
"""
if self.redis_exception_happen:
Namespace.connect_namespace_dbs(self.db_conn)

self.if_name_map, \
self.if_alias_map, \
self.if_id_map, \
Expand Down
3 changes: 3 additions & 0 deletions src/sonic_ax_impl/mibs/vendor/cisco/ciscoSwitchQosMIB.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ def reinit_data(self):
"""
Subclass update interface information
"""
if self.redis_exception_happen:
Namespace.connect_namespace_dbs(self.db_conn)

self.if_name_map, \
self.if_alias_map, \
self.if_id_map, \
Expand Down
66 changes: 65 additions & 1 deletion tests/test_rfc1213.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import asyncio
import os
import sonic_ax_impl
import sys
from unittest import TestCase

Expand All @@ -10,7 +12,8 @@
modules_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
sys.path.insert(0, os.path.join(modules_path, 'src'))

from sonic_ax_impl.mibs.ietf.rfc1213 import NextHopUpdater
from sonic_ax_impl.mibs.ietf.rfc1213 import NextHopUpdater, InterfacesUpdater


class TestNextHopUpdater(TestCase):

Expand Down Expand Up @@ -43,3 +46,64 @@ def test_NextHopUpdater_route_no_next_hop(self):
mocked_warning.assert_has_calls(expected)

self.assertTrue(len(updater.route_list) == 0)


class TestNextHopUpdaterRedisException(TestCase):
def __init__(self, name):
super().__init__(name)
self.throw_exception = True
self.updater = NextHopUpdater()

# setup mock method, throw exception when first time call it
def mock_dbs_keys(self, *args, **kwargs):
if self.throw_exception:
self.throw_exception = False
raise RuntimeError

self.updater.run_event.clear()
return None

@mock.patch('sonic_ax_impl.mibs.Namespace.dbs_get_all', mock.MagicMock(return_value=({"ifname": "Ethernet0,Ethernet4"})))
def test_NextHopUpdater_redis_exception(self):
with mock.patch('sonic_ax_impl.mibs.Namespace.dbs_keys', self.mock_dbs_keys):
with mock.patch('ax_interface.logger.exception') as mocked_exception:
self.updater.run_event.set()
self.updater.frequency = 1
self.updater.reinit_rate = 1
self.updater.update_counter = 1
loop = asyncio.get_event_loop()
loop.run_until_complete(self.updater.start())
loop.close()

# check warning
expected = [
mock.call("MIBUpdater.start() caught an unexpected exception during update_data()")
]
mocked_exception.assert_has_calls(expected)

self.assertTrue(self.updater.redis_exception_happen == False)


@mock.patch('sonic_ax_impl.mibs.init_mgmt_interface_tables', mock.MagicMock(return_value=([{}, {}])))
def test_InterfacesUpdater_re_init_redis_exception(self):

def mock_get_sync_d_from_all_namespace(per_namespace_func, db_conn):
if per_namespace_func == sonic_ax_impl.mibs.init_sync_d_interface_tables:
return [{}, {}, {}, {}]

if per_namespace_func == sonic_ax_impl.mibs.init_sync_d_vlan_tables:
return [{}, {}, {}]

if per_namespace_func == sonic_ax_impl.mibs.init_sync_d_rif_tables:
return [{}, {}]

return [{}, {}, {}, {}, {}]

updater = InterfacesUpdater()
updater.redis_exception_happen = True
with mock.patch('sonic_ax_impl.mibs.Namespace.get_sync_d_from_all_namespace', mock_get_sync_d_from_all_namespace):
with mock.patch('sonic_ax_impl.mibs.Namespace.connect_namespace_dbs') as connect_namespace_dbs:
updater.reinit_data()

# check re-init
connect_namespace_dbs.assert_called()
14 changes: 13 additions & 1 deletion tests/test_rfc3433.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,16 @@ def test_PhysicalSensorTableMIBUpdater_transceiver_info_key_missing(self):
# check warning
mocked_warn.assert_called()

self.assertTrue(len(updater.sub_ids) == 0)
self.assertTrue(len(updater.sub_ids) == 0)

@mock.patch('sonic_ax_impl.mibs.Namespace.dbs_keys', mock.MagicMock(return_value=(None)))
@mock.patch('swsscommon.swsscommon.SonicV2Connector.keys', mock.MagicMock(return_value=(None)))
def test_PhysicalSensorTableMIBUpdater_re_init_redis_exception(self):
updater = PhysicalSensorTableMIBUpdater()
updater.redis_exception_happen = True

with mock.patch('sonic_ax_impl.mibs.Namespace.connect_all_dbs') as connect_all_dbs:
updater.reinit_data()

# check re-init
connect_all_dbs.assert_called()
12 changes: 12 additions & 0 deletions tests/test_rfc4292.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,15 @@ def test_RouteUpdater_route_no_iframe(self):
mocked_warning.assert_has_calls(expected)

self.assertTrue(len(updater.route_dest_list) == 0)


@mock.patch('sonic_ax_impl.mibs.Namespace.dbs_keys', mock.MagicMock(return_value=(None)))
def test_RouteUpdater_re_init_redis_exception(self):
updater = RouteUpdater()
updater.redis_exception_happen = True

with mock.patch('sonic_ax_impl.mibs.Namespace.connect_all_dbs') as connect_all_dbs:
updater.reinit_data()

# check re-init
connect_all_dbs.assert_called()
21 changes: 21 additions & 0 deletions tests/test_rfc4363.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import os
import sys
import sonic_ax_impl
from unittest import TestCase

if sys.version_info.major == 3:
Expand All @@ -26,3 +27,23 @@ def test_FdbUpdater_ent_bridge_port_id_attr_missing(self):
mocked_warn.assert_called()

self.assertTrue(len(updater.vlanmac_ifindex_list) == 0)


@mock.patch('sonic_ax_impl.mibs.Namespace.dbs_keys', mock.MagicMock(return_value=(None)))
@mock.patch('sonic_ax_impl.mibs.Namespace.dbs_get_bridge_port_map', mock.MagicMock(return_value=(None)))
def test_RouteUpdater_re_init_redis_exception(self):
updater = FdbUpdater()
updater.redis_exception_happen = True

def mock_get_sync_d_from_all_namespace(per_namespace_func, db_conn):
if per_namespace_func == sonic_ax_impl.mibs.init_sync_d_interface_tables:
return [{}, {}, {}, {}]

return [{}, {}, {}, {}, {}]

with mock.patch('sonic_ax_impl.mibs.Namespace.get_sync_d_from_all_namespace', mock_get_sync_d_from_all_namespace):
with mock.patch('sonic_ax_impl.mibs.Namespace.connect_namespace_dbs') as connect_namespace_dbs:
updater.reinit_data()

# check re-init
connect_namespace_dbs.assert_called()