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

Adding platform support for Juniper QFX5200 #4376

Merged
merged 1 commit into from
Apr 16, 2020
Merged

Adding platform support for Juniper QFX5200 #4376

merged 1 commit into from
Apr 16, 2020

Conversation

ciju-juniper
Copy link
Contributor

This is a 1RU switch with 32 QSFP28 (40G/100G) ports on Broadcom Tomahawk I chipset. CPU used in QFX5200-32C-S is Intel Ivy Bridge. The switch has Redundant and hot-swappable Power Supply (1+1) and also has Redundant and hot swappable fans (5).

Signed-off-by: Ciju Rajan K crajank@juniper.net
Signed-off-by: Ashish Bhensdadia bashish@juniper.net

- What I did

  • Added the device specific files, bcm configuration, portmapping file for Broadcom TH1
  • Developed device drivers for platform specific fpga
  • Developed the platform scripts

- How I did it
There is a issue #4347 which is being debugged. This patch set has been verified after applying the workarounds/fixes provide on the latest of master branch.

- How to verify it

  1. show platform summary
  2. show interface status
  3. show interface transceiver eeprom
  4. sfputil show presence
  5. sensors
  6. show platform psustatus

The detailed UT was captured in the file qfx5200_unittest_logs.txt
qfx5200_unittest_logs.txt

- Description for the changelog
Adding platform support for Juniper QFX5200

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

@lgtm-com
Copy link

lgtm-com bot commented Apr 6, 2020

This pull request introduces 105 alerts when merging 82ba140216cdfe91521a048f70f2a057b0f9a6a8 into 60b1649 - view on LGTM.com

new alerts:

  • 42 for Unused import
  • 30 for Unused local variable
  • 11 for Use of 'global' at module level
  • 8 for Except block handles 'BaseException'
  • 6 for Variable defined multiple times
  • 2 for Unreachable code
  • 2 for 'import *' may pollute namespace
  • 2 for __init__ method returns a value
  • 1 for Unnecessary pass
  • 1 for Result of integer division may be truncated

@ciju-juniper
Copy link
Contributor Author

@lguohan @jleveque Could you review and merge this platform?

@lguohan
Copy link
Collaborator

lguohan commented Apr 7, 2020

can you clean up the alerts raised by lgtm?

@ciju-juniper
Copy link
Contributor Author

@lguohan Alerts are being cleaned up now. Shall upload the files soon.

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 12 alerts when merging 190336a66ae5ae37931fcbca4cca36a2b66f943a into 2a59551 - view on LGTM.com

new alerts:

  • 6 for Except block handles 'BaseException'
  • 3 for Variable defined multiple times
  • 2 for 'import *' may pollute namespace
  • 1 for Result of integer division may be truncated

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 11 alerts when merging 5636091273fed7da5db1eee2bde0d73121e3e92a into 2a59551 - view on LGTM.com

new alerts:

  • 6 for Except block handles 'BaseException'
  • 2 for 'import *' may pollute namespace
  • 2 for Variable defined multiple times
  • 1 for Result of integer division may be truncated

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 11 alerts when merging 17eaf522c6adfb5995d088b1b260b15ee3c5375d into 2a59551 - view on LGTM.com

new alerts:

  • 6 for Except block handles 'BaseException'
  • 2 for 'import *' may pollute namespace
  • 2 for Variable defined multiple times
  • 1 for Result of integer division may be truncated

@ciju-juniper
Copy link
Contributor Author

@lguohan We have addressed all the LGTM alerts. Summary is not updated automatically as we edited in the pull request directly. Please consider merging the patchset.

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 11 alerts when merging db02711e45021cf033cafab5fbb3d894cbbaaec8 into 2a59551 - view on LGTM.com

new alerts:

  • 6 for Except block handles 'BaseException'
  • 2 for 'import *' may pollute namespace
  • 2 for Variable defined multiple times
  • 1 for Result of integer division may be truncated

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 11 alerts when merging 242a6c71b92667fff678a06a0adb0395a5acd022 into 2a59551 - view on LGTM.com

new alerts:

  • 6 for Except block handles 'BaseException'
  • 2 for 'import *' may pollute namespace
  • 2 for Variable defined multiple times
  • 1 for Result of integer division may be truncated

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 11 alerts when merging 5220b1d6b9b7c129446466928327ddc6efb85252 into 2a59551 - view on LGTM.com

new alerts:

  • 6 for Except block handles 'BaseException'
  • 2 for 'import *' may pollute namespace
  • 2 for Variable defined multiple times
  • 1 for Result of integer division may be truncated

@lgtm-com
Copy link

lgtm-com bot commented Apr 7, 2020

This pull request introduces 11 alerts when merging 946483a12a30d944b0d343e0dd398a8c77b08d66 into 2a59551 - view on LGTM.com

new alerts:

  • 6 for Except block handles 'BaseException'
  • 2 for 'import *' may pollute namespace
  • 2 for Variable defined multiple times
  • 1 for Result of integer division may be truncated

