QOS fieldvalue reference ABNF format to string #1626

AshokDaparthi · 2021-05-20T16:45:23Z

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

stephenxs

Hi @AshokDaparthi
Some other parts need to be updated as well:

The buffer configuration is stored in APPL_DB. The warm reboot will fail if the switch is upgrading from an image before this change to one after. So db migrator needs to handle APPL_DB as well.
Suggest adding unit test cases for db migrator.
The version defined at the beginning of db_migrator.py needs to be updated as well.
Existing unit test cases need to be updated accordingly, like mock_tables, and mock tables for qos, filter fdb, counter poll, SKU create tool, etc... You might want to go over all the test cases.
I believe the unit test is failing without these changes

Thanks

AshokDaparthi · 2021-05-24T22:26:13Z

@stephenxs - Hi Stephan, I have query regarding "The buffer configuration is stored in APPL_DB. The warm reboot will fail if the switch is upgrading from an image before this change to one after. So db migrator needs to handle APPL_DB as well." I have not seen complete warm reboot design. Can you help me to understand, While warm/cold reboot db_migratory.py will trigger config_db update for qos right, which internally trigger swss to update APP_DB with new QOS field values right or during warmreboot swss will not process config_db updates?

stephenxs · 2021-05-25T00:56:31Z

@stephenxs - Hi Stephan, I have query regarding "The buffer configuration is stored in APPL_DB. The warm reboot will fail if the switch is upgrading from an image before this change to one after. So db migrator needs to handle APPL_DB as well." I have not seen complete warm reboot design. Can you help me to understand, While warm/cold reboot db_migratory.py will trigger config_db update for qos right, which internally trigger swss to update APP_DB with new QOS field values right or during warmreboot swss will not process config_db updates?

The buffer tables are consumed from APPL_DB instead of CONFIG_DB.
In a normal starting flow,

the buffer manager generates table entries and pushes them into APPL_DB.
the orchagent consumes the entries in APPL_DB and pushes entries into ASIC_DB.

This is for both dynamic and traditional buffer models.
In a warm reboot flow,

the APPL_DB is saved before warm reboot and restored after it, which means the entries consumed by the orchagent are restored instead of pushed by the manager daemons.
the buffer manager generates table entries as well, but that's too late for warm reboot to meet its converging time.
given the data format has been updated before and after warm reboot, we need to update the entries in APPL_DB as well

We have a similar flow which the entries in APPL_DB is prepared for warm reboot. prepare_dynamic_buffer_for_warm_reboot. It works for both buffer model.
Hope this helps.

AshokDaparthi · 2021-06-01T23:37:46Z

/azpw run

stephenxs · 2021-07-12T01:11:45Z

config/main.py

@@ -3703,7 +3703,7 @@ def update_pg(ctx, interface_name, pg_map, override_profile, add = True):
            ctx.fail("Profile {} doesn't exist".format(override_profile))
        if not 'xoff' in profile_dict.keys() and 'size' in profile_dict.keys():
            ctx.fail("Profile {} doesn't exist or isn't a lossless profile".format(override_profile))
-        profile_full_name = "[BUFFER_PROFILE|{}]".format(override_profile)
+        profile_full_name = "{}".format(override_profile)


I think profile_full_name is no longer necessary. Just using override_profile should be fine.
Originally, profile_full_name was introduced because the profile reference differed from the profile name. Now they are the same.

config/main.py

scripts/db_migrator.py

stephenxs · 2021-07-12T03:10:58Z

scripts/db_migrator.py

+                    db_key = table_name + db_delimeter + key
+
+                for field in fields_list:
+                  if field in value:


Please fix the indent in this block as well. The indent should be 4.

stephenxs · 2021-07-12T03:39:01Z

scripts/db_migrator.py

+                        log.log_info("Found ABNF format field value in table {} key {} field {} val {}".format(table_name, db_key, field, fieldVal))
+                        value_list = fieldVal.split(",")
+                        for item in value_list:
+                          if "[" not in item or db_delimeter not in item or "]" not in item:


Suggested change

if "[" not in item or db_delimeter not in item or "]" not in item:

if "[" != item[0] or db_delimeter not in item or "]" != item[-1]:

