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.

Sign up for GitHub

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

Jump to bottom

Orchagent changes for synchronizing npu/phy device Tx in the data path before enabling transceiver<CMIS compliant> Tx. #2277

Merged
merged 6 commits into from
Jul 13, 2022
Merged

Orchagent changes for synchronizing npu/phy device Tx in the data path before enabling transceiver<CMIS compliant> Tx. #2277

merged 6 commits into from
Jul 13, 2022

Conversation

jaganbal-a
Copy link
Contributor

@jaganbal-a jaganbal-a commented May 16, 2022 •
edited
Loading

What I did
Orchagent changes for sonic-net/SONiC#916

  1. Create host_tx_ready field in state-db during OA init.
  2. Set the host_tx_ready to true in STATE-DB PORT_TABLE when both NPU and PHY device Tx is enabled successfully .
  3. Set the field 'host_tx_ready' to false in STATE-DB PORT_TABLE when NPU and PHY line side Tx gets muted/disabled which happens during admin shutdown configuration is committed.

Why I did it
xcvrd will be handling "host_tx_ready" change of value event for CMIS complaint modules to bring it to appropriate CMIS state on the CMIS complaint modules.

How I verified it
UT logs attached.

Details if related

@jaganbal-a jaganbal-a requested a review from prsunny as a code owner May 16, 2022 19:34
@ghost
Copy link

ghost commented May 16, 2022 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@prsunny prsunny requested a review from prgeor May 16, 2022 20:10
prsunny
prsunny reviewed May 16, 2022
View reviewed changes
Copy link
Collaborator

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

Please try add VS tests.

orchagent/portsorch.cpp Outdated Show resolved Hide resolved
orchagent/portsorch.cpp Outdated Show resolved Hide resolved
orchagent/portsorch.cpp Outdated Show resolved Hide resolved
orchagent/portsorch.cpp Outdated Show resolved Hide resolved
orchagent/portsorch.cpp Outdated Show resolved Hide resolved
shyam77git
shyam77git reviewed Jun 1, 2022
View reviewed changes
orchagent/portsorch.cpp Outdated Show resolved Hide resolved
shyam77git
shyam77git reviewed Jun 1, 2022
View reviewed changes
orchagent/portsorch.cpp Outdated Show resolved Hide resolved
@shyam77git shyam77git mentioned this pull request Jun 1, 2022
Merged
@jaganbal-a
Copy link
Contributor Author

host_tx_ready_UT.txt
Unit test log

@jaganbal-a
Copy link
Contributor Author

Please try add VS tests.

May you please explain 'VS tests' .

shyam77git
shyam77git previously approved these changes Jun 3, 2022
View reviewed changes
Copy link

@shyam77git shyam77git left a comment

Choose a reason for hiding this comment

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

LGTM

@prgeor
Copy link
Contributor

prgeor commented Jun 7, 2022

What I did Orchagent changes for sonic-net/SONiC#916

  1. Create host_tx_ready field in state-db during OA init.
  2. set and clear the field 'host_tx_ready' with true/false when admin startup and admin shutdown configuration is committed.

Why I did it xcvrd will be handling "host_tx_ready" change of value event for CMIS complaint modules to bring it to appropriate CMIS state on the CMIS complaint modules.

How I verified it TBD.

Details if related

The description seems to suggest that if admin is UP then host_tx_ready = true, which is not the case. Can we rephrase?

@prgeor
Copy link
Contributor

prgeor commented Jun 7, 2022

can you add unit test here:- https://github.com/Azure/sonic-swss/tree/master/tests

  1. If admin status is DOWN, host_tx_ready is false
  2. If admin_status is UP, host_tx_ready can be true/false ..mimic the two cases

@jaganbal-a jaganbal-a changed the title Orchagent changes for https://github.com/sonic-net/SONiC/pull/916 Orchagent changes for synchronizing npu/phy device Tx in the data path before enabling transceiver<CMIS compliant> Tx. Jun 9, 2022
@rlhui rlhui added the chassis label Jun 17, 2022
@jaganbal-a
Copy link
Contributor Author

Please try add VS tests.

@prsunny , I understood that it requires a ubuntu machine to setup the environment to run the VS test code. I am working with IT team to setup the environment.
I have the VS changes and will create a separate git issue to track and commit the VS test changes, is that fine?
In that case, please approve this PR, The xcvrd PR #254 has a dependency with this changes, so both needs to merged at once.

