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

SONIC QOS YANG - Remove qos tables field value reference format #7752

Merged

Conversation

AshokDaparthi
Copy link
Contributor

@AshokDaparthi AshokDaparthi commented May 29, 2021

Depends on sonic-net/sonic-utilities#1626
Depends on sonic-net/sonic-swss#1754

QOS tables in config db used ABNF format i.e "[TABLE_NAME|name] to refer fieldvalue to other qos tables.

Example:
Config DB:
"Ethernet92|3": {
"scheduler": "[SCHEDULER|scheduler.1]",
"wred_profile": "[WRED_PROFILE|AZURE_LOSSLESS]"
},
"Ethernet0|0": {
"profile": "[BUFFER_PROFILE|ingress_lossy_profile]"
},
"Ethernet0": {
"dscp_to_tc_map": "[DSCP_TO_TC_MAP|AZURE]",
"pfc_enable": "3,4",
"pfc_to_queue_map": "[MAP_PFC_PRIORITY_TO_QUEUE|AZURE]",
"tc_to_pg_map": "[TC_TO_PRIORITY_GROUP_MAP|AZURE]",
"tc_to_queue_map": "[TC_TO_QUEUE_MAP|AZURE]"
},

This format is not consistent with other DB schema followed in sonic.
And also this reference in DB is not required, This is taken care by YANG "leafref".

Removed this format from all platform files to consistent with other sonic db schema.
Example:
"Ethernet92|3": {
"scheduler": "scheduler.1",
"wred_profile": "AZURE_LOSSLESS"
},

Dependent pull requests:
#7752 - To modify platfrom files
#7281 - Yang model
sonic-net/sonic-utilities#1626 - DB migration
sonic-net/sonic-swss#1754 - swss change to remove ABNF format

@anshuv-mfst
Copy link

Hi @neethajohn , @stephenxs- could you please review this PR. This is related to sonic-net/sonic-utilities#1626 and sonic-net/sonic-swss#1754, thanks.

@stephenxs
Copy link
Collaborator

Hi @neethajohn , @stephenxs- could you please review this PR. This is related to Azure/sonic-utilities#1626 and Azure/sonic-swss#1754, thanks.

Sure. Will review it.
Btw, is there a PR for the regression test? I think we need to update regression test as well

@neethajohn
Copy link
Contributor

Hi @neethajohn , @stephenxs- could you please review this PR. This is related to Azure/sonic-utilities#1626 and Azure/sonic-swss#1754, thanks.

Sure. Will review it.
Btw, is there a PR for the regression test? I think we need to update regression test as well

Are you referring to the sonic-mgmt test? I don't think there is a PR yet. @AshokDaparthi, please update that as well

@AshokDaparthi
Copy link
Contributor Author

Hi @neethajohn , @stephenxs- could you please review this PR. This is related to Azure/sonic-utilities#1626 and Azure/sonic-swss#1754, thanks.

Sure. Will review it.
Btw, is there a PR for the regression test? I think we need to update regression test as well

Are you referring to the sonic-mgmt test? I don't think there is a PR yet. @AshokDaparthi, please update that as well

@neethajohn - I will explore what to update, i am not aware of sonic-mgmt tests.

@AshokDaparthi
Copy link
Contributor Author

@stephenxs @neethajohn - Regarding sonic-mgmt test cases, i am not doing any new functionality, Do you want me to change existing test cases to remove the ABNF format reference?

@neethajohn
Copy link
Contributor

@stephenxs @neethajohn - Regarding sonic-mgmt test cases, i am not doing any new functionality, Do you want me to change existing test cases to remove the ABNF format reference?

Yes

stephenxs
stephenxs previously approved these changes Jul 12, 2021
@AshokDaparthi
Copy link
Contributor Author

@stephenxs @neethajohn - Updated the sonic-mgmt test cases sonic-net/sonic-mgmt#3824

@stephenxs
Copy link
Collaborator

@AshokDaparthi can you fix the conflicts

@rathnasabapathyv
Copy link
Collaborator

