Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HLD for 'Have a deterministic approach in SONiC for Interface Link bring-up sequence' #916

Merged
merged 14 commits into from
Feb 8, 2022

Conversation

shyam77git
Copy link
Contributor

No description provided.

@ghost
Copy link

ghost commented Dec 23, 2021

CLA assistant check
All CLA requirements met.

@shyam77git shyam77git changed the title HLD to have determistic approach for Itnerface Link bring-up sequence HLD to have determistic approach for Interface Link bring-up sequence Dec 23, 2021
@prgeor prgeor self-requested a review December 23, 2021 14:16
@prgeor
Copy link
Contributor

prgeor commented Dec 23, 2021

@keboliu can you review

@shyam77git shyam77git changed the title HLD to have determistic approach for Interface Link bring-up sequence HLD for 'Have a deterministic approach in SONiC for Interface Link bring-up sequence' Dec 23, 2021
@rlhui
Copy link
Contributor

rlhui commented Dec 24, 2021

Is this needed for 400G transceivers only? Is CMIS transceivers support in sonic a pre-requisite of this feature or no?

@shyam77git
Copy link
Contributor Author

shyam77git commented Jan 17, 2022

Is this needed for 400G transceivers only?

  • For 400G optics/transceivers its mandatory as per the CMIS spec.
  • For 100G and other optics type/variant, its highly recommended and followed as well (on existing LCs)
    Please refer to "Proposal" section of this HLD for further details and reasonings.

Is CMIS transceivers support in sonic a pre-requisite of this feature or no?

Yes. CMIS transceiver API is expected to be in place & working prior to having this HLD workflow.
This workflow would come on top of the CMIS API i.e.,
xcvrd would invoke/call the CMIS transceiver API on receiving state update of host_tx_ready field in PORT_TABLE of STATE_DB. This API in turn would perform data_path init (on 400G optics) and tx laser enable (for 100G, 40G optics)





Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do we plan to test this feature? can we test this in the virtual switch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lguohan
At present the plan is to validate on the HW with 400G optical/copper and 100G optical/copper modules.
We have started the discussion with @prgeor 'Prince George' to determine/understand feasibility to validate these scenarios in simulated environment.

Address review comments, added more details to 'Proposal' section, added 'Out of Scope' section
doc/sfp-cmis/Interface-Link-bring-up-sequence.md Outdated Show resolved Hide resolved
doc/sfp-cmis/Interface-Link-bring-up-sequence.md Outdated Show resolved Hide resolved
doc/sfp-cmis/Interface-Link-bring-up-sequence.md Outdated Show resolved Hide resolved
doc/sfp-cmis/Interface-Link-bring-up-sequence.md Outdated Show resolved Hide resolved
doc/sfp-cmis/Interface-Link-bring-up-sequence.md Outdated Show resolved Hide resolved
doc/sfp-cmis/Interface-Link-bring-up-sequence.md Outdated Show resolved Hide resolved
jaganbal-a and others added 3 commits January 20, 2022 20:56
Update Interface-Link-bring-up-sequence.md
* Update Interface-Link-bring-up-sequence.md

* Update Interface-Link-bring-up-sequence.md

* Update Interface-Link-bring-up-sequence.md

* Update Interface-Link-bring-up-sequence.md

* Update Interface-Link-bring-up-sequence.md
@rlhui
Copy link
Contributor

rlhui commented Jan 22, 2022

@mikeberesford - please help to approve if this looks good to you. Thanks.

Copy link
Contributor

@prgeor prgeor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shyam77git can you address few comments

doc/sfp-cmis/Interface-Link-bring-up-sequence.md Outdated Show resolved Hide resolved
doc/sfp-cmis/Interface-Link-bring-up-sequence.md Outdated Show resolved Hide resolved
doc/sfp-cmis/Interface-Link-bring-up-sequence.md Outdated Show resolved Hide resolved
doc/sfp-cmis/Interface-Link-bring-up-sequence.md Outdated Show resolved Hide resolved
Further review comments addressed
@prgeor
Copy link
Contributor

prgeor commented Jan 27, 2022

@liat-grozovik @keboliu @Junchao-Mellanox please review this new link bring up sequence.

@prgeor prgeor self-assigned this Jan 27, 2022
@liat-grozovik
Copy link
Collaborator

@liat-grozovik @keboliu @Junchao-Mellanox please review this new link bring up sequence.

please expect delay in review comments due to Chinese new year vacation.

@liat-grozovik liat-grozovik requested a review from keboliu January 27, 2022 12:20
@liat-grozovik
Copy link
Collaborator

please also add a cleat flow of how you are going to do the TX setting. This cannot be a direct call to layer but rather go via SAI. How do you plan to have it? Is there a SAI API / Attribute who reflect it?

@jaganbal-a
Copy link
Contributor

jaganbal-a commented Jan 28, 2022

