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

[Inventec] Add support for D6332 platform #5304

Merged
merged 3 commits into from
Oct 20, 2020

Conversation

CynthiaINV
Copy link
Contributor

- Why I did it
Add new platform ( Inventec D6332) on master branch

- How I did it
Support D6332 platform, and upgrade kernel to 4.19

- How to verify it

  • Basic traffic tests
  • Run "show interface status" "show interface transceiver presence" to check transceiver/port status
  • Run "show platform fan", "show temperature", "show environment" ...etc to check device status
    Please refer to the test report for more details
    d6332_master.pdf

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

  • 201811
  • 201911
  • 202006

- Description for the changelog

Support platform Inventec D6332

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

@lgtm-com
Copy link

lgtm-com bot commented Sep 3, 2020

This pull request introduces 41 alerts when merging 9fc4c90ebc8612dc363a39fc0624d4b1b082cab3 into dd908c2 - view on LGTM.com

new alerts:

  • 13 for Wrong number of arguments in a class instantiation
  • 10 for Variable defined multiple times
  • 9 for Unused import
  • 5 for Unused local variable
  • 1 for Testing equality to None
  • 1 for Module imports itself
  • 1 for Except block handles 'BaseException'
  • 1 for Unused argument in a formatting call

Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

Please fix LGTM alerts

@lgtm-com
Copy link

lgtm-com bot commented Sep 22, 2020

This pull request introduces 20 alerts when merging 66814d0575e6aca7075435d9b142b5cde7f3084a into 03f82a0 - view on LGTM.com

new alerts:

  • 13 for Wrong number of arguments in a class instantiation
  • 2 for Unused import
  • 1 for Testing equality to None
  • 1 for Module imports itself
  • 1 for Except block handles 'BaseException'
  • 1 for Unused argument in a formatting call
  • 1 for Variable defined multiple times

@lgtm-com
Copy link

lgtm-com bot commented Sep 24, 2020

This pull request introduces 15 alerts when merging 5e761c54705a83d4323a6ac1aa0588929eb452a5 into a56ad41 - view on LGTM.com

new alerts:

  • 13 for Wrong number of arguments in a class instantiation
  • 1 for Module imports itself
  • 1 for Except block handles 'BaseException'

@lgtm-com
Copy link

lgtm-com bot commented Sep 28, 2020

This pull request introduces 15 alerts when merging bf6f95c32eb1307e9d8d53f7d4aa512da03c9a92 into e3f8159 - view on LGTM.com

new alerts:

  • 13 for Wrong number of arguments in a class instantiation
  • 1 for Module imports itself
  • 1 for Except block handles 'BaseException'

@lgtm-com
Copy link

lgtm-com bot commented Sep 30, 2020

This pull request introduces 15 alerts when merging 2b4a7a8d4885ecee4e3aef205fbed76d4e9f26f3 into 79bda7d - view on LGTM.com

new alerts:

  • 13 for Wrong number of arguments in a class instantiation
  • 1 for Module imports itself
  • 1 for Except block handles 'BaseException'

@lgtm-com
Copy link

lgtm-com bot commented Sep 30, 2020

This pull request introduces 15 alerts when merging 3ad302c407079f57b611ea89ca76559aa7e6436a into 79bda7d - view on LGTM.com

new alerts:

  • 13 for Wrong number of arguments in a class instantiation
  • 1 for Module imports itself
  • 1 for Unused local variable

@CynthiaINV CynthiaINV force-pushed the master_0903 branch 2 times, most recently from db05ca4 to 816de6b Compare September 30, 2020 07:18
@lgtm-com
Copy link

lgtm-com bot commented Sep 30, 2020

This pull request introduces 14 alerts when merging 816de6b52d4196804f7298b73c14a9b72220b1c3 into 79bda7d - view on LGTM.com

new alerts:

  • 13 for Wrong number of arguments in a class instantiation
  • 1 for Module imports itself

@CynthiaINV
Copy link
Contributor Author

Retest vsimage please

@CynthiaINV
Copy link
Contributor Author

Please fix LGTM alerts

Hi @jleveque ,
I'm sorry for bothering. About the LGTM alerts, I've tried to add some comments in code to avoid them, however, they seems to still be counted in the LGTM report. Also, some of the alerts occur in the codes of other vendor, so I didn't modify them.
If there's something I can do, please let me know, thank you very much for your help!

@jleveque
Copy link
Contributor

Hi @jleveque ,
I'm sorry for bothering. About the LGTM alerts, I've tried to add some comments in code to avoid them, however, they seems to still be counted in the LGTM report. Also, some of the alerts occur in the codes of other vendor, so I didn't modify them.
If there's something I can do, please let me know, thank you very much for your help!

@CynthiaINV: You can remove the comments you added to try to suppress these warnings. As you stated, other platforms also get flagged with these warnings. With the platform API, there are multiple definitions of the same functions, and LGTM isn't smart enough to find the proper ones.

@@ -0,0 +1,33 @@
# name lanes index speed autoneg
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest aligning header with columns

Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

Also, there is a swap file (.psu.py.swp) added in this PR. Please remove it.

@@ -0,0 +1,3 @@
d6332/utils/inventec_d6332_util.py /usr/local/bin
systemd/platform-modules-d6332.service /lib/systemd/system
d6332/utils/sonic_platform-1.0-py2-none-any.whl /usr/share/sonic/device/x86_64-inventec_d6332-r0
Copy link
Contributor

Choose a reason for hiding this comment

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

We are in the process of migrating SONiC to Python 3. During the transition, we would like all vendors to build, install and copy both a Python 2 sonic_platform package, and a Python 3 package (sonic_platform-1.0-py3-none-any.whl). Can you please also take care of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @jleveque ,
I've tried to build Python3 platform API package, and install the new image on our DUT . However, I've encountered some problems when I tried to test it inside the pmon docker (please refer to the log below)

