OA crash handling to reinitialize port through xcvrd #1432

mihirpat1 · 2023-07-25T06:58:52Z

Proposal to reinitialize ports through xcvrd during OA crash.
The current proposal is created to resolve sonic-net/sonic-platform-daemons#356

PR title	state	context
[sonic-swss] [orchagent]: admin-disable port before setPortSerdesAttribute()
Changes for port reinitialization in case of syncd/swss/oa crash and NPU SI settings update for CMIS transceivers
APIs to help in finding NPU SI settings
[orchagent]Modifying NPU_SI_SETTINGS_SYNC_STATUS based on NPU SI settings application status
Backup STATE_DB PORT_TABLE during warm-reboot

mihirpat1 · 2023-07-25T07:05:26Z

@prgeor @shyam77git @jaganbal-a @amnsinghal - It will be great if you can help in reviewing this HLD

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

amnsinghal · 2023-07-28T05:57:16Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+1. XCVRD main thread init
+	- XCVRD main thread creates the key CMIS_REINIT_REQUIRED in PORT_TABLE:\<port\> (APPL_DB) with value as true for ports which do NOT have this key present 
+	- XCVRD main thread creates the key MEDIA_SETTINGS_SYNC_STATUS in PORT_TABLE:\<port\> (APPL_DB) with value MEDIA_SETTINGS_DEFAULT for ports which do NOT have this key present.  
+      - For transceivers which do not support media settings, MEDIA_SETTINGS_SYNC_STATUS will stay with value MEDIA_SETTINGS_DEFAULT


Suggestion: For transceivers which do not "require" media settings, since this not a module feature, it is a requirement on the NPU side.

I have modified this now.

amnsinghal · 2023-07-28T06:00:34Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+| MEDIA_SETTINGS_DONE     | Orchagent after applying the SI   settings             | CmisManagerTask for proceeding   to CMIS_STATE_DP_DEINIT from CMIS_STATE_MEDIA_SETTINGS_WAIT |
+
+2. SfpStateUpdateTask thread will notify the media settings to OA based on the value of PORT_TABLE:\<port\>.MEDIA_SETTINGS_SYNC_STATUS  
+If PORT_TABLE:\<port\>.MEDIA_SETTINGS_SYNC_STATUS != MEDIA_SETTINGS_DONE, notify media settings will be invoked and will be set to MEDIA_SETTINGS_NOTIFIED for a port supporting media settings.


Should we also check for MEDIA_SETTINGS_NOTIFIED also? Could there be a scenario where SfpStateUpdateTask tries to renotify media settings before MEDIA_SETTINGS_DONE?

SfpStateUpdateTask could try to renotify the media settings before MEDIA_SETTINGS_DONE if xcvrd crashes just after notifying the media settings (in this case, MEDIA_SETTINGS_SYNC_STATUS would be MEDIA_SETTINGS_NOTIFIED).
In such case, OA would apply the media settings but CmisManagerTask will not perform CMIS initialization (along with handling port toggle message from OA) since CmisManagerTask is already dead.
Eventually, once xcvrd spawn again, SfpStateUpdateTask would renotify the media settings to OA followed by CmisManagerTask proceeding with CMIS initialization.
Hence, to allow port toggle message to be sent from OA to CmisManagerTask and allow CMIS initialization in the above xcvrd crash scenario, I am currently not checking for MEDIA_SETTINGS_NOTIFIED. Do you think there could be an issue if we apply the media settings twice?

In xcvrd restart scenario, re-applying or re-notifying media settings might have an impact to the system i.e.
OA is just a conduit. Re-applying would lead to re-notifying media_settings from OA->syncd->SAI->SDK and in turn making port toggle.
Should avoid port toggling here

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