As per the discussion in yang-subgroup-meeting (8/26) with @lguohan , we have decided to merge sonic-utilities PR (sonic-net/sonic-utilities#1626) 1st followed by sub-module update in sonic-buildimage. Then this sonic-swss PR (sonic-net/sonic-swss#1754) should be merged.

Merge can happen in this order:
sonic-utilities PR for DB-migration logic : sonic-net/sonic-utilities#1626
sonic-buildimage PR for new config-DB changes : #7752
sonic-swss PR : sonic-net/sonic-swss#1754

@AshokDaparthi AshokDaparthi force-pushed the sonic-yang-qos-fv-db-ref-remove branch from d94b14e to 569722a Compare August 31, 2021 19:34
lguohan pushed a commit to sonic-net/sonic-utilities that referenced this pull request Sep 2, 2021
Qos tables in config db and app db used ABNF format i.e "[TABLE_NAME|name] to refer fieldvalue other qos tables.

Example:
Config DB:
"Ethernet92|3": {
"scheduler": "[SCHEDULER|scheduler.1]",
"wred_profile": "[WRED_PROFILE|AZURE_LOSSLESS]"
},
"Ethernet0|0": {
"profile": "[BUFFER_PROFILE|ingress_lossy_profile]"
},
"Ethernet0": {
"dscp_to_tc_map": "[DSCP_TO_TC_MAP|AZURE]",
"pfc_enable": "3,4",
"pfc_to_queue_map": "[MAP_PFC_PRIORITY_TO_QUEUE|AZURE]",
"tc_to_pg_map": "[TC_TO_PRIORITY_GROUP_MAP|AZURE]",
"tc_to_queue_map": "[TC_TO_QUEUE_MAP|AZURE]"
},

AppDB:
"BUFFER_QUEUE_TABLE:Ethernet88:3-4": {
"profile": "[BUFFER_PROFILE_TABLE:egress_lossless_profile]"
},

1#This format is not consistent with other DB schema followed in sonic.
2# Added db_migrator.py case to  change from old format in config_db and appl_db  to new format. 
3#Modified the test case 

Dependent pull requests: 
sonic-net/sonic-buildimage#7752  - To modify platfrom files 
sonic-net/sonic-buildimage#7281 - Yang model 
sonic-net/sonic-swss#1754    - swss change to remove ABNF format
@neethajohn
Copy link
Contributor

@AshokDaparthi, please resolve the merge conflicts

@neethajohn
Copy link
Contributor

@AshokDaparthi, how are these changes verified? Also you have only mentioned #7281 as a dependency in this PR. That contains only the SCHEDULER, WRED_PROFILE and QUEUE related changes. What about the other MMU tables (BUFFER PROFILE, BUFFER_POOL etc)? since this PR contains changes for all MMU/QOS attributes

@AshokDaparthi
Copy link
Contributor Author

@neethajohn - #7281 this PR is YNAG PR. For ALL QOS YANG PR's are blocked by this DB format. These YNAG PR's for buffers will are opened by broadcom else once this PR's are merged i will push sonic YNAG PR's.
Regarding testing, i get sonic-utilities , sonic-swss. sonic-buildiimage changes and created image and tested manually broadcom platform.

@neethajohn neethajohn changed the title SONIC QOS YANG - Remove qos tables field value refernce format SONIC QOS YANG - Remove qos tables field value reference format Sep 14, 2021
@neethajohn
Copy link
Contributor

@AshokDaparthi , can you please mention the merge order that you had discussed with Prince in this PR

@AshokDaparthi
Copy link
Contributor Author

@neethajohn - Merged order is below
1#sonic-net/sonic-swss#1754
2# sonic-net/sonic-mgmt#3824
3# submodule update of swss
4# Merge this pull request
Can you help with merge first 2.

@neethajohn neethajohn merged commit 6cbdf11 into sonic-net:master Sep 28, 2021
malletvapid23 added a commit to malletvapid23/Sonic-Utility that referenced this pull request Aug 3, 2023
Qos tables in config db and app db used ABNF format i.e "[TABLE_NAME|name] to refer fieldvalue other qos tables.

Example:
Config DB:
"Ethernet92|3": {
"scheduler": "[SCHEDULER|scheduler.1]",
"wred_profile": "[WRED_PROFILE|AZURE_LOSSLESS]"
},
"Ethernet0|0": {
"profile": "[BUFFER_PROFILE|ingress_lossy_profile]"
},
"Ethernet0": {
"dscp_to_tc_map": "[DSCP_TO_TC_MAP|AZURE]",
"pfc_enable": "3,4",
"pfc_to_queue_map": "[MAP_PFC_PRIORITY_TO_QUEUE|AZURE]",
"tc_to_pg_map": "[TC_TO_PRIORITY_GROUP_MAP|AZURE]",
"tc_to_queue_map": "[TC_TO_QUEUE_MAP|AZURE]"
},

AppDB:
"BUFFER_QUEUE_TABLE:Ethernet88:3-4": {
"profile": "[BUFFER_PROFILE_TABLE:egress_lossless_profile]"
},

1#This format is not consistent with other DB schema followed in sonic.
2# Added db_migrator.py case to  change from old format in config_db and appl_db  to new format. 
3#Modified the test case 

Dependent pull requests: 
sonic-net/sonic-buildimage#7752  - To modify platfrom files 
sonic-net/sonic-buildimage#7281 - Yang model 
sonic-net/sonic-swss#1754    - swss change to remove ABNF format
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.

5 participants