@prgeor
Copy link
Contributor

prgeor commented Jun 25, 2022

@liat-grozovik @keboliu can you please review this PR?

@prsunny
Copy link
Collaborator

prsunny commented Jun 29, 2022

@prgeor to signoff/merge

@jaganbal-a
Copy link
Contributor Author

@prgeor to signoff/merge

@prsunny @prgeor , please approve and merge the PR.

prgeor
prgeor requested changes Jun 29, 2022
View reviewed changes
orchagent/portsorch.cpp Show resolved Hide resolved
@jaganbal-a jaganbal-a requested a review from prgeor June 29, 2022 22:13
@jaganbal-a jaganbal-a force-pushed the master branch 2 times, most recently from 3d44c37 to 357b335 Compare July 1, 2022 20:38
@jaganbal-a jaganbal-a requested a review from prgeor July 1, 2022 20:43
prgeor
prgeor previously approved these changes Jul 5, 2022
View reviewed changes
@prgeor
Copy link
Contributor

prgeor commented Jul 5, 2022

@jaganbal-a please update this section in the description

How I verified it
TBD.

@prgeor
Copy link
Contributor

prgeor commented Jul 5, 2022

@jaganbal-a VS test failure is due to this issue

@jaganbal-a
Copy link
Contributor Author

jaganbal-a commented Jul 6, 2022 •
edited
Loading

Unit test log
Link_bring_up_xcvrd_unit_test_log.txt

@prgeor
Copy link
Contributor

prgeor commented Jul 8, 2022

@jaganbal-a please rebase your changes with latest swss

@prsunny
Copy link
Collaborator

prsunny commented Jul 11, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jaganbal-a jaganbal-a mentioned this pull request Jul 12, 2022
Open
@prgeor
Copy link
Contributor

prgeor commented Jul 13, 2022

@jaganbal-a can you rebase once again?

@prgeor prgeor merged commit 3fd812d into sonic-net:master Jul 13, 2022
@jaganbal-a jaganbal-a mentioned this pull request Jul 15, 2022
Closed
yxieca pushed a commit that referenced this pull request Jul 28, 2022
@jaganbal-a @yxieca
Orchagent changes for synchronizing npu/phy device Tx in the data pat…
1a93ec9 
…h before enabling transceiver<CMIS compliant> Tx. (#2277)

* Signed-off-by: Jaganathan Anbalagan <[email protected]>

orchagent changes for sonic-net/SONiC#916

* Signed-off-by: Jaganathan Anbalagan <[email protected]>

Addressing PR comment

* Signed-off-by: Jaganathan Anbalagan <[email protected]>

Addressing PR comments-cosmetic

* Signed-off-by: Jaganathan Anbalagan <[email protected]>

fixed typo

* Signed-off-by: Jaganathan Anbalagan <[email protected]>

VS test code and addressing PR comment

* Signed-off-by: Jaganathan Anbalagan <[email protected]>

set host_tx_ready to false if gbsyncd SAI API fails.

Co-authored-by: Jaganathan Anbalagan <[email protected]>
preetham-singh pushed a commit to preetham-singh/sonic-swss that referenced this pull request Aug 6, 2022
@jaganbal-a @preetham-singh
Orchagent changes for synchronizing npu/phy device Tx in the data pat…
3870373 
…h before enabling transceiver<CMIS compliant> Tx. (sonic-net#2277)

* Signed-off-by: Jaganathan Anbalagan <[email protected]>

orchagent changes for sonic-net/SONiC#916

* Signed-off-by: Jaganathan Anbalagan <[email protected]>

Addressing PR comment

* Signed-off-by: Jaganathan Anbalagan <[email protected]>

Addressing PR comments-cosmetic

* Signed-off-by: Jaganathan Anbalagan <[email protected]>

fixed typo

* Signed-off-by: Jaganathan Anbalagan <[email protected]>

VS test code and addressing PR comment

* Signed-off-by: Jaganathan Anbalagan <[email protected]>

set host_tx_ready to false if gbsyncd SAI API fails.

Co-authored-by: Jaganathan Anbalagan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prsunny prsunny prsunny left review comments

@prgeor prgeor prgeor approved these changes

@shyam77git shyam77git shyam77git left review comments

Assignees

@prgeor prgeor

Labels
chassis Included in 202205 Branch Request for 202205 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jaganbal-a @prgeor @prsunny @shyam77git @yxieca @rlhui