amnsinghal · 2023-07-28T06:08:05Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+    SfpStateUpdateTask ->> SfpStateUpdateTask : event = SFP_STATUS_REMOVED
+    SfpStateUpdateTask -x STATE_DB : Delete TRANSCEIVER_INFO table for the port
+    par         CmisManagerTask, SfpStateUpdateTask
+        CmisManagerTask ->> CmisManagerTask : Transition CMIS SM to CMIS_STATE_REMOVED


Should we set CMIS_REINIT_REQUIRED to True upon module removal?

I am not currently changing the value of CMIS_REINIT_REQUIRED after setting it as False since the purpose of this flag is to drive CMIS re-initialization and not CMIS initialization.
The existing flow in the CMIS SM already ensures that CMIS initialization will be performed after module insertion.

Let me know your thoughts on this.

shyam77git · 2023-07-28T16:40:20Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+|          Value          |                Modifier thread and event               |                                  Consumer thread and purpose                                 |
+|:-----------------------:|:------------------------------------------------------:|:--------------------------------------------------------------------------------------------:|
+| MEDIA_SETTINGS_DEFAULT  | XCVRD main thread during cold   start of xcvrd         | XCVRD main thread   during boot-up for deciding to notify media settings                     |
+|                         | SfpStateUpdateTask during   transceiver removal        |                                                                                              |


Please correct indentation (unnecessary gaps between words)
Also bullet the two threads for readability, as:

XCVRD main thread....

SfpStateUpdateTask thread...

I have addressed this now.

shyam77git · 2023-07-28T16:45:17Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+
+|          Value          |                Modifier thread and event               |                                  Consumer thread and purpose                                 |
+|:-----------------------:|:------------------------------------------------------:|:--------------------------------------------------------------------------------------------:|
+| MEDIA_SETTINGS_DEFAULT  | XCVRD main thread during cold   start of xcvrd         | XCVRD main thread   during boot-up for deciding to notify media settings                     |


In this row, I believe consumer thread captures both uses cases - boot-up and transceiver insertion.
In that case, please add 'transceiver insertion' here too

Should we add a check in OA as well to perform port toggle and set MEDIA_SETTINGS_DONE only if MEDIA_SETTINGS_SYNC_STATUS == MEDIA_SETTINGS_NOTIFIED?

As we discussed, SfpStateUpdateTask will perform apply media settings upon transceiver insertion irrespective of the value of MEDIA_SETTINGS_SYNC_STATUS (unlike the case of SfpStateUpdateTask checking the value during boot-up).

shyam77git · 2023-07-28T17:04:34Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+3. The OA upon receiving media settings will
+	- Disable port admin status
+	- Apply SI settings
+	- PORT_TABLE:\<port\>.MEDIA_SETTINGS_SYNC_STATUS = MEDIA_SETTINGS_DONE


[1] Please elaborate "Apply SI settings" to something on lines...Ask SAI-SDK to apply SI settings
[2] Add Next bullet - Based on the update/response as SUCCESS from SAI call,
set PORT_TABLE:<port>.MEDIA_SETTINGS_SYNC_STATUS = MEDIA_SETTINGS_DONE
Enable port admin status

In case of failure update/response from SAI call, what would be the system flow? how would the failure be handled?
I beleive host_tx_ready settings is part of this workflow. Please add it here to the workflow

I have addressed this now.

shyam77git · 2023-07-28T17:12:28Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+	- MEDIA_SETTINGS_SYNC_STATUS != MEDIA_SETTINGS_DONE  
+If all the above conditions are true, CMIS SM transitions to CMIS_STATE_MEDIA_SETTINGS_WAIT state.  
+If port doesn't require media settings to be applied, CMIS SM will proceed with normal code flow (transitions to CMIS_STATE_DP_DEINIT)  
+Overall, no functionality change related to CMIS SM transitions is intended for ports not supporting media settings


"If port doesn't require media settings to be applied,..."
This should be checked as part of previous point/bullet (i.e. the port supports media settings...)
bail out from there itself (i.e. no further action) in case port doesn't require media settings

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