scripts/db_migrator.py

lguohan · 2021-08-26T17:44:54Z

/azp run

azure-pipelines · 2021-08-26T17:45:07Z

Azure Pipelines successfully started running 1 pipeline(s).

zhangyanzhao · 2021-08-26T17:51:31Z

@AshokDaparthi please help to monitor the build progress and check the result.

rathnasabapathyv · 2021-08-26T17:59:18Z

As per the discussion in yang-subgroup-meeting (8/26) with @lguohan , we have decided to merge sonic-utilities PR (#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 : #1626
sonic-buildimage PR for new config-DB changes : sonic-net/sonic-buildimage#7752
sonic-swss PR : sonic-net/sonic-swss#1754

AshokDaparthi · 2021-08-31T17:16:44Z

/azpw run

mssonicbld · 2021-08-31T17:16:45Z

/AzurePipelines run

azure-pipelines · 2021-08-31T17:16:56Z

Azure Pipelines successfully started running 1 pipeline(s).

AshokDaparthi · 2021-08-31T23:53:28Z

@stephenxs , @neethajohn - Could you please re approve. Plan is to
1# Merge sonic-utilities
2# Modify SHA 3# Then re trigger sonic-swss Checks and merge it 4# Merge sonic-buildimage

stephenxs · 2021-09-01T01:02:41Z

@stephenxs , @neethajohn - Could you please re approve. Plan is to
1# Merge sonic-utilities
2# Modify SHA 3# Then re trigger sonic-swss Checks and merge it 4# Merge sonic-buildimage

Hi @AshokDaparthi
Two questions.

What's the difference between this one and the commit we approved? Is it just rebased to the latest master commit?
To what branch will this feature be merged?
I was wondering whether we will have a dedicated sonic-mgmt branch for testing sonic switch without this feature. Check this PR for details
@neethajohn FYI.

Thanks

AshokDaparthi · 2021-09-01T04:42:12Z

@stephenxs , @neethajohn - Could you please re approve. Plan is to
1# Merge sonic-utilities
2# Modify SHA 3# Then re trigger sonic-swss Checks and merge it 4# Merge sonic-buildimage

Hi @AshokDaparthi
Two questions.
1. What's the difference between this one and the commit we approved? Is it just rebased to the latest master commit?

2. To what branch will this feature be merged?
   I was wondering whether we will have a dedicated sonic-mgmt branch for testing sonic switch without this feature. Check [this PR](https://github.com/Azure/sonic-mgmt/pull/3824#issuecomment-906797660) for details
   @neethajohn FYI.
Thanks

1# No changes, I did rebase all my pull requests to latest master
2# Yes, We required sonic-mgmt branch separation before and after merge this changes.

AshokDaparthi · 2021-09-01T20:59:54Z

@neethajohn - Could you please review and approve.

zhangyanzhao · 2021-09-02T17:20:46Z

@neethajohn can you please help to review and approve this.

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

stephenxs suggested changes May 21, 2021

View reviewed changes

AshokDaparthi force-pushed the sonic-qos-yangs branch 4 times, most recently from 7572243 to 6acf650 Compare May 28, 2021 18:45

AshokDaparthi force-pushed the sonic-qos-yangs branch 3 times, most recently from 56c5dab to e1b6e42 Compare June 2, 2021 17:42

AshokDaparthi mentioned this pull request Jun 3, 2021

[sonic-swss] test case of test_dpb_arp_flush in test_port_dpb_system.py are failing randomly sonic-net/sonic-buildimage#7791

Open

AshokDaparthi force-pushed the sonic-qos-yangs branch 2 times, most recently from 02f1290 to cfe3542 Compare June 21, 2021 20:58

anshuv-mfst requested a review from neethajohn July 1, 2021 17:56

stephenxs reviewed Jul 12, 2021

View reviewed changes

AshokDaparthi force-pushed the sonic-qos-yangs branch 4 times, most recently from ba2d86a to 180c48d Compare July 13, 2021 22:50

AshokDaparthi mentioned this pull request Jul 20, 2021

Modified test cases by QOS DB reference format remove sonic-net/sonic-mgmt#3824

Merged

stephenxs previously approved these changes Jul 30, 2021

View reviewed changes

QOS fieldvalue refernce ABNF format to string

b1bd4a3

AshokDaparthi dismissed stephenxs’s stale review via b1bd4a3 August 26, 2021 19:52

AshokDaparthi force-pushed the sonic-qos-yangs branch from 180c48d to b1bd4a3 Compare August 26, 2021 19:52

stephenxs approved these changes Sep 1, 2021

View reviewed changes

lguohan approved these changes Sep 2, 2021

View reviewed changes

lguohan merged commit 6483b0b into sonic-net:master Sep 2, 2021

zhangyanzhao mentioned this pull request Sep 2, 2021

SONiC QoS MAPs and PORT_QOS_MAP Yang sonic-net/sonic-buildimage#7375

Merged

4 tasks

AndriiLozovyi mentioned this pull request Oct 12, 2021

[pfcwd] Fix test_pfcwd_mmu_change IndexError sonic-net/sonic-mgmt#4465

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QOS fieldvalue reference ABNF format to string #1626

QOS fieldvalue reference ABNF format to string #1626

AshokDaparthi commented May 20, 2021 •

edited

Loading

stephenxs left a comment

AshokDaparthi commented May 24, 2021

stephenxs commented May 25, 2021 •

edited

Loading

AshokDaparthi commented Jun 1, 2021

stephenxs Jul 12, 2021

AshokDaparthi Jul 12, 2021

stephenxs Jul 12, 2021

AshokDaparthi Jul 12, 2021

stephenxs Jul 12, 2021

AshokDaparthi Jul 12, 2021

lguohan commented Aug 26, 2021

azure-pipelines bot commented Aug 26, 2021

zhangyanzhao commented Aug 26, 2021

rathnasabapathyv commented Aug 26, 2021 •

edited

Loading

AshokDaparthi commented Aug 31, 2021

mssonicbld commented Aug 31, 2021

azure-pipelines bot commented Aug 31, 2021

AshokDaparthi commented Aug 31, 2021

stephenxs commented Sep 1, 2021 •

edited

Loading

AshokDaparthi commented Sep 1, 2021

AshokDaparthi commented Sep 1, 2021

zhangyanzhao commented Sep 2, 2021

	if "[" not in item or db_delimeter not in item or "]" not in item:
	if "[" != item[0] or db_delimeter not in item or "]" != item[-1]:

QOS fieldvalue reference ABNF format to string #1626

QOS fieldvalue reference ABNF format to string #1626

Conversation

AshokDaparthi commented May 20, 2021 • edited Loading

stephenxs left a comment

Choose a reason for hiding this comment

AshokDaparthi commented May 24, 2021

stephenxs commented May 25, 2021 • edited Loading

AshokDaparthi commented Jun 1, 2021

stephenxs Jul 12, 2021

Choose a reason for hiding this comment

AshokDaparthi Jul 12, 2021

Choose a reason for hiding this comment

stephenxs Jul 12, 2021

Choose a reason for hiding this comment

AshokDaparthi Jul 12, 2021

Choose a reason for hiding this comment

stephenxs Jul 12, 2021

Choose a reason for hiding this comment

AshokDaparthi Jul 12, 2021

Choose a reason for hiding this comment

lguohan commented Aug 26, 2021

azure-pipelines bot commented Aug 26, 2021

zhangyanzhao commented Aug 26, 2021

rathnasabapathyv commented Aug 26, 2021 • edited Loading

AshokDaparthi commented Aug 31, 2021

mssonicbld commented Aug 31, 2021

azure-pipelines bot commented Aug 31, 2021

AshokDaparthi commented Aug 31, 2021

stephenxs commented Sep 1, 2021 • edited Loading

AshokDaparthi commented Sep 1, 2021

AshokDaparthi commented Sep 1, 2021

zhangyanzhao commented Sep 2, 2021

AshokDaparthi commented May 20, 2021 •

edited

Loading

stephenxs commented May 25, 2021 •

edited

Loading

rathnasabapathyv commented Aug 26, 2021 •

edited

Loading

stephenxs commented Sep 1, 2021 •

edited

Loading