[bgpcfgd] Add bgpcfgd support for static routes #7233

shi-su · 2021-04-05T21:04:15Z

Why I did it

Add bgpcfgd support for static routes.

How I did it

Add bgpcfgd support to subscribe changes in STATIC_ROUTE table in CONFIG_DB and program via vtysh. The key of STATIC_ROUTE table is formatted as STATIC_ROUTE|vrf|ip_prefix, while the vrf is optional. If would be treated the same as "default" if no vrf is given.
Add unit tests.

How to verify it

Confirm that changes in STATIC_ROUTE table in CONFIG_DB are applied. CONFIG_DB example:

"STATIC_ROUTE": {
    "10.1.1.0/24": {
        "nexthop": "10.0.0.57,10.0.0.59"
    },
    "default|10.1.2.0/24": {
        "nexthop": "10.0.0.57,10.0.0.59", 
        "ifname": "PortChannel0001,PortChannel0002", 
        "distance": "10,10", 
        "blackhole": "false,false"
    },
    "default|10.1.3.0/24": {
        "blackhole": "true"
    },
    "default|fc00:10::/64": {
        "nexthop": "fc00::72", 
        "ifname": "PortChannel0001", 
        "distance": "10", 
        "nexthop-vrf": "default", 
        "blackhole": "false"
    }
}

Static routes:

admin@vlab-01:~$ show ip route static
Codes: K - kernel route, C - connected, S - static, R - RIP,
       O - OSPF, I - IS-IS, B - BGP, E - EIGRP, N - NHRP,
       T - Table, v - VNC, V - VNC-Direct, A - Babel, D - SHARP,
       F - PBR, f - OpenFabric,
       > - selected route, * - FIB route, q - queued, r - rejected, b - backup

S>* 0.0.0.0/0 [200/0] via 10.250.0.1, eth0, weight 1, 00:00:34
S>* 10.1.1.0/24 [1/0] via 10.0.0.57, PortChannel0001, weight 1, 00:00:20
  *                   via 10.0.0.59, PortChannel0002, weight 1, 00:00:20
S>* 10.1.2.0/24 [10/0] via 10.0.0.57, PortChannel0001, weight 1, 00:00:20
  *                    via 10.0.0.59, PortChannel0002, weight 1, 00:00:20
S>* 10.1.3.0/24 [1/0] unreachable (blackhole), weight 1, 00:00:30

admin@vlab-01:~$ show ipv6 route static
Codes: K - kernel route, C - connected, S - static, R - RIPng,
       O - OSPFv3, I - IS-IS, B - BGP, N - NHRP, T - Table,
       v - VNC, V - VNC-Direct, A - Babel, D - SHARP, F - PBR,
       f - OpenFabric,
       > - selected route, * - FIB route, q - queued, r - rejected, b - backup

S>* fc00:1::/64 [1/0] is directly connected, Loopback0, weight 1, 00:01:09
S>* fc00:10::/64 [10/0] via fc00::72, PortChannel0001, weight 1, 00:01:05

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

201811
201911
202006
202012

Description for the changelog

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

venkatmahalingam · 2021-04-06T00:15:00Z

src/sonic-bgpcfgd/tests/test_static_route.py

