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]the port index in port_config.ini should starts from 0 #4152

Merged
merged 4 commits into from
Mar 5, 2020
Merged

[Mellanox]the port index in port_config.ini should starts from 0 #4152

merged 4 commits into from
Mar 5, 2020

Conversation

stephenxs
Copy link
Collaborator

- What I did
The port index in port_config.ini should starts from 0, otherwise it will cause xcvrd crash.

Feb 11 21:06:01.716349 arc-switch1004 INFO pmon#supervisord: xcvrd SFP index 32 out of range (0-31)
Feb 11 21:06:01.716613 arc-switch1004 INFO pmon#supervisord: xcvrd Traceback (most recent call last):
Feb 11 21:06:01.716720 arc-switch1004 INFO pmon#supervisord: xcvrd   File "/usr/bin/xcvrd", line 1083, in <module>
Feb 11 21:06:01.716805 arc-switch1004 INFO pmon#supervisord: xcvrd     main()
Feb 11 21:06:01.716886 arc-switch1004 INFO pmon#supervisord: xcvrd   File "/usr/bin/xcvrd", line 1080, in main
Feb 11 21:06:01.716963 arc-switch1004 INFO pmon#supervisord: xcvrd     xcvrd.run()
Feb 11 21:06:01.717039 arc-switch1004 INFO pmon#supervisord: xcvrd   File "/usr/bin/xcvrd", line 1044, in run
Feb 11 21:06:01.717116 arc-switch1004 INFO pmon#supervisord: xcvrd     self.init()
Feb 11 21:06:01.717191 arc-switch1004 INFO pmon#supervisord: xcvrd   File "/usr/bin/xcvrd", line 1028, in init
Feb 11 21:06:01.717266 arc-switch1004 INFO pmon#supervisord: xcvrd     post_port_sfp_dom_info_to_db(is_warm_start, self.stop_event)
Feb 11 21:06:01.717343 arc-switch1004 INFO pmon#supervisord: xcvrd   File "/usr/bin/xcvrd", line 387, in post_port_sfp_dom_info_to_db
Feb 11 21:06:01.717421 arc-switch1004 INFO pmon#supervisord: xcvrd     post_port_sfp_info_to_db(logical_port_name, int_tbl, transceiver_dict, stop_event)
Feb 11 21:06:01.717498 arc-switch1004 INFO pmon#supervisord: xcvrd   File "/usr/bin/xcvrd", line 223, in post_port_sfp_info_to_db
Feb 11 21:06:01.717573 arc-switch1004 INFO pmon#supervisord: xcvrd     if not _wrapper_get_presence(physical_port):
Feb 11 21:06:01.717665 arc-switch1004 INFO pmon#supervisord: xcvrd   File "/usr/bin/xcvrd", line 123, in _wrapper_get_presence
Feb 11 21:06:01.717740 arc-switch1004 INFO pmon#supervisord: xcvrd     return platform_chassis.get_sfp(physical_port).get_presence()
Feb 11 21:06:01.717815 arc-switch1004 INFO pmon#supervisord: xcvrd AttributeError: 'NoneType' object has no attribute 'get_presence'

- How I did it

- How to verify it

- Description for the changelog

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

@stephenxs stephenxs mentioned this pull request Feb 14, 2020
Closed
@zhenggen-xu
Copy link
Collaborator

What is the definition of the index here? I saw most of the implementation was "front pannel" port number. In such case, it should be "1" based, and we need fix the other side instead.

liat-grozovik
liat-grozovik previously approved these changes Feb 16, 2020
View reviewed changes
@stephenxs
Copy link
Collaborator Author

stephenxs commented Feb 17, 2020 •
edited
Loading

What is the definition of the index here? I saw most of the implementation was "front pannel" port number. In such case, it should be "1" based, and we need fix the other side instead.

Hi @zhenggen-xu,
AFAIK it should be 0 based otherwise it will fail the xcvrd.
Could you please provide an example of a "1"-based port_config.ini? I'd like to check.
Thanks.

@prsunny
Copy link
Contributor

prsunny commented Feb 19, 2020

Looks like this change is specific to fix a platform driver and would like Joe/Guohan to comment. Approving from my side!

prsunny
prsunny previously approved these changes Feb 19, 2020
View reviewed changes
nazariig
nazariig suggested changes Feb 20, 2020
View reviewed changes
device/mellanox/x86_64-mlnx_msn2700-r0/Mellanox-SN2700-C28D8/port_config.ini Outdated
@@ -1,37 +1,37 @@
# name lanes speed alias index
Copy link
Collaborator

@nazariig nazariig Feb 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@stephenxs # name lanes alias index speed fec autoneg

jleveque
jleveque previously approved these changes Feb 20, 2020
View reviewed changes
@jleveque jleveque dismissed their stale review February 20, 2020 23:04

Dismissing my approval until comment from @nazariig is resolved

