diff --git a/sonic-xcvrd/tests/test_xcvrd.py b/sonic-xcvrd/tests/test_xcvrd.py index 1c31cbc7f..6d3b41323 100644 --- a/sonic-xcvrd/tests/test_xcvrd.py +++ b/sonic-xcvrd/tests/test_xcvrd.py @@ -582,7 +582,7 @@ def test_DaemonXcvrd_run_with_exception(self, mock_task_join_sff, mock_task_join mock_init.return_value = PortMapping() xcvrd = DaemonXcvrd(SYSLOG_IDENTIFIER) xcvrd.enable_sff_mgr = True - xcvrd.dom_temperature_poll_interval = 10 + xcvrd.config.dom_temperature_poll_interval = 10 xcvrd.load_feature_flags = MagicMock() xcvrd.stop_event.wait = MagicMock() xcvrd.run() @@ -6942,27 +6942,26 @@ def test_DomInfoUpdateTask_dom_update_interval_parameter(self): # Test 5: Verify that DEFAULT_DOM_INFO_UPDATE_PERIOD_SECS is not modified assert DomInfoUpdateTask.DEFAULT_DOM_INFO_UPDATE_PERIOD_SECS == 60 - def test_DaemonXcvrd_dom_update_interval_parameter(self): - """Test that DaemonXcvrd correctly handles and passes dom_update_interval parameter""" - # Test 1: When dom_update_interval is None - daemon = DaemonXcvrd(SYSLOG_IDENTIFIER, skip_cmis_mgr=False, enable_sff_mgr=False, - dom_temperature_poll_interval=None, dom_update_interval=None) - assert daemon.dom_update_interval is None - - # Test 2: When dom_update_interval is 0 - daemon = DaemonXcvrd(SYSLOG_IDENTIFIER, skip_cmis_mgr=False, enable_sff_mgr=False, - dom_temperature_poll_interval=None, dom_update_interval=0) - assert daemon.dom_update_interval == 0 - - # Test 3: When dom_update_interval is a custom value - daemon = DaemonXcvrd(SYSLOG_IDENTIFIER, skip_cmis_mgr=False, enable_sff_mgr=False, - dom_temperature_poll_interval=None, dom_update_interval=120) - assert daemon.dom_update_interval == 120 - - # Test 4: When dom_update_interval is 1000 - daemon = DaemonXcvrd(SYSLOG_IDENTIFIER, skip_cmis_mgr=False, enable_sff_mgr=False, - dom_temperature_poll_interval=None, dom_update_interval=1000) - assert daemon.dom_update_interval == 1000 + def test_DaemonXcvrd_resolves_config(self): + """DaemonXcvrd populates self.config from XcvrdConfig.resolve(). + + The resolution logic itself (platform-file layering, coercion, defaults) + is covered in tests/test_xcvrd_config.py; here we only verify the wiring. + """ + resolved = MagicMock(dom_temperature_poll_interval=5, dom_update_interval=30) + with patch('xcvrd.xcvrd.XcvrdConfig.resolve', return_value=resolved) as mock_resolve: + daemon = DaemonXcvrd(SYSLOG_IDENTIFIER, skip_cmis_mgr=False, enable_sff_mgr=False) + mock_resolve.assert_called_once() + assert daemon.config is resolved + assert daemon.config.dom_temperature_poll_interval == 5 + assert daemon.config.dom_update_interval == 30 + + def test_DaemonXcvrd_config_defaults(self): + """With no platform overrides, dom_* tunables fall back to None defaults.""" + with patch('xcvrd.xcvrd.XcvrdConfig.resolve', return_value=XcvrdConfig()): + daemon = DaemonXcvrd(SYSLOG_IDENTIFIER) + assert daemon.config.dom_temperature_poll_interval is None + assert daemon.config.dom_update_interval is None def wait_until(total_wait_time, interval, call_back, *args, **kwargs): wait_time = 0 diff --git a/sonic-xcvrd/tests/test_xcvrd_config.py b/sonic-xcvrd/tests/test_xcvrd_config.py new file mode 100644 index 000000000..af8b49540 --- /dev/null +++ b/sonic-xcvrd/tests/test_xcvrd_config.py @@ -0,0 +1,234 @@ +""" +Unit tests for XcvrdConfig: the layered resolver for xcvrd's dom_* tunables. + +Precedence under test (highest wins): + 1. "xcvrd" section of pmon_daemon_control.json (hwsku file over platform file) + 2. built-in dataclass defaults +""" +import json +import os +import sys + +from unittest.mock import patch + +test_path = os.path.dirname(os.path.abspath(__file__)) +modules_path = os.path.dirname(test_path) +sys.path.insert(0, modules_path) + +from sonic_py_common.pmon_daemon_config import PMON_DAEMON_CONTROL_FILE +from xcvrd.xcvrd_utilities.xcvrd_config import XcvrdConfig, MAX_INTERVAL_SECS + +# Path patched in _read_platform_section's module so no real device dir is +# touched. The read lives in the shared base, not in xcvrd's schema module. +PATHS_FN = "sonic_py_common.pmon_daemon_config.device_info.get_paths_to_platform_and_hwsku_dirs" + + +def write_control_file(directory, payload): + """Write a pmon_daemon_control.json with the given dict into directory.""" + os.makedirs(directory, exist_ok=True) + path = os.path.join(directory, PMON_DAEMON_CONTROL_FILE) + with open(path, "w") as f: + json.dump(payload, f) + return path + + +class TestXcvrdConfigDefaults: + def test_defaults_when_no_overrides(self): + cfg = XcvrdConfig.resolve(platform_section={}) + assert cfg.dom_temperature_poll_interval is None + assert cfg.dom_update_interval is None + + def test_bare_construction_matches_defaults(self): + # Legacy path: DaemonXcvrd builds XcvrdConfig() directly. + cfg = XcvrdConfig() + assert cfg.dom_temperature_poll_interval is None + assert cfg.dom_update_interval is None + + +class TestXcvrdConfigMerge: + def test_platform_section_overrides_defaults(self): + cfg = XcvrdConfig.resolve(platform_section={ + "dom_temperature_poll_interval": 5, + "dom_update_interval": 30, + }) + assert cfg.dom_temperature_poll_interval == 5 + assert cfg.dom_update_interval == 30 + + def test_partial_section_leaves_other_field_at_default(self): + cfg = XcvrdConfig.resolve(platform_section={"dom_update_interval": 30}) + assert cfg.dom_update_interval == 30 + assert cfg.dom_temperature_poll_interval is None + + def test_none_value_does_not_override(self): + cfg = XcvrdConfig.resolve(platform_section={"dom_update_interval": None}) + assert cfg.dom_update_interval is None + + def test_zero_is_preserved(self): + # 0 is a meaningful value (continuous polling) and must not be dropped. + cfg = XcvrdConfig.resolve(platform_section={"dom_update_interval": 0}) + assert cfg.dom_update_interval == 0 + + def test_string_value_is_coerced_to_int(self): + # JSON could carry a stringified number; mirror the old argparse type=int. + cfg = XcvrdConfig.resolve(platform_section={"dom_update_interval": "30"}) + assert cfg.dom_update_interval == 30 + assert isinstance(cfg.dom_update_interval, int) + + def test_invalid_value_is_ignored_and_keeps_default(self): + cfg = XcvrdConfig.resolve(platform_section={"dom_update_interval": "not-a-number"}) + assert cfg.dom_update_interval is None + + def test_unknown_key_is_ignored(self): + cfg = XcvrdConfig.resolve(platform_section={ + "dom_update_interval": 30, + "some_future_unknown_key": 99, + }) + assert cfg.dom_update_interval == 30 + assert not hasattr(cfg, "some_future_unknown_key") + + +class TestReadPlatformSection: + def test_missing_files_yield_empty(self, tmp_path): + platform_dir = str(tmp_path / "platform") + hwsku_dir = str(tmp_path / "hwsku") + with patch(PATHS_FN, return_value=(platform_dir, hwsku_dir)): + assert XcvrdConfig._read_platform_section() == {} + + def test_reads_platform_file_when_no_hwsku_file(self, tmp_path): + platform_dir = str(tmp_path / "platform") + hwsku_dir = str(tmp_path / "hwsku") + write_control_file(platform_dir, {"xcvrd": {"dom_update_interval": 30}}) + with patch(PATHS_FN, return_value=(platform_dir, hwsku_dir)): + assert XcvrdConfig._read_platform_section() == {"dom_update_interval": 30} + + def test_hwsku_file_takes_precedence_over_platform_file(self, tmp_path): + platform_dir = str(tmp_path / "platform") + hwsku_dir = str(tmp_path / "hwsku") + write_control_file(platform_dir, {"xcvrd": {"dom_update_interval": 30}}) + write_control_file(hwsku_dir, {"xcvrd": {"dom_update_interval": 99}}) + with patch(PATHS_FN, return_value=(platform_dir, hwsku_dir)): + # Mirrors docker_init: the hwsku file wins; no cross-file merge. + assert XcvrdConfig._read_platform_section() == {"dom_update_interval": 99} + + def test_hwsku_file_without_xcvrd_section_does_not_fall_back(self, tmp_path): + # docker_init consults only the first existing file; if the hwsku file + # exists but lacks an "xcvrd" section, we do not read the platform file. + platform_dir = str(tmp_path / "platform") + hwsku_dir = str(tmp_path / "hwsku") + write_control_file(platform_dir, {"xcvrd": {"dom_update_interval": 30}}) + write_control_file(hwsku_dir, {"skip_xcvrd": False}) + with patch(PATHS_FN, return_value=(platform_dir, hwsku_dir)): + assert XcvrdConfig._read_platform_section() == {} + + def test_no_xcvrd_section_yields_empty(self, tmp_path): + platform_dir = str(tmp_path / "platform") + hwsku_dir = str(tmp_path / "hwsku") + write_control_file(platform_dir, {"skip_ledd": True}) + with patch(PATHS_FN, return_value=(platform_dir, hwsku_dir)): + assert XcvrdConfig._read_platform_section() == {} + + def test_malformed_json_yields_empty(self, tmp_path): + platform_dir = str(tmp_path / "platform") + hwsku_dir = str(tmp_path / "hwsku") + os.makedirs(platform_dir) + with open(os.path.join(platform_dir, PMON_DAEMON_CONTROL_FILE), "w") as f: + f.write("{ this is not valid json") + with patch(PATHS_FN, return_value=(platform_dir, hwsku_dir)): + assert XcvrdConfig._read_platform_section() == {} + + def test_non_dict_xcvrd_section_yields_empty(self, tmp_path): + platform_dir = str(tmp_path / "platform") + hwsku_dir = str(tmp_path / "hwsku") + write_control_file(platform_dir, {"xcvrd": "oops-not-an-object"}) + with patch(PATHS_FN, return_value=(platform_dir, hwsku_dir)): + assert XcvrdConfig._read_platform_section() == {} + + def test_device_info_failure_yields_empty(self): + with patch(PATHS_FN, side_effect=RuntimeError("platform undetermined")): + assert XcvrdConfig._read_platform_section() == {} + + def test_empty_dir_path_is_skipped(self, tmp_path): + # get_paths_to_platform_and_hwsku_dirs may return an empty hwsku path; + # that entry is skipped rather than joined into a bogus path. + platform_dir = str(tmp_path / "platform") + write_control_file(platform_dir, {"xcvrd": {"dom_update_interval": 30}}) + with patch(PATHS_FN, return_value=(platform_dir, "")): + assert XcvrdConfig._read_platform_section() == {"dom_update_interval": 30} + + +class TestResolveEndToEnd: + def test_resolve_reads_from_disk(self, tmp_path): + platform_dir = str(tmp_path / "platform") + hwsku_dir = str(tmp_path / "hwsku") + write_control_file(platform_dir, {"xcvrd": { + "dom_temperature_poll_interval": 5, + "dom_update_interval": 30, + }}) + with patch(PATHS_FN, return_value=(platform_dir, hwsku_dir)): + cfg = XcvrdConfig.resolve() + assert cfg.dom_temperature_poll_interval == 5 + assert cfg.dom_update_interval == 30 + + def test_resolve_defaults_when_nothing_on_disk(self, tmp_path): + platform_dir = str(tmp_path / "platform") + hwsku_dir = str(tmp_path / "hwsku") + with patch(PATHS_FN, return_value=(platform_dir, hwsku_dir)): + cfg = XcvrdConfig.resolve() + assert cfg.dom_temperature_poll_interval is None + assert cfg.dom_update_interval is None + + +class TestRangeValidation: + """Values that coerce cleanly but are not valid configuration are rejected. + + A rejected value keeps the built-in default and logs a warning; it never + stops xcvrd from starting. + """ + + def test_negative_dom_temperature_poll_interval_keeps_default(self): + # This is the case the range check exists for: DomThermalInfoUpdateTask + # never validated poll_interval, so a negative value left its next-poll + # time permanently in the past and the sweep ran back-to-back instead of + # once a minute. Default None means the thermal thread is not started. + cfg = XcvrdConfig.resolve(platform_section={"dom_temperature_poll_interval": -60}) + assert cfg.dom_temperature_poll_interval is None + + def test_negative_dom_update_interval_keeps_default(self): + # DomInfoUpdateTask already guarded against this one and fell back to its + # 60s default; the check now happens once, before the value is handed out. + cfg = XcvrdConfig.resolve(platform_section={"dom_update_interval": -1}) + assert cfg.dom_update_interval is None + + def test_negative_interval_as_string_keeps_default(self): + # Validation runs after coercion, so the stringified form is caught too. + cfg = XcvrdConfig.resolve(platform_section={"dom_update_interval": "-1"}) + assert cfg.dom_update_interval is None + + def test_interval_above_maximum_keeps_default(self): + cfg = XcvrdConfig.resolve(platform_section={ + "dom_update_interval": MAX_INTERVAL_SECS + 1}) + assert cfg.dom_update_interval is None + + def test_interval_boundaries_are_accepted(self): + # Bounds are inclusive; 0 stays meaningful (continuous polling). + assert XcvrdConfig.resolve( + platform_section={"dom_update_interval": 0}).dom_update_interval == 0 + assert XcvrdConfig.resolve( + platform_section={"dom_update_interval": MAX_INTERVAL_SECS} + ).dom_update_interval == MAX_INTERVAL_SECS + + def test_both_fields_are_bounded(self): + cfg = XcvrdConfig.resolve(platform_section={ + "dom_temperature_poll_interval": MAX_INTERVAL_SECS + 1, + "dom_update_interval": MAX_INTERVAL_SECS + 1, + }) + assert cfg.dom_temperature_poll_interval is None + assert cfg.dom_update_interval is None + + def test_one_rejected_field_does_not_drop_the_others(self): + cfg = XcvrdConfig.resolve(platform_section={ + "dom_update_interval": -1, + "dom_temperature_poll_interval": 5, + }) + assert cfg.dom_update_interval is None + assert cfg.dom_temperature_poll_interval == 5 diff --git a/sonic-xcvrd/xcvrd/xcvrd.py b/sonic-xcvrd/xcvrd/xcvrd.py index ee1545334..3560cc555 100644 --- a/sonic-xcvrd/xcvrd/xcvrd.py +++ b/sonic-xcvrd/xcvrd/xcvrd.py @@ -36,6 +36,7 @@ from .xcvrd_utilities import media_settings_parser from .xcvrd_utilities import optics_si_parser from .xcvrd_utilities import common + from .xcvrd_utilities.xcvrd_config import XcvrdConfig from xcvrd.dom.utilities.dom_sensor.db_utils import DOMDBUtils from xcvrd.dom.utilities.vdm.db_utils import VDMDBUtils @@ -875,14 +876,16 @@ def update_log_level(self): class DaemonXcvrd(daemon_base.DaemonBase): - def __init__(self, log_identifier, skip_cmis_mgr=False, enable_sff_mgr=False, dom_temperature_poll_interval=None, dom_update_interval=None): + def __init__(self, log_identifier, skip_cmis_mgr=False, enable_sff_mgr=False): super(DaemonXcvrd, self).__init__(log_identifier, enable_runtime_log_config=True) self.stop_event = threading.Event() self.sfp_error_event = threading.Event() self.skip_cmis_mgr = skip_cmis_mgr self.enable_sff_mgr = enable_sff_mgr - self.dom_temperature_poll_interval = dom_temperature_poll_interval - self.dom_update_interval = dom_update_interval + # Resolve dom_* tunables from the "xcvrd" section of the per-platform + # pmon_daemon_control.json (see XcvrdConfig). Degrades to built-in + # defaults when the file/section is absent or unreadable. + self.config = XcvrdConfig.resolve() self.namespaces = [''] self.threads = [] self.sfp_obj_dict = {} @@ -1162,15 +1165,15 @@ def run(self): self.threads.append(cmis_manager) # Start the dom sensor info update thread - dom_info_update = DomInfoUpdateTask(self.namespaces, port_mapping_data, self.sfp_obj_dict, self.stop_event, self.skip_cmis_mgr, self.dom_update_interval) + dom_info_update = DomInfoUpdateTask(self.namespaces, port_mapping_data, self.sfp_obj_dict, self.stop_event, self.skip_cmis_mgr, self.config.dom_update_interval) dom_info_update.start() self.threads.append(dom_info_update) # Start the dom thermal sensor info update thread dom_thermal_info_update = None - if self.dom_temperature_poll_interval is not None: + if self.config.dom_temperature_poll_interval is not None: dom_thermal_info_update = DomThermalInfoUpdateTask(self.namespaces, port_mapping_data, self.sfp_obj_dict, self.stop_event, - self.dom_temperature_poll_interval) + self.config.dom_temperature_poll_interval) dom_thermal_info_update.start() self.threads.append(dom_thermal_info_update) @@ -1246,12 +1249,9 @@ def main(): parser = argparse.ArgumentParser() parser.add_argument('--skip_cmis_mgr', action='store_true') parser.add_argument('--enable_sff_mgr', action='store_true') - parser.add_argument('--dom_temperature_poll_interval', default=None, type=int) - parser.add_argument('--dom_update_interval', default=None, type=int) args = parser.parse_args() - xcvrd = DaemonXcvrd(SYSLOG_IDENTIFIER, args.skip_cmis_mgr, args.enable_sff_mgr, - args.dom_temperature_poll_interval, args.dom_update_interval) + xcvrd = DaemonXcvrd(SYSLOG_IDENTIFIER, args.skip_cmis_mgr, args.enable_sff_mgr) xcvrd.run() diff --git a/sonic-xcvrd/xcvrd/xcvrd_utilities/xcvrd_config.py b/sonic-xcvrd/xcvrd/xcvrd_utilities/xcvrd_config.py new file mode 100644 index 000000000..85f1fd81e --- /dev/null +++ b/sonic-xcvrd/xcvrd/xcvrd_utilities/xcvrd_config.py @@ -0,0 +1,61 @@ +""" +xcvrd's schema for the shared pmon daemon configuration resolver. + +The mechanism - locating pmon_daemon_control.json, extracting a daemon's +section, layering it over the built-in defaults, coercing types, validating +ranges, and degrading safely on any error - lives in +sonic_py_common.pmon_daemon_config and is shared with the other pmon daemons. +This module only declares what xcvrd accepts. + +Precedence, highest wins: + 1. Per-platform / per-hwsku file - the "xcvrd" section of pmon_daemon_control.json + 2. Built-in defaults - the dataclass field defaults below + +The per-platform file is read from the same device directories (and with the +same hwsku-over-platform precedence) that docker_init.j2 uses and that the +existing media_settings.json / optics_si_settings.json parsers already read. + +A new tunable is added by declaring one field on XcvrdConfig plus one +_FIELD_SPECS entry giving its type coercion and valid range. Platform owners set +it in the "xcvrd" section they already maintain; no template, argparse, or +constructor change. +""" + +from dataclasses import dataclass +from typing import Optional + +from sonic_py_common.pmon_daemon_config import FieldSpec, PmonDaemonConfig + +XCVRD_SECTION = "xcvrd" + +# Shared upper bound for the cadence tunables. A poll interval longer than a day +# is operationally indistinguishable from "disabled" and is far more likely a +# units mistake (milliseconds entered where seconds are expected) than an intent, +# so it is rejected rather than obeyed. Not a functional limit. +MAX_INTERVAL_SECS = 86400 + +# Coercion and validation applied to file values before they are stored. JSON +# numbers already arrive as the right type; the caster guards against a value +# given as a string (e.g. "20") and mirrors the int parsing the old --flag +# arguments did. The bounds then reject values that coerce cleanly but are not +# valid configuration - notably a negative cadence, which DomThermalInfoUpdateTask +# would otherwise turn into an undelayed poll loop. A rejected value keeps the +# built-in default and logs a warning; it never stops xcvrd from starting. None +# values are never coerced or stored - they mean "no override". +_FIELD_SPECS = { + 'dom_temperature_poll_interval': FieldSpec(caster=int, minimum=0, maximum=MAX_INTERVAL_SECS), + 'dom_update_interval': FieldSpec(caster=int, minimum=0, maximum=MAX_INTERVAL_SECS), +} + + +@dataclass +class XcvrdConfig(PmonDaemonConfig): + SECTION_NAME = XCVRD_SECTION + FIELD_SPECS = _FIELD_SPECS + + # Built-in defaults (lowest precedence). None is meaningful and must be + # preserved: downstream a None dom_temperature_poll_interval disables the + # thermal poll thread, and a None dom_update_interval lets DomInfoUpdateTask + # fall back to its own DEFAULT_DOM_INFO_UPDATE_PERIOD_SECS. + dom_temperature_poll_interval: Optional[int] = None + dom_update_interval: Optional[int] = None