shyam77git · 2023-07-28T17:36:35Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+```
+
+## Test plan and expectation
+|       Event      | APPL_DB cleared | Xcvrd restarted | Media renotify | MEDIA_SETTINGS_SYNC_STATUS value on   xcvrd boot-up for initialized transceiver | CMIS re-init triggered | Link flap |


Change Media renotify to Media settings renotify

I have addressed this now

shyam77git · 2023-07-28T17:44:08Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+| Swss restart     | Y               | Y               | Y              | MEDIA_SETTINGS_DEFAULT                                                        | Y                      | Y         |
+| Syncd restart    | Y               | Y               | Y              | MEDIA_SETTINGS_DEFAULT                                                        | Y                      | Y         |
+| config   reload  | Y               | Y               | Y              | MEDIA_SETTINGS_DEFAULT                                                        | Y                      | Y         |
+| Cold reboot      | Y               | Y               | Y              | MEDIA_SETTINGS_DEFAULT                                                        | Y                      | Y         |


config reload and cold reboot triggers/use-cases doesn't have link flap (until link come operationally up and then go down post bring up).
Since all SW components went down along with HW ones (NPU, PHY, optics etc.), so should mark this case as N/A under this link flap column

I have addressed this now.

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

eddyk-nvidia · 2023-07-29T08:05:47Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+    CMIS_STATE_DP_TXON --> CMIS_STATE_DP_ACTIVATE
+    CMIS_STATE_DP_ACTIVATE --> CMIS_STATE_READY
+```
+


Probably not related to this specific design but what is the reason for setting CMIS_STATE_READY when either "host_tx_ready" is not yet TRUE or admin_status is not UP ? Why do we skip to the final state of the CMIS state machine in this use-case ?

When this was initially implemented, the thought at that time was to move the CMIS SM to a fixed state rather than waiting for an event.
However, if you think we should create a new state to handle host_tx_ready and admin_status rather than moving to CMIS_STATE_READY if either fields have the value false/down respectively, I can plan to implement it accordingly. Let me know your opinion on this.

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

eddyk-nvidia · 2023-07-29T08:18:51Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+	- XCVRD main thread creates the key CMIS_REINIT_REQUIRED in PORT_TABLE:\<port\> (APPL_DB) with value as true for ports which do NOT have this key present 
+	- XCVRD main thread creates the key MEDIA_SETTINGS_SYNC_STATUS in PORT_TABLE:\<port\> (APPL_DB) with value MEDIA_SETTINGS_DEFAULT for ports which do NOT have this key present.  
+      - For transceivers which do not require media settings, MEDIA_SETTINGS_SYNC_STATUS will stay with value MEDIA_SETTINGS_DEFAULT
+


To know that the XCVRD main thread might need to parse "media_settings.json" file. Ports which have no match in this file don't require media settings. Is it the intention to do ? I know that with recent changes parsing/setting of media settings moved from init code of the XCVRD main thread to init of SfpStateUpdate task. May be this setting after parsing of .json file should be done there ? Or MEDIA_SETTING_DEFAULT can be put for all ports regardless media settings for them in .json file ? Then if SfpStateUpdate taks finds data for a port in .json it will set MEDIA_SETTING_NOTIFIED ? What is the impact of having MEDIA_SETTING_DEFAULT for all ports ?

To know that the XCVRD main thread might need to parse "media_settings.json" file. Ports which have no match in this file don't require media settings. Is it the intention to do ?
[MP] Yes - if the media settings of a transceiver are not part of "media_settings.json", we would still keep the port with the value MEDIA_SETTING_DEFAULT

I know that with recent changes parsing/setting of media settings moved from init code of the XCVRD main thread to init of SfpStateUpdate task. May be this setting after parsing of .json file should be done there ? Or MEDIA_SETTING_DEFAULT can be put for all ports regardless media settings for them in .json file ?
[MP] I think it is better to initialize this key to MEDIA_SETTING_DEFAULT from xcvrd main thread since this key is used by both SfpStateUpdateTask and CmisManagerTask for handling media settings application for a port and these threads are running parallelly.

