From f882ad66a2cdba7b059a20af1a03101cb5ee52e1 Mon Sep 17 00:00:00 2001 From: Tamer Ahmed Date: Tue, 28 Jul 2026 09:45:19 -0700 Subject: [PATCH 1/2] [xcvrd] Bootstrap the CMIS state machine for ports that miss their PORT_SET A port only enters the CMIS state machine once a PORT_SET event assigns it an initial state through force_cmis_reinit(). get_cmis_state_from_state_db() reports an absent cmis_state as UNKNOWN, and process_single_lport() returns immediately on UNKNOWN, so a port whose PORT_SET was never delivered is skipped on every iteration. Such a port is left completely unmanaged: no CMIS state is ever written, the transceiver is never queried and nothing is logged. This was observed on a port created by a dynamic port breakout, which had no cmis_state at all while every other port on the switch reported READY. Only restarting xcvrd recovered it. Bootstrap the state machine when a port has no CMIS state but a complete configuration, so a missed event no longer leaves the port unmanaged. Ports without a usable configuration are still skipped, and the validity check is shared with the existing one through is_port_config_complete(). Also always record a physical index in port_dict. on_port_update_event() only stored it when it was valid, while port_event_helper defaults a missing index to -1, so a port could be admitted without one and is_decomm_pending() and is_decomm_lead_lport() would raise KeyError when indexing into it. Signed-off-by: Tamer Ahmed --- sonic-xcvrd/tests/test_xcvrd.py | 105 ++++++++++++++++++++ sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py | 38 ++++++- 2 files changed, 142 insertions(+), 1 deletion(-) diff --git a/sonic-xcvrd/tests/test_xcvrd.py b/sonic-xcvrd/tests/test_xcvrd.py index a25d742ea..da9583a45 100644 --- a/sonic-xcvrd/tests/test_xcvrd.py +++ b/sonic-xcvrd/tests/test_xcvrd.py @@ -3294,6 +3294,111 @@ def test_CmisManagerTask_process_single_lport_invalid_host_lanes_mask(self, mock # Verify state transitioned to FAILED assert common.get_cmis_state_from_state_db('Ethernet0', mock_get_status_sw_tbl) == CMIS_STATE_FAILED + @pytest.mark.parametrize("info, expected", [ + ({'index': 1, 'speed': '400000', 'lanes': '1,2,3,4', 'subport': 0}, True), + ({'index': -1, 'speed': '400000', 'lanes': '1,2,3,4', 'subport': 0}, False), + ({'speed': '400000', 'lanes': '1,2,3,4', 'subport': 0}, False), + ({'index': 1, 'speed': '0', 'lanes': '1,2,3,4', 'subport': 0}, False), + ({'index': 1, 'lanes': '1,2,3,4', 'subport': 0}, False), + ({'index': 1, 'speed': '400000', 'lanes': '', 'subport': 0}, False), + ({'index': 1, 'speed': '400000', 'subport': 0}, False), + ({'index': 1, 'speed': '400000', 'lanes': '1,2,3,4', 'subport': -1}, False), + ({'index': 'N/A', 'speed': '400000', 'lanes': '1,2,3,4', 'subport': 0}, False), + ]) + def test_CmisManagerTask_is_port_config_complete(self, info, expected): + port_mapping = PortMapping() + stop_event = threading.Event() + task = CmisManagerTask(DEFAULT_NAMESPACE, port_mapping, stop_event, platform_chassis=MagicMock()) + + assert task.is_port_config_complete(info) == expected + + def test_CmisManagerTask_on_port_update_event_always_records_index(self): + """ + A PORT_SET event without an 'index' field must still leave a physical index + behind, otherwise consumers indexing into port_dict raise KeyError. + """ + port_mapping = PortMapping() + stop_event = threading.Event() + task = CmisManagerTask(DEFAULT_NAMESPACE, port_mapping, stop_event, platform_chassis=MagicMock()) + task.xcvr_table_helper = MagicMock() + + # port_event_helper defaults a missing 'index' field to -1 + port_change_event = PortChangeEvent('Ethernet0', -1, 0, PortChangeEvent.PORT_SET, + {'speed': '400000', 'lanes': '1,2,3,4'}) + task.on_port_update_event(port_change_event) + + assert task.port_dict['Ethernet0']['index'] == -1 + assert task.is_decomm_pending('Ethernet0') == False + + # A later event carrying a valid index must take effect + port_change_event = PortChangeEvent('Ethernet0', 1, 0, PortChangeEvent.PORT_SET, + {'speed': '400000', 'lanes': '1,2,3,4'}) + task.on_port_update_event(port_change_event) + + assert task.port_dict['Ethernet0']['index'] == 1 + + @patch('xcvrd.xcvrd.XcvrTableHelper.get_status_sw_tbl') + @patch('xcvrd.xcvrd.platform_chassis') + def test_CmisManagerTask_process_single_lport_bootstraps_missing_cmis_state(self, mock_chassis, mock_get_status_sw_tbl): + """ + A port only enters the CMIS state machine when a PORT_SET event assigns it an + initial state. If that event is missed, for instance when the port is created + by a dynamic port breakout after the port map was built, the port has no CMIS + state. An absent state reads back as UNKNOWN and used to be skipped forever, + leaving the port unmanaged until xcvrd was restarted. + """ + mock_get_status_sw_tbl = Table("STATE_DB", TRANSCEIVER_STATUS_SW_TABLE) + + mock_sfp = MagicMock() + mock_sfp.get_presence = MagicMock(return_value=True) + mock_chassis.get_sfp = MagicMock(return_value=mock_sfp) + + port_mapping = PortMapping() + stop_event = threading.Event() + task = CmisManagerTask(DEFAULT_NAMESPACE, port_mapping, stop_event, platform_chassis=mock_chassis) + task.xcvr_table_helper = XcvrTableHelper(DEFAULT_NAMESPACE) + task.xcvr_table_helper.get_status_sw_tbl.return_value = mock_get_status_sw_tbl + task.get_host_tx_status = MagicMock(return_value='true') + task.get_port_admin_status = MagicMock(return_value='up') + + # The port is fully configured but never received a PORT_SET event + task.port_dict['Ethernet0'] = {'asic_id': 0, 'index': 1, 'speed': '400000', + 'lanes': '1,2,3,4,5,6,7,8', 'subport': 0} + assert common.get_cmis_state_from_state_db('Ethernet0', mock_get_status_sw_tbl) == CMIS_STATE_UNKNOWN + + task._gearbox_lanes_dict = {} + task.process_single_lport('Ethernet0', task.port_dict['Ethernet0']) + + # The state machine is bootstrapped instead of the port being skipped + assert common.get_cmis_state_from_state_db('Ethernet0', mock_get_status_sw_tbl) != CMIS_STATE_UNKNOWN + assert mock_sfp.get_presence.called + + @patch('xcvrd.xcvrd.XcvrTableHelper.get_status_sw_tbl') + @patch('xcvrd.xcvrd.platform_chassis') + def test_CmisManagerTask_process_single_lport_skips_incomplete_config(self, mock_chassis, mock_get_status_sw_tbl): + """A port without a usable configuration must not be bootstrapped.""" + mock_get_status_sw_tbl = Table("STATE_DB", TRANSCEIVER_STATUS_SW_TABLE) + + mock_sfp = MagicMock() + mock_sfp.get_presence = MagicMock(return_value=True) + mock_chassis.get_sfp = MagicMock(return_value=mock_sfp) + + port_mapping = PortMapping() + stop_event = threading.Event() + task = CmisManagerTask(DEFAULT_NAMESPACE, port_mapping, stop_event, platform_chassis=mock_chassis) + task.xcvr_table_helper = XcvrTableHelper(DEFAULT_NAMESPACE) + task.xcvr_table_helper.get_status_sw_tbl.return_value = mock_get_status_sw_tbl + + # No physical index, so the port cannot be driven yet + task.port_dict['Ethernet0'] = {'asic_id': 0, 'index': -1, 'speed': '400000', + 'lanes': '1,2,3,4,5,6,7,8', 'subport': 0} + + task._gearbox_lanes_dict = {} + task.process_single_lport('Ethernet0', task.port_dict['Ethernet0']) + + assert common.get_cmis_state_from_state_db('Ethernet0', mock_get_status_sw_tbl) == CMIS_STATE_UNKNOWN + assert not mock_sfp.get_presence.called + @patch('xcvrd.xcvrd.XcvrTableHelper.get_status_sw_tbl') @patch('xcvrd.xcvrd.platform_chassis') @patch('xcvrd.xcvrd_utilities.common.is_fast_reboot_enabled', MagicMock(return_value=False)) diff --git a/sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py b/sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py index b39992fa7..44d949273 100644 --- a/sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py +++ b/sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py @@ -122,6 +122,9 @@ def on_port_update_event(self, port_change_event): if lport not in self.port_dict: self.port_dict[lport] = {"asic_id": port_change_event.asic_id, "forced_tx_disabled": False} + # Always record a physical index, even when it is still unknown, so that + # the consumers indexing into port_dict do not have to handle its absence. + self.port_dict[lport].setdefault('index', -1) if pport >= 0: self.port_dict[lport]['index'] = pport if 'speed' in port_change_event.port_dict and port_change_event.port_dict['speed'] != 'N/A': @@ -1249,8 +1252,41 @@ def process_cmis_state_machine(self, lport): common.log_exception_traceback() self.update_port_transceiver_status_table_sw_cmis_state(lport, CMIS_STATE_FAILED) + def is_port_config_complete(self, info): + """ + Check whether a port has all the configuration the CMIS state machine needs. + + Args: + info: + Dictionary, the port_dict entry of a logical port + + Returns: + Boolean, true if the physical index, speed, lanes and subport are all + present and valid. + """ + try: + pport = int(info.get('index', "-1")) + speed = int(info.get('speed', "0")) + subport = int(info.get('subport', 0)) + except (TypeError, ValueError): + return False + + lanes = info.get('lanes', "").strip() + return pport >= 0 and speed != 0 and len(lanes) >= 1 and subport >= 0 + def process_single_lport(self, lport, info): state = common.get_cmis_state_from_state_db(lport, self.xcvr_table_helper.get_status_sw_tbl(self.get_asic_id(lport))) + if state == CMIS_STATE_UNKNOWN and self.is_port_config_complete(info): + # A port only enters the state machine once a PORT_SET event assigns it an + # initial CMIS state. When that event is missed, for instance because the + # port was created by a dynamic port breakout after the port map was built, + # the port keeps an empty CMIS state. Since an empty state reads back as + # UNKNOWN, and UNKNOWN is skipped below, the port would stay unmanaged until + # xcvrd is restarted. Bootstrap the state machine instead. + self.log_notice("{}: no CMIS state found, starting the CMIS state machine".format(lport)) + self.update_port_transceiver_status_table_sw_cmis_state(lport, CMIS_STATE_INSERTED) + state = CMIS_STATE_INSERTED + if state in CMIS_TERMINAL_STATES or state == CMIS_STATE_UNKNOWN: if state != CMIS_STATE_READY: self.port_dict[lport]['appl'] = 0 @@ -1269,7 +1305,7 @@ def process_single_lport(self, lport, info): speed = int(info.get('speed', "0")) lanes = info.get('lanes', "").strip() subport = info.get('subport', 0) - if pport < 0 or speed == 0 or len(lanes) < 1 or subport < 0: + if not self.is_port_config_complete(info): return host_lane_count = self.get_host_lane_count(lport, lanes) From 2b619b695bdb841f29a524397fac3dcb02463731 Mon Sep 17 00:00:00 2001 From: Tamer Ahmed Date: Mon, 3 Aug 2026 17:43:21 -0700 Subject: [PATCH 2/2] [xcvrd] Do not let an unknown ASIC id terminate the CMIS task update_port_transceiver_status_table_sw_cmis_state() passes the result of get_asic_id() straight into get_status_sw_tbl(). get_asic_id() reports -1 for a port that is not tracked in port_dict, and the per-ASIC tables are dicts keyed by asic_id, so -1 is never a valid key and the lookup raises. The existing "if status_table is None" guard below it is therefore unreachable for that case: the exception is raised inside the getter, before the guard can run. The exception propagates out of the CmisManagerTask thread, which terminates the whole daemon. This was seen after a dynamic port breakout, where events for the removed subports still referenced ports that had already left port_dict: on_port_update_event -> force_cmis_reinit -> update_port_transceiver_status_table_sw_cmis_state -> get_status_tbl(asic_index) KeyError Exception occured at CmisManagerTask thread Exiting main loop as child thread raised exception! gave up: xcvrd entered FATAL state, too many start retries too quickly Resolve the ASIC id before indexing so the existing guard becomes reachable and an event for an untracked port is logged and skipped instead of taking xcvrd down. Behaviour for tracked ports is unchanged. Signed-off-by: Tamer Ahmed --- sonic-xcvrd/tests/test_xcvrd.py | 26 +++++++++++++++++++++ sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py | 6 ++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/sonic-xcvrd/tests/test_xcvrd.py b/sonic-xcvrd/tests/test_xcvrd.py index da9583a45..f3bb752cd 100644 --- a/sonic-xcvrd/tests/test_xcvrd.py +++ b/sonic-xcvrd/tests/test_xcvrd.py @@ -3399,6 +3399,32 @@ def test_CmisManagerTask_process_single_lport_skips_incomplete_config(self, mock assert common.get_cmis_state_from_state_db('Ethernet0', mock_get_status_sw_tbl) == CMIS_STATE_UNKNOWN assert not mock_sfp.get_presence.called + def test_CmisManagerTask_update_sw_cmis_state_for_untracked_port(self): + """ + An event can arrive for a port that is no longer in port_dict, for instance a + stale STATE_DB entry left behind by a dynamic port breakout. get_asic_id() then + reports -1, which is not a key of the per-ASIC tables. The lookup must not raise + out of the CmisManagerTask thread, because that terminates the whole daemon. + """ + port_mapping = PortMapping() + stop_event = threading.Event() + task = CmisManagerTask(DEFAULT_NAMESPACE, port_mapping, stop_event, platform_chassis=MagicMock()) + + # Per-ASIC tables are dicts keyed by asic_id, so -1 would raise KeyError. + task.xcvr_table_helper = MagicMock() + task.xcvr_table_helper.get_status_sw_tbl = MagicMock(side_effect=lambda asic_id: {0: MagicMock()}[asic_id]) + + assert 'Ethernet0' not in task.port_dict + task.update_port_transceiver_status_table_sw_cmis_state('Ethernet0', CMIS_STATE_INSERTED) + + # The unknown asic_id is resolved before indexing, so the table is never queried + task.xcvr_table_helper.get_status_sw_tbl.assert_not_called() + + # A tracked port still reaches its table + task.port_dict['Ethernet0'] = {'asic_id': 0} + task.update_port_transceiver_status_table_sw_cmis_state('Ethernet0', CMIS_STATE_INSERTED) + task.xcvr_table_helper.get_status_sw_tbl.assert_called_once_with(0) + @patch('xcvrd.xcvrd.XcvrTableHelper.get_status_sw_tbl') @patch('xcvrd.xcvrd.platform_chassis') @patch('xcvrd.xcvrd_utilities.common.is_fast_reboot_enabled', MagicMock(return_value=False)) diff --git a/sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py b/sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py index 44d949273..8dbbf1467 100644 --- a/sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py +++ b/sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py @@ -83,7 +83,11 @@ def get_asic_id(self, lport): return self.port_dict.get(lport, {}).get("asic_id", -1) def update_port_transceiver_status_table_sw_cmis_state(self, lport, cmis_state_to_set): - status_table = self.xcvr_table_helper.get_status_sw_tbl(self.get_asic_id(lport)) + # get_asic_id() reports -1 for a port that is no longer tracked, which is not a + # valid key for the per-ASIC tables, so resolve it before indexing rather than + # letting the lookup raise out of this thread and terminate the daemon. + asic_id = self.get_asic_id(lport) + status_table = self.xcvr_table_helper.get_status_sw_tbl(asic_id) if asic_id >= 0 else None if status_table is None: helper_logger.log_error("status_table is None while updating " "sw CMIS state for lport {}".format(lport))