Skip to content

[sonic-py-common] Add shared PmonDaemonConfig resolver for pmon daemon tunables - #28859

Draft
aditya-nexthop wants to merge 1 commit into
sonic-net:masterfrom
nexthop-ai:aditya.pmon-daemon-config
Draft

[sonic-py-common] Add shared PmonDaemonConfig resolver for pmon daemon tunables#28859
aditya-nexthop wants to merge 1 commit into
sonic-net:masterfrom
nexthop-ai:aditya.pmon-daemon-config

Conversation

@aditya-nexthop

Copy link
Copy Markdown
Contributor

Why I did it

pmon daemon tunables are plumbed end-to-end as command-line flags. A platform sets a value in the daemon's section of pmon_daemon_control.json, sonic-cfggen loads that file while rendering docker-pmon.supervisord.conf.j2, the template flattens it into --flag value, argparse re-parses it, and the daemon constructor grows another parameter. Adding one knob means editing four places, and it has to be redone for every daemon.

This is not specific to one daemon. thermalctld carries the pattern today with five tunables (thermal_monitor_initial_interval, thermal_monitor_update_interval, thermal_monitor_update_elapsed_threshold, enable_liquid_cooling, liquid_cooling_update_interval), xcvrd carries it with two, and every future pmon tunable would repeat it.

There is a second problem the flag path leaves unsolved: nothing validates the value. A negative interval parses as a perfectly good int, and consumers disagree about what happens next. In xcvrd, DomInfoUpdateTask checks for a negative dom_update_interval and falls back to its 60s default, while DomThermalInfoUpdateTask never checks poll_interval at all — a negative value there leaves the next scheduled poll permanently in the past, so the sweep runs back-to-back with no delay. A typo like -60 produces maximum transceiver I2C load instead of one poll per minute.

Design discussion: sonic-net/SONiC#2362.

Work item tracking
  • Microsoft ADO (number only): N/A

How I did it

Added sonic_py_common/pmon_daemon_config.py, which owns everything that is not specific to a daemon: locating pmon_daemon_control.json (hwsku over platform, mirroring docker_init.j2), extracting the daemon's section, layering it over the built-in defaults, coercing types, validating ranges, and degrading to defaults on any error.

A daemon adopts it by declaring a dataclass subclass:

@dataclass
class XcvrdConfig(PmonDaemonConfig):
    SECTION_NAME = 'xcvrd'
    FIELD_SPECS = {
        'dom_update_interval': FieldSpec(caster=int, minimum=0, maximum=86400),
    }

    dom_update_interval: Optional[int] = None

Adoption is per-daemon and independent — a daemon that has not migrated keeps its flags and is unaffected.

Details worth calling out:

  • FieldSpec carries per-field policy: caster, inclusive minimum/maximum, and choices. Bounds are checked after coercion, so "-1" is caught as well as -1. A field that is genuinely unbounded declares FieldSpec(caster=...) with no bounds, keeping "unbounded" an explicit choice.
  • to_bool exists because bool() cannot be a caster for boolean tunables: bool("false") is True, so a platform writing the string "false" would enable the feature it meant to turn off.
  • An __init_subclass__ guard rejects a schema whose fields and FIELD_SPECS disagree. FIELD_SPECS is keyed by field name, so a key matching no field would never be applied and would silently leave that tunable with no coercion and no range check. That is a static error in schema code, so it raises at import and is caught by the first unit test that loads the module.
  • _post_merge() is a hook for constraints spanning two fields, which FieldSpec cannot express declaratively. It is a no-op by default.

Nothing here raises on input. An unreadable file, a malformed section, an uncoercible value, or an out-of-range value keeps the built-in default and logs a warning, so a bad tunable can never keep a pmon daemon down.

This PR adds the module and its tests only. There is no in-tree consumer yet and no behavior change to any existing daemon; the first adopter is sonic-net/sonic-platform-daemons#854, which imports from here and therefore needs this to merge first.

How to verify it

cd src/sonic-py-common
pytest tests/pmon_daemon_config_test.py

