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

Define SYSTEM_DEFAULTS table to control tunnel_qos_remap #10877

Merged
merged 4 commits into from
May 28, 2022

Conversation

bingwang-ms
Copy link
Contributor

@bingwang-ms bingwang-ms commented May 19, 2022

Signed-off-by: bingwang [email protected]

Why I did it

This PR is to define a new table SYSTEM_DEFAULTS to turn on/off new features in SONiC.
Currently, only the flag to turn on/off tunnel_qos_remap is in the new table.
HLD sonic-net/SONiC#982

How I did it

  1. Define a new section in minigraph.xml
<SystemDefaultsDeclaration>
    <a:SystemDefaults xmlns:a="http://schemas.datacontract.org/2004/07/Microsoft.Search.Autopilot.Evolution">
		 <a:SystemDefault>
            <a:Name>TunnelQosRemapEnabled</a:Name>
            <a:Value>True</a:Value>
         </a:SystemDefault>
    </a:SystemDefaults>
</SystemDefaultsDeclaration>
  1. Update minigrapg.py to parse the new section, and write into config_db.
  2. The Tunnel Qos remapping feature will be turn off if TunnelQosRemapEnabled=False. Otherwise minigraph.py will try to parse new fields in minigraph.xml, and write the parsed value into config_db.
    The extra values for tunnel qos remapping are as below.
 "src_ip": "25.1.1.10",
 "decap_dscp_to_tc_map": "AZURE_TUNNEL",
 "decap_tc_to_pg_map": "AZURE_TUNNEL",
 "encap_tc_to_dscp_map": "AZURE_TUNNEL",
 "encap_tc_to_queue_map": "AZURE_TUNNEL"

Changes for the templates will be raised in another PR.

How to verify it

Verified by adding new UT cases.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111

Description for the changelog

Define SYSTEM_DEFAULTS table to control tunnel_qos_remap.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@bingwang-ms
Copy link
Contributor Author

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

lguohan
lguohan previously approved these changes May 19, 2022
@@ -1714,7 +1765,14 @@ def get_tunnel_entries(tunnel_intfs, lo_intfs, hostname):
for type, tunnel_dict in tunnel_intfs.items():
for tunnel_key, tunnel_attr in tunnel_dict.items():
tunnel_attr['dst_ip'] = lo_addr

if (tunnel_qos_remap.get('status') == 'enabled') and (mux_tunnel_name == tunnel_key) and (peer_switch_ip is not None):
tunnel_attr['src_ip'] = peer_switch_ip
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the minigraph is anyways changing, can we have the src_ip to be coming from minigraph TunnelInterface instead of having the logic to determine from peer_switch_ip ?

Copy link
Contributor Author

@bingwang-ms bingwang-ms May 20, 2022

Choose a reason for hiding this comment

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

I was also thinking about that. The problem is, I'm afraid it's not easy to implement that logic in Metadata.
For example, for upper_tor, the peer_ip would be 10.1.0.33, while for lower_tor, it would be 10.1.0.32. That means we have to generate different minigraph for upper_tor/lower_tor. I don't konw if it's doable. It may require a bunch of code change. So I put it in minigraph parser.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, this would be pretty simple to implement. And btw, the minigraph is indeed different for upper and lower tor. Approving this!

@bingwang-ms
Copy link
Contributor Author

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bingwang-ms
Copy link
Contributor Author

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@lolyu lolyu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@bingwang-ms
Copy link
Contributor Author

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bingwang-ms bingwang-ms merged commit b62526c into sonic-net:master May 28, 2022
liushilongbuaa pushed a commit to liushilongbuaa/sonic-buildimage that referenced this pull request Jun 20, 2022
…anch

Related work items: #52, #71, #73, #75, #77, sonic-net#1306, sonic-net#1588, sonic-net#1991, sonic-net#2031, sonic-net#2040, sonic-net#2053, sonic-net#2066, sonic-net#2069, sonic-net#2087, sonic-net#2107, sonic-net#2110, sonic-net#2112, sonic-net#2113, sonic-net#2117, sonic-net#2124, sonic-net#2125, sonic-net#2126, sonic-net#2128, sonic-net#2130, sonic-net#2131, sonic-net#2132, sonic-net#2133, sonic-net#2134, sonic-net#2135, sonic-net#2136, sonic-net#2137, sonic-net#2138, sonic-net#2139, sonic-net#2140, sonic-net#2143, sonic-net#2158, sonic-net#2161, sonic-net#2233, sonic-net#2243, sonic-net#2250, sonic-net#2254, sonic-net#2260, sonic-net#2261, sonic-net#2267, sonic-net#2278, sonic-net#2282, sonic-net#2285, sonic-net#2288, sonic-net#2289, sonic-net#2292, sonic-net#2294, sonic-net#8887, sonic-net#9279, sonic-net#9390, sonic-net#9511, sonic-net#9700, sonic-net#10025, sonic-net#10322, sonic-net#10479, sonic-net#10484, sonic-net#10493, sonic-net#10500, sonic-net#10580, sonic-net#10595, sonic-net#10628, sonic-net#10634, sonic-net#10635, sonic-net#10644, sonic-net#10670, sonic-net#10691, sonic-net#10716, sonic-net#10731, sonic-net#10750, sonic-net#10751, sonic-net#10752, sonic-net#10761, sonic-net#10769, sonic-net#10775, sonic-net#10776, sonic-net#10779, sonic-net#10786, sonic-net#10792, sonic-net#10793, sonic-net#10800, sonic-net#10806, sonic-net#10826, sonic-net#10839, sonic-net#10840, sonic-net#10842, sonic-net#10844, sonic-net#10847, sonic-net#10849, sonic-net#10852, sonic-net#10865, sonic-net#10872, sonic-net#10877, sonic-net#10886, sonic-net#10889, sonic-net#10903, sonic-net#10904, sonic-net#10905, sonic-net#10913, sonic-net#10914, sonic-net#10916, sonic-net#10919, sonic-net#10925, sonic-net#10926, sonic-net#10929, sonic-net#10933, sonic-net#10934, sonic-net#10937, sonic-net#10941, sonic-net#10947, sonic-net#10952, sonic-net#10953, sonic-net#10957, sonic-net#10959, sonic-net#10971, sonic-net#10972, sonic-net#10980
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.

6 participants