sonic-buildimage icon indicating copy to clipboard operation
sonic-buildimage copied to clipboard

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

Open anamehra opened this issue 11 months ago • 8 comments

Signed-off-by: Anand Mehra [email protected]

Fixes: https://github.com/sonic-net/sonic-buildimage/issues/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
  • [x] 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)

anamehra avatar Mar 23 '24 08:03 anamehra

Hi @abdosi , please review.

anamehra avatar Mar 26 '24 17:03 anamehra

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

anamehra avatar Mar 29 '24 03:03 anamehra

I raised a PR to fix this https://github.com/sonic-net/sonic-buildimage/pull/18452 Please help to review.

anamehra avatar Apr 03 '24 17:04 anamehra

Please help with the review. Thanks

anamehra avatar Apr 10 '24 01:04 anamehra

@SuvarnaMeenakshi : can you help with review of this.

abdosi avatar Apr 22 '24 18:04 abdosi

Thank you for the fix. Seems like was missed in this PR https://github.com/sonic-net/sonic-buildimage/pull/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?

SuvarnaMeenakshi avatar Apr 29 '24 17:04 SuvarnaMeenakshi

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.

anamehra avatar Apr 29 '24 23:04 anamehra

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.

SuvarnaMeenakshi avatar May 01 '24 00:05 SuvarnaMeenakshi

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

anamehra avatar May 12 '24 03:05 anamehra

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

anamehra avatar May 14 '24 22:05 anamehra

@anamehra please help resolve conflicts

rlhui avatar May 15 '24 18:05 rlhui

@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 avatar May 16 '24 00:05 anamehra

/azpw ms_conflict -f

anamehra avatar May 16 '24 22:05 anamehra

Hi @abdosi , please merge. Thanks

anamehra avatar May 20 '24 05:05 anamehra

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

abdosi avatar May 22 '24 00:05 abdosi