Then if SfpStateUpdate taks finds data for a port in .json it will set MEDIA_SETTING_NOTIFIED ? What is the impact of having MEDIA_SETTING_DEFAULT for all ports ?
[MP] Yes, once SfpStateUpdateTask find the media settings in .json file, the value of MEDIA_SETTINGS_SYNC_STATUS changes to MEDIA_SETTING_NOTIFIED. I think there is no harm in setting to MEDIA_SETTING_DEFAULT for all ports irrespective of its media settings requirement from the xcvrd main thread perspective. Also, the advantage with this approach is we do not introduce any race conditions between OA, SfpStateUpdateTask and CmisManagerTask threads related to the role or sequence in modifying the value of MEDIA_SETTINGS_SYNC_STATUS .

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

Junchao-Mellanox · 2023-07-31T03:24:25Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+|:-----------------------:|:------------------------------------------------------:|:--------------------------------------------------------------------------------------------:|
+| MEDIA_SETTINGS_DEFAULT  | XCVRD main thread during cold   start of xcvrd         | XCVRD main thread   during boot-up for deciding to notify media settings                     |
+|                         | SfpStateUpdateTask during   transceiver removal        |                                                                                              |
+| MEDIA_SETTINGS_NOTIFIED | SfpStateUpdateTask while   updating the media settings | Not being used currently                                                                     |


what is this state used for? no consumer?

This state is currently unused. We can use this for debugging purpose for now to check if XCVRD has applied and notified the media settings to OA

…teUpdateTask and CmisManagerTask threads

…g module v/s NPU SI settings in future

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

