[xcvrd] Bootstrap the CMIS state machine for ports that miss their PORT_SET

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))

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)