Xcvrd Refactor 2/12: Move CmisManagerTask into it's own module #691

bobby-nexthop · 2025-09-26T14:54:38Z

Description

No logic changes. The changes are:

Move CmisManagerTask into its own module in xcvrd/cmis/cmis_manager_task.py
Move common definitions into common.py
Update unit test file and fix some whitespace issues

Motivation and Context

xcvrd has gotten to 4000 lines long. To make things easier, we'd like to refactor it. This is the second PR in a series that aims to do the following:

Task	PR
1) Move functions used across multiple files in xcvrd to utils/common.py	#654
2) Move CmisManagerTask into cmis/cmis_manager_task.py	bobby-nexthop#1
3) Split task_worker into process_lport and process_cmis_state	bobby-nexthop#2
4) Rename xcvrd/xcvrd_utilities to xcvrd/utils	bobby-nexthop#3
5) Add handle_cmis_inserted_state function	bobby-nexthop#4
6) Add handle_cmis_dp_pre_init_check_state function	bobby-nexthop#5
7) Add handle_cmis_dpdeinit_state function	bobby-nexthop#6
8) Add handle_cmis_ap_conf_state function	bobby-nexthop#7
9) Add handle_cmis_dp_init_state	bobby-nexthop#8
10) Add handle_cmis_txon_state	bobby-nexthop#9
11) Add handle_cmis_activate_state	bobby-nexthop#10

The new flow will go from calling task_worker() to

How Has This Been Tested?

All unit tests pass
tested on device with no issues, links all up.

Additional Information (Optional)

mssonicbld · 2025-09-26T14:54:47Z

/azp run

azure-pipelines · 2025-09-26T14:54:55Z

Azure Pipelines successfully started running 1 pipeline(s).

bobby-nexthop · 2025-09-26T14:54:57Z

@prgeor

Copilot

Pull Request Overview

This PR moves the CmisManagerTask class from the main xcvrd.py file into its own dedicated module as part of a larger refactoring effort to improve code organization and maintainability.

Key changes:

Moves CmisManagerTask class to xcvrd/cmis/cmis_manager_task.py
Migrates CMIS state constants and common functions to xcvrd_utilities/common.py
Updates imports and references throughout the codebase to use the new module structure

Reviewed Changes

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

Show a summary per file

File	Description
sonic-xcvrd/xcvrd/xcvrd_utilities/common.py	Adds CMIS state constants and is_syncd_warm_restore_complete function
sonic-xcvrd/xcvrd/xcvrd.py	Removes CmisManagerTask class and related functions, updates imports and constructor call
sonic-xcvrd/xcvrd/dom/dom_mgr.py	Updates import to use CMIS_TERMINAL_STATES from common module
sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py	New file containing the extracted CmisManagerTask class
sonic-xcvrd/xcvrd/cmis/init.py	Module initialization file for the cmis package
sonic-xcvrd/tests/test_xcvrd.py	Updates test imports and patches to use new module structure

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

sonic-xcvrd/xcvrd/xcvrd_utilities/common.py

sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py

sonic-xcvrd/tests/test_xcvrd.py

mihirpat1

@bobby-nexthop Can you please help with the below

Can you please capture a sample log message from a CMIS manager related log
Capture log showing all the expected threads of xcvrd are spawned

mihirpat1 · 2025-10-08T21:36:03Z

sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py

+        self.xcvr_table_helper = XcvrTableHelper(self.namespaces)
+
+    def log_debug(self, message):
+        helper_logger.log_debug("CMIS: {}".format(message))


@bobby-nexthop Wondering if we still need to have CMIS: in the log since the syslogger now should display the thread name by default.

2025 Oct 24 19:19:24.176885 sonic NOTICE pmon#CmisManagerTask[32]: CMIS: Ethernet56: READY 2025 Oct 24 19:19:24.178387 sonic NOTICE pmon#CmisManagerTask[32]: CMIS: Ethernet56: updated TRANSCEIVER_INFO_TABLE [('active_apsel_hostlane1', '1'), ('active_apsel_hostlane2', '1'), ('active_apsel_hostlane3', '1'), ('active_apsel_hostlane4', '1'), ('active_apsel_hostlane5', '1'), ('active_apsel_hostlane6', '1'), ('active_apsel_hostlane7', '1'), ('active_apsel_hostlane8', '1'), ('host_lane_count', '8'), ('media_lane_count', '8')]

and

2025 Oct 24 19:19:17.758374 sonic NOTICE pmon#xcvrd[32]: Started thread SffManagerTask 2025 Oct 24 19:19:17.758374 sonic NOTICE pmon#xcvrd[32]: Started thread CmisManagerTask 2025 Oct 24 19:19:17.758374 sonic NOTICE pmon#xcvrd[32]: Started thread DomInfoUpdateTask 2025 Oct 24 19:19:17.758478 sonic NOTICE pmon#xcvrd[32]: Started thread SfpStateUpdateTask

I agree the CMIS is now redundant. Let me update that and push.

linux-foundation-easycla · 2025-10-24T21:45:30Z

The committers listed above are authorized under a signed CLA.

✅ login: bobby-nexthop / name: Bobby McGonigle (197a792, 39beb67)

mssonicbld · 2025-10-24T21:45:33Z

/azp run

azure-pipelines · 2025-10-24T21:45:42Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-10-24T21:48:27Z

/azp run

azure-pipelines · 2025-10-24T21:48:36Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull Request Overview

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

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-10-24T23:15:24Z

sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py

+    def log_debug(self, message):
+        helper_logger.log_debug(message)
+
+    def log_notice(self, message):
+        helper_logger.log_notice(message)
+
+    def log_error(self, message):
+        helper_logger.log_error(message)
+


The logging methods are simple wrappers that add no value and could be replaced with direct calls to helper_logger. This would reduce code duplication and simplify maintenance. Alternatively, if custom formatting is needed, the wrapper methods should add the "CMIS: " prefix that was present in the original implementation.

Suggested change

def log_debug(self, message):

helper_logger.log_debug(message)

def log_notice(self, message):

helper_logger.log_notice(message)

def log_error(self, message):

helper_logger.log_error(message)

Copilot · 2025-10-24T23:15:24Z

sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py

+                # A retry should always start over at INSETRTED state, while the
+                # expiration will reset the state to INSETRTED and advance the


Corrected spelling of 'INSERTED' in the comment. The original version had 'INSETRTED'.

Suggested change

# A retry should always start over at INSETRTED state, while the

# expiration will reset the state to INSETRTED and advance the

# A retry should always start over at INSERTED state, while the

# expiration will reset the state to INSERTED and advance the

Copilot · 2025-10-24T23:15:25Z

sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py

+                # A retry should always start over at INSETRTED state, while the
+                # expiration will reset the state to INSETRTED and advance the


Corrected spelling of 'INSERTED' in the comment. The original version had 'INSETRTED'.

Suggested change

# A retry should always start over at INSETRTED state, while the

# expiration will reset the state to INSETRTED and advance the

# A retry should always start over at INSERTED state, while the

# expiration will reset the state to INSERTED and advance the

Copilot · 2025-10-24T23:15:25Z

sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py

+
+                        need_update = self.is_cmis_application_update_required(api, appl, host_lanes_mask)
+
+                        # For ZR module, Datapath needs to be re-initialized on new channel selection


[nitpick] Inconsistent capitalization: 'needs' should be 'needs' (lowercase 'n') for consistency, or change 'Datapath' to 'datapath' for consistency with the rest of the comment style in the file.

Suggested change

# For ZR module, Datapath needs to be re-initialized on new channel selection

# For ZR module, datapath needs to be re-initialized on new channel selection

Copilot · 2025-10-24T23:15:25Z

sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py

+                                self.force_cmis_reinit(lport, retries + 1)
+                                continue
+
+                        # Ensure the Datapath is NOT Activated unless the host Tx siganl is good.


Corrected spelling of 'signal' in the comment. The original version had 'siganl'.

Suggested change

# Ensure the Datapath is NOT Activated unless the host Tx siganl is good.

# Ensure the Datapath is NOT Activated unless the host Tx signal is good.

Copilot · 2025-10-24T23:15:26Z

sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py

+                        self.update_cmis_state_expiration_time(lport, max(modulePwrUpDuration, dpDeinitDuration))
+
+                    elif state == CMIS_STATE_AP_CONF:
+                        # Explicit control bit to apply custom Host SI settings. 


[nitpick] The comment spacing is inconsistent with a trailing space after the period on line 971, while line 972 has no trailing space. This should be standardized for consistency.

mihirpat1 requested a review from Copilot October 8, 2025 19:01

Copilot AI reviewed Oct 8, 2025

View reviewed changes

mihirpat1 reviewed Oct 9, 2025

View reviewed changes

Refactor CMIS management into its own class

197a792

bobby-nexthop force-pushed the xcvrd-common-refactor-2 branch from 0162f09 to 494dd0d Compare October 24, 2025 21:45

Address review comments

39beb67

bobby-nexthop force-pushed the xcvrd-common-refactor-2 branch from 494dd0d to 39beb67 Compare October 24, 2025 21:48

mihirpat1 requested a review from Copilot October 24, 2025 23:13

Copilot AI reviewed Oct 24, 2025

View reviewed changes

mihirpat1 approved these changes Oct 25, 2025

View reviewed changes

		# A retry should always start over at INSETRTED state, while the
		# expiration will reset the state to INSETRTED and advance the


		need_update = self.is_cmis_application_update_required(api, appl, host_lanes_mask)

		# For ZR module, Datapath needs to be re-initialized on new channel selection

	# Ensure the Datapath is NOT Activated unless the host Tx siganl is good.
	# Ensure the Datapath is NOT Activated unless the host Tx signal is good.

Xcvrd Refactor 2/12: Move CmisManagerTask into it's own module #691

Are you sure you want to change the base?

Xcvrd Refactor 2/12: Move CmisManagerTask into it's own module #691

Uh oh!

Conversation

bobby-nexthop commented Sep 26, 2025

Description

Motivation and Context

How Has This Been Tested?

Additional Information (Optional)

Uh oh!

mssonicbld commented Sep 26, 2025

Uh oh!

azure-pipelines bot commented Sep 26, 2025

Uh oh!

bobby-nexthop commented Sep 26, 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!

Uh oh!

Uh oh!

mihirpat1 left a comment

Choose a reason for hiding this comment

Uh oh!

mihirpat1 Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

bobby-nexthop Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

linux-foundation-easycla bot commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mssonicbld commented Oct 24, 2025

Uh oh!

azure-pipelines bot commented Oct 24, 2025

Uh oh!

mssonicbld commented Oct 24, 2025

Uh oh!

azure-pipelines bot commented Oct 24, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

linux-foundation-easycla bot commented Oct 24, 2025 •

edited

Loading