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

[multi-asic] Use asic_id/asic_name for APIs to get port config data #18452

Merged
merged 6 commits into from
May 23, 2024

Conversation

anamehra
Copy link
Contributor

@anamehra anamehra commented Mar 23, 2024

Signed-off-by: Anand Mehra anamehra@cisco.com

Fixes: #18431

Why I did it

sonic-cfggen misses handling for namespace at couple of locations. Due to this, when port config is fetched, it returns data from global config and not the namespace.

The function calls for
get_path_to_port_config_file
get_port_config

needs to be fixed to handle namespace for multi-asic.

Work item tracking
  • Microsoft ADO (number only):

How I did it

1.Pass asic_id as argument when calling get_path_to_port_config_file()
For single asic, asic_id is None and the function returns the correct port_config.ini file path.
For multi-asic, using asic_id provides the correct file path for the asic.

  1. Pass asic_name as argument when calling get_port_config()

How to verify it

sonic-cfggen -H --print -n -k
This command should return the correct port config for the namespace.

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • [] 202211
  • 202305

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

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

Signed-off-by: Anand Mehra anamehra@cisco.com
@anamehra anamehra marked this pull request as draft March 23, 2024 08:04
@anamehra anamehra changed the title Use asic_id/asic_name for APIs to get port config related data [multi-asic] Use asic_id/asic_name for APIs to get port config data Mar 26, 2024
@anamehra anamehra marked this pull request as ready for review March 26, 2024 17:03
@anamehra
Copy link
Contributor Author

Hi @abdosi , please review.

@anamehra
Copy link
Contributor Author

Hi @abdosi , @rlhui , please help with the review. Thanks

@anamehra
Copy link
Contributor Author

anamehra commented Apr 3, 2024

I raised a PR to fix this #18452
Please help to review.

@anamehra
Copy link
Contributor Author

Please help with the review. Thanks

Copy link
Contributor

@mlok-nokia mlok-nokia left a comment

Choose a reason for hiding this comment

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

Looks good

@abdosi
Copy link
Contributor

abdosi commented Apr 22, 2024

@SuvarnaMeenakshi : can you help with review of this.

@abdosi abdosi requested a review from SuvarnaMeenakshi April 22, 2024 19:10
@SuvarnaMeenakshi
Copy link
Contributor

Thank you for the fix.
Seems like was missed in this PR #7632 and issue will NOT be seen if sonic-cfggen -m <minigraph.xml> is used.
Can we add a unit-test in sonic-config-engine multi-asic unit-test, so that if minigraph.xml is not passed, then the expected PORT from namespace is present in the output?

@anamehra
Copy link
Contributor Author

Thank you for the fix. Seems like was missed in this PR #7632 and issue will NOT be seen if sonic-cfggen -m <minigraph.xml> is used. Can we add a unit-test in sonic-config-engine multi-asic unit-test, so that if minigraph.xml is not passed, then the expected PORT from namespace is present in the output?

Thanks for review @SuvarnaMeenakshi ! May I use another PR to add UT? Busy with a couple of other activities. This PR needs to help with nightly T2 runs.

@SuvarnaMeenakshi
Copy link
Contributor

Thank you for the fix. Seems like was missed in this PR #7632 and issue will NOT be seen if sonic-cfggen -m <minigraph.xml> is used. Can we add a unit-test in sonic-config-engine multi-asic unit-test, so that if minigraph.xml is not passed, then the expected PORT from namespace is present in the output?

Thanks for review @SuvarnaMeenakshi ! May I use another PR to add UT? Busy with a couple of other activities. This PR needs to help with nightly T2 runs.

It might fall through later, it appears to be a simple UT can do the test in this case.
similar to test done here: https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-config-engine/tests/test_multinpu_cfggen.py#L209, without -m option and could check for presence of BP interface.
Without this current fix, the BP interface would not be present in the result as it would pick from global config_db.

@anamehra
Copy link
Contributor Author

Hi @SuvarnaMeenakshi , added the test case, please review. Thanks

Copy link
Contributor

@SuvarnaMeenakshi SuvarnaMeenakshi left a comment

Choose a reason for hiding this comment

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

lgtm

@anamehra
Copy link
Contributor Author

Hi @SuvarnaMeenakshi , @abdosi , please suggest what to be done for ms_conflict. Thanks

@rlhui
Copy link
Contributor

rlhui commented May 15, 2024

@anamehra please help resolve conflicts

@anamehra
Copy link
Contributor Author

@anamehra please help resolve conflicts

Hi @rlhui , I am not able to find details of the failure and what needs to be done. Waiting for inputs from @abdosi . Thanks

@anamehra
Copy link
Contributor Author

/azpw ms_conflict -f

@anamehra
Copy link
Contributor Author

Hi @abdosi , please merge. Thanks

@abdosi
Copy link
Contributor

abdosi commented May 22, 2024

@qiluo-msft : can you help with merge of this.

@qiluo-msft qiluo-msft merged commit de1a73f into sonic-net:master May 23, 2024
19 checks passed
@anamehra anamehra deleted the anamehra/sonic_cfggen branch November 24, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[multi-asic][202305]: 'config reload -l <>' option loads incorrect config
6 participants