[xcvrd] Add new thread to poll temperatures more aggressively #685

Staphylo · 2025-09-19T14:22:30Z

Description

This change introduces a new DomThermalUpdateInfoTask thread. Its sole purpose is to poll the temperature information out of the transceivers and publish them in a new table called TRANSCEIVER_DOM_TEMPERATURE in STATE_DB.

This change mostly introduces new code.
It however refactors slightly the dom_mgr.py to maximize code reuse between the existing DomUpdateInfoTask and the newly introduced DomThermalInfoUpdateTask by inheriting from a common base class.

The new thread has a polling frequency of 5s to ensure that the data used.
Moving the entire post_port_dom_sensor_info_to_db to this new thread is not ideal because it takes much longer to run that simply gathering the temperature. For this reason a dedicated post_port_dom_temperature_info_to_db was introduced which runs in a fraction of the time.

Motivation and Context

Cooling becomes a challenge for more powerful optics.
These modules can heat up pretty quickly and the cooling algorithm needs to adjust quickly to these variations.
This can however only happen if the algorithm has recent data to use.
As of today the existing DomUpdateInfoTask responsible for updating all DOM, VDM, ... data into STATE_DB, but it is not going this frequently enough.
On a system fully loaded with 64 ports, the data would remain stale in the database for 2+ minutes.
This is not frequent enough to ensure safe, stable and efficient cooling.

How Has This Been Tested?

Unit tests where extended to test the features of this new table and thread.
Testing on a 32x800G and 64x800G system populated with OSFP optics.

Additional Information (Optional)

Benchmarking shows that polling the temperature out of a xcvr and publishing it in the databasse takes around 0.0016 for a 800G optics.
Considering a worst case of 0.002 and considering a device with 64 ports, this amounts to around 0.128s for every loop.
With this loop running every 5s this means this represent a ~2.5% of busyness time which is acceptable.

It should be noted that the DomUpdateInfoTask is currently not efficient at all due to the rather hidden select calls each with a 1s timeout, which for handle_port_update_event happens for every port.

mssonicbld · 2025-09-19T14:22:38Z

/azp run

azure-pipelines · 2025-09-19T14:22:48Z

Azure Pipelines successfully started running 1 pipeline(s).

Staphylo · 2025-09-19T14:23:29Z

@prgeor @judyjoseph FYI this is still being tested but feel free to comment early

bobby-nexthop

Thanks for adding this change @Staphylo! LGTM

sonic-xcvrd/xcvrd/dom/dom_mgr.py

Staphylo · 2025-09-25T16:47:10Z

sonic-xcvrd/xcvrd/dom/utilities/dom_sensor/utils.py

