[fpmsyncd] Add VNET routes support#772
[fpmsyncd] Add VNET routes support#772lguohan merged 8 commits intosonic-net:masterfrom baiwei0427:master
Conversation
prsunny
left a comment
prsunny
left a comment
There was a problem hiding this comment.
Please find my initial review comments.
fpmsyncd/routesync.cpp
Outdated
| using namespace std; | ||
| using namespace swss; | ||
|
|
||
| #define APP_VNET_ROUTE_TABLE_NAME "VNET_ROUTE_TABLE" |
There was a problem hiding this comment.
These are defined in schema.h. Please use those. It is already included here as part of "producerstatetable.h" [APP_VNET_RT_TABLE_NAME, APP_VNET_RT_TUNNEL_TABLE_NAME]
fpmsyncd/routesync.cpp
Outdated
| unsigned int table_index = rtnl_route_get_table(route_obj); | ||
|
|
||
| /* Default routing table. This line may have problems. */ | ||
| if (table_index == RT_TABLE_UNSPEC) { |
fpmsyncd/routesync.cpp
Outdated
| onRouteMsg(nlmsg_type, obj); | ||
|
|
||
| /* VNET route */ | ||
| } else { |
fpmsyncd/routesync.cpp
Outdated
| vector<FieldValueTuple> fvVector; | ||
| FieldValueTuple nh("nexthop", nexthops); | ||
| FieldValueTuple idx("ifname", ifnames); | ||
| FieldValueTuple nh("nexthop", gw_ips); |
There was a problem hiding this comment.
Why don't we use the same names which is almost same as new? Less diffs to review!
| char vrf_name[IFNAMSIZ] = {0}; | ||
|
|
||
| /* If we cannot get the VRF name */ | ||
| if (!getIfName(vrf_index, vrf_name, IFNAMSIZ)) |
There was a problem hiding this comment.
Not sure if we want to proceed if vrf_name is unknown. Orchagent checks for the VRF state and if not found, it just keeps retrying. I would think this is an exceptional case.
There was a problem hiding this comment.
@prsunny Do you mean the function should immediately return if it cannot get the VRF name?
There was a problem hiding this comment.
Yes, my point is, we got a route update on a VRF but the VRF device is not found/deleted. In this case, I would suggest to log error and return.
| unsigned int table_index = rtnl_route_get_table(route_obj); | ||
|
|
||
| /* Default routing table. This line may have problems. */ | ||
| if (table_index == RT_TABLE_UNSPEC) |
There was a problem hiding this comment.
This could be unsafe to classify every other routes to be VNET. As we discussed, it would be safe if we can use the id range check for table index
There was a problem hiding this comment.
Will address this in the future
|
retest this please |
baiwei0427
changed the title
|
retest this please |
|
retest this please |
…ts (sonic-net#772) * [fwutil]: initial version. Signed-off-by: Nazarii Hnydyn <nazariig@mellanox.com> * [fwutil]: Fix UI: enable progressbar render finalizer. Signed-off-by: Nazarii Hnydyn <nazariig@mellanox.com> * [fwutil]: integrate utility with SONiC CLI. Signed-off-by: Nazarii Hnydyn <nazariig@mellanox.com> * [fwutil]: update CLI command reference documentation. Signed-off-by: Nazarii Hnydyn <nazariig@mellanox.com> * [fwutil]: Revisit CLI architecture: avoid direct imports. Signed-off-by: Nazarii Hnydyn <nazariig@mellanox.com> * [fwutil]: Fix review comments: refactor CLI command reference. Signed-off-by: Nazarii Hnydyn <nazariig@mellanox.com> * [fwutil]: Fix review comments: update CLI documentation. Signed-off-by: Nazarii Hnydyn <nazariig@mellanox.com>
* Add VNET support to fpmsyncd
* Use marcos defined in SONiC SWSS common
* Use { for new line
* Use old variable names
* Return if we cannot get VRF name
* Use switch statement to handle route types
* Add comment for future VRF routes support
3 participants
fpmsyncd: Handle VNET routes
Pending items:
Signed-off-by: Wei Bai baiwei0427@gmail.com