@stephenxs stephenxs dismissed stale reviews from prsunny and liat-grozovik via 8af77e5 February 25, 2020 08:07
@zhenggen-xu
Copy link
Collaborator

What is the definition of the index here? I saw most of the implementation was "front pannel" port number. In such case, it should be "1" based, and we need fix the other side instead.

Hi @zhenggen-xu,
AFAIK it should be 0 based otherwise it will fail the xcvrd.
Could you please provide an example of a "1"-based port_config.ini? I'd like to check.
Thanks.

https://github.com/Azure/sonic-buildimage/blob/master/device/celestica/x86_64-cel_seastone-r0/Celestica-DX010-C32/port_config.ini

My point is the xcvrd should handle both cases, it should probably use the port_config.ini as Source of Truth?

@stephenxs
Copy link
Collaborator Author

What is the definition of the index here? I saw most of the implementation was "front pannel" port number. In such case, it should be "1" based, and we need fix the other side instead.

Hi @zhenggen-xu,
AFAIK it should be 0 based otherwise it will fail the xcvrd.
Could you please provide an example of a "1"-based port_config.ini? I'd like to check.
Thanks.

https://github.com/Azure/sonic-buildimage/blob/master/device/celestica/x86_64-cel_seastone-r0/Celestica-DX010-C32/port_config.ini

My point is the xcvrd should handle both cases, it should probably use the port_config.ini as Source of Truth?

Hi zhenggen,
Thank you for pointing out.
Let's handle the issue on Mellanox-2700-D48C8 for now in this PR and consider a comprehensive solution in the future.

@zhenggen-xu
Copy link
Collaborator

What is the definition of the index here? I saw most of the implementation was "front pannel" port number. In such case, it should be "1" based, and we need fix the other side instead.

Hi @zhenggen-xu,
AFAIK it should be 0 based otherwise it will fail the xcvrd.
Could you please provide an example of a "1"-based port_config.ini? I'd like to check.
Thanks.

https://github.com/Azure/sonic-buildimage/blob/master/device/celestica/x86_64-cel_seastone-r0/Celestica-DX010-C32/port_config.ini
My point is the xcvrd should handle both cases, it should probably use the port_config.ini as Source of Truth?

Hi zhenggen,
Thank you for pointing out.
Let's handle the issue on Mellanox-2700-D48C8 for now in this PR and consider a comprehensive solution in the future.

If the indexes are presenting front pannel ports, to fix the error in xvrd, we need reload some of the plugins in your platform code in sonic_platform directory. The changes are fine if the 0 based port index is what you are looking for.

@liat-grozovik
Copy link
Collaborator

retest vsimage please

@stephenxs
Copy link
Collaborator Author

Retest vsimage please

@stephenxs
Copy link
Collaborator Author

retest vsimage please

2 similar comments
@liat-grozovik
Copy link
Collaborator

retest vsimage please

@stephenxs
Copy link
Collaborator Author

retest vsimage please

@jleveque jleveque merged commit f6ec95c into sonic-net:master Mar 5, 2020
@stephenxs stephenxs deleted the fix-2700-other-sku branch March 5, 2020 03:15
@stephenxs
Copy link
Collaborator Author

Hi @jleveque, could you please cherry-pick it to 201911? Thanks.

rlhui pushed a commit that referenced this pull request Mar 15, 2020
@jleveque @rlhui
[Mellanox]the port index in port_config.ini should starts from 0 (#4152)
bcfc3d1
tiantianlv pushed a commit to SONIC-DEV/sonic-buildimage that referenced this pull request Apr 24, 2020
@jleveque @tiantianlv
[Mellanox]the port index in port_config.ini should starts from 0 (son…
659fa85 
…ic-net#4152)
@keboliu keboliu mentioned this pull request Apr 30, 2020
Open
@qiluo-msft
Copy link
Collaborator

Fixed sonic-net/sonic-utilities#588

@lguohan
Copy link
Collaborator

lguohan commented May 6, 2020

@zhenggen-xu is right. Check out the porting guide here.

https://github.com/Azure/SONiC/wiki/Porting-Guide

index: Physical index of port (optional - needed if ports do not begin with index zero or increment by one), When this column is missing the index starts from 0 and increase by 1 for each row. It is recommend to explicitly set this index column and make the index number match the labeling of the front panel ports. AKA: if the front panel ports are starting from 1, the index should start from 1. If the front panel ports start from 0, the index should start form 0. The break out ports from the same physical port should have same index.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jleveque jleveque jleveque approved these changes

@prsunny prsunny prsunny approved these changes

@nazariig nazariig nazariig approved these changes

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

Assignees
No one assigned
Labels
Enhancement ➕ Included in 201911 Branch Platform: Mellanox Request_for_bmtor Request for 201911 Branch :shipit:
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@stephenxs @zhenggen-xu @prsunny @liat-grozovik @qiluo-msft @lguohan @jleveque @nazariig @rlhui