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

[common]: Fix of get_port_map #3643

Merged
merged 2 commits into from Jul 8, 2021
Merged

Conversation

vmorokhx
Copy link
Contributor

@vmorokhx vmorokhx commented Jun 8, 2021

Signed-off-by: Vladyslav Morokhovych vladyslavx.morokhovych@intel.com

Description of PR

Summary:
Patch with fix for get_port_map in interface_utils.py
According to Porting Guide: plugins/getportmap.py and port_config.ini are deprecated, but was used in get_port_map

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Back port request

  • 201911

Approach

What is the motivation for this PR?

Deprecation of

  • plugins/getportmap.py
  • port_config.ini ( In favor of the use of platform.json and hwsku.json )

How did you do it?

Change get_port_map in interface_utils.py
Switched from using getportmap.py and port_config.ini
to retrieving port mapping from config_facts depending on asic

How did you verify/test it?

Run sfp tests, test_reboot.py, test_sequential_restart.py on t0, t0-64-32 and t1

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

@vmorokhx vmorokhx requested a review from a team as a code owner June 8, 2021 16:22
*Fix get_port_map due to deprecation of plugins/getportmap.py

  Signed-off-by: Vladyslav Morokhovych <vladyslavx.morokhovych@intel.com>
@lgtm-com
Copy link

lgtm-com bot commented Jun 8, 2021

This pull request introduces 1 alert when merging cbfafb1 into 4309468 - view on LGTM.com

new alerts:

  • 1 for Unused import

@wangxin
Copy link
Collaborator

wangxin commented Jun 28, 2021

@vmorokhx Can you fix the LTM alert 1 for Unused import?

@vmorokhx
Copy link
Contributor Author

@wangxin LGTM unused import alert fixed.

@wangxin wangxin merged commit e96abc9 into sonic-net:master Jul 8, 2021
@rawal01
Copy link
Contributor

rawal01 commented Jul 9, 2021

@wangxin @shubav
This breaks multi asic platforms as platform.json is not have support for multi asic platforms we are using port_config.ini to get the information after this change it breaks sfp and reboot test for voq chassis

@shubav
Copy link
Contributor

shubav commented Jul 9, 2021

@wangxin @vmorokhx can this be reverted, please?

@vmorokhx
Copy link
Contributor Author

I see no reason to revert, port_config.ini was deprecated, so we need this change. platform.json along with hwsku.json replace port_config.ini, and they must support multi ASIC platforms. If not, there should be a issue on this.

@rawal01
Copy link
Contributor

rawal01 commented Jul 12, 2021

config_facts ansible module gives port_index_map in each asic starting with index 0. Code from config_facts ansible module:

    if 'PORT' in config:
        port_name_list = config["PORT"].keys()
        port_name_list_sorted = natsorted(port_name_list)

        for idx, val in enumerate(port_name_list_sorted):
            port_index_map[val] = idx

So, it is not getting the 'port index' defined in platform.json / portconfig.ini, For example, for asic1 with starting port Ethernet18 (first port in the asic1), the port_index_map would be:

{
   Ethernet18: 0,
   Ethernet19: 1,
   .
   .
}

With the old method - this was:

{
   Ethernet18: 19,
   Ethernet19: 20,
   .
   .
}

So, using config_facts breaks this utility for multi-asic DUT.  We need to either resolve this, or revert the PR for now.

@shubav
Copy link
Contributor

shubav commented Jul 12, 2021

@arlakshm @rlhui can you please share how this is tested or planned to be tested for multi-asic platforms? Thanks.

@shubav
Copy link
Contributor

shubav commented Jul 19, 2021

@arlakshm we believe this breaks multi-asic tests. Could you please review and help get this reverted or propose how we can resolve this?

arlakshm added a commit that referenced this pull request Jul 28, 2021
…3897)

What is the motivation for this PR?
The change done in PR #3643 to use the get_port_map, does not work for multi-ASIC platforms. The existing logic in config_facts gives the same index for ports on different ASICs.

The PR is a fix for this problem. get_port_map will use the port index present in the config_db.

How did you do it?
Change in config_facts to get the port index from config_db if its available. If the index is not present use the exisiting lofic to generate the index

How did you verify/test it?
run the test platform_tests/rest_reload_config.py to verify

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>
vmittal-msft pushed a commit to vmittal-msft/sonic-mgmt that referenced this pull request Sep 28, 2021
Summary:
Patch with fix for get_port_map in interface_utils.py According to Porting Guide (https://github.com/Azure/SONiC/wiki/Porting-Guide): plugins/getportmap.py and port_config.ini are deprecated, but was used in `get_port_map`.

What is the motivation for this PR?
Deprecation of:
* plugins/getportmap.py
* port_config.ini ( In favor of the use of platform.json and hwsku.json )

How did you do it?
Change get_port_map in interface_utils.py. Switched from using getportmap.py and port_config.ini to retrieving port mapping from config_facts depending on asic

How did you verify/test it?
Run sfp tests, test_reboot.py, test_sequential_restart.py on t0, t0-64-32 and t1

Signed-off-by: Vladyslav Morokhovych <vladyslavx.morokhovych@intel.com>
vmittal-msft pushed a commit to vmittal-msft/sonic-mgmt that referenced this pull request Sep 28, 2021
…onic-net#3897)

What is the motivation for this PR?
The change done in PR sonic-net#3643 to use the get_port_map, does not work for multi-ASIC platforms. The existing logic in config_facts gives the same index for ports on different ASICs.

The PR is a fix for this problem. get_port_map will use the port index present in the config_db.

How did you do it?
Change in config_facts to get the port index from config_db if its available. If the index is not present use the exisiting lofic to generate the index

How did you verify/test it?
run the test platform_tests/rest_reload_config.py to verify

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants