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

Remove sorting from qos_config.j2 #5505

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

abdosi
Copy link
Contributor

@abdosi abdosi commented Sep 29, 2020

Why I did:
Optimize not use use use sort function as if interface name changes we need to update sort function.
Also this is not needed as all Ports are derive from port_config.ini where ports are listed
in fixed static sort order.
Also we do not sort port in buffer_config.j2 so made logic similar to
that and keep it simple instead of enhancing sort function.

How I did:
Fix is to to remove sorting from j2 template as this is not needed

How I verified:
config load_minigraph -y is fine which internally invoke config qos reload.

The sort Function on Portname is not able to understand
Backplance Interface name.

Fix is to to remove sorting from j2 template as this is not needed
as all Ports are dervice port_config.ini where ports are listed
in fixed static sort order.

Also we do not sort port in buffer_config.j2 so made logic similar to
that and keep it simple instead of enhancing sort function.

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
@smaheshm
Copy link
Contributor

smaheshm commented Sep 29, 2020

Can you add more info about where is it breaking. This change is there in both master and 201911.

https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-config-engine/sonic-cfggen#L58

@smaheshm thanks for pointing out that was git merge issue that caused this. Things are fine now.

However I still feel this is good change to have as it remove dependency on updating sort function with any new alias
and sorted order is already maintained as the way port_config.ini is define. As mention above even buffer_config.j2 do not sort and rely on port_config.ini define order.

Please review

@abdosi
Copy link
Contributor Author

abdosi commented Oct 2, 2020

retest broadcom please

2 similar comments
@abdosi
Copy link
Contributor Author

abdosi commented Oct 4, 2020

retest broadcom please

@abdosi
Copy link
Contributor Author

abdosi commented Oct 6, 2020

retest broadcom please

@abdosi abdosi changed the title Fix config qos reload on Multi-asic platforms. Remove sorting from qos_config.j2 Oct 21, 2020
@smaheshm
Copy link
Contributor

retest broadcom please

@smaheshm
Copy link
Contributor

@abdosi Is this still relevant?

@abdosi
Copy link
Contributor Author

abdosi commented May 30, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@smaheshm
Copy link
Contributor

smaheshm commented Nov 9, 2021

@abdosi Can you also update sonic-cfggen to remove the sort_by_index. This change is required for chassis.

@smaheshm
Copy link
Contributor

smaheshm commented Nov 9, 2021

/azp run

@azure-pipelines
Copy link

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

@smaheshm
Copy link
Contributor

smaheshm commented Nov 9, 2021

/azp run 1

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@smaheshm smaheshm added the Chassis 🤖 Modular chassis support label Nov 9, 2021
@abdosi
Copy link
Contributor Author

abdosi commented Nov 13, 2021

@abdosi Can you also update sonic-cfggen to remove the sort_by_index. This change is required for chassis.

@smaheshm updated UT output accordingly.

@smaheshm
Copy link
Contributor

can you also remove the API from sonic-cfggen

@smaheshm
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

@abdosi abdosi requested a review from a team as a code owner June 10, 2022 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants