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

[Mellanox] Refactor Mellanox platform API to support dynamic port configuration #8422

Merged
merged 13 commits into from
Oct 25, 2021
Merged

[Mellanox] Refactor Mellanox platform API to support dynamic port configuration #8422

merged 13 commits into from
Oct 25, 2021

Conversation

Junchao-Mellanox
Copy link
Collaborator

@Junchao-Mellanox Junchao-Mellanox commented Aug 11, 2021 •
edited by liat-grozovik
Loading

Why I did it

  1. To support systems with dynamic port configuration
  2. Apply lazy initialization to faster the speed of loading platform API

How I did it

  1. Add module.py to implement dynamic port configuration (aka line card model)
  2. Adjust chassis.py, platform.py, thermal.py, sfp.py to support dynamic port configuration
  3. Optimize existing code

How to verify it

  1. Platform regression on MSN4700, MSN3800 and MSN2700, 100% pass
  2. Unit test covers all new changes.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

@Junchao-Mellanox Junchao-Mellanox mentioned this pull request Aug 11, 2021
Merged
keboliu
keboliu previously approved these changes Aug 11, 2021
View reviewed changes
@lgtm-com
Copy link

lgtm-com bot commented Aug 11, 2021

This pull request introduces 8 alerts and fixes 16 when merging d70896e into d6433d1 - view on LGTM.com

new alerts:

  • 6 for Unused import
  • 1 for Module is imported more than once
  • 1 for First argument to super() is not enclosing class

fixed alerts:

  • 6 for Unused import
  • 5 for Wrong number of arguments in a class instantiation
  • 4 for Unused local variable
  • 1 for Except block handles 'BaseException'

@lgtm-com
Copy link

lgtm-com bot commented Aug 12, 2021

This pull request fixes 16 alerts when merging b163ae0 into 9417fe9 - view on LGTM.com

fixed alerts:

  • 6 for Unused import
  • 5 for Wrong number of arguments in a class instantiation
  • 4 for Unused local variable
  • 1 for Except block handles 'BaseException'

@liat-grozovik liat-grozovik changed the title [Mellanox] Refactor Mellanox platform API to support modular chassis [Mellanox] Refactor Mellanox platform API to support dynamic port configuration Aug 16, 2021
@liat-grozovik
Copy link
Collaborator

@keboliu please review
@lguohan please assign someone to review

keboliu
keboliu previously approved these changes Aug 27, 2021
View reviewed changes
@lgtm-com
Copy link

lgtm-com bot commented Aug 27, 2021

This pull request fixes 16 alerts when merging 02fd3ae into 48ba459 - view on LGTM.com

fixed alerts:

  • 6 for Unused import
  • 5 for Wrong number of arguments in a class instantiation
  • 4 for Unused local variable
  • 1 for Except block handles 'BaseException'

@keboliu keboliu requested review from sujinmkang and prgeor September 9, 2021 01:04
@liat-grozovik liat-grozovik removed the request for review from lguohan September 13, 2021 09:25
keboliu
keboliu previously approved these changes Sep 14, 2021
View reviewed changes
sujinmkang
sujinmkang previously approved these changes Sep 15, 2021
View reviewed changes
Copy link
Collaborator

@sujinmkang sujinmkang left a comment

Choose a reason for hiding this comment

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

LGTM

@liat-grozovik
Copy link
Collaborator

@Junchao-Mellanox please note

liat-grozovik
liat-grozovik requested changes Sep 29, 2021
View reviewed changes
Copy link
Collaborator

@liat-grozovik liat-grozovik left a comment

Choose a reason for hiding this comment

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

see comment

@Junchao-Mellanox Junchao-Mellanox dismissed stale reviews from sujinmkang and keboliu via 0111475 September 30, 2021 00:56
@Junchao-Mellanox
Copy link
Collaborator Author

To avoid conflict with #8799, I will add copyright to newly added files of this PR.

@Junchao-Mellanox
Copy link
Collaborator Author 

E           Failed: Processes "['check_other_vms--ARISTA11T2']" had failures. Please check the logs

Looks like some deploy issue.

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lgtm-com
Copy link

lgtm-com bot commented Oct 18, 2021

This pull request fixes 16 alerts when merging fa4a6d7 into 5356244 - view on LGTM.com

fixed alerts:

  • 6 for Unused import
  • 5 for Wrong number of arguments in a class instantiation
  • 4 for Unused local variable
  • 1 for Except block handles 'BaseException'

qiluo-msft pushed a commit to sonic-net/sonic-platform-daemons that referenced this pull request Oct 18, 2021
@Junchao-Mellanox
[thermalctld] Update line card thermal sensor status to DB (#211)
1565a23 
Update line card thermal sensor status to DB, includes PSU thermal sensors and SFP thermal sensors on line card. Depends on sonic-net/sonic-buildimage#8422.

#### Description

In thermal update function, update PSU, SFP and direct thermal of line card

#### Motivation and Context

To support modular chassis

#### How Has This Been Tested?

1. Full platform regression, 100% passed
2. Unit test passed
keboliu
keboliu previously approved these changes Oct 19, 2021
View reviewed changes
prgeor
prgeor previously approved these changes Oct 20, 2021
View reviewed changes
liat-grozovik
liat-grozovik reviewed Oct 20, 2021
View reviewed changes
Copy link
Collaborator

@liat-grozovik liat-grozovik left a comment

Choose a reason for hiding this comment

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

please update mellanox files with Nvidia copyright

@lgtm-com
Copy link

lgtm-com bot commented Oct 20, 2021

This pull request fixes 16 alerts when merging ceef999 into 9527cbe - view on LGTM.com

fixed alerts:

  • 6 for Unused import
  • 5 for Wrong number of arguments in a class instantiation
  • 4 for Unused local variable
  • 1 for Except block handles 'BaseException'

@Junchao-Mellanox Junchao-Mellanox mentioned this pull request Oct 22, 2021
Merged
5 tasks
@Junchao-Mellanox Junchao-Mellanox dismissed stale reviews from prgeor and keboliu via 7325a5a October 22, 2021 02:57
keboliu
keboliu previously approved these changes Oct 22, 2021
View reviewed changes
@lgtm-com
Copy link

lgtm-com bot commented Oct 22, 2021

This pull request introduces 1 alert and fixes 16 when merging 7325a5a into 5b5f3f0 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 6 for Unused import
  • 5 for Wrong number of arguments in a class instantiation
  • 4 for Unused local variable
  • 1 for Except block handles 'BaseException'

@lgtm-com
Copy link

lgtm-com bot commented Oct 22, 2021

This pull request fixes 16 alerts when merging 91ddc95 into 5b5f3f0 - view on LGTM.com

fixed alerts:

  • 6 for Unused import
  • 5 for Wrong number of arguments in a class instantiation
  • 4 for Unused local variable
  • 1 for Except block handles 'BaseException'

@liat-grozovik liat-grozovik merged commit e8b4c2a into sonic-net:master Oct 25, 2021
@Junchao-Mellanox Junchao-Mellanox deleted the modular-chassis branch October 29, 2021 01:42
@tomer-israel tomer-israel mentioned this pull request Nov 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liat-grozovik liat-grozovik liat-grozovik left review comments

@prgeor prgeor prgeor approved these changes

@keboliu keboliu keboliu approved these changes

@sujinmkang sujinmkang sujinmkang left review comments

Assignees
No one assigned
Labels
Platform: Mellanox
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Junchao-Mellanox @liat-grozovik @mssonicbld @keboliu @prgeor @sujinmkang