Skip to content

Commit

Permalink
fix(xcvrd): fix DPB support with CMIS transceivers
Browse files Browse the repository at this point in the history 
CmisManagerTask's `port_dict` and `port_mapping` must be updated
according to the port add/remove events.

Before this commit, `port_mapping` is only intialized when
CmisManagerTask is initialized and not updated after that,
which was causing KeyError exception when DBP is used.
(sonic-net/sonic-buildimage#18893)

This commit removes the `port_mapping` field from CmisManagerTask as
`port_mapping` was used just for storing `asic_id` information
and that can be simply done by `port_dict` instead.

Also, this commit updates `port_dict` accoding to the port add/remove
events to support DPB.

Signed-off-by: Wataru Ishida <wataru.ishid@gmail.com>
  • Loading branch information
@ishidawataru
ishidawataru committed Sep 17, 2024
1 parent f581c06 commit 7c1b475
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 63 deletions.
61 changes: 29 additions & 32 deletions sonic-xcvrd/tests/test_xcvrd.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ def test_CmisManagerTask_get_xcvr_api_exception(self, mock_platform_chassis, moc
mock_sfp.get_xcvr_api = MagicMock(side_effect=NotImplementedError)
task.task_worker()
assert mock_log_exception_traceback.call_count == 1
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_FAILED
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_FAILED

# Case 2: is_flat_memory() raises AttributeError. In this case, CMIS SM should transition to READY state
mock_xcvr_api = MagicMock()
Expand All @@ -223,7 +223,7 @@ def test_CmisManagerTask_get_xcvr_api_exception(self, mock_platform_chassis, moc
task.task_stopping_event.is_set = MagicMock(side_effect=[False, False, True])
task.on_port_update_event(port_change_event)
task.task_worker()
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_READY
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_READY

# Case 3: get_cmis_application_desired() raises an exception
mock_xcvr_api.is_flat_memory = MagicMock(return_value=False)
Expand All @@ -234,7 +234,7 @@ def test_CmisManagerTask_get_xcvr_api_exception(self, mock_platform_chassis, moc
task.get_cmis_host_lanes_mask = MagicMock()
task.task_worker()
assert mock_log_exception_traceback.call_count == 2
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_FAILED
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_FAILED
assert task.get_cmis_host_lanes_mask.call_count == 0

@patch('xcvrd.xcvrd_utilities.port_event_helper.subscribe_port_config_change', MagicMock(side_effect = NotImplementedError))
Expand Down Expand Up @@ -297,7 +297,7 @@ def test_SfpStateUpdateTask_task_run_with_exception(self):
@patch('xcvrd.xcvrd.SffManagerTask.join')
def test_DaemonXcvrd_run_with_exception(self, mock_task_join_sff, mock_task_join_sfp,
mock_task_join_dom, mock_init, mock_os_kill):
mock_init.return_value = (PortMapping(), set())
mock_init.return_value = PortMapping()
xcvrd = DaemonXcvrd(SYSLOG_IDENTIFIER)
xcvrd.enable_sff_mgr = True
xcvrd.load_feature_flags = MagicMock()
Expand Down Expand Up @@ -1199,7 +1199,7 @@ def test_DaemonXcvrd_wait_for_port_config_done(self, mock_select, mock_sub_table
@patch('xcvrd.xcvrd.DomInfoUpdateTask.join')
@patch('xcvrd.xcvrd.SfpStateUpdateTask.join')
def test_DaemonXcvrd_run(self, mock_task_stop1, mock_task_stop2, mock_task_run1, mock_task_run2, mock_deinit, mock_init):
mock_init.return_value = (PortMapping(), set())
mock_init.return_value = PortMapping()
xcvrd = DaemonXcvrd(SYSLOG_IDENTIFIER)
xcvrd.load_feature_flags = MagicMock()
xcvrd.stop_event.wait = MagicMock()
Expand Down Expand Up @@ -1492,9 +1492,10 @@ def test_CmisManagerTask_handle_port_change_event(self):

port_change_event = PortChangeEvent('Ethernet0', 1, 0, PortChangeEvent.PORT_DEL)
task.on_port_update_event(port_change_event)
assert len(task.port_dict) == 1
assert len(task.port_dict) == 0

port_change_event = PortChangeEvent('Ethernet0', 1, 0, PortChangeEvent.PORT_SET)
port_dict = {'speed':'400000', 'lanes':'1,2,3,4,5,6,7,8'}
port_change_event = PortChangeEvent('Ethernet0', 1, 0, PortChangeEvent.PORT_SET, port_dict)
task.on_port_update_event(port_change_event)
assert len(task.port_dict) == 1

Expand Down Expand Up @@ -1896,16 +1897,15 @@ def test_CmisManagerTask_task_worker(self, mock_chassis, mock_get_status_tbl):
mock_chassis.get_sfp = MagicMock(return_value=mock_sfp)

port_mapping = PortMapping()
port_mapping.handle_port_change_event(PortChangeEvent('Ethernet0', 1, 0, PortChangeEvent.PORT_ADD))
stop_event = threading.Event()
task = CmisManagerTask(DEFAULT_NAMESPACE, port_mapping, stop_event)
task.port_mapping.logical_port_list = ['Ethernet0']
task.xcvr_table_helper = XcvrTableHelper(DEFAULT_NAMESPACE)
task.xcvr_table_helper.get_status_tbl.return_value = mock_get_status_tbl
task.task_stopping_event.is_set = MagicMock(side_effect=[False, False, True])
task.task_worker()
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_UNKNOWN
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_UNKNOWN

task.port_mapping.logical_port_list = MagicMock()
port_change_event = PortChangeEvent('PortConfigDone', -1, 0, PortChangeEvent.PORT_SET)
task.on_port_update_event(port_change_event)
assert task.isPortConfigDone
Expand All @@ -1914,7 +1914,7 @@ def test_CmisManagerTask_task_worker(self, mock_chassis, mock_get_status_tbl):
{'speed':'400000', 'lanes':'1,2,3,4,5,6,7,8'})
task.on_port_update_event(port_change_event)
assert len(task.port_dict) == 1
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_INSERTED
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_INSERTED

task.get_host_tx_status = MagicMock(return_value='true')
task.get_port_admin_status = MagicMock(return_value='up')
Expand All @@ -1928,51 +1928,50 @@ def test_CmisManagerTask_task_worker(self, mock_chassis, mock_get_status_tbl):
mock_xcvr_api.decommission_all_datapaths = MagicMock(return_value=True)
task.task_stopping_event.is_set = MagicMock(side_effect=[False, False, True])
task.task_worker()
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_DP_DEINIT
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_DP_DEINIT
task.task_stopping_event.is_set = MagicMock(side_effect=[False, False, True])
task.task_worker()
assert mock_xcvr_api.set_datapath_deinit.call_count == 1
assert mock_xcvr_api.tx_disable_channel.call_count == 1
assert mock_xcvr_api.set_lpmode.call_count == 1
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_AP_CONF
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_AP_CONF

# Case 2: DP_DEINIT --> AP Configured
task.task_stopping_event.is_set = MagicMock(side_effect=[False, False, True])
task.task_worker()
assert mock_xcvr_api.set_application.call_count == 1
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_DP_INIT
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_DP_INIT

# Case 3: AP Configured --> DP_INIT
task.task_stopping_event.is_set = MagicMock(side_effect=[False, False, True])
task.task_worker()
assert mock_xcvr_api.set_datapath_init.call_count == 1
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_DP_TXON
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_DP_TXON

# Case 4: DP_INIT --> DP_TXON
task.task_stopping_event.is_set = MagicMock(side_effect=[False, False, True])
task.task_worker()
assert mock_xcvr_api.tx_disable_channel.call_count == 2
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_DP_ACTIVATE
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_DP_ACTIVATE

# Case 5: DP_TXON --> DP_ACTIVATION
task.task_stopping_event.is_set = MagicMock(side_effect=[False, False, True])
task.post_port_active_apsel_to_db = MagicMock()
task.task_worker()
assert task.post_port_active_apsel_to_db.call_count == 1
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_READY
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_READY

# Fail test coverage - Module Inserted state failing to reach DP_DEINIT
port_mapping = PortMapping()
port_mapping.handle_port_change_event(PortChangeEvent('Ethernet1', 1, 0, PortChangeEvent.PORT_ADD))
stop_event = threading.Event()
task = CmisManagerTask(DEFAULT_NAMESPACE, port_mapping, stop_event)
task.port_mapping.logical_port_list = ['Ethernet1']
task.xcvr_table_helper = XcvrTableHelper(DEFAULT_NAMESPACE)
task.xcvr_table_helper.get_status_tbl.return_value = mock_get_status_tbl
task.task_stopping_event.is_set = MagicMock(side_effect=[False, False, True])
task.task_worker()
assert get_cmis_state_from_state_db('Ethernet1', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet1'))) == CMIS_STATE_UNKNOWN
assert get_cmis_state_from_state_db('Ethernet1', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet1'))) == CMIS_STATE_UNKNOWN

task.port_mapping.logical_port_list = MagicMock()
port_change_event = PortChangeEvent('PortConfigDone', -1, 0, PortChangeEvent.PORT_SET)
task.on_port_update_event(port_change_event)
assert task.isPortConfigDone
Expand All @@ -1981,7 +1980,7 @@ def test_CmisManagerTask_task_worker(self, mock_chassis, mock_get_status_tbl):
{'speed':'400000', 'lanes':'1,2,3,4,5,6,7,8'})
task.on_port_update_event(port_change_event)
assert len(task.port_dict) == 1
assert get_cmis_state_from_state_db('Ethernet1', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet1'))) == CMIS_STATE_INSERTED
assert get_cmis_state_from_state_db('Ethernet1', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet1'))) == CMIS_STATE_INSERTED

task.get_host_tx_status = MagicMock(return_value='true')
task.get_port_admin_status = MagicMock(return_value='up')
Expand All @@ -1995,7 +1994,7 @@ def test_CmisManagerTask_task_worker(self, mock_chassis, mock_get_status_tbl):
mock_xcvr_api.decommission_all_datapaths = MagicMock(return_value=False)
task.task_stopping_event.is_set = MagicMock(side_effect=[False, False, True])
task.task_worker()
assert not get_cmis_state_from_state_db('Ethernet1', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet1'))) == CMIS_STATE_DP_DEINIT
assert not get_cmis_state_from_state_db('Ethernet1', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet1'))) == CMIS_STATE_DP_DEINIT

@patch('xcvrd.xcvrd.XcvrTableHelper.get_status_tbl')
@patch('xcvrd.xcvrd.platform_chassis')
Expand Down Expand Up @@ -2100,16 +2099,15 @@ def test_CmisManagerTask_task_worker_fastboot(self, mock_chassis, mock_get_statu
mock_chassis.get_sfp = MagicMock(return_value=mock_sfp)

port_mapping = PortMapping()
port_mapping.handle_port_change_event(PortChangeEvent('Ethernet0', 1, 0, PortChangeEvent.PORT_ADD))
stop_event = threading.Event()
task = CmisManagerTask(DEFAULT_NAMESPACE, port_mapping, stop_event)
task.port_mapping.logical_port_list = ['Ethernet0']
task.xcvr_table_helper = XcvrTableHelper(DEFAULT_NAMESPACE)
task.xcvr_table_helper.get_status_tbl.return_value = mock_get_status_tbl
task.task_stopping_event.is_set = MagicMock(side_effect=[False, False, True])
task.task_worker()
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_UNKNOWN
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_UNKNOWN

task.port_mapping.logical_port_list = MagicMock()
port_change_event = PortChangeEvent('PortConfigDone', -1, 0, PortChangeEvent.PORT_SET)
task.on_port_update_event(port_change_event)
assert task.isPortConfigDone
Expand All @@ -2118,7 +2116,7 @@ def test_CmisManagerTask_task_worker_fastboot(self, mock_chassis, mock_get_statu
{'speed':'400000', 'lanes':'1,2,3,4,5,6,7,8'})
task.on_port_update_event(port_change_event)
assert len(task.port_dict) == 1
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_INSERTED
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_INSERTED

task.get_host_tx_status = MagicMock(return_value='false')
task.get_port_admin_status = MagicMock(return_value='up')
Expand All @@ -2130,7 +2128,7 @@ def test_CmisManagerTask_task_worker_fastboot(self, mock_chassis, mock_get_statu
task.task_stopping_event.is_set = MagicMock(side_effect=[False, False, True])
task.task_worker()

assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_READY
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_READY


@patch('xcvrd.xcvrd.XcvrTableHelper.get_status_tbl')
Expand Down Expand Up @@ -2236,16 +2234,15 @@ def test_CmisManagerTask_task_worker_host_tx_ready_false(self, mock_chassis, moc
mock_chassis.get_sfp = MagicMock(return_value=mock_sfp)

port_mapping = PortMapping()
port_mapping.handle_port_change_event(PortChangeEvent('Ethernet0', 1, 0, PortChangeEvent.PORT_ADD))
stop_event = threading.Event()
task = CmisManagerTask(DEFAULT_NAMESPACE, port_mapping, stop_event)
task.port_mapping.logical_port_list = ['Ethernet0']
task.xcvr_table_helper = XcvrTableHelper(DEFAULT_NAMESPACE)
task.xcvr_table_helper.get_status_tbl.return_value = mock_get_status_tbl
task.task_stopping_event.is_set = MagicMock(side_effect=[False, False, True])
task.task_worker()
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_UNKNOWN
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_UNKNOWN

task.port_mapping.logical_port_list = MagicMock()
port_change_event = PortChangeEvent('PortConfigDone', -1, 0, PortChangeEvent.PORT_SET)
task.on_port_update_event(port_change_event)
assert task.isPortConfigDone
Expand All @@ -2254,7 +2251,7 @@ def test_CmisManagerTask_task_worker_host_tx_ready_false(self, mock_chassis, moc
{'speed':'400000', 'lanes':'1,2,3,4,5,6,7,8'})
task.on_port_update_event(port_change_event)
assert len(task.port_dict) == 1
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_INSERTED
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_INSERTED

task.get_host_tx_status = MagicMock(return_value='false')
task.get_port_admin_status = MagicMock(return_value='up')
Expand All @@ -2267,7 +2264,7 @@ def test_CmisManagerTask_task_worker_host_tx_ready_false(self, mock_chassis, moc
task.task_worker()

assert mock_xcvr_api.tx_disable_channel.call_count == 1
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.port_mapping.get_asic_id_for_logical_port('Ethernet0'))) == CMIS_STATE_READY
assert get_cmis_state_from_state_db('Ethernet0', task.xcvr_table_helper.get_status_tbl(task.get_asic_id('Ethernet0'))) == CMIS_STATE_READY

@pytest.mark.parametrize("lport, expected_dom_polling", [
('Ethernet0', 'disabled'),
Expand Down
Loading

0 comments on commit 7c1b475

Please sign in to comment.