[cfgmgr]: Add vrfmgrd #621

marian-pritsak · 2018-09-16T14:36:31Z

vrfmgrd is responsible for VRF configuration in Linux
It creates VRF-Lite device for every VRF entry in Config DB

vrfmgrd is responsible for VRF configuration in Linux It creates VRF-Lite device for every VRF entry in Config DB Signed-off-by: Marian Pritsak <[email protected]>

marian-pritsak · 2018-09-16T18:35:57Z

Retest this please

lguohan · 2018-09-17T07:29:30Z

cfgmgr/vrfmgr.cpp

+#include "shellcmd.h"
+
+#define VRF_TABLE_START 100
+#define VRF_TABLE_END 164


what are these two numbers? does this limit to 64 vrfs?

Linux routing table numbers. Do we need more?

maybe can change to 1k initially.

Done
Used 1001..2000 since there are a few reserved values in the first 256 tables.

lguohan · 2018-09-17T07:57:53Z

cfgmgr/vrfmgr.cpp

+    {
+        return false;
+    }
+


can we check the vrfTableMap, if it is there means the vrf has been created, then we can just return true. Instead of creating a new one.

lguohan · 2018-09-17T08:01:04Z

cfgmgr/vrfmgr.h

+private:
+    bool delLink(const string& vrfName);
+    bool setLink(const string& vrfName);
+    void returnTable(uint32_t table);


can you rename to recycleTableId?

lguohan · 2018-09-17T08:02:21Z

cfgmgr/vrfmgr.cpp

+        return false;
+    }
+
+    returnTable(m_vrfTableMap[vrfName]);


i feel we should move this after line 69 since we should only recycle the id when it is free.

lguohan

i have some minor comments, but overall it looks good to me.

jipanyang · 2018-09-17T18:27:29Z

cfgmgr/vrfmgr.cpp

+    }
+
+    cmd << IP_CMD << " link add " << vrfName << " type vrf table " << table;
+    int ret = swss::exec(cmd.str(), res);


I'm think about warm restart support for vrfmgrd. If the vrf table has been configured on linux, adding it again would return error. Could it be changed to warm restart agnostic, like checking the existence of vrf table before adding it?

root@sonic:/home/admin# ip link add vrftest1 type vrf table 120 root@sonic:/home/admin# ip link add vrftest1 type vrf table 120 RTNETLINK answers: File exists root@sonic:/home/admin# echo $? 2

Not sure if I follow. Warm restart reboots kernel. VRF netdevs will be gone after reboot.

There is SWSS docker warm restart. In that case, Linux env is kept.

there is no problem to get all vrfs using following command.

admin@str-s6000-on-4:~$ ip -br link show type vrf vrftest1 DOWN c2:31:e9:01:c8:f4 <NOARP,MASTER>

the problem is to get the route table id.

do we need the table id? I can see the ip route add can use vrfname directly. @marian-pritsak , do you know who is going to use that table id?

from what I can see, it seems we can get form netlink messages.

https://github.com/FRRouting/frr/blob/master/zebra/if_netlink.c#L305

I do not know if there is a command line that can get this.

ip -d shows table ID

nikos-github · 2018-09-17T18:48:01Z

@marian-pritsak @lguohan Is there a plan on how sonic will handle IP addresses for interfaces enslaved to vrfs? Will they be handled the same way kernel does and also be checking the sysctl variables?

marian-pritsak · 2018-09-17T19:24:45Z

@nikos-github Here's the link to design draft

Signed-off-by: Marian Pritsak <[email protected]>

lguohan · 2018-09-18T15:47:43Z

@prsunny and @jipanyang , can you check the pr, any more comments?

jipanyang

Two minor comments.

jipanyang · 2018-09-18T18:37:33Z

cfgmgr/vrfmgr.cpp

