Change nexthop key to ip & ifname #977

tylerlinp · 2019-07-17T09:30:04Z

What I did
Changed nexthop key including ifname.
Made a supplement to vs test of ACL redirect.

Why I did it
To implement VRF, single IP cannot indicate nexthop uniquely.

How I verified it
vs test

Details if related

tylerlinp · 2019-07-17T09:32:59Z

[VRF]: submit vrf feature

[MirrorOrch]: Init the next hop ip with 0 instead of default constructor

…thop_key

orchagent/neighorch.cpp

orchagent/intfsorch.cpp

prsunny · 2019-08-12T20:52:09Z

orchagent/routeorch.cpp


-            // TODO: set to blackhold if nexthop is empty?
-            if (ip_addresses.getSize() == 0)


I think, if ips = "", you can erase and continue. I don't think we have to introduce 0.0.0.0 to handle this case

Introduce 0.0.0.0 is to prepare for handling directly connected routes. It is better to keep ips and aliases same size, in general handling tokenize("", ',') will get empty vector. Zero ip would be used to construct NextHopGroupKey.

prsunny · 2019-08-12T20:58:15Z

fpmsyncd/routesync.cpp

@@ -325,6 +325,17 @@ string RouteSync::getNextHopGw(struct rtnl_route *route_obj)
            nl_addr2str(addr, gw_ip, MAX_ADDR_SIZE);
            result += gw_ip;
        }
+        else
+        {
+            if (rtnl_route_get_family(route_obj) == AF_INET)


Why do we need this change? The RouteOrch can delete the entry if nexthop.size == 0

prsunny · 2019-08-12T21:28:36Z

orchagent/routeorch.h

+    }
+
+    inline bool operator<(const NextHopGroupKey &o) const
+    {


Why do we need these operators for NHGroupKey. I don't see the usage elsewhere. Please remove if not used.

Maybe useless, NextHopGroupKey just extended from IpAddresses.

Please remove the unused functions

It is used actually. NextHopGroupKey is used as map key. Compare function is needed for map key. So overload operator< is needed.

prsunny · 2019-08-12T21:59:08Z

tests/test_acl.py

+        fvs = swsscommon.FieldValuePairs([
+                                        ("priority", "100"),
+                                        ("L4_SRC_PORT", "65000"),
+                                        ("PACKET_ACTION", "REDIRECT:10.0.0.2|Ethernet4")])


This is a schema change, Can you update the swss-schema.md ?

OK. I will update later.

prsunny · 2019-08-12T22:08:39Z

orchagent/intfsorch.cpp

    return port.m_rif_id;
 }

+string IntfsOrch::getRouterIntfsAlias(const IpAddress &ip)


This function is ambiguous as same IP can be existing on multiple interfaces in a multi VRF scenario. I don't think we can have a function like this. Is it for some backward compatibility?

This function will add a vrf argument to support VRF. The VRF PR includes this. Mirror session is using it now, in getNeighborEntry.

Can you add VRF argument to this function in this PR? It can pass default vrf from the caller.

OK, actually I had removed it just for this PR.

orchagent/mirrororch.cpp

prsunny · 2019-08-12T22:23:15Z

orchagent/neighorch.h


 #define NHFLAGS_IFDOWN                  0x1 // nexthop's outbound i/f is down

-struct NeighborEntry
+struct NextHopKey


To me it looks better to be named as NeighborKey than NextHopKey.

Now NeighborEntry is same as NextHopKey, I think NeighborEntry can be named as NeighborKey, but I haven't changed it. It is better to distinguish nexthop and neighbor, in some cases just need nexthop although now they are created together.

My thought is, it is Neighorch and this is the key structure. So with NeighborKey, it aligns with the neighbor orch functionality. Nexthop is created from the Neighbor. Just a suggestion to think, not a blocker

I agree with Prince to replace the NextHopKey with NeighborKey and also remove the NeighborEntry. Otherwise it is confusing to have various class names.

Also, since the two classes NextHopKey and NextHopGroupKey are very large. Please move them outside the header and create a separate header file and the implementation file.

I think NexthopKey semantic is more than NeighborKey. A Route has nexthop but not neigh. Neigh can reuse NexthopKey as NeighborKey. But route can't use NeighborKey as nexthop.
Spliting Nexthopkey and NextHopGroupKey is good idea. We will fix it soon.

prsunny · 2019-08-14T00:48:22Z

@tylerlinp , we need to make sure that an old config_db.json with ACL redirect/Mirror session without ifname in schema also shall work with this changes. It is expected to work as if in default VRF.

shine4chen · 2019-08-15T07:39:11Z

@tylerlinp , we need to make sure that an old config_db.json with ACL redirect/Mirror session without ifname in schema also shall work with this changes. It is expected to work as if in default VRF.

@prsunny Sure, we will revise our HLD and code. In fact We doesn't change mirror session schema in current VRF PR. So mirror backward compatibility is not issue. For Acl redirect we can add support for single ip without interface name.

karthikarum

we don't have any additional comments.

prsunny · 2019-09-12T01:30:53Z

retest this please

prsunny · 2019-09-12T04:26:29Z

retest this please

…thop_key

stcheng

Could you

stcheng · 2019-09-17T16:48:04Z

orchagent/neighorch.h


 #define NHFLAGS_IFDOWN                  0x1 // nexthop's outbound i/f is down

-struct NeighborEntry
+struct NextHopKey


I agree with Prince to replace the NextHopKey with NeighborKey and also remove the NeighborEntry. Otherwise it is confusing to have various class names.

stcheng · 2019-09-17T16:48:52Z

orchagent/neighorch.h


 #define NHFLAGS_IFDOWN                  0x1 // nexthop's outbound i/f is down

-struct NeighborEntry
+struct NextHopKey


Also, since the two classes NextHopKey and NextHopGroupKey are very large. Please move them outside the header and create a separate header file and the implementation file.

stcheng · 2019-09-17T16:49:03Z

orchagent/neighorch.h