jaganbal-a · 2023-09-21T17:15:26Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+    end
+```
+
+## XCVRD termination during syncd/swss/orchagent crash


What if any SFF complaint transceiver requires different SI value to be programmed to NPU. eg copper module with different length without ANLT may need a different SI on NPU serdes.
This approach is tied with cmis manager/CMIS complaint modules only.

The below will take care of handling SFF compliant transceivers requiring different NPU SI settings. Let me know if I am missing something here.

"For non-CMIS SM driven transceivers, SfpStateUpdateTask thread will update NPU SI settings in the PORT_TABLE (APPL_DB) and notify to OA based on the value of PORT_TABLE:.NPU_SI_SETTINGS_SYNC_STATUS"

shyam77git · 2023-09-22T16:37:05Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+6. XCVRD will subscribe to PORT_TABLE in APPL_DB and trigger self-restart if the PORT_TABLE is deleted for the namespace.  
+All threads will be gracefully terminated and xcvrd deinit will be performed followed by issuing a SIGABRT to ensure XCVRD is restarted automatically by supervisord. After respawn, CMIS re-init and NPU_SI_SETTINGS notified is triggered for the ports belonging to the affected namespace
+
+7. syncd/swss/orchagent restart clears the entire APPL-DB, including “NPU_SI_SETTINGS_SYNC_STATUS” and "CMIS_REINIT_REQUIRED" in PORT_TABLE


This section is referring to "syncd/swss/orchagent crash" handling scenario.
Here "syncd/swss/orchagent restart" is mentioned.

Does 'restart' of these docker containers has the same workflow?

Please check from present codebase/image working state as well and ensure same/different handling (if any)

Yes, 'restart' of these docker containers has the same workflow.
I have now verified from that restarting swss indeed clears the APPL_DB. I have also added "restart swss" as part of my test plan to cover this scenario.

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

…e_required

Junchao-Mellanox

This solution looks compliated. How about following:

If xcvrd crashes and restarts, it always notify media settings. And orchagent should keep a cache of media settings, only apply media settings when configuration changes
If a cable is plug out and plug in, xcvrd should remove media settings from APP DB and re-apply it.
if orchagent crashes, it should re-read from redis automatically, so, no issue.

Please share your thought. Thanks.

mihirpat1 · 2023-10-12T06:25:04Z

This solution looks compliated. How about following:

If xcvrd crashes and restarts, it always notify media settings. And orchagent should keep a cache of media settings, only apply media settings when configuration changes

If a cable is plug out and plug in, xcvrd should remove media settings from APP DB and re-apply it.

if orchagent crashes, it should re-read from redis automatically, so, no issue.

Please share your thought. Thanks.

Hi Junchao,

With the above suggestion, we will not be able to hold CMIS initialization (for CMIS based modules) while the NPU SI settings are being applied by the orchagent.
Currently, the orchagent disables the port before applying NPU SI settings followed by enabling the port again after application.
Hence, we can have a case wherein CMIS initialization has completed for a port and the link is up. After sometime, the orchagent applies NPU SI settings. The port now goes through the disable->enable sequence by orchagent which causes the link flap.

To solve this issue (link flap), the current HLD has a separate code path for CMIS modules to notify orchagent with NPU SI settings.

prgeor

@mihirpat1 can you add revision 1.0 to the Revision table at the top of this document?

prgeor · 2023-10-17T18:57:36Z

Proposal to reinitialize porta through xcvrd during OA crash. The current proposal is created to resolve sonic-net/sonic-platform-daemons#356

The PR details will be updated soon.

@mihirpat1 please update the PR details (including OA PR as well)

mihirpat1 · 2023-10-17T22:56:13Z

This solution looks compliated. How about following:

If xcvrd crashes and restarts, it always notify media settings. And orchagent should keep a cache of media settings, only apply media settings when configuration changes

If a cable is plug out and plug in, xcvrd should remove media settings from APP DB and re-apply it.

if orchagent crashes, it should re-read from redis automatically, so, no issue.

Please share your thought. Thanks.

Hi Junchao,

With the above suggestion, we will not be able to hold CMIS initialization (for CMIS based modules) while the NPU SI settings are being applied by the orchagent. Currently, the orchagent disables the port before applying NPU SI settings followed by enabling the port again after application. Hence, we can have a case wherein CMIS initialization has completed for a port and the link is up. After sometime, the orchagent applies NPU SI settings. The port now goes through the disable->enable sequence by orchagent which causes the link flap.

To solve this issue (link flap), the current HLD has a separate code path for CMIS modules to notify orchagent with NPU SI settings.

In addition to the above, XCVRD can miss the port toggle message being sent from orchagent to xcvrd if the CmisTaskManager thread is busy handling other tasks.
The port toggle message is sent from orchagent to xcvrd to disable port before NPU settings are being applied and then enable port after the settings have been applied.
The current design of redis-db ensures only the final state of the DB being handled when the worker thread starts handling requests. So if CmisTaskManager thread is busy handling other events while port toggle message is sent out, both the port disable and enable events could be missed.

prgeor · 2023-10-17T18:39:38Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+
+When syncd/swss/orchagent crashes, all ports in the corresponding namespace will be reinitialized by xcvrd irrespective of the current state of the port. All the corresponding ports are expected to experience link down until the initialization is complete.  
+If just xcvrd crashes and restarts, then forced re-initialization (CMIS reinit + NPU SI settings notification) of ports will not be performed. Hence, the ports will not experience link downtime during scenario.
+CMIS_REINIT_REQUIRED and NPU_SI_SETTINGS_SYNC_STATUS keys in PORT_TABLE:\<port\> (APPL_DB) are used to determine if port re-initialization is required or not.


@mihirpat1 any specific reason why APPL_DB and not STATE_DB?

I have added the description for it now.

prgeor · 2023-10-17T19:27:51Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+| Value                    | Modifier thread and event                                                                                                                                                                                           | Consumer thread and purpose                                                                     |
+| ------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------- |
+| NPU_SI_SETTINGS_DEFAULT  | 1\. XCVRD main thread during cold start of XCVRD<br>2. SfpStateUpdateTask during transceiver removal                                                                                                                | XCVRD main thread during boot-up for deciding to notify NPU SI settings                         |
+| NPU_SI_SETTINGS_NOTIFIED | 1\. SfpStateUpdateTask while updating and notifying the NPU SI settings for non-CMIS SM driven transceivers<br> 2. CmisManagerTask while updating and notifying the NPU SI settings for CMIS SM driven transceivers | Not being used currently                                                                        |


can you also mention in the table we SfpStateUpdateTask for notifying SI setting for non-CMIS transceviers for historical reason

I have added the description for it now.

prgeor · 2023-10-17T19:44:18Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+
+5. The CmisManagerTask thread will set “CMIS_REINIT_REQUIRED" to false after CMIS SM reaches to a steady state (CMIS_STATE_UNKNOWN, CMIS_STATE_FAILED, CMIS_STATE_READY and CMIS_STATE_REMOVED) for the corresponding port
+
+6. XCVRD will subscribe to PORT_TABLE in APPL_DB and trigger self-restart if the PORT_TABLE is deleted for the namespace.  


@mihirpat1 in which all cases PORT_TABLE deletion happens?

I have covered the same in "Test plan and expectation" section.

@mihirpat1 can you also describe the rationale behind restarting Xcvrd in case swss/syncd/OA restart/crash?

prgeor · 2023-10-17T20:41:22Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+If just xcvrd crashes and restarts, then forced re-initialization (CMIS reinit + NPU SI settings notification) of ports will not be performed. Hence, the ports will not experience link downtime during scenario.
+CMIS_REINIT_REQUIRED and NPU_SI_SETTINGS_SYNC_STATUS keys in PORT_TABLE:\<port\> (APPL_DB) are used to determine if port re-initialization is required or not.
+  - CMIS_REINIT_REQUIRED key states if CMIS re-initialization is required for a port after xcvrd is spawned. CMIS_REINIT_REQUIRED helps in mainly driving CMIS re-initialization after syncd/swss/orchagent crash since it will allow reinitializing ports belonging to the relevant namespace of the crashing process. This key is not planned to drive CMIS initialization after transceiver insertion.  
+  - NPU_SI_SETTINGS_SYNC_STATUS key is used as a means to communicate the status of applying NPU SI settings for a transceiver requiring NPU SI settings. This key is used to update the NPU SI settings application status between SfpStateUpdateTask, CmisManagerTask and Orchagent. In case of warm reboot or xcvrd restart, this key will prevent application of NPU SI settings on the port if the settings are already applied. In case of transceiver insertion, NPU SI settings will be applied irrespective of the NPU SI settings application status for the port.  


@mihirpat1 this key prevents additional link flap ?

I have added the description for it now.

mihirpat1 · 2023-10-20T18:26:46Z

@mihirpat1 can you add revision 1.0 to the Revision table at the top of this document?

I have added it now.

mihirpat1 · 2023-10-20T18:44:51Z

Proposal to reinitialize porta through xcvrd during OA crash. The current proposal is created to resolve sonic-net/sonic-platform-daemons#356
The PR details will be updated soon.

@mihirpat1 please update the PR details (including OA PR as well)

Added OA PR for now. Will add other PRs once I upload.

mihirpat1 · 2023-10-23T20:49:39Z

@eddyk-nvidia - It will be great if you can help in reviewing this

Junchao-Mellanox

LGTM

prgeor · 2023-12-05T03:36:36Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+In case of continuous restart of xcvrd, both the keys will still hold the same value as before the restart. This would ensure that the port re-initialization is resumed from the last known state.  
+
+Rationale for choosing APPL_DB over STATE_DB for storing CMIS_REINIT_REQUIRED and NPU_SI_SETTINGS_SYNC_STATUS keys
+ - APPL_DB is the first DB which gets cleared as part of SWSS crash handling. This inturn helps XCVRD to detect DB removal and terminate child threads and itself earlier than other DBs.


@mihirpat1 should we reconsider APPL_DB because, i do see oper status in APPL_DB as well

@prgeor - I have now updated the HLD with the usage of STATE_DB for storing the NPU_SI_SETTINGS_SYNC_STATUS and CMIS_REINIT_REQUIRED keys.

prgeor · 2023-12-05T03:41:30Z

doc/sfp-cmis/Interface-Link-bring-up-sequence.md

+| ------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------- |
+| NPU_SI_SETTINGS_DEFAULT  | 1\. XCVRD main thread during cold start of XCVRD<br>2. SfpStateUpdateTask during transceiver removal                                                                                                                | XCVRD main thread during boot-up for deciding to notify NPU SI settings                         |
+| NPU_SI_SETTINGS_NOTIFIED | 1\. SfpStateUpdateTask while updating and notifying the NPU SI settings for non-CMIS SM driven transceivers (this approach was chosen to preserve the existing behavior for non-CMIS SM driven transceivers)<br> 2. CmisManagerTask while updating and notifying the NPU SI settings for CMIS SM driven transceivers | Not being used currently                                                                        |
+| NPU_SI_SETTINGS_DONE     | Orchagent after applying the SI settings                                                                                                                                                                            | CmisManagerTask for proceeding from CMIS_STATE_NPU_SI_SETTINGS_WAIT to CMIS_STATE_DP_INIT state |


@mihirpat1 does platform that does not need SI settings, do those port's CMIS state machine transition to DP init?

Yes - For platforms or ports not requiring NPU SI settings, the CMIS state machine will transition to DP init state (i.e. skip transition to CMIS_STATE_NPU_SI_SETTINGS_WAIT)

…TE_DB

* OA crash handling to reinitialize port through xcvrd * Added table to list values for MEDIA_SETTINGS_SYNC_STATUS * Fixed typo * Addressed PR comment * Addressed PR comments and separated media notification between SfpStateUpdateTask and CmisManagerTask threads * Modified media settings to NPU SI settings for clearly differentiating module v/s NPU SI settings in future * Fixed typo * Addressed PR comments * Reverted unrelated changeset * Addressed PR comments * Added pre-requisite section and added testcase for OA restart * Corrected typo in XCVRD init sequence diagram * Added description and functioning related to is_npu_si_settings_update_required * Fixed typo * Addressed review comments * Replaced usage of PORT_TABLE for storing the keys from APPL_DB to STATE_DB

OA crash handling to reinitialize port through xcvrd

f4533cd

mihirpat1 marked this pull request as ready for review July 25, 2023 07:03

mihirpat1 requested review from shyam77git and prgeor July 25, 2023 07:04

shivuv reviewed Jul 25, 2023

View reviewed changes

doc/sfp-cmis/Interface-Link-bring-up-sequence.md Show resolved Hide resolved

mihirpat1 added 2 commits July 25, 2023 21:20

Added table to list values for MEDIA_SETTINGS_SYNC_STATUS

de1c276

Fixed typo

b5713ec

prgeor requested a review from keboliu July 26, 2023 21:22

amnsinghal reviewed Jul 28, 2023

View reviewed changes

Addressed PR comment

2fc0904

shyam77git reviewed Jul 28, 2023

View reviewed changes

doc/sfp-cmis/Interface-Link-bring-up-sequence.md Show resolved Hide resolved

shyam77git reviewed Jul 28, 2023

View reviewed changes

doc/sfp-cmis/Interface-Link-bring-up-sequence.md Outdated Show resolved Hide resolved

shyam77git reviewed Jul 28, 2023

View reviewed changes

doc/sfp-cmis/Interface-Link-bring-up-sequence.md Outdated Show resolved Hide resolved

eddyk-nvidia reviewed Jul 29, 2023

View reviewed changes

Junchao-Mellanox reviewed Jul 31, 2023

View reviewed changes

mihirpat1 added 3 commits August 1, 2023 18:03

Addressed PR comments and separated media notification between SfpSta…

04f8417

…teUpdateTask and CmisManagerTask threads

Modified media settings to NPU SI settings for clearly differentiatin…

2aadb30

…g module v/s NPU SI settings in future

Fixed typo

cb4e2d2

jaganbal-a reviewed Sep 21, 2023

View reviewed changes

mihirpat1 added 2 commits September 22, 2023 05:20

Addressed PR comments

49acc4c

Reverted unrelated changeset

2b298bf

shyam77git reviewed Sep 22, 2023

View reviewed changes

doc/sfp-cmis/Interface-Link-bring-up-sequence.md Outdated Show resolved Hide resolved

Added pre-requisite section and added testcase for OA restart

2d1e79b

jaganbal-a approved these changes Oct 2, 2023

View reviewed changes

mihirpat1 added 3 commits October 5, 2023 21:43

Corrected typo in XCVRD init sequence diagram

b68d292

Added description and functioning related to is_npu_si_settings_updat…

2bfc779

…e_required

Fixed typo

2fe94b5

mihirpat1 mentioned this pull request Oct 11, 2023

Add intelligence to xcvrd to understand process restart and config reload sonic-net/sonic-platform-daemons#377

Closed

Junchao-Mellanox suggested changes Oct 11, 2023

View reviewed changes

prgeor reviewed Oct 17, 2023

View reviewed changes

prgeor reviewed Oct 18, 2023

View reviewed changes

Addressed review comments

23b7aa3

aravindmani-1 approved these changes Oct 24, 2023

View reviewed changes

Junchao-Mellanox approved these changes Oct 26, 2023

View reviewed changes

Junchao-Mellanox mentioned this pull request Nov 27, 2023

[CMIS manager] CMIS state machine flap once due to apply SI parameter sonic-net/sonic-buildimage#17303

Open

prgeor reviewed Dec 5, 2023

View reviewed changes

Replaced usage of PORT_TABLE for storing the keys from APPL_DB to STA…

ce17121

…TE_DB

mihirpat1 mentioned this pull request Sep 26, 2024

Xcvrd crash and restart should not cause link flap on platforms needing custom NPU SI settings sonic-net/sonic-platform-daemons#541

Merged

prgeor approved these changes Sep 27, 2024

View reviewed changes

prgeor merged commit 95d91ff into sonic-net:master Sep 27, 2024
1 check passed


		5. The CmisManagerTask thread will set “CMIS_REINIT_REQUIRED" to false after CMIS SM reaches to a steady state (CMIS_STATE_UNKNOWN, CMIS_STATE_FAILED, CMIS_STATE_READY and CMIS_STATE_REMOVED) for the corresponding port

		6. XCVRD will subscribe to PORT_TABLE in APPL_DB and trigger self-restart if the PORT_TABLE is deleted for the namespace.

OA crash handling to reinitialize port through xcvrd #1432

OA crash handling to reinitialize port through xcvrd #1432

Conversation

mihirpat1 commented Jul 25, 2023 • edited Loading

mihirpat1 commented Jul 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shyam77git Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Junchao-Mellanox left a comment

Choose a reason for hiding this comment

mihirpat1 commented Oct 12, 2023

prgeor left a comment

Choose a reason for hiding this comment

prgeor commented Oct 17, 2023

mihirpat1 commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prgeor Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihirpat1 commented Oct 20, 2023

mihirpat1 commented Oct 20, 2023

mihirpat1 commented Oct 23, 2023

Junchao-Mellanox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihirpat1 commented Jul 25, 2023 •

edited

Loading

shyam77git Jul 28, 2023 •

edited

Loading

prgeor Dec 5, 2023 •

edited

Loading