diff --git a/sonic-xcvrd/tests/test_xcvrd.py b/sonic-xcvrd/tests/test_xcvrd.py index a25d742ea..f3bb752cd 100644 --- a/sonic-xcvrd/tests/test_xcvrd.py +++ b/sonic-xcvrd/tests/test_xcvrd.py @@ -3294,6 +3294,137 @@ 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 + + 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 b39992fa7..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)) @@ -122,6 +126,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 +1256,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 +1309,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)