@ciju-juniper
Copy link
Contributor Author

@lguohan Now that #4379 has been committed, could you consider merging this platform?

@ciju-juniper
Copy link
Contributor Author

@lguohan Rebased the patches to latest sonic-buildimage master.

@lgtm-com
Copy link

lgtm-com bot commented Apr 8, 2020

This pull request introduces 11 alerts when merging 4fa848d1457ba02a21145c8554c73860be060d6b into de377eb - view on LGTM.com

new alerts:

  • 6 for Except block handles 'BaseException'
  • 2 for 'import *' may pollute namespace
  • 2 for Variable defined multiple times
  • 1 for Result of integer division may be truncated

@lguohan
Copy link
Collaborator

lguohan commented Apr 9, 2020

it looks like there are still some alerts from lgtm.

@ciju-juniper
Copy link
Contributor Author

ciju-juniper commented Apr 9, 2020

@lguohan A few alerts for nested exception handling. We had marked it as false positives ad these are valid scenarios.

2 Alerts are for "import *". This is needed right now.
2 Alerts are of variable defined. LGTM doesn't understand "os.command" so it's just flagging it as false positive.
Integer division alert is expected to have truncation. We are handling it already.

@ciju-juniper
Copy link
Contributor Author

@lguohan I've resolved your comments. Also provided the reason why we need adt7470 driver customization for QFX5200.

@lgtm-com
Copy link

lgtm-com bot commented Apr 9, 2020

This pull request introduces 11 alerts when merging 10ef2f3ec2bf988a3ee503c98e2b07cd70efe4ad into 7405f8c - view on LGTM.com

new alerts:

  • 6 for Except block handles 'BaseException'
  • 2 for 'import *' may pollute namespace
  • 2 for Variable defined multiple times
  • 1 for Result of integer division may be truncated

@ciju-juniper
Copy link
Contributor Author

@lguohan We re-organized the code and fixed all the lgtm python alerts. Requesting you to merge this platform.

@ciju-juniper
Copy link
Contributor Author

@lguohan Could you merge this pull request?

@ciju-juniper
Copy link
Contributor Author

retest vsimage

@ciju-juniper
Copy link
Contributor Author

@lguohan We have tested on the latest of master and everything is working as expected. Please do merge the patch set.

@jleveque
Copy link
Contributor

Retest vsimage please

@ciju-juniper
Copy link
Contributor Author

@jleveque I see that you were able to trigger the vsimage test. I tried it earlier by adding 'retest vsimage' comment, but it didn't trigger. Does it mean only maintainers can trigger it?

I see that last test in vsimage test suite is always failing with most of the pull requests. Are there any issues with the test suite? At least this pull request doesn't touch any of the VS platform files.

@jleveque
Copy link
Contributor

@ciju-juniper: You can also trigger it. The plugin requires you add the word "please" at the end (it forces you to be polite :) ).

I'm not aware of issues with the vsimage test suite, but there may be. I'll see if anyone is aware of this.

@ciju-juniper
Copy link
Contributor Author

@jleveque Thanks.. I will be more polite from now on ;-)

@ciju-juniper
Copy link
Contributor Author

@jleveque Would you be able to merge this pull request? There aren't any changes which affects vsimage.

@jleveque
Copy link
Contributor

I will wait for @lguohan to approve and make the decision.

This is a 1RU switch with 32 QSFP28 (40G/100G) ports on
Broadcom Tomahawk I chipset. CPU used in QFX5200-32C-S
is Intel Ivy Bridge. The machine has Redundant and
hot-swappable Power Supply (1+1) and also has Redundant
and hot swappable fans (5).

Signed-off-by: Ciju Rajan K <crajank@juniper.net>
Signed-off-by: Ashish Bhensdadia <bashish@juniper.net>
@ciju-juniper
Copy link
Contributor Author

@lguohan I've refreshed the pull request after removing the adt7470 private driver.

@jleveque
Copy link
Contributor

@ciju-juniper: Please try to use force-pushes only when necessary. Having a history of commits in GitHub make reviewing iterations easier.

@ciju-juniper
Copy link
Contributor Author

@jleveque I will keep that in mind. I was trying to get a single clean commit in the pull request as it's a new platform addition.

@jleveque
Copy link
Contributor

@ciju-juniper: You don't need to worry about getting a single, clean commit. When we merge pull requests, we use a "squash merge" which will squash all commits in the PR into one single, clean commit.

@ciju-juniper
Copy link
Contributor Author

@jleveque Thanks for the info.

@ciju-juniper
Copy link
Contributor Author

@lguohan @jleveque Looks like all the builds/tests passed this time. Would you be able to merge now? Also, I've addressed your comment regarding adt7470 driver.

@ciju-juniper
Copy link
Contributor Author

@lguohan @jleveque I see that all the checks have passed now including the review. Would this pull request be merged automatically?

@lguohan lguohan merged commit 695652c into sonic-net:master Apr 16, 2020
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.

3 participants