75 tests covering defaults and overrides, coercion, inclusive range boundaries, choices, to_bool spellings (including "false"False), the schema guard in both directions, the _post_merge hook, section isolation between two schemas, hwsku-over-platform file precedence, and every degradation path (missing file, malformed JSON, non-dict section, empty directory entry, device_info failure).

Which release branch to backport (provide reason below if selected)

  • 202305
  • 202311
  • 202405
  • 202411
  • 202505
  • 202511
  • 202512
  • 202605
  • 202608

Not applicable — this is a new feature, not a fix.

Tracking issue/work item for backport/cherry-pick request (GitHub issue or Microsoft ADO):
Failure type: N/A

Tested branch

  • master

Test result

master: pytest tests/pmon_daemon_config_test.py → 75 passed.

Description for the changelog

Add a shared PmonDaemonConfig resolver so pmon daemons can read validated runtime tunables from their pmon_daemon_control.json section instead of command-line flags.

Link to config_db schema for YANG module changes

N/A — no YANG or Config DB changes. Tunables are sourced from the per-platform pmon_daemon_control.json, not from Config DB.

…n tunables

pmon daemon tunables are plumbed as command-line flags: the platform sets a
value in pmon_daemon_control.json, sonic-cfggen loads it while rendering
docker-pmon.supervisord.conf.j2, the template flattens it into "--flag value",
argparse re-parses it, and the daemon constructor grows a parameter. Adding one
knob means editing four places, once per daemon. xcvrd and thermalctld both
carry the pattern today, thermalctld with five tunables.

Add PmonDaemonConfig, which owns everything not specific to a daemon: locating
pmon_daemon_control.json (hwsku over platform, mirroring docker_init.j2),
extracting the daemon's section, layering it over the built-in defaults,
coercing types, validating ranges, and degrading to defaults on any error. A
daemon adopts it by declaring a dataclass subclass with a SECTION_NAME and one
field per tunable, so adoption is per-daemon and independent.

FieldSpec carries the per-field policy. Coercion alone is not validation: a
negative interval coerces to a perfectly good int but is not a valid cadence,
and consumers differ in whether they notice. An __init_subclass__ guard rejects
a schema whose fields and FIELD_SPECS disagree, since a spec keyed by a name no
field declares would silently leave that tunable unvalidated.

to_bool exists because bool() cannot be a caster for boolean tunables:
bool("false") is True, so a platform writing the string "false" would enable
the feature it meant to turn off.

Signed-off-by: aditya-nexthop <aditya@nexthop.ai>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

aditya-nexthop added a commit to nexthop-ai/sonic-platform-daemons that referenced this pull request Aug 5, 2026
XcvrdConfig owned the whole resolution mechanism: locating
pmon_daemon_control.json, extracting the section, merging, coercing, and error
handling. None of that is xcvrd-specific, and thermalctld would have to copy it
verbatim to get the same benefit, so it moves to
sonic_py_common.pmon_daemon_config.PmonDaemonConfig. This module is left as
xcvrd's schema: the section it owns, its fields, and their specs.

Replace _FIELD_CASTERS with _FIELD_SPECS so each tunable declares a valid range
alongside its caster. Coercion alone let through values that are not valid
configuration, and the two consumers disagreed about what happened next:
DomInfoUpdateTask rejected a negative dom_update_interval and fell back to its
60s default, while DomThermalInfoUpdateTask never checked poll_interval at all.
A negative value there left the next scheduled poll permanently in the past, so
the sweep ran back-to-back with no delay: a typo like -60 produced maximum
transceiver I2C load instead of one poll per minute. Bounds are now enforced in
one place, after coercion, so the stringified form is caught too. A rejected
value keeps the built-in default and logs a warning; it never stops xcvrd from
starting. The DomInfoUpdateTask guard stays as defense-in-depth for direct
constructor callers.

Depends on the sonic-py-common change adding PmonDaemonConfig
(sonic-net/sonic-buildimage#28859), which must merge first.

Signed-off-by: aditya-nexthop <aditya@nexthop.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants