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

Apply DSCP_TO_TC_MAP from PORT_QOS_MAP|global to switch level #2314

Merged
merged 5 commits into from
Jun 15, 2022

Conversation

bingwang-ms
Copy link
Contributor

@bingwang-ms bingwang-ms commented Jun 7, 2022

Signed-off-by: bingwang [email protected]

What I did
This PR is to update the code for applying switch level DSCP_TO_TC_MAP.
After PR sonic-net/sonic-buildimage#10565, there will be two DSCP_TO_TC_MAP

  • DSCP_TO_TC_MAP|AZURE is the default map, which is used at port level and switch level
  • DSCP_TO_TC_MAP|AZURE_TUNNEL is used to remap the priority of tunnel traffic in dualtor deployment

To address the issue, an entry PORT_QOS_MAP|global will be added into config_db

"PORT_QOS_MAP": {
        "global": {
            "dscp_to_tc_map": "AZURE"
        }
}

The entry will be consumed by qosorch, and the specified map will be applied to switch.

Why I did it
This change is to ensure the correct DSCP_TO_TC_MAP is applied to switch level.

How I verified it
Verified by a new test case test_dscp_to_tc_map_applied_to_switch

collected 9 items                                                                                                                                                                                     

test_qos_map.py::TestDot1p::test_dot1p_cfg PASSED                                                                                                                                               [ 11%]
test_qos_map.py::TestDot1p::test_port_dot1p PASSED                                                                                                                                              [ 22%]
test_qos_map.py::TestCbf::test_dscp_to_fc PASSED                                                                                                                                                [ 33%]
test_qos_map.py::TestCbf::test_exp_to_fc PASSED                                                                                                                                                 [ 44%]
test_qos_map.py::TestCbf::test_per_port_cbf_binding PASSED                                                                                                                                      [ 55%]
test_qos_map.py::TestMplsTc::test_mpls_tc_cfg PASSED                                                                                                                                            [ 66%]
test_qos_map.py::TestMplsTc::test_port_mpls_tc PASSED                                                                                                                                           [ 77%]
test_qos_map.py::TestDscpToTcMap::test_dscp_to_tc_map_applied_to_switch PASSED                                                                                                                  [ 88%]
test_qos_map.py::test_nonflaky_dummy PASSED                                                                                                                                                     [100%]

Details if related

Signed-off-by: bingwang <[email protected]>
@stephenxs
Copy link
Collaborator

Hi
It definitely works and resolves the issue to hardcode it for now but I don’t tend to think it is the best way to hardcode the name of the global DSCP_TO_TC_MAP. We'd better not assume that all SONiC customers will have AZURE as their default item name.
Currently, AZURE does not exist in swss except for the test codes.
I think maybe a formal way is to introduce a table in CONFIG_DB and designate the global DSCP_TO_TC_MAP there. If the info does not exist, we can choose

  • to apply any DSCP_TO_TC_MAP once it is configured to the system, for the purpose of backward compatibility
  • not to apply any DSCP_TO_TC_MAP globally

I would prefer the second option because by doing so the global DSCP_TO_TC_MAP won't be applied to vendors who doesn't need it. Db_migrator can be leveraged for backward caompatibility.
Regarding the table, we can

  • either introduce a new table to indicate which DSCP_TO_TC_MAP to apply globally
  • or introduce a new key in PORT_QOS_MAP, like PORT_QOS_MAP|global.

Stephen

@bingwang-ms
Copy link
Contributor Author

Hi It definitely works and resolves the issue to hardcode it for now but I don’t tend to think it is the best way to hardcode the name of the global DSCP_TO_TC_MAP. We'd better not assume that all SONiC customers will have AZURE as their default item name. Currently, AZURE does not exist in swss except for the test codes. I think maybe a formal way is to introduce a table in CONFIG_DB and designate the global DSCP_TO_TC_MAP there. If the info does not exist, we can choose

  • to apply any DSCP_TO_TC_MAP once it is configured to the system, for the purpose of backward compatibility
  • not to apply any DSCP_TO_TC_MAP globally

I would prefer the second option because by doing so the global DSCP_TO_TC_MAP won't be applied to vendors who doesn't need it. Db_migrator can be leveraged for backward caompatibility. Regarding the table, we can

  • either introduce a new table to indicate which DSCP_TO_TC_MAP to apply globally
  • or introduce a new key in PORT_QOS_MAP, like PORT_QOS_MAP|global.

Stephen

Thanks @stephenxs for the great suggestions.
I think option 2 introduce a new key in PORT_QOS_MAP, like PORT_QOS_MAP|global. would be better.
I checked the code in qosorch, this special case can be handled by adding some logic in handlePortQosMapTable to handle the DSCP_TO_TC_MAP for global.
And also, we need to update YANG to support entry whose name is not a port name.

orchagent/qosorch.cpp Outdated Show resolved Hide resolved
Signed-off-by: bingwang <[email protected]>
@bingwang-ms bingwang-ms changed the title Apply DSCP_TO_TC_MAP|AZURE to switch level Apply DSCP_TO_TC_MAP from PORT_QOS_MAP|global to switch level Jun 9, 2022
cfgmgr/buffermgr.cpp Show resolved Hide resolved
orchagent/qosorch.cpp Outdated Show resolved Hide resolved
task_process_status task_status = task_process_status::task_success;
if (OP == DEL_COMMAND)
{
// Ignore the DEL operation for switch level mapping
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be set to SAI_NULL_OBJECT_ID when being removed?
It will be unable to clean the reference to the global map if we ignore the global map removing operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Thanks

Signed-off-by: bingwang <[email protected]>
Signed-off-by: bingwang <[email protected]>
@bingwang-ms
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bingwang-ms bingwang-ms merged commit 4374348 into sonic-net:master Jun 15, 2022
yxieca pushed a commit that referenced this pull request Jun 20, 2022
)

* Apply DSCP_TO_TC_MAP|AZURE to switch level

Signed-off-by: bingwang <[email protected]>
preetham-singh pushed a commit to preetham-singh/sonic-swss that referenced this pull request Aug 6, 2022
…nic-net#2314)

* Apply DSCP_TO_TC_MAP|AZURE to switch level

Signed-off-by: bingwang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants