From c0b9ec6964f0b1fa5b5db051c0f926553972fb17 Mon Sep 17 00:00:00 2001 From: ryanzhu706 Date: Wed, 10 Sep 2025 14:03:49 -0700 Subject: [PATCH] =?UTF-8?q?Handle=20restore=5Fcount=20and=20system=5Fenabl?= =?UTF-8?q?e=20=E2=80=9DNoneType=E2=80=9C=20value=20scenario.=20(#680)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Handle restore_count and system_enable ”NoneType“ value scenario. * Pass self * Use helper_logger instead of passing self. * Remove self * Add log_exception_traceback() * Delete whitespace --- sonic-xcvrd/tests/test_xcvrd.py | 32 ++++++++++++++++++++++++++++++++ sonic-xcvrd/xcvrd/xcvrd.py | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/sonic-xcvrd/tests/test_xcvrd.py b/sonic-xcvrd/tests/test_xcvrd.py index d58fa2e..c8c24f6 100644 --- a/sonic-xcvrd/tests/test_xcvrd.py +++ b/sonic-xcvrd/tests/test_xcvrd.py @@ -509,6 +509,38 @@ def test_post_port_sfp_firmware_info_to_db_lport_list_None(self, mock_get_presen dom_info_update.post_port_sfp_firmware_info_to_db(logical_port_name, port_mapping, firmware_info_tbl, stop_event) assert firmware_info_tbl.set.call_count == 0 + @pytest.mark.parametrize( + "restore_count, system_enabled, expected", + [ + (1, None, True), + (0, None, False), + ("2", None, True), + ("0", None, False), + (None, "true", True), + (None, "false", False), + (None, None, False), + ] + ) + def test_is_syncd_warm_restore_complete_valid_cases(self, restore_count, system_enabled, expected): + mock_db = MagicMock() + mock_db.hget.side_effect = lambda table, key: ( + restore_count if "WARM_RESTART_TABLE|syncd" in table else system_enabled + ) + + with patch("xcvrd.xcvrd.daemon_base.db_connect", return_value=mock_db): + assert is_syncd_warm_restore_complete() == expected + + def test_is_syncd_warm_restore_complete_invalid_restore_count(self): + # restore_count = "abc" triggers ValueError in int("abc") + mock_db = MagicMock() + mock_db.hget.side_effect = lambda table, key: ( + "abc" if "WARM_RESTART_TABLE|syncd" in table else None + ) + + with patch("xcvrd.xcvrd.daemon_base.db_connect", return_value=mock_db): + result = is_syncd_warm_restore_complete() + assert result is False + def test_post_port_dom_sensor_info_to_db(self): def mock_get_transceiver_dom_sensor_real_value(physical_port): return { diff --git a/sonic-xcvrd/xcvrd/xcvrd.py b/sonic-xcvrd/xcvrd/xcvrd.py index 8c4f32b..f544893 100644 --- a/sonic-xcvrd/xcvrd/xcvrd.py +++ b/sonic-xcvrd/xcvrd/xcvrd.py @@ -462,6 +462,34 @@ def is_warm_reboot_enabled(): is_warm_start = warmstart.isWarmStart() return is_warm_start +def is_syncd_warm_restore_complete(): + """ + This function determins whether syncd's restore count is not 0, which indicates warm-reboot + to avoid premature config push by xcvrd that caused port flaps. + """ + state_db = daemon_base.db_connect("STATE_DB") + restore_count = state_db.hget("WARM_RESTART_TABLE|syncd", "restore_count") + system_enabled = state_db.hget("WARM_RESTART_ENABLE_TABLE|system", "enable") + try: + # --- Handle restore_count (could be int, str, or None) --- + if restore_count is not None: + if isinstance(restore_count, int): + if restore_count > 0: + return True + elif isinstance(restore_count, str): + if restore_count.strip().isdigit() and int(restore_count.strip()) > 0: + return True + + # --- Handle system_enabled (only care about "true"/"false"/None) --- + if isinstance(system_enabled, str): + if system_enabled.strip().lower() == "true": + return True + + except Exception as e: + helper_logger.log_warning(f"Unexpected value: restore_count={restore_count}, system_enabled={system_enabled}, error={e}") + log_exception_traceback() + return False + # # Helper classes =============================================================== # @@ -1493,7 +1521,7 @@ def _post_port_sfp_info_and_dom_thr_to_db_once(self, port_mapping, xcvr_table_he transceiver_dict = {} retry_eeprom_set = set() - is_warm_start = is_warm_reboot_enabled() + is_warm_start = is_syncd_warm_restore_complete() # Post all the current interface sfp/dom threshold info to STATE_DB logical_port_list = port_mapping.logical_port_list for logical_port_name in logical_port_list: @@ -2216,7 +2244,7 @@ def init(self): def deinit(self): self.log_info("Start daemon deinit...") - is_warm_fast_reboot = is_warm_reboot_enabled() or is_fast_reboot_enabled() + is_warm_fast_reboot = is_syncd_warm_restore_complete() or is_fast_reboot_enabled() # Delete all the information from DB and then exit port_mapping_data = port_event_helper.get_port_mapping(self.namespaces)