+    def get_transceiver_dom_temperature(self, physical_port):
+        try:
+            return {
+                'temperature': self.sfp_obj_dict[physical_port].get_temperature(),


get_temperature reports 0 when it's not supported by the xcvr.
We should report N/A in such cases after initially checking whether this is supported by the xcvr during init.
We need to also re-verify this when an insertion/removal event is detected.

judyjoseph · 2025-09-30T02:00:33Z

sonic-xcvrd/xcvrd/dom/dom_mgr.py

-            self.on_remove_logical_port(port_change_event)
-        self.port_mapping.handle_port_change_event(port_change_event)
-
    def on_remove_logical_port(self, port_change_event):


Do we need to keep this API here as it is already defined in the base class DomInfoUpdateBase

We want this logic to only run once. I could however move the code related to the dom_temperature_tbl in the DomThermalInfoUpdateTask and not have it in the main Dom loop.
The main advantage was that at this point the DomThermalInfoUpdateTask didn't have to be aware of OIR.

prgeor

@Staphylo can you please test physical OIR? Removal of the optics should clear the temp field and insertion should populate the table and values should get updated periodically?

prgeor · 2025-10-01T14:55:29Z

@Staphylo "With this loop running every 5s this means this represent a ~2.5% of busyness time which is acceptable."
Can you show the math to arrive at 2.5%?

Copilot

Pull Request Overview

This PR introduces a new dedicated thread DomThermalInfoUpdateTask to poll temperature information from transceivers more frequently (every 5 seconds) compared to the existing DOM monitoring thread (60 seconds). This enables more responsive cooling algorithms for high-power optics that heat up quickly.

Adds new TRANSCEIVER_DOM_TEMPERATURE table for storing temperature-only data
Refactors DOM management by creating a common base class DomInfoUpdateBase
Implements optimized temperature-only polling function for better performance

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
xcvr_table_helper.py	Adds support for new TRANSCEIVER_DOM_TEMPERATURE table
xcvrd.py	Integrates new thermal monitoring thread and updates cleanup operations
utils.py	Adds temperature-specific data retrieval function
db_utils.py	Implements database posting function for temperature data
dom_mgr.py	Refactors DOM management with base class and adds thermal monitoring thread
test_xcvrd.py	Updates tests to cover new thread and temperature table functionality

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

sonic-xcvrd/xcvrd/dom/dom_mgr.py

Copilot · 2025-10-03T05:23:46Z

sonic-xcvrd/xcvrd/dom/dom_mgr.py

+                    continue
+
+                if not sfp_status_helper.detect_port_in_error_status(logical_port_name, self.xcvr_table_helper.get_status_sw_tbl(asic_index)):
+                    if not xcvrd._wrapper_get_presence(physical_port):


Using the module-level function xcvrd._wrapper_get_presence creates a circular dependency and tight coupling. Consider injecting this dependency or using the common module's equivalent function.

Suggested change

if not xcvrd._wrapper_get_presence(physical_port):

if not common.get_presence(physical_port):

mssonicbld · 2025-10-06T14:13:39Z

/azp run

azure-pipelines · 2025-10-06T14:13:48Z

Azure Pipelines successfully started running 1 pipeline(s).

mihirpat1

@Staphylo Can you please help with the below?

Ensure that all the tables in update_port_db_diagnostics_on_link_change are updated with link change event
Please capture an output of the TRANSCEIVER_DOM_TEMPERATURE table
Please fix the test failure

sonic-xcvrd/xcvrd/xcvrd_utilities/xcvr_table_helper.py

Junchao-Mellanox · 2025-10-09T02:12:40Z

Which service / component is going to consume the temperature data? Why not change thermalctld who is already providing the temperature data to state DB?

judyjoseph · 2025-10-09T22:05:53Z

Which service / component is going to consume the temperature data? Why not change thermalctld who is already providing the temperature data to state DB?

The idea here was to have a separate thread in xcvrd to get the optics temperature alone, to drive the cooling algorithm faster. The creation of this thread can be controlled via a knob -- need to be turned on only on those platforms which need this.

Btw @keboliu in thermalctld are you referring to this loop where SFP thermals are retrieved:

sonic-platform-daemons/sonic-thermalctld/scripts/thermalctld

Line 614 in 7e594d1

for thermal_index, thermal in enumerate(sfp.get_all_thermals()):

? My understanding was that only xcvrd is polling/getting the thermals of optics and storing it in TRANSRECEIVER_SENSOR tables.

judyjoseph · 2025-10-09T22:11:32Z

/azp run

azure-pipelines · 2025-10-09T22:11:41Z

Azure Pipelines successfully started running 1 pipeline(s).

Junchao-Mellanox · 2025-10-10T02:08:14Z

Hi @judyjoseph, I am OK about the knob. However, thermalcltd is already saving the module temperature to STATE_DB::TEMPERATURE_INFO table, as you mentioned in your latest reply:

sonic-platform-daemons/sonic-thermalctld/scripts/thermalctld

Line 614 in 7e594d1

for thermal_index, thermal in enumerate(sfp.get_all_thermals()):

. I am not sure that we want to invent another thread to do the same job.

Staphylo · 2025-10-14T14:56:42Z

We actually used to report xcvr temperature via the dedicated SfpBase.get_all_thermals() a while ago and were asked to not do so.
Not sure if that's something that would be accepted again but it does mean that on our platforms, only xcvrd is polling the temperature data and publishing it.

From my understanding there is a desire for xcvrd to have sole ownership of all the xcvrs to prevent unwanted IO during critical operations such as firmware upgrades.

On the other hand, thermalctld has a configurable polling rate (default is 60s) so it could be tweaked per platform to some desirable value.
This however would apply to every single sensor and not only xcvrs.

Junchao-Mellanox · 2025-10-15T01:45:21Z

@eddyk-nvidia @keboliu could you please kindly share your comment?

For high power modules it is important to query the temperature often as they can heat up quickly and the cooling algorithm needs to react quickly to avoid damaging the transceiver modules. Introducing a new DomThermalUpdateInfoTask thread which is solely polling the temperature information out of the xcvrs and publishing these in a new TRANSCEIVER_DOM_TEMPERATURE table in STATE_DB. This is mostly additional code being added. To maximize code reuse, the existing DomUpdateInfoTask has been refactored so that it now share a base with the newly introduced DomThermalInfoUpdateTask. Benchmarking shows that polling the temperature out of a xcvrand publishing it inr the databasse takes around 0.0016 for 800G optics. Considering a worst case of 0.002 and considering a device with 64 ports, this amounts to around 0.128s for every loop. With this loop running every 5s this means this represent a ~2.5% of busyness time.

Make the new DomThermalInfoUpdateTask optional. A program argument is now necessary for xcvrd which can be set via the `pmon_daemon_control.json` The new `--dom_temperature_poll_interval` option takes an update interval in seconds. Address misc comments

mssonicbld · 2025-10-17T12:51:38Z

/azp run

azure-pipelines · 2025-10-17T12:51:48Z

Azure Pipelines successfully started running 1 pipeline(s).

bobby-nexthop reviewed Sep 23, 2025

View reviewed changes

Staphylo commented Sep 25, 2025

View reviewed changes

judyjoseph requested review from Junchao-Mellanox, mihirpat1, mlok-nokia and prgeor September 25, 2025 18:08

judyjoseph reviewed Sep 30, 2025

View reviewed changes

prgeor reviewed Oct 1, 2025

View reviewed changes

mihirpat1 requested a review from Copilot October 3, 2025 05:23

Copilot AI reviewed Oct 3, 2025

View reviewed changes

Staphylo force-pushed the xcvrd-thermal-thread branch from 875f690 to b85433b Compare October 6, 2025 14:13

mihirpat1 reviewed Oct 7, 2025

View reviewed changes

sonic-xcvrd/xcvrd/xcvrd_utilities/xcvr_table_helper.py Outdated Show resolved Hide resolved

Staphylo added 3 commits October 17, 2025 12:43

xcvrd: Address review comments

b00b63b

Make the new DomThermalInfoUpdateTask optional. A program argument is now necessary for xcvrd which can be set via the `pmon_daemon_control.json` The new `--dom_temperature_poll_interval` option takes an update interval in seconds. Address misc comments

xcvrd: fix test issue and address more comments

3a0f79d

Staphylo force-pushed the xcvrd-thermal-thread branch from b85433b to 3a0f79d Compare October 17, 2025 12:51

	if not xcvrd._wrapper_get_presence(physical_port):
	if not common.get_presence(physical_port):

[xcvrd] Add new thread to poll temperatures more aggressively #685

Are you sure you want to change the base?

[xcvrd] Add new thread to poll temperatures more aggressively #685

Uh oh!

Conversation

Staphylo commented Sep 19, 2025

Description

Motivation and Context

How Has This Been Tested?

Additional Information (Optional)

Uh oh!

mssonicbld commented Sep 19, 2025

Uh oh!

azure-pipelines bot commented Sep 19, 2025

Uh oh!

Staphylo commented Sep 19, 2025

Uh oh!

bobby-nexthop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Staphylo Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

judyjoseph Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Staphylo Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

prgeor left a comment

Choose a reason for hiding this comment

Uh oh!

prgeor commented Oct 1, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Oct 6, 2025

Uh oh!

azure-pipelines bot commented Oct 6, 2025

Uh oh!

mihirpat1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Junchao-Mellanox commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

judyjoseph commented Oct 9, 2025

Uh oh!

judyjoseph commented Oct 9, 2025

Uh oh!

azure-pipelines bot commented Oct 9, 2025

Uh oh!

Junchao-Mellanox commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Staphylo commented Oct 14, 2025

Uh oh!

Junchao-Mellanox commented Oct 15, 2025

Uh oh!

mssonicbld commented Oct 17, 2025

Uh oh!

azure-pipelines bot commented Oct 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

mihirpat1 left a comment •

edited

Loading

Junchao-Mellanox commented Oct 9, 2025 •

edited

Loading

Junchao-Mellanox commented Oct 10, 2025 •

edited

Loading