+    NextHopKey(const IpAddress &ip, const std::string &alias) : ip_address(ip), alias(alias) {}
+    NextHopKey(const std::string &str)
+    {
+        if (str.find(',') != string::npos)


Why do you need this check?

It originally come from ipAddress class. Some apps use multi-ipaddresses string to create NexthopKey class. It needs to throw an exception to reminder the apps to use NexthopGroupKey class

stcheng · 2019-09-17T16:52:45Z

tests/test_acl.py

@@ -1215,6 +1215,141 @@ def test_AclRuleIcmpV6(self, dvs, testlog):

        self.remove_acl_table(acl_table)

+    def set_admin_status(self, interface, status):


Could you move these helper functions into conftest.py file?

Yes, we will do it.

I have done this in PR #943.

stcheng · 2019-09-17T16:55:10Z

tests/test_acl.py

+        tbl._del(interface + "|" + ip)
+        time.sleep(1)
+
+    def test_AclRuleRedirectToNexthop(self, dvs, testlog):


Since this is to test a new functionality, could you move this portion into a separate PR?
It could be better to add some unit tests that covers the current changes

In fact ACL redirect is the existing function. But no vs test cases can cover it. Since we change nexthop key we add a test case to verify if this function works.

I have removed the test in this PR and revised it in PR #943. So all new unit tests are added in PR #943.

prsunny · 2019-09-18T22:47:05Z

retest this please

stcheng · 2019-09-19T20:45:47Z

debian/changelog

@@ -1,5 +1,9 @@
 sonic (1.0.0) stable; urgency=medium

+  * next-hop key add ifname


please keep the changelog as it is since right now we're not updating this file routinely.

New version dpkg-deb use the newest time in chaneglog to set deb files timestamp. There is a problem that new files with same size and timestamp cannot be saved to image in process of PR build, so in this PR fpmsynd cannot be updated and many test cases failed. I add changelog just for building swss.deb with newer timestamp and pass tests.
THE PROBLEM MUST BE RESOLVED, or else the VS test become meaningless, for the running version may not be the codes compiled.

are you referring to the VS test running under this pull request? and the fpmsyncd is not replaced with the latest binary? if that is the case as you described, I'll check.

Yes，I have forward the email which describes this issue detailly to you. It is common issue, not just on this PR.

could you check right now if the binaries could overwrite the old ones?
I have added Azure/sonic-build-tools@e9e3957 to fix this issue.

stcheng · 2019-09-19T20:46:12Z

could you also resolve the conflicts? thanks!

…thop_key

1. create separate file for NextHopKey and NextHopGroupKey 2. remove test_AclRuleRedirectToNexthop

shine4chen · 2019-09-20T15:58:58Z

@prsunny @stcheng we have resolved the conflicts and address your comments.After changing the timestamp of changelog file, all files can be updated and this PR passed all vs test cases.

prsunny · 2019-09-23T20:41:26Z

retest this please

…thop_key Resolve conflicts and change nexthop delimiter to '@' Conflicts: orchagent/mirrororch.cpp Modifications: doc/swss-schema.md orchagent/nexthopkey.h orchagent/nexthopgroupkey.h

tylerlinp · 2019-09-24T06:34:58Z

retest this please

stcheng

it's better not to update the changelog

stcheng · 2019-10-02T20:50:18Z

debian/changelog

@@ -1,5 +1,9 @@
 sonic (1.0.0) stable; urgency=medium

+  * next-hop key add ifname


could you check right now if the binaries could overwrite the old ones?
I have added Azure/sonic-build-tools@e9e3957 to fix this issue.

Change nexthop key to ip & ifname

6735110

add back pr953 overwrote

e7ac33f

[MirrorOrch]: Init the next hop ip with 0 instead of default constructor

stcheng added the Enhancement ➕ label Jul 18, 2019

Merge branch 'master' of https://github.com/Azure/sonic-swss into nex…

aa98cc1

…thop_key

lguohan requested a review from prsunny July 26, 2019 21:30

prsunny requested changes Aug 12, 2019

View reviewed changes

stcheng self-requested a review August 12, 2019 23:27

karthikarum approved these changes Aug 19, 2019

View reviewed changes

Revise for acl redirect backward compatibility and review comments

6042eaa

Tyler Li added 2 commits September 16, 2019 19:24

is_subnet change for nexthop ip fill zero if empty

6b495cd

Merge branch 'master' of https://github.com/Azure/sonic-swss into nex…

c542a6a

…thop_key

stcheng mentioned this pull request Sep 17, 2019

[neighorch]: Return false when the next hop IP exists before adding #1032

Closed

stcheng suggested changes Sep 17, 2019

View reviewed changes

add changelog for update timestamp

37a7c49

stcheng reviewed Sep 19, 2019

View reviewed changes

qiluo-msft self-requested a review September 19, 2019 22:19

Tyler Li added 2 commits September 19, 2019 18:31

Merge branch 'master' of https://github.com/Azure/sonic-swss into nex…

06ab194

…thop_key

Revise for review comments

698d10a

1. create separate file for NextHopKey and NextHopGroupKey 2. remove test_AclRuleRedirectToNexthop

AkhileshSamineni mentioned this pull request Sep 23, 2019

Natmgrd changes in sonic-swss sub module to support NAT feature. #1059

Merged

Merge branch 'master' of https://github.com/Azure/sonic-swss into nex…

67aca81

…thop_key Resolve conflicts and change nexthop delimiter to '@' Conflicts: orchagent/mirrororch.cpp Modifications: doc/swss-schema.md orchagent/nexthopkey.h orchagent/nexthopgroupkey.h

tylerlinp force-pushed the nexthop_key branch from 89dac4c to 67aca81 Compare September 24, 2019 01:56

prsunny approved these changes Oct 2, 2019

View reviewed changes

stcheng approved these changes Oct 2, 2019

View reviewed changes

prsunny merged commit bd36751 into sonic-net:master Oct 2, 2019

This was referenced Oct 2, 2019

Revert changelog that was done for passing VS test. #1080

Merged

[VRF]: submit vrf feature #943

Merged

dawnbeauty mentioned this pull request Nov 11, 2019

[Doc] Update nexthop seperator to "@" in vrf_ansible_test_plan sonic-net/SONiC#520

Merged

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022

Enable HW watchdog before fast-reboot (sonic-net#977)

1b992b3


		// TODO: set to blackhold if nexthop is empty?
		if (ip_addresses.getSize() == 0)

		@@ -1215,6 +1215,141 @@ def test_AclRuleIcmpV6(self, dvs, testlog):

		self.remove_acl_table(acl_table)

		def set_admin_status(self, interface, status):

		@@ -1,5 +1,9 @@
		sonic (1.0.0) stable; urgency=medium

		* next-hop key add ifname

Change nexthop key to ip & ifname #977

Change nexthop key to ip & ifname #977

Conversation

tylerlinp commented Jul 17, 2019

tylerlinp commented Jul 17, 2019 • 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

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 Aug 14, 2019

shine4chen commented Aug 15, 2019

karthikarum left a comment

Choose a reason for hiding this comment

prsunny commented Sep 12, 2019

prsunny commented Sep 12, 2019

stcheng 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

Choose a reason for hiding this comment

prsunny commented Sep 18, 2019

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

stcheng commented Sep 19, 2019

shine4chen commented Sep 20, 2019 • edited Loading

prsunny commented Sep 23, 2019

tylerlinp commented Sep 24, 2019

stcheng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerlinp commented Jul 17, 2019 •

edited

Loading

shine4chen commented Sep 20, 2019 •

edited

Loading