Skip to content

[config]Static routes to config_db#1534

Merged
prsunny merged 19 commits intosonic-net:masterfrom
d-dashkov:sroute_to_configdb
May 13, 2021
Merged

[config]Static routes to config_db#1534
prsunny merged 19 commits intosonic-net:masterfrom
d-dashkov:sroute_to_configdb

Conversation

@d-dashkov
Copy link
Copy Markdown
Contributor

@d-dashkov d-dashkov commented Mar 30, 2021

What I did
fixes sonic-net/sonic-buildimage#6997
Added process for writing static routes to config_db.

fixes sonic-net/sonic-buildimage#7261
Added interface validation before creating route.

How I did it
Refactored add/del route functions. Wrote generic function that returns a vtysh command and an entry for config_db with name.

sudo config route add prefix 1.2.3.4/32 nexthop 30.0.0.5
sudo config route add prefix 1.3.3.4/32 nexthop 30.0.0.5
sudo config route add prefix 1.4.3.4/32 nexthop 30.0.0.5
sudo config route add prefix vrf Vrf-RED 1.5.3.4/32 nexthop 30.0.0.5
sudo config save -y
sudo reboot

Instead of running the vtysh command, it creates config_db entries that will be loaded by the ffr daemon:

    "STATIC_ROUTE": {
        "1.2.3.4/32": {
            "nexthop": "30.0.0.5"
        },
        "1.3.3.4/32": {
            "nexthop": "30.0.0.5"
        },
        "1.4.3.4/32": {
            "nexthop": "30.0.0.5"
        },
        "Vrf-RED|1.5.3.4/32": {
            "nexthop": "30.0.0.5"
        }
    }

Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
@prsunny
Copy link
Copy Markdown
Contributor

prsunny commented Mar 30, 2021

Here, the user is expected to run route update after every reboot?

@d-dashkov
Copy link
Copy Markdown
Contributor Author

@prsunny
I don't think so, user shouldn't run this command manually after reboot. But I really don't know where this update function can be called, because it should be executed after the RIFs are generated, and running it in the startup script shouldn't work.

@prsunny
Copy link
Copy Markdown
Contributor

prsunny commented Mar 30, 2021

@prsunny
I don't think so, user shouldn't run this command manually after reboot. But I really don't know where this update function can be called, because it should be executed after the RIFs are generated, and running it in the startup script shouldn't work.

ok, so how is it expected to be restored from config_db after reboot?

@d-dashkov
Copy link
Copy Markdown
Contributor Author

@prsunny
I don't think so, user shouldn't run this command manually after reboot. But I really don't know where this update function can be called, because it should be executed after the RIFs are generated, and running it in the startup script shouldn't work.

ok, so how is it expected to be restored from config_db after reboot?

I'm not sure, but maybe the update can be triggered using crontab or along with running the bgp container.

@prsunny prsunny requested a review from shi-su March 30, 2021 23:08
@prsunny
Copy link
Copy Markdown
Contributor

prsunny commented Mar 30, 2021

Can you add unit-tests as well?

@prsunny
Copy link
Copy Markdown
Contributor

prsunny commented Mar 30, 2021

Why is the schema showing nexthop in key?

       "1.2.3.4/32|30.0.0.5": {
            "nexthop": "30.0.0.5",
            "prefix": "1.2.3.4/32"
        },

@caizhenghui-juniper
Copy link
Copy Markdown

And also why do we need prefix in the value field? It's already part of the key...
For static route table the scheme is prefix as key and nexthop as field as the simplest case..

@caizhenghui-juniper
Copy link
Copy Markdown

we already have frrcfgd monitoring config db, so the part of programming frr cfg is not necessary as long as config_db is updated, isn't it?

Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
@d-dashkov
Copy link
Copy Markdown
Contributor Author

@prsunny @caizhenghui-juniper
I updated the record name with a prefix as the key and nexthop as the value, and also added base UT. But I cannot find frrcfgd in the repositories, can you tell me where it is?

@caizhenghui-juniper
Copy link
Copy Markdown

caizhenghui-juniper commented Mar 31, 2021 via email

@caizhenghui-juniper
Copy link
Copy Markdown

and can you put support for ECMP too? With more than 1 next hops for the static routes?

Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
["prefix", "3.2.3.4/32", "nexthop", "vrf", "Vrfred", "30.0.0.6"], obj=obj)
print(result.exit_code, result.output)
assert ('3.2.3.4/32') in db.cfgdb.get_table('STATIC_ROUTE')
assert db.cfgdb.get_entry('STATIC_ROUTE', '3.2.3.4/32') == {"nexthop": "30.0.0.6", "nexthop_vrf": "Vrfred"}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we need to follow a certain schema design for the STATIC_ROUTE table. In sonic-net/SONiC#585, there is a schema for the table but it seems different here.

Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
@d-dashkov
Copy link
Copy Markdown
Contributor Author