+        }),
+        True,
+        [
+            "ip route 10.1.2.0/24 blackhole 10"


Does this feature consider mgmt route as well? e.g mgmt route in default & mgmt VRFs.

The scope of this PR is to cover STATIC_ROUTE table in config_db. Routes that are reflected in this table are considered, otherwise not.

I see all test cases are with PortChannel0001/PortChannel0002, can we have some tests with Etherent and Vlan interfaces as well?

Added a test case with Ethernet interfaces. Since bgpcfgd is basically a translator from config_db to the commands. The interface type should not make any difference from the unit test perspective.

I understand, it is better to test other interfaces as well.

venkatmahalingam · 2021-04-06T00:18:39Z

src/sonic-bgpcfgd/bgpcfgd/manager_static_route.py

+from .manager import Manager
+from .template import TemplateFabric
+import socket
+


Did we test save & restore for STATIC_ROUTE table?

Do you mean saving and restoring items in STATIC_ROUTE table in config_db? There should be a config command to apply changes to config_db. I think the test for that is not going to be covered by this PR. There would be unit tests for the config command. And we are planning to have sonic-mgmt tests to test the static route feature as a whole.

venkatmahalingam · 2021-04-06T00:19:25Z

src/sonic-bgpcfgd/bgpcfgd/manager_static_route.py

+        cmd_list = self.static_route_commands(ip_nh_set, cur_nh_set, ip_prefix, vrf)
+
+        if cmd_list:
+            self.cfg_mgr.push_list(cmd_list)


Can we add test cases with other L3 interfaces as well e.g PHY/VLAN..etc.?

I am not sure if I fully understood your suggestion. Could you please maybe give an example of what additional test case you are suggesting?

Looks like you have added the test cases with Ethernet already, thanks.

lguohan · 2021-04-06T04:05:12Z

@prsunny

prsunny

Please find comments. Also, "default" is optional in the STATIC_ROUTE schema. Would suggest modify your description so as to avoid any confusion.

prsunny · 2021-04-06T16:40:12Z

src/sonic-bgpcfgd/bgpcfgd/manager_static_route.py

@@ -0,0 +1,173 @@
+import traceback


Please rename this file to managers_static_rt.py to be consistent with existing files

Renamed the file name to managers_static_rt.py.

prsunny · 2021-04-06T16:52:53Z

src/sonic-bgpcfgd/tests/test_static_route.py

+        "SET",
+        ("default|10.1.1.0/24", {
+            "nexthop": "10.0.0.57, 10.0.0.59",
+            "ifname": "PortChannel0001",


ifname is optional, so is nexthop.. so if either one is provided, you should consider it valid. But if user specifies both, then there should be empty string to specify positional arguments. Could you please add few cases for this as well?

Yes, both ifname and nexthop are optional. In the case where both ifname and nexthop are given, they should have the number of items to avoid confusion. An empty string separated by a comma is needed if only one item has no value given, for example, "nexthop": "10.0.0.57,10.0.0.59", "ifname": "PortChannel0001," is valid, "nexthop": "10.0.0.57, 10.0.0.59", "ifname": "PortChannel0001" is not. I added more test cases to better cover these scenarios.

venkatmahalingam · 2021-04-06T23:28:46Z

src/sonic-bgpcfgd/bgpcfgd/main.py

@@ -53,6 +54,8 @@ def do_work():
        BGPAllowListMgr(common_objs, "CONFIG_DB", "BGP_ALLOWED_PREFIXES"),
        # BBR Manager
        BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR"),
+        # Static Route Managers
+        StaticRouteMgr(common_objs, "CONFIG_DB", "STATIC_ROUTE"),
    ]
    runner = Runner(common_objs['cfg_mgr'])
    for mgr in managers:


I dont see any code in "RouteOrch::addRoute" to trigger the proactive nbr resolution when nexthop is not resolved, is this already planned? Did we test fast path routing with static route config in the HW?

Resolving nexthop in RouteOrch is our next step for the static route tasks.

prsunny · 2021-04-06T23:34:54Z

src/sonic-bgpcfgd/tests/test_static_rt.py

+        mgr,
+        "SET",
+        ("vrfRED|10.1.3.0/24", {
+            "nexthop": "10.0.0.57,10.0.0.59,10.0.0.61,10.0.0.63",


please also have a test to remove one nexthop

Added the scenario of removing part of the nexthops.

prsunny · 2021-04-07T16:21:09Z

@venkatmahalingam , could you please re-review/sign-off?

Why I did it Add bgpcfgd support for static routes. How I did it Add bgpcfgd support to subscribe changes in STATIC_ROUTE table in CONFIG_DB and program via vtysh. The key of STATIC_ROUTE table is formatted as STATIC_ROUTE|vrf|ip_prefix, while the vrf is optional. If would be treated the same as "default" if no vrf is given. Add unit tests.

Add bgpcfgd support for static routes

b677eff

shi-su requested a review from lguohan as a code owner April 5, 2021 21:04

shi-su requested a review from prsunny April 5, 2021 21:04

venkatmahalingam reviewed Apr 6, 2021

View reviewed changes

prsunny requested changes Apr 6, 2021

View reviewed changes

shi-su added 3 commits April 6, 2021 19:28

Address review comments and fix for dst_ip is empty

c31b363

remove a space

ce12e83

Add test case for adding a nexthop

2640144

venkatmahalingam reviewed Apr 6, 2021

View reviewed changes

prsunny reviewed Apr 6, 2021

View reviewed changes

add test case for del nexthop and ethernet interfaces

f8e1a1e

shi-su requested a review from prsunny April 7, 2021 16:10

prsunny approved these changes Apr 7, 2021

View reviewed changes

venkatmahalingam approved these changes Apr 8, 2021

View reviewed changes

shi-su merged commit 42059ac into sonic-net:master Apr 8, 2021

shi-su deleted the static_route branch April 8, 2021 20:52

shi-su added the Request for 202012 Branch label May 13, 2021

qiluo-msft added the Included in 202012 Branch label May 24, 2021

vivekrnv mentioned this pull request Jun 30, 2021

[bgpcfgd] Couldn't configure a Static Route without BGP_ASN field #8027

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bgpcfgd] Add bgpcfgd support for static routes #7233

[bgpcfgd] Add bgpcfgd support for static routes #7233

shi-su commented Apr 5, 2021 •

edited

Loading

venkatmahalingam Apr 6, 2021

shi-su Apr 6, 2021

venkatmahalingam Apr 6, 2021 •

edited

Loading

shi-su Apr 7, 2021

venkatmahalingam Apr 8, 2021

venkatmahalingam Apr 6, 2021

shi-su Apr 6, 2021

venkatmahalingam Apr 8, 2021

venkatmahalingam Apr 6, 2021 •

edited

Loading

shi-su Apr 6, 2021

venkatmahalingam Apr 8, 2021

lguohan commented Apr 6, 2021

prsunny left a comment

prsunny Apr 6, 2021

shi-su Apr 6, 2021

prsunny Apr 6, 2021

shi-su Apr 6, 2021

venkatmahalingam Apr 6, 2021

shi-su Apr 7, 2021

venkatmahalingam Apr 8, 2021

prsunny Apr 6, 2021

shi-su Apr 7, 2021

prsunny commented Apr 7, 2021

[bgpcfgd] Add bgpcfgd support for static routes #7233

[bgpcfgd] Add bgpcfgd support for static routes #7233

Conversation

shi-su commented Apr 5, 2021 • edited Loading

Why I did it

How I did it

How to verify it

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

Description for the changelog

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venkatmahalingam Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venkatmahalingam Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lguohan commented Apr 6, 2021

prsunny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny commented Apr 7, 2021

shi-su commented Apr 5, 2021 •

edited

Loading

venkatmahalingam Apr 6, 2021 •

edited

Loading

venkatmahalingam Apr 6, 2021 •

edited

Loading