root@sonic:/usr/local/bin# python3
Python 3.7.3 (default, Jul 25 2020, 13:03:44)
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from sonic_platform.psu import Psu
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/dist-packages/sonic_platform_base/sonic_sfp/sfputilhelper.py", line 17, in <module>
    from portconfig import get_port_config
ModuleNotFoundError: No module named 'portconfig'


During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/dist-packages/sonic_platform/qsfp.py", line 15, in <module>
    from sonic_platform_base.sonic_sfp.sfputilhelper import SfpUtilHelper
  File "/usr/local/lib/python3.7/dist-packages/sonic_platform_base/sonic_sfp/sfputilhelper.py", line 22, in <module>
    raise ImportError("%s - required module not found" % str(e))
ImportError: No module named 'portconfig' - required module not found

During handling of the above exception, another exception occurred:
......

I'm not sure it's caused by some build errors or etc, so python can't find the required module, or maybe some modules are not adapted to python3 yet...?

And when I check outside the docker, it seems that the sonic_platform module isn't installed successfully under pip3, but the python2 module works well. (please refer to the log below)

root@sonic:~# pip3 list | grep platform
sonic-platform-common 1.0
root@sonic:~# pip list | grep platform
sonic-platform               1.0
sonic-platform-common        1.0
root@sonic:~# cd /usr/share/sonic/device/x86_64-inventec_d6332-r0/
root@sonic:/usr/share/sonic/device/x86_64-inventec_d6332-r0# ls
board_thermal_1  coretemp            installer.conf     pmon_daemon_control.json             sonic_platform-1.0-py3-none-any.whl
board_thermal_2  custom_init.soc.j2  INVENTEC-D6332     psu1
board_thermal_3  custom_led.bin      led_proc_init.soc  sensors.conf
board_thermal_4  default_sku         plugins            sonic_platform-1.0-py2-none-any.whl
root@sonic:/usr/share/sonic/device/x86_64-inventec_d6332-r0#

I'm not sure if there's something I missed after adding sonic_platform-1.0-py3-none-any.whl to platform-modules-d6356.install? (as the log shows, the .whl file is under the correct path but not installed)

Would you please give me some advice about these issues? Thank you very much for your help!

Copy link
Contributor

@jleveque jleveque Oct 15, 2020

Choose a reason for hiding this comment

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

@CynthiaINV: The first problem you stated would be expected if the Python 3 version of the sonic-config-engine package is not installed in the PMon container, as it is currently a dependency. Support for Python 3 was added to this package very recently, so it may not be present in your image. However, this is OK for now, since we haven't yet made the PMon daemons Python 3-compatible, so the package is not expected to get used just yet. As long as both the Python 2 and Python 3 wheels are getting installed in the PMon container, which it looks like they are, I think you're good for now.

The second issue looks like an actual problem, as it appears the Python 3 wheel is not getting installed in the host OS. You probably need to look at the image build logs to see why it's not getting installed. Please let me know if you have further questions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jleveque,
Thank you very much for your advice! I'll try to solve the second issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @jleveque ,
I've tried to solve the sonic_platform not installed on linux issue, it can be installed with pip3 now. ( please refer to the log below)

root@sonic:/home/admin# pip3 list | grep platform
sonic-platform        1.0
sonic-platform-common 1.0
root@sonic:/home/admin# pip list | grep platform
sonic-platform               1.0
sonic-platform-common        1.0
root@sonic:/home/admin#

however, as the portconfig module can't installed issue mentioned above, the python3 version of platform API is still unested.
Thank you very much for your help!

@lgtm-com
Copy link

lgtm-com bot commented Oct 14, 2020

This pull request introduces 14 alerts when merging 83aac2ea638eb8bb0043b3c4d6e942c3e8900981 into 3cd1d8e - view on LGTM.com

new alerts:

  • 13 for Wrong number of arguments in a class instantiation
  • 1 for Module imports itself

@lgtm-com
Copy link

lgtm-com bot commented Oct 17, 2020

This pull request introduces 15 alerts when merging f7ce8f2 into b57272f - view on LGTM.com

new alerts:

  • 13 for Wrong number of arguments in a class instantiation
  • 1 for Module imports itself
  • 1 for 'import *' may pollute namespace

@CynthiaINV
Copy link
Contributor Author

This pull request introduces 15 alerts when merging f7ce8f2 into b57272f - view on LGTM.com

new alerts:

  • 13 for Wrong number of arguments in a class instantiation
  • 1 for Module imports itself
  • 1 for 'import *' may pollute namespace

Hi @jleveque ,
about the LGTM test, I just noticed that a new alert
* 1 for 'import *' may pollute namespace
occurs this time. However, the code in
root/platform/broadcom/sonic-platform-modules-inventec/d6332/sonic_platform/__init__.py
is similar to other platforms, and remain unchanged after last update. I'm not sure the reason caused it, would you please give me some advice about this problem?
Thank you very much for your assistance!

@jleveque
Copy link
Contributor

@CynthiaINV: It's not proper practice to import * from a module in Python. All the vendor platform API packages will need to be refactored to eliminate this. However, since there are multiple instances of these already in the codebase, I will allow these alerts on this PR.

@jleveque jleveque changed the title Support platform Inventec D6332 on master branch [Inventec] Add support for D6332 platform Oct 20, 2020
@jleveque jleveque merged commit 38bd6be into sonic-net:master Oct 20, 2020
santhosh-kt pushed a commit to santhosh-kt/sonic-buildimage that referenced this pull request Feb 25, 2021
Add support for D6332 platform

Signed-off-by: cynthia <[email protected]>
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.

2 participants