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

[xcvrd] initial support for integrating vendor specfic class objects for calling Y-Cable API's inside xcvrd #197

Merged
merged 17 commits into from
Aug 25, 2021

Conversation

vdahiya12
Copy link
Contributor

@vdahiya12 vdahiya12 commented Jun 28, 2021

Signed-off-by: vaibhav-dahiya vdahiya@microsoft.com

Description

This PR integrates vendor specific class objects inside xcvrd for Y-Cable API's to be called.
Detailed designed document can be found sonic-net/SONiC#757

Motivation and Context

Basically xcvrd now has an interface for Y-Cable to interact with PMON docker and HOST
which can be uniform across all vendors. As part of this refactor, we will be moving towards a model where only xcvrd interacts with the cables/transceivers, and host-side processes will communicate with xcvrd rather than with the devices directly with Y-Cable.

How Has This Been Tested?

Ran the changes on Arista7050cx3 switch, making changes inside the container.

Additional Information (Optional)

…for calling Y-Cable API's inside xcvrd

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Jun 28, 2021

This pull request introduces 4 alerts when merging 368ff10 into 2d2749a - view on LGTM.com

new alerts:

  • 2 for Variable defined multiple times
  • 1 for Unused local variable
  • 1 for Unused import

@vdahiya12
Copy link
Contributor Author

dependent on sonic-net/sonic-platform-common#203

@vdahiya12 vdahiya12 marked this pull request as draft June 28, 2021 22:48
@vdahiya12 vdahiya12 requested a review from jleveque June 28, 2021 22:49
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
jleveque
jleveque previously approved these changes Jul 1, 2021
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Jul 23, 2021

This pull request introduces 16 alerts when merging 7091782 into 53e1532 - view on LGTM.com

new alerts:

  • 8 for Testing equality to None
  • 5 for Unused local variable
  • 3 for Variable defined multiple times


model = port_info_dict.get('model')

if vendor is None:
Copy link

Choose a reason for hiding this comment

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

if model is None: ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@lguohan
Copy link
Contributor

lguohan commented Jul 26, 2021

can you add unit test accordingly?

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@vdahiya12 vdahiya12 requested a review from tahmed-dev August 3, 2021 20:26
mux_info_dict['version_nic_active'] = '0.8'
mux_info_dict['version_nic_inactive'] = '0.7'
mux_info_dict['version_nic_next'] = '0.7'
set_show_firmware_fields("Ethernet0", mux_info_dict, xcvrd_show_fw_res_tbl)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is being tested in this test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added an assert for return code
fixed

@@ -22,6 +26,10 @@

helper_logger = logger.Logger(SYSLOG_IDENTIFIER)

y_cable_port_instances = {}
y_cable_port_locks = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

are those locks shared between CLI and applications?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's correct, moving to the abstract class model, there is a lock for each port. Each api to y_cable driver will lock it for that port and only release when the API is returned. Basically all y_cable api's will be called by xcvrd only. cli will call the y_cable api through redis and invoke it per port.

Copy link
Contributor

Choose a reason for hiding this comment

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

This means that presence of cli activity such show, could potentially affect our switching time, right?

Copy link
Contributor Author

@vdahiya12 vdahiya12 Aug 6, 2021

Choose a reason for hiding this comment

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

For the same port, Yes it will. Cli activities such as show hwmode, download firmware etc would slow these, but these will not be used as is for our production. show hwmode etc will be used for debugging, download, activate etc will go through ASK mode.

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@vdahiya12 vdahiya12 marked this pull request as ready for review August 6, 2021 23:21
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Aug 9, 2021

This pull request introduces 1 alert when merging db13a08 into f63fc94 - view on LGTM.com

new alerts:

  • 1 for Too few arguments in formatting call

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
tahmed-dev
tahmed-dev previously approved these changes Aug 11, 2021
@vdahiya12 vdahiya12 marked this pull request as draft August 12, 2021 00:08
@vdahiya12 vdahiya12 marked this pull request as ready for review August 12, 2021 20:38
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vdahiya12
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Aug 20, 2021

This pull request introduces 1 alert when merging b1f916a into 57e3d78 - view on LGTM.com

new alerts:

  • 1 for Redundant comparison

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@vdahiya12 vdahiya12 merged commit e038bc2 into sonic-net:master Aug 25, 2021
@qiluo-msft
Copy link
Contributor

This commit could not be cleanly cherry-pick to 202012. Please submit another PR.

vdahiya12 added a commit to vdahiya12/sonic-platform-daemons that referenced this pull request Aug 25, 2021
…for calling Y-Cable API's inside xcvrd (sonic-net#197)

Signed-off-by: vaibhav-dahiya vdahiya@microsoft.com

Description
This PR integrates vendor specific class objects inside xcvrd for Y-Cable API's to be called.
Detailed designed document can be found sonic-net/SONiC#757

Motivation and Context
Basically xcvrd now has an interface for Y-Cable to interact with PMON docker and HOST
which can be uniform across all vendors. As part of this refactor, we will be moving towards a model where only xcvrd interacts with the cables/transceivers, and host-side processes will communicate with xcvrd rather than with the devices directly with Y-Cable.

How Has This Been Tested?
Ran the changes on Arista7050cx3 switch, making changes inside the container.

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
vdahiya12 added a commit that referenced this pull request Aug 26, 2021
…for calling Y-Cable API's inside xcvrd (#197) (#213)

Signed-off-by: vaibhav-dahiya vdahiya@microsoft.com
Merging this in 202012 branch because there was a merge conflict in original PR
#197

Description
This PR integrates vendor specific class objects inside xcvrd for Y-Cable API's to be called.
Detailed designed document can be found sonic-net/SONiC#757

Motivation and Context
Basically xcvrd now has an interface for Y-Cable to interact with PMON docker and HOST
which can be uniform across all vendors. As part of this refactor, we will be moving towards a model where only xcvrd interacts with the cables/transceivers, and host-side processes will communicate with xcvrd rather than with the devices directly with Y-Cable.

How Has This Been Tested?
Ran the changes on Arista7050cx3 switch, making changes inside the container.

Signed-off-by: vaibhav-dahiya vdahiya@microsoft.com
vdahiya12 pushed a commit to vdahiya12/sonic-platform-daemons that referenced this pull request Apr 4, 2022
…-net#197)

Unifying the platform api for get_pcie_aer_stats with PcieBase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants