-
Notifications
You must be signed in to change notification settings - Fork 531
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
QOS fieldvalue reference ABNF format to string changes #1754
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AshokDaparthi
I haven't got time to go over the entire PR. Some comments for now:
- Need to update the vs test cases as well, like test_buffer_dynamic.py, test_speed.py, and test_qos_map.py
I believe the current vs test is failing without these changes. - Need to update the db schema description.
- Can you fill the fields in the PR description?
By doing so reviewers can have a better understanding of this PR.
Thanks.
@stephenxs - Thanks for information, i will dig to test cases. i compiled only for sonic-broadcom.bin , i do not see any failure at build time. |
Yes. The vs test won't run when the sonic image (like sonic-broadcom.bin) is being built. It runs as part of the checks of the PR ( |
42b15cb
to
f0896e7
Compare
/azpw run |
f0896e7
to
b3e507e
Compare
/azpw run |
1 similar comment
/azpw run |
b3e507e
to
4273441
Compare
4273441
to
c0e0862
Compare
c0e0862
to
868f939
Compare
@@ -284,14 +277,6 @@ void BufferMgr::doBufferTableTask(Consumer &consumer, ProducerStateTable &applTa | |||
|
|||
for (auto i : kfvFieldsValues(t)) | |||
{ | |||
SWSS_LOG_INFO("Inserting field %s value %s", fvField(i).c_str(), fvValue(i).c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transformSeperator which is called in L269 can be removed as well. There is no separator now.
The function transformSeperator
and transformReference
can be removed as they are no longer called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transformSeperator is used to convert from config_db to app_db
@@ -1951,8 +1939,7 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key, | |||
{ | |||
// Headroom override | |||
pureDynamic = false; | |||
transformReference(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transformReference
is no longer called and can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// Deem it as a valid format | ||
// clear both type_name and object_name | ||
// as an indication to the caller that | ||
// such a case has been encountered | ||
type_name.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can an empty string be passed by redis-db? If no and this is a valid flow, we need to find another way to indicate clearing reference. Probably we can use "NULL".
orchagent/orch.cpp
Outdated
@@ -301,7 +301,7 @@ bool Orch::bake() | |||
} | |||
|
|||
/* | |||
- Validates reference has proper format which is [table_name:object_name] | |||
- Validates reference has proper format which is not ABNF [table_name:object_name] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emphasizing not ABNF
looks strange. I think Validates reference has proper format which is object_name
should be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
tokens = tokenize(ref_content, delimiter); | ||
if (tokens.size() != 2) | ||
|
||
if ((ref_in[0] == ref_start) || (ref_in[ref_in.size()-1] == ref_end)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether we need to check whether it's surrounded by []
here.
868f939
to
b42784f
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@AshokDaparthi please help to monitor the build progress and check the result. |
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 (#1754) should be merged. Merge can happen in this order: |
b42784f
to
3ab4d4b
Compare
/azpw run |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@stephenxs, @neethajohn - Please relook, i did modification to pass swss test cases until sonic-buildimage check in happens. Plan is to push ASAP and update sub modules. |
/azpw run |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
…ests Skip test cases until sonic-buildimage changes in Skip test cases until sonic-buildimage changes in
6c20b35
to
b0cc853
Compare
@neethajohn @stephenxs - Could you please help to review and merge, so that i can push other PR's |
@stephenxs , could you please sign-off? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@prsunny approved. |
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
Hi @AshokDaparthi |
@stephenxs - Yes, i can do. I am waiting for build to be ready with today sonic-buildimage changes. i did not see this yet in artifacts. only docker-sonic-vs.gz comes out of today image only checks will pass. So i will rise it tomorrow. |
What I did Reverted skipped test_buffer_dynamic as part of #1754 Why I did it How I verified it sudo pytest --dvsname=vs --forcedvs -sv --keeptb test_buffer_dynamic.py ======================================================= test session starts ======================================================== platform linux -- Python 3.6.9, pytest-6.2.4, py-1.10.0, pluggy-0.13.1 -- /usr/bin/python3 cachedir: .pytest_cache rootdir: /home/ashokd/swss-vs/ashok-swss/sonic-swss/tests plugins: flaky-3.7.0 collected 9 items test_buffer_dynamic.py::TestBufferMgrDyn::test_changeSpeed remove extra link dummy PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_changeCableLen PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_MultipleLosslessPg PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_headroomOverride PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_mtuUpdate PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_nonDefaultAlpha PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_sharedHeadroomPool PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_shutdownPort PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_autoNegPort PASSED
* QOS field reference ABNF format to string changes
What I did Reverted skipped test_buffer_dynamic as part of sonic-net#1754 Why I did it How I verified it sudo pytest --dvsname=vs --forcedvs -sv --keeptb test_buffer_dynamic.py ======================================================= test session starts ======================================================== platform linux -- Python 3.6.9, pytest-6.2.4, py-1.10.0, pluggy-0.13.1 -- /usr/bin/python3 cachedir: .pytest_cache rootdir: /home/ashokd/swss-vs/ashok-swss/sonic-swss/tests plugins: flaky-3.7.0 collected 9 items test_buffer_dynamic.py::TestBufferMgrDyn::test_changeSpeed remove extra link dummy PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_changeCableLen PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_MultipleLosslessPg PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_headroomOverride PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_mtuUpdate PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_nonDefaultAlpha PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_sharedHeadroomPool PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_shutdownPort PASSED test_buffer_dynamic.py::TestBufferMgrDyn::test_autoNegPort PASSED
…nic-net#1754) What I did Allow system with no ports in config db run without errors. This is needed for modular system which should boot properly without line cards. How I did it Do not raise warning if ports dictionary is empty. How to verify it Run show interfaces status
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
Depends on sonic-net/sonic-utilities#1626
Depends on sonic-net/sonic-buildimage#7752
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]"
},
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 and made it as string.
Example:
"Ethernet92|3": {
"scheduler": "scheduler.1",
"wred_profile": "AZURE_LOSSLESS"
},
Dependent pul; requests:
sonic-net/sonic-buildimage#7752 - To modify platfrom files
sonic-net/sonic-buildimage#7281 - Yang model
sonic-net/sonic-utilities#1626 - DB migration