@dgsudharsan
1.What are the consequences does this have on fast reboot?
Jagan> As the fast reboot mechanism re-initialize host ASIC (NPU, external PHY) , With this HLD implementation, the optics DP-Init and DP-activate will happen after host device tx enable which is executed in parallel today. So the additional time consumption will be based on optics module and a fixed time for the "host_tx_ready" notification handling.
2. How is config reload scenario handled?
Jagan> Config reload is like system boot-up w.r.t to this feature.
3. Are there any special considerations for warm reboot?
Jagan> Since data path is not disturbed in warm reboot, there shouldn't any impact.
4. Will there be an option provided to disable it per platform?
5. Will there be options provided to enable it per transceiver type/speed? Disabling and enabling Tx might be time consuming for certain transceivers and would increase the downtime.
Jagan> No. W.r.t CMIS spec it is not optional, so there is no plan to enable/disable per platform or per optics basis.
The durations/limitation comes from the optics module and not from SONiC infrastructure.
6. Can you update which platform APIs would be used in xcvrd for tx enable/disable?
Jagan> It will be based on the transceiver that is present, CMIS/SFF.
@prgeor Can you please comment on API availability/Signature.
7. Can you brief error handling when Tx settings fail in xcvrd?
Jagan> Optics Tx settings failure is not in scope of this HLD.
Please check CMIS/SFP refactor HLD.
https://github.com/Azure/SONiC/blob/master/doc/sfp-cmis/cmis-init.md
https://github.com/Azure/SONiC/blob/master/doc/sfp-refactor/sfp-refactor.md

Addressed further review comments
@shyam77git
Copy link
Contributor Author

@liat-grozovik

please also add a cleat flow of how you are going to do the TX setting. This cannot be a direct call to layer but rather go via SAI. How do you plan to have it? Is there a SAI API / Attribute who reflect it?

SAI_PORT_ATTR_ADMIN_STATE is the existing attribute and its already being used to program host ASIC (NPU/PHY) Tx.
Also, same is mentioned in this HLD's 2nd & 3rd workflows. Please refer to syncd-libSAI and gbsyncd-libSAI verticals in these workflows.

@rlhui
Copy link
Contributor

rlhui commented Feb 1, 2022

Can we describe how this can be backward compatible or how it can allow gradual migration for different platforms.

shyam77git and others added 4 commits February 2, 2022 12:44
Added additional work-flow (Ability to enable/disable this feature)
Added Sequence diagram (Workflow) to enable/disable this feature
Added breakout handling section.
Updated 'Table of Contents' with 'Breakout handling ' item
@shyam77git
Copy link
Contributor Author

shyam77git commented Feb 3, 2022

Can we describe how this can be backward compatible or how it can allow gradual migration for different platforms.

@rlhui
We would be adding a mechanism to enable this feature on per platform basis.

I've added 'Feature enablement' section to the HLD describing the required workflow

@prgeor prgeor merged commit 6a344c5 into sonic-net:master Feb 8, 2022
@liat-grozovik
Copy link
Collaborator

@shyam77git once PRs are available please include them in the PR description and please wait for @keboliu to review and approve them as well.

Are there any sonic-mgmt tests that should be modified or the existing ones will for sure cover the new flows?

@zhangyanzhao
Copy link
Collaborator

Can the contributor of this feature add the code PRs list into this HLD? Thanks.

@shyam77git
Copy link
Contributor Author

Can the contributor of this feature add the code PRs list into this HLD? Thanks.

@zhangyanzhao
PRs for this HLD are:
sonic-net/sonic-platform-daemons#254
sonic-net/sonic-swss#2277
Both are under active review

I'd add them to this HLD as well

jaganbal-a pushed a commit to jaganbal-a/sonic-swss_link_det that referenced this pull request Jul 1, 2022
jaganbal-a pushed a commit to jaganbal-a/sonic-swss_link_det that referenced this pull request Jul 8, 2022
jaganbal-a pushed a commit to jaganbal-a/sonic-swss_link_det that referenced this pull request Jul 13, 2022
prgeor pushed a commit to sonic-net/sonic-swss that referenced this pull request Jul 13, 2022
…h before enabling transceiver<CMIS compliant> Tx. (#2277)

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

orchagent changes for sonic-net/SONiC#916

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

Addressing PR comment

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

Addressing PR comments-cosmetic

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

fixed typo

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

VS test code and addressing PR comment

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

set host_tx_ready to false if gbsyncd SAI API fails.

Co-authored-by: Jaganathan Anbalagan <jaganbal@cisco.com>
yxieca pushed a commit to sonic-net/sonic-swss that referenced this pull request Jul 28, 2022
…h before enabling transceiver<CMIS compliant> Tx. (#2277)

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

orchagent changes for sonic-net/SONiC#916

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

Addressing PR comment

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

Addressing PR comments-cosmetic

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

fixed typo

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

VS test code and addressing PR comment

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

set host_tx_ready to false if gbsyncd SAI API fails.

Co-authored-by: Jaganathan Anbalagan <jaganbal@cisco.com>
preetham-singh pushed a commit to preetham-singh/sonic-swss that referenced this pull request Aug 6, 2022
…h before enabling transceiver<CMIS compliant> Tx. (sonic-net#2277)

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

orchagent changes for sonic-net/SONiC#916

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

Addressing PR comment

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

Addressing PR comments-cosmetic

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

fixed typo

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

VS test code and addressing PR comment

* Signed-off-by: Jaganathan Anbalagan <jaganbal@cisco.com>

set host_tx_ready to false if gbsyncd SAI API fails.

Co-authored-by: Jaganathan Anbalagan <jaganbal@cisco.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.