+    int ret = swss::exec(cmd.str(), res);
+    if (ret)
+    {
+        SWSS_LOG_ERROR("Command %s failed, rc: %i", res.c_str(), ret);


Probably EXEC_WITH_ERROR_THROW(cmd, res) is better here.

jipanyang · 2018-09-18T18:50:21Z

cfgmgr/Makefile.am

@@ -30,3 +30,7 @@ buffermgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI)
 buffermgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI)
 buffermgrd_LDADD = -lswsscommon

+vrfmgrd_SOURCES = vrfmgrd.cpp vrfmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h


Since every daemon here is taking "$(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h", would it make sense to have a macro for them like

CFGMGR_COMMON_SOURCES = $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h

Maybe, but I don't like mixing that kind of refactoring with new features in a single PR

Signed-off-by: Marian Pritsak <[email protected]>

lguohan · 2018-09-19T16:09:48Z

retest this please

prsunny · 2018-09-19T16:11:17Z

Looks good. Based on the Vxlan discussion today, we've to update the STATE_DB/APP_DB after the VRF is created in kernel. But this can be done in the next commit.

…nic-net#621) [fast-reboot] Check if ASIC config has changed before fast reboot * Adds script to compare checksums for config files between SONiC images * Adds pre-check to fast reboot script to confirm config files match before performing fast reboot Signed-off-by: Danny Allen <[email protected]> * Fix formatting * Incorporate feedback * Add check for reboot type and override ability * Clean up error checking

…cements (sonic-net#621) * Fix debian/rules makefile: use shell commands instead of dollar replacements because: 1. `ifeq` evaluates conditionals when it reads a Makefile. 2. no need to replace with shell command outputs ref: https://stackoverflow.com/a/11994561/2514803 * Fix one more `echo` * Revert back one shell replacement * Fix some bash command and Makefile escaping issue

[cfgmgr]: Add vrfmgrd

b310a9c

vrfmgrd is responsible for VRF configuration in Linux It creates VRF-Lite device for every VRF entry in Config DB Signed-off-by: Marian Pritsak <[email protected]>

marian-pritsak requested review from lguohan and prsunny September 16, 2018 14:36

lguohan reviewed Sep 17, 2018

View reviewed changes

lguohan approved these changes Sep 17, 2018

View reviewed changes

jipanyang reviewed Sep 17, 2018

View reviewed changes

marian-pritsak added 3 commits September 17, 2018 22:43

Fix comments

d42830c

Signed-off-by: Marian Pritsak <[email protected]>

[vrfmgrd]: Recover VRF info from linux after restart

6a27177

Signed-off-by: Marian Pritsak <[email protected]>

[vrfmgrd]: Set Table ID range from 1001 to 2000

6eebd6f

Signed-off-by: Marian Pritsak <[email protected]>

marian-pritsak mentioned this pull request Sep 18, 2018

[docker-orchagent]: Add vrfmgrd to supervisorctl sonic-net/sonic-buildimage#2055

Merged

jipanyang reviewed Sep 18, 2018

View reviewed changes

Throw when cmd fails

710feed

Signed-off-by: Marian Pritsak <[email protected]>

jipanyang approved these changes Sep 19, 2018

View reviewed changes

prsunny approved these changes Sep 19, 2018

View reviewed changes

lguohan merged commit 13df5a9 into sonic-net:master Sep 19, 2018

stcheng added the Enhancement ➕ label Sep 20, 2018

[cfgmgr]: Add vrfmgrd #621

[cfgmgr]: Add vrfmgrd #621

Uh oh!

Conversation

marian-pritsak commented Sep 16, 2018

Uh oh!

marian-pritsak commented Sep 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikos-github commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marian-pritsak commented Sep 17, 2018

Uh oh!

lguohan commented Sep 18, 2018

Uh oh!

jipanyang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan commented Sep 19, 2018

Uh oh!

prsunny commented Sep 19, 2018

Uh oh!

Uh oh!

nikos-github commented Sep 17, 2018 •

edited

Loading