[intfmgrd]: Merge intfsyncd into intfmgrd #635

marian-pritsak · 2018-10-02T12:33:27Z

Move intfsyncd functionality to intfmgrd, add VRF membership support.

Signed-off-by: Marian Pritsak marianp@mellanox.com

Move intfsyncd functionality to intfmgrd, add VRF membership support. Signed-off-by: Marian Pritsak <marianp@mellanox.com>

prsunny · 2018-10-02T21:05:31Z

cfgmgr/intfmgr.cpp


-        if (keys.size() != 2)
+        if (!isIntfStateOk(vrf_name))


vrf_name is optional and can be empty here. You would want to check if it is empty. User may just want to configure "mtu" for the interface.

prsunny · 2018-10-02T21:06:25Z

cfgmgr/intfmgr.cpp

+    if (op == SET_COMMAND)
+    {
+        /*
+         * Don't proceed if port/LAG/VLAN is not ready yet.


Same comment is repeated down. I think you can remove this.

prsunny · 2018-10-02T21:10:23Z

cfgmgr/intfmgr.cpp

        }

-        string alias(keys[0]);
-        IpPrefix ip_prefix(keys[1]);
+        setIntfIp(alias, "add", ip_prefix.to_string(), ip_prefix.isV4());


We have to check for return value here. Previously it was via Netlink and hence ensured it was configured in kernel. Now, we have to explicitly check if the address is successfully configured in kernel

Using EXEC_WITH_ERROR_THROW

nikos-github · 2018-10-02T21:45:03Z

cfgmgr/intfmgr.cpp

@@ -43,6 +45,23 @@ bool IntfMgr::setIntfIp(const string &alias, const string &opCmd,
    return (ret == 0);
 }

+bool IntfMgr::setIntfVrf(const string &alias, const string vrfName)


The return value shouldn't be a boolean. For proper error checking, you should return the return value which can be 0, 1 or 2 I believe.

Changed function to use EXEC_WITH_ERROR_THROW

nikos-github · 2018-10-02T21:45:28Z

cfgmgr/intfmgr.cpp

+            return false;
+        }
+
+        setIntfVrf(alias, vrf_name);


Check the return values please and call m_appIntfTableProducer only on 0. Print a syslog error for 1 and 2 along with the value.

Changed function to use EXEC_WITH_ERROR_THROW

nikos-github · 2018-10-02T21:45:35Z

cfgmgr/intfmgr.cpp

+    }
+    else if (op == DEL_COMMAND)
+    {
+        setIntfVrf(alias, "");


Same comment about return values.

Changed function to use EXEC_WITH_ERROR_THROW

nikos-github · 2018-10-02T22:11:09Z

cfgmgr/intfmgr.cpp

        }

-        string alias(keys[0]);
-        IpPrefix ip_prefix(keys[1]);
+        setIntfIp(alias, "add", ip_prefix.to_string(), ip_prefix.isV4());


nikos-github · 2018-10-02T22:11:15Z

cfgmgr/intfmgr.cpp

+    }
+    else if (op == DEL_COMMAND)
+    {
+        setIntfIp(alias, "del", ip_prefix.to_string(), ip_prefix.isV4());


Changed function to use EXEC_WITH_ERROR_THROW

nikos-github · 2018-10-02T22:12:34Z

cfgmgr/intfmgr.cpp

        string op = kfvOp(t);
-        if (op == SET_COMMAND)
+
+        if (keys.size() == 1)


What does it mean for the key size to be 1? Shouldn't use literals in the code like that. Please define somewhere and explain.

It is not some kind enum to define. It means exactly if there are two keys or one

My question was to illustrate the need to not use literals in the code. I know what it means. You can still #define somewhere common or even use an enum.

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

prsunny · 2018-10-03T17:49:31Z

@marian-pritsak , @lguohan, looking at the VS test failures, there are test cases that do "ifconfig <ip_address>" configurations and rely on the Netlink msgs (intfsyncd) to eventually program the ASIC_DB. The failed tests are route, nexthop, neighbor, port_state and crm (route/ip related) to name a few. We would need to discuss how this can be resolved. Need to fix the VS test cases

lguohan · 2018-10-04T22:04:48Z

cfgmgr/intfmgr.cpp

@@ -13,6 +13,7 @@ using namespace swss;

 #define VLAN_PREFIX         "Vlan"
 #define LAG_PREFIX          "PortChannel"
+#define VNET_PREFIX         "Vnet"



can you add vs test for this refactoring.

I am think you need to first create

INTERFACE|eth0 with vrf, then check if the interface has been moved to vrf or not.

INTERFACE|eth0|ipaddress, then check if the interface has been assigned with IP address or not.

add another ip address, and check if the interface has been assigned with additional IP.

then, remove the one ip address, and check.

remove another ip address, and check.

Then, check vrf name and check if the interface is moved to another vrf or not.

lguohan

need to fix the vs test case and add a few more since you have changed the INTERFACE schema.

nikos-github · 2018-10-05T00:06:11Z

cfgmgr/intfmgr.cpp

+
+    if (!vrfName.empty())
+    {
+        cmd << IP_CMD << " link set " << alias << " master " << vrfName;


When the interface is enslaved in a vrf, the connected (subnet route) and receive (ip2me route) need to be removed from the old vrf. Is the expectation that SAI will do this automatically?

I don't see this PR taking care of that and I don't see #632 taking care of that either.

Please clarify whether it's been agreed that SAI will do this. If not, then either this PR or the one I mentioned above, need to take care of it, unless I've missed something.

Seems #632 won't be taking care of this. As such, this PR will need to address the issue.

@nikos-github , for the current use-case, the interface configuration (in ConfigDB) will have the interface with the VRF name. That means, when it is created first time, it will be enslaved to this VRF. My understanding is in phase 1, we are not planning for dynamically changing the interface from one VRF to another. When we have the full VRF implementation as part of sonic-net/SONiC#242, this shall be taken care.

* VNET/VRF Changes (#6) * VRF changes * Fixed an IPv6 address parsing issue * Updated logs, removed intfmgr changes in favour of PR #635 * Updated VRF table name * Addressed review comment, test fixes * Remove extra semi-colon * Route handling, review comments (#8) * Vnet route table handling * Addressed review comments * Fix for interface routes, add debug logs

marian-pritsak · 2018-10-12T13:35:12Z

retest this please

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

lguohan · 2018-10-19T00:51:34Z

@marian-pritsak , there is conflict, can you resolve it?

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

qiluo-msft · 2018-10-19T21:20:37Z

retest this please

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

prsunny · 2018-12-08T02:15:11Z

retest this please

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

marian-pritsak · 2018-12-13T09:44:37Z

@lguohan Fixed vs tests

prsunny · 2018-12-13T18:48:35Z

tests/test_warm_reboot.py

@@ -836,6 +835,10 @@ def test_swss_port_state_syncup(dvs, testlog):
    dvs.stop_swss()
    time.sleep(3)

+    dvs.runcmd("ip addr flush dev Ethernet0")


Can we use the same method as deleting from Interface table? intf_tbl._del.
It looks like we add one way and flush another way

prsunny · 2018-12-13T18:51:49Z

tests/test_warm_reboot.py

+    # Defining create neighbor entries (4 ipv4 and 4 ip6, two each on each interface) in linux kernel
+    intf_tbl = swsscommon.Table(conf_db, "INTERFACE")
+    fvs = swsscommon.FieldValuePairs([("NULL","NULL")])
+    intf_tbl.set("Ethernet0|111.0.0.1/24", fvs)


Is it possible to use the same intfs list here as well for consistency, since that is used throughout the function?

prsunny · 2018-12-13T18:54:03Z

tests/test_warm_reboot.py

@@ -1556,6 +1560,14 @@ def test_routing_WarmRestart(dvs, testlog):
    rt_key = json.loads(addobjs[0]['key'])
    assert rt_key['dest'] == "192.168.100.0/24"

+    intf_tbl._del("Ethernet0|111.0.0.1/24")


Same comment to use intfs list

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

prsunny · 2018-12-14T20:55:11Z

@marian-pritsak , can you help look at the VS failure with the latest changes?

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

marian-pritsak · 2018-12-15T12:33:46Z

@prsunny, routing warm reboot passes on my VM

prsunny · 2018-12-18T22:59:53Z

retest this please

marian-pritsak · 2018-12-19T09:52:53Z

@prsunny I see that other PRs are failing on routingWarmRestart like #723

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

Conflicts: tests/test_warm_reboot.py

jipanyang · 2019-01-03T06:39:05Z

@jipanyang, that's right. The loopback config is done via /etc/network. @marian-pritsak , I think intfmgr would need to subscribe for "CFG_LOOPBACK_INTERFACE_TABLE_NAME" as well and update the INTF_TABLE for "lo".

I didn't see this issue got addressed, or have I missed anything here?

* [syncd] Fix notification on shutdown request * Move check to the beginning of the function

marian-pritsak force-pushed the intfsyncd branch from 7707d61 to 4e26ebe Compare October 2, 2018 12:41

[intfmgrd]: Merge intfsyncd into intfmgrd

ab1741c

Move intfsyncd functionality to intfmgrd, add VRF membership support. Signed-off-by: Marian Pritsak <marianp@mellanox.com>

marian-pritsak force-pushed the intfsyncd branch from 4e26ebe to ab1741c Compare October 2, 2018 12:51

prsunny added a commit to prsunny/sonic-swss that referenced this pull request Oct 2, 2018

Updated logs, removed intfmgr changes in favour of PR sonic-net#635

7915d97

prsunny reviewed Oct 2, 2018

View reviewed changes

nikos-github suggested changes Oct 2, 2018

View reviewed changes

marian-pritsak added 2 commits October 3, 2018 15:11

[intfmgrd]: Fix error handling in liunx cmd calls

484f89f

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

[intfmgrd]: fix vrf check and comments

9e50f48

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

lguohan reviewed Oct 4, 2018

View reviewed changes

lguohan requested changes Oct 4, 2018

View reviewed changes

nikos-github mentioned this pull request Oct 4, 2018

VNET/VRF Changes #632

Merged

nikos-github reviewed Oct 5, 2018

View reviewed changes

stcheng added the Enhancement ➕ label Oct 12, 2018

[intfmgrd]: Fix test_CrmIpv4Route

78b21f5

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

marian-pritsak force-pushed the intfsyncd branch from b309a80 to ebc02e7 Compare October 16, 2018 15:01

[vs]: Fix route test

dda84b3

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

marian-pritsak force-pushed the intfsyncd branch from ebc02e7 to dda84b3 Compare October 16, 2018 15:39

marian-pritsak added 7 commits October 19, 2018 14:45

Merge branch 'master' into intfsyncd

3a533cd

Fix table declaration

cdee87e

[vs]: Put interfaces up after setting IP address

634a7e2

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

[vs]: Fix Crm and Warm reboot tests

65ade0a

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

[vs]: Fix CRM and NHG tests

2afc916

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

[vs]: Fix test_swss_neighbor_syncup

544d5ab

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

[vs]: Fix interface names

a54c2fc

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

[vs]: Remove unused variable

2a282ef

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

Fix test_VlanMgrdWarmRestart

7ce4c66

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

marian-pritsak force-pushed the intfsyncd branch from 2708d6d to 7ce4c66 Compare December 5, 2018 12:29

prsunny mentioned this pull request Dec 6, 2018

[VS]: remove incorrect vlan interface create operation #720

Closed

prsunny and others added 2 commits December 11, 2018 02:06

VS test fix for CRM

a0a2217

Fix crm intf cleanup after each test

166c4d2

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

marian-pritsak force-pushed the intfsyncd branch 4 times, most recently from e7f1c4e to 53efde8 Compare December 12, 2018 19:10

prsunny reviewed Dec 13, 2018

View reviewed changes

marian-pritsak force-pushed the intfsyncd branch from 53efde8 to 1fb5368 Compare December 14, 2018 12:45

Fix test_system_warmreboot_neighbor_syncup and test_routing_WarmRestart

9917b52

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

marian-pritsak force-pushed the intfsyncd branch from 1fb5368 to 9917b52 Compare December 14, 2018 14:12

Fix stop sequence in test_swss_port_state_syncup

f5b4f68

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

sonic-net deleted a comment from marian-pritsak Dec 18, 2018

Make setIntfIp no throw

35e3b69

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

marian-pritsak force-pushed the intfsyncd branch from 2b67da6 to 35e3b69 Compare December 20, 2018 19:59

Merge remote-tracking branch 'origin/master' into intfsyncd

e2680d7

Conflicts: tests/test_warm_reboot.py

lguohan approved these changes Dec 27, 2018

View reviewed changes

prsunny merged commit 0c07a80 into sonic-net:master Dec 27, 2018

jipanyang mentioned this pull request Jan 9, 2019

lo address not synced to the asic sonic-net/sonic-buildimage#2427

Closed

nikos-github mentioned this pull request Jan 9, 2019

[intfmgrd]: Support loopback #742

Merged

oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023

[syncd] Fix notification on shutdown request (sonic-net#635)

1885a8c

* [syncd] Fix notification on shutdown request * Move check to the beginning of the function

[intfmgrd]: Merge intfsyncd into intfmgrd #635

[intfmgrd]: Merge intfsyncd into intfmgrd #635

Conversation

marian-pritsak commented Oct 2, 2018

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

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 Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

lguohan 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

marian-pritsak commented Oct 12, 2018

lguohan commented Oct 19, 2018

qiluo-msft commented Oct 19, 2018

prsunny commented Dec 8, 2018

marian-pritsak commented Dec 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny commented Dec 14, 2018

marian-pritsak commented Dec 15, 2018 • edited Loading

prsunny commented Dec 18, 2018

marian-pritsak commented Dec 19, 2018

jipanyang commented Jan 3, 2019

prsunny commented Oct 3, 2018 •

edited

Loading

marian-pritsak commented Dec 15, 2018 •

edited

Loading