d-dashkov commented Apr 6, 2021

I'm not sure if this is a bug or not, but after reboot, the frr daemon does not start with the system, and also after manual start, the daemon sees a static route, but does not add it to frr cfg

Before reboot

sudo config route add prefix 1.2.3.4/32 nexthop 30.0.0.5
"STATIC_ROUTE": {
	"1.2.3.4/32": {
		"nexthop": "30.0.0.5"
	}
}


admin@sonic:~$ redis-cli -n 4 keys "*" | grep STATIC_ROUTE
STATIC_ROUTE|1.2.3.4/32
admin@sonic:~$ redis-cli -n 4 hgetall "STATIC_ROUTE|1.2.3.4/32"
1) "nexthop"
2) "30.0.0.5"
admin@sonic:~$ show ip route
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 192.168.111.3, eth0, weight 1, 00:05:24
S>  1.2.3.4/32 [1/0] via 30.0.0.7 (recursive), weight 1, 00:00:06
C>* 10.1.0.1/32 is directly connected, Loopback0, 00:05:07
C>* 192.168.111.0/24 is directly connected, eth0, 00:05:25

After reboot

Syslog

May  7 21:22:18.776046 sonic DEBUG bgp#/frrcfgd: entering BGP configuration daemon
May  7 21:22:18.780913 sonic DEBUG bgp#/frrcfgd: send initial enable command to bgpd
May  7 21:22:18.784148 sonic DEBUG bgp#/frrcfgd: send initial enable command to zebra
May  7 21:22:18.784148 sonic DEBUG bgp#/frrcfgd: send initial enable command to staticd
May  7 21:22:18.784148 sonic DEBUG bgp#/frrcfgd: send initial enable command to bfdd
May  7 21:22:18.784148 sonic DEBUG bgp#/frrcfgd: send initial enable command to ospfd
May  7 21:22:18.784148 sonic DEBUG bgp#/frrcfgd: send initial enable command to pimd
May  7 21:22:18.810021 sonic DEBUG bgp#/frrcfgd: entering VTYSH proxy thread
May  7 21:22:18.810021 sonic DEBUG bgp#/frrcfgd: waiting for client connection ...
May  7 21:22:18.899398 sonic DEBUG bgp#/frrcfgd: Init Cached DB data
May  7 21:22:19.926529 sonic DEBUG bgp#/frrcfgd:   STATIC_ROUTE&&1.2.3.4/32 : {'nexthop': '30.0.0.5'}
admin@sonic:~$ show ip route
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 192.168.111.3, eth0, weight 1, 00:05:24
C>* 10.1.0.1/32 is directly connected, Loopback0, 00:05:07
C>* 192.168.111.0/24 is directly connected, eth0, 00:05:25

And if I try to change the nexthop on this entry, staticd crashes because it has no previous entry

sudo config route add prefix 1.2.3.4/32 nexthop 30.0.0.5
May  7 21:09:27.483443 sonic DEBUG bgp#/frrcfgd: ----------------------------------
May  7 21:09:27.483607 sonic DEBUG bgp#/frrcfgd:  BGP table handling
May  7 21:09:27.483690 sonic DEBUG bgp#/frrcfgd: ----------------------------------
May  7 21:09:27.483764 sonic DEBUG bgp#/frrcfgd: table : STATIC_ROUTE
May  7 21:09:27.483881 sonic DEBUG bgp#/frrcfgd: key   : 1.2.3.4/32
May  7 21:09:27.483958 sonic DEBUG bgp#/frrcfgd: op    : SET
May  7 21:09:27.484074 sonic DEBUG bgp#/frrcfgd: data  :
May  7 21:09:27.484152 sonic DEBUG bgp#/frrcfgd:         nexthop    - 30.0.0.7
May  7 21:09:27.484225 sonic DEBUG bgp#/frrcfgd:
May  7 21:09:27.484326 sonic INFO bgp#/frrcfgd: value for table STATIC_ROUTE prefix default key 1.2.3.4/32 changed to {'nexthop': (30.0.0.7, UPDATE)}
May  7 21:09:27.484439 sonic INFO bgp#/frrcfgd: Set static IP route for vrf default prefix 1.2.3.4/32
May  7 21:09:27.484525 sonic DEBUG bgp#/frrcfgd: execute command vtysh -c 'configure terminal' -c 'vrf default' -c 'no ip route 1.2.3.4/32  30.0.0.5     ' for table STATIC_ROUTE.
May  7 21:09:27.484646 sonic DEBUG bgp#/frrcfgd: VTYSH CMD: configure terminal daemons: ['staticd']
May  7 21:09:27.484725 sonic DEBUG bgp#/frrcfgd: VTYSH CMD: vrf default daemons: ['staticd']
May  7 21:09:27.485096 sonic DEBUG bgp#/frrcfgd: VTYSH CMD: no ip route 1.2.3.4/32  30.0.0.5 daemons: ['staticd']
May  7 21:09:27.485581 sonic DEBUG bgp#/frrcfgd: [staticd] command return code: 1
May  7 21:09:27.485777 sonic DEBUG bgp#/frrcfgd:
May  7 21:09:27.485883 sonic DEBUG bgp#/frrcfgd: VTYSH CMD: end daemons: ['staticd']
May  7 21:09:27.485995 sonic ERR bgp#/frrcfgd: command execution failure. Command: "vtysh -c 'configure terminal' -c 'vrf default' -c 'no ip route 1.2.3.4/32  30.0.0.5     '"
May  7 21:09:27.486073 sonic ERR bgp#/frrcfgd: failed running FRR command: no ip route 1.2.3.4/32  30.0.0.5
May  7 21:09:27.486161 sonic ERR bgp#/frrcfgd: failed running static route config command
May  7 21:09:27.486235 sonic DEBUG bgp#/frrcfgd: ignore cache update for nexthop because of STAT_FAIL
May  7 21:09:27.486308 sonic INFO bgp#/frrcfgd: delete table row STATIC_ROUTE&&default|1.2.3.4/32 from cache

@d-dashkov d-dashkov requested a review from shi-su April 6, 2021 19:24
Copy link
Copy Markdown
Contributor

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

I see that this PR is missing ecmp handling. In the existing code, by calling vtysh, it does handle ECMP. My suggestion is, when user configure the same route prefix with a different nexthop, it should modify the STATIC_ROUTE DB entry instead of overwriting thus supporting ECMP (similar to vtysh). @shi-su, this should be handled by bgpcfgd.

runner = CliRunner()
obj = {'config_db':db.cfgdb}

# config route add prefix 2.2.3.4/32 nexthop 30.0.0.6
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please update the comment to include vrf

runner = CliRunner()
obj = {'config_db':db.cfgdb}

# config route add prefix 3.2.3.4/32 nexthop 30.0.0.6
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here, please update the correct command with vrf

config/main.py Outdated
clicommon.run_command(cmd)
key, route = cli_sroute_to_config(ctx, command_str)
config_db = ctx.obj['config_db']
config_db.set_entry("STATIC_ROUTE", key, route)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If user update the route, this would overwrite the entry, correct?

@prsunny
Copy link
Copy Markdown
Contributor

prsunny commented Apr 6, 2021

Please update CLI command reference document

@shi-su
Copy link
Copy Markdown
Contributor

shi-su commented Apr 6, 2021

I see that this PR is missing ecmp handling. In the existing code, by calling vtysh, it does handle ECMP. My suggestion is, when user configure the same route prefix with a different nexthop, it should modify the STATIC_ROUTE DB entry instead of overwriting thus supporting ECMP (similar to vtysh). @shi-su, this should be handled by bgpcfgd.

Yes, the STATIC_ROUTE DB entry needs to be updated when nexthop changes. The bgpcfgd should be able to know the changes and only apply the differences for the same route prefix.

Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
if vrf_name == "":
cmd += ' {} nexthop-vrf {}'.format(ip, vrf_name_dst)
config_db = ctx.obj['config_db']
key, route = cli_sroute_to_config(ctx, command_str)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now that we are writing to DB, lets check the correctness of key (like a valid ip prefix)

if 'ifname' in current_entry:
ifname = current_entry['ifname'].split(',')

nh_zip = list(itertools.zip_longest(nh, nh_vrf, ifname, fillvalue=''))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you add some comments to understand the logic in this section?

ctx.fail('Not found {} in {}'.format(cmd_tuple, key))

if len(nh) == 0 or (len(nh) == 1 and nh[0] == ''):
config_db.set_entry("STATIC_ROUTE", key, None)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if user just deletes the route without mentioning any nexthops, then the route is expected to be fully deleted.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If the user tries to delete a route without nexthop (for example, 'сonfig del config route del prefix 1.2.3.4/32'), he will receive an error: 'argument is not in pattern'. So if the user has previously added ECMP routes, he must remove them one by one.
This condition checks if the record still has any nexthop, if so, it just updates this record, if it doesn't have the nexthop, it removes the key. Is this the correct behavior or should user be able to delete the entire entry at once?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO, user should be able to delete the entire route at once.

print(result.exit_code, result.output)
assert not ('3.2.3.4/32') in db.cfgdb.get_table('STATIC_ROUTE')

def test_several_nexthops_static_route(self):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please rename it to test_multiple_nexthops_static_route

print(result.exit_code, result.output)
assert not ('11.2.3.4/32') in db.cfgdb.get_table('STATIC_ROUTE')

def test_static_route_ECMP_nextfop_vrf(self):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nexthop - typo

assert ('6.2.3.4/32') in db.cfgdb.get_table('STATIC_ROUTE')
assert db.cfgdb.get_entry('STATIC_ROUTE', '6.2.3.4/32') == {"nexthop": "30.0.0.6,30.0.0.7"}

# config route del prefix 6.2.3.4/32 nexthop 30.0.0.7
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please have a test to del prefix without any nexthops and ensure its removed from DB.

key, route = cli_sroute_to_config(ctx, command_str)

# If defined intf name, check if it exists
if 'ifname' in route:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As commented by @arlakshm for PR #1535 , please check if ifname belongs to VLAN_INTERFACE, INTERFACE and PORTCHANNEL_INTERFACE. If not return failure as "IP interface doesn't exist"

@prsunny prsunny mentioned this pull request Apr 23, 2021
Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
config_db = ConfigDBConnector()
config_db.connect()
ctx.obj = {}
ctx.obj['config_db'] = config_db
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you add the support for multi asic ?
Similar to how it is done for interface command https://github.com/Azure/sonic-utilities/blob/08337aa7637b290bb8407c38b2a5dbe3e8383b3e/config/main.py#L2359

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@arlakshm, can we add multiasic as another enhancement PR?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@prsunny, Sure. Can we create an issue or task to track this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Created issue - #1608

Copy link
Copy Markdown
Contributor

@shi-su shi-su left a comment

Choose a reason for hiding this comment

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

I do not have any other comments. Please check with other reviewers.

else:
route['distance'] = '0'

if 'blackhole' in route:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We would need to handle one scenario for blackhole routes. If the user configure with "ifname" as "null", could you please add the attribute 'blackhole' as true.

andyivanov
andyivanov previously approved these changes May 5, 2021
Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
else:
route['blackhole'] = 'false'
# If the user configure with "ifname" as "null", set 'blackhole' attribute as true.
if 'ifname' in route and route['ifname'] == 'null':
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just want to confirm. Does this mean that it is necessary to confirm the route is completely removed before setting it to blackhole? If a route already exists in Config_DB and then we configure it to blackhole, the command would simply get ignored? If this is the case, could you please add logic to notify the caller that the command is getting ignored (e.g., fail the command with an error message)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it will be ignored if some entries already exist in the db. @prsunny can this suggestion will be add in #1608?

@dprital
Copy link
Copy Markdown
Collaborator

dprital commented May 30, 2021

@prsunny - Can you please add the tag "Required to 202012" ?

qiluo-msft pushed a commit that referenced this pull request Jun 3, 2021
* Write static routes to config_db
* Update configuration with "ifname" as "null"

Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
gitsabari pushed a commit to gitsabari/sonic-utilities that referenced this pull request Jun 15, 2021
* Write static routes to config_db
* Update configuration with "ifname" as "null"

Signed-off-by: d-dashkov <Dmytro_Dashkov@Jabil.com>
@haslersn
Copy link
Copy Markdown

haslersn commented Jul 3, 2021

Is this already available in the prebuilt sonic-aboot-broadcom.swi ? If not, then how to configure (and persist across reboots) static routes?

stepanblyschak pushed a commit to stepanblyschak/sonic-utilities that referenced this pull request Apr 28, 2022
7f50b9815e14d90c02d9dce63fd08d90e25cee3f (HEAD -> 201911, origin/201911) handled update() function of fdb orchagent for FDB FLUSH event (sonic-net#1534)
17adc13b6ca21846fe27c94d6a16f9909c712d77 Add a check for warm-restart, and do a clear only when warm-restart is enable. (sonic-net#1498)
d097260a5aa7bd611babd5062e220056374e23d8 Fixed compilation failure with debug option (sonic-net#1518)

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

10 participants