zebra: Send RMAC along with Type-5 prefix #7852

kishorekunal01 · 2021-01-12T01:57:13Z

Send RMAC along with VNI to FPM for EVPN Type-5 routes

When Zebra receive EVPN Type-5 prefix it create nexthop, adding code to copy RMAC as well in the nexthop. So that when prefix is encoded to send to the FPM, it encode RMAC as well.
In current system RMAC can be retrieved from RMAC send to FPM, It requires lot of processing in FPM, if RMAC for the tunnel changes from MAC1 to MAC2. This is overhead in FPM, we can optimize this by send RMAC as well with the prefix.

Signed-off-by: Kishore Kunal kishorekunal01@broadcom.com

Send RMAC along with VNI to FPM for EVPN Type-5 routes Signed-off-by: Kishore Kunal <kishorekunal01@broadcom.com>

kishorekunal01 · 2021-01-12T02:02:11Z

Raise this new PR, since in the old PR #7469 I was not able to modify the comment in the commit and due to that polychaeta was giving error again and again(tried git rebase -i <>, old commit has some conflict and was not able to resolve). Hence created this new PR.

LabN-CI · 2021-01-12T03:11:19Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	0
Date	0
Start	0
Finish	vncregress-2021-01-11-21:35:55.txt
Run-Time	autoscript-2021-01-11-21:36:53.log.bz2
Total	504 497 426
Pass	SUCCESS git merge/7399 `d7e5c30`
Fail	11/09/2020
Valgrind-Errors	21:35:56
Valgrind-Loss	22:11:09
Details	35:13
Log	1815
Memory	1815
SUCCESS git merge/7852 `b8fcd79`	0
01/11/2021	0
21:35:55	0
22:11:13	vncregress-2020-11-09-21:35:56.txt
35:18	autoscript-2020-11-09-21:36:57.log.bz2
1815	463 483 417
1815

For details, please contact louberger

NetDEF-CI · 2021-01-12T04:21:58Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-16441/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Debian 10 amd64 build: Successful with additional warnings

Debian Package lintian failed for Debian 10 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-16441/artifact/DEB10BUILD/ErrorLog/log_lintian.txt)

W: frr source: pkg-js-tools-test-is-missing
W: frr source: newer-standards-version 4.5.0.3 (current is 4.3.0)
W: frr source: pkg-js-tools-test-is-missing
W: frr source: newer-standards-version 4.5.0.3 (current is 4.3.0)
W: frr: changelog-file-missing-explicit-entry 7.5-0 -> 7.6-dev-20210112-00-gb8fcd799d-0 (missing) -> 7.6-dev-20210112-00-gb8fcd799d-0~deb10u1
W: frr-snmp: changelog-file-missing-explicit-entry 7.5-0 -> 7.6-dev-20210112-00-gb8fcd799d-0 (missing) -> 7.6-dev-20210112-00-gb8fcd799d-0~deb10u1
W: frr-pythontools: changelog-file-missing-explicit-entry 7.5-0 -> 7.6-dev-20210112-00-gb8fcd799d-0 (missing) -> 7.6-dev-20210112-00-gb8fcd799d-0~deb10u1
W: frr-rpki-rtrlib: changelog-file-missing-explicit-entry 7.5-0 -> 7.6-dev-20210112-00-gb8fcd799d-0 (missing) -> 7.6-dev-20210112-00-gb8fcd799d-0~deb10u1
W: frr-doc: changelog-file-missing-explicit-entry 7.5-0 -> 7.6-dev-20210112-00-gb8fcd799d-0 (missing) -> 7.6-dev-20210112-00-gb8fcd799d-0~deb10u1

sworleys · 2021-01-12T05:50:21Z

Since you are modifying the heavily used lib/nexthop struct, I would like this to be two commits.

One for the addition of this information in the encap on lib/nexthop.
One for sending that new encapped rmac over fpm.

Also, I think any changes to the older fpm_netlink interface should also be done in the newer fpm_dplane_netlink interface. So if we are gonna start sending an encapped rmac in the old one, it should also happen in the new one.

I think @donaldsharp had some unresolved comments in the old PR about whether this data should even be included in the nexthop so I will let him speak to that.

kishorekunal01 · 2021-01-12T06:38:13Z

Since you are modifying the heavily used lib/nexthop struct, I would like this to be two commits.

One for the addition of this information in the encap on lib/nexthop.
One for sending that new encapped rmac over fpm.

Also, I think any changes to the older fpm_netlink interface should also be done in the newer fpm_dplane_netlink interface. So if we are gonna start sending an encapped rmac in the old one, it should also happen in the new one.

I think @donaldsharp had some unresolved comments in the old PR about whether this data should even be included in the nexthop so I will let him speak to that.

Thanks for the review, Sure I am make the suggested changes.

Split the PR into 2 part.
Send RMAC via newer fpm_dplane_netlink interface as well.

mjstapp · 2021-01-12T13:32:06Z

lib/nexthop.h

@@ -134,7 +139,7 @@ struct nexthop {
 	/* Encapsulation information. */
 	enum nh_encap_type nh_encap_type;
 	union {
-		vni_t vni;
+		struct vxlan_nh_encap encap_data;


why a union that is just a single struct - why not just use an instance of the struct directly?

This is done for future use, based on the nh_encap_type we can have different data to store.

mjstapp · 2021-01-12T13:34:23Z

lib/nexthop.h

@@ -134,7 +139,7 @@ struct nexthop {
 	/* Encapsulation information. */
 	enum nh_encap_type nh_encap_type;
 	union {


Is this info exclusive with anything else in the nexthop struct - the label info, for example? if so, we should consider creating a union so that we don't increase the overall size.

mjstapp · 2021-01-12T13:56:34Z

lib/nexthop.h

@@ -65,6 +65,11 @@ enum nh_encap_type {
 /* Backup index value is limited */
 #define NEXTHOP_BACKUP_IDX_MAX 255

+struct vxlan_nh_encap {


is mac info completely vxlan-specific - is there a more general evpn use for mac info, for example?

I don't understand how a rmac is a property of the nexthop. It's a property of the l2 information not the nexthop. Why are we storing it in the nexthop? If the answer if convenience then double triple boo

this should be a mapping in a different data structure. I also believe we should move vni's out of nexthops as well.

This changes is done based on the current code flow where nexthop store the VNI information. Hence RMAC is also added in the similar lines.

Please reread we should move vni's out of nexthops as well. it's not the right data abstraction. continuing down a wrong line does us no good.

Sorry our message crossed, so did see your earlier response before my reply.

all nexthops need relevant l2 information in order for it to work properly. Yet we don't carry that information in the nexthop.
A nexthop is L3 reachability. VNI's RMACS are l2 reachability. If we stuff this together we are going to have fun with nexthops when a L3 reachability stays live but the l2 changes underneath. If we need to pass on l2 reachability to lower levels than that needs to be a separate action controlled by seperate data structures in the zebra code. This is the reason they are separate things.

@donaldsharp I have one more question. Is it ok to extract the RMAC information in the function netlink_route_info_add_nh using API zl3vni_nh_lookup(zl3vni, nexthop) and send the RMAC along with prefix via FPM channel.

I think this merits an actual discussion on how to properly do this in FRR. I am not sure I have an off the cuff answer here. I also think that what you proposing is not the right thing to do. Let me get together w/ Mark/Vivek/Anuradha and figure it out.

Thanks for your response

donaldsharp · 2021-01-20T13:14:25Z

@kishorekunal01 What timezone are you in? I'd like to setup a meeting where we can discuss the api we want between the master pthread of zebra and the dplane pthread for handling this.

kishorekunal01 · 2021-01-20T16:42:26Z

@kishorekunal01 What timezone are you in? I'd like to setup a meeting where we can discuss the api we want between the master pthread of zebra and the dplane pthread for handling this.

Sure, I am in PST timezone. Thanks

vivek-cumulus · 2021-02-02T04:00:16Z

I haven't see the code changes here. However, @sworleys pointed me to this PR today and I have a couple of comments.

The VNI that comes with the route has to looked at as a 'label' for all practical purposes. A route can have multipath/ECMP, and each of those next-hops/sources could have announced a different label (VNI) for the route. So, the VNI has to be installed from bgpd down to zebra per zapi_nexthop and then maintained just like the label in zebra. The bgpd->zebra part is not there yet AFAIK since we have only dealt with global VNIs. We are currently working on downstream (remote) allocated VNIs, so this will come in soon and will probably need some zebra/lib changes also, unless everything seamless fits into the 'label' fields.

One could extend the above to say that it is the (VNI, RMAC) that needs to be per zapi_nexthop. The same next hop could not only announce different VNIs for different routes, but also different RMACs. In fact, the RMAC is currently being passed down from bgpd->zebra as part of the zapi_nexthop. It is not stored as part of the nexthop in zebra which has given rise to a few bugs. However, while a RMAC per route/nexthop combination is theoretically possible, practically it is a single RMAC announced by a remote device for all its routes across all VRFs. For EVPN-MH, the bgpd->zebra code has been reworked by @AnuradhaKaruppiah to install the nexthop->RMAC directly, separate from the route install; the changes are possibly in the queue or waiting to be merged with EVPN changes by others. I plan to extend this interface for non MH also. This means that the RMAC will be handled separately from the route. This is also one of the option specified in the type-5 standards. With this option, there shouldn't be a need to maintain the RMAC with the nexthop.

We should discuss this though some more.

FYI, here is an example of a route and neighbor addition into the kernel that illustrates the above. Look closely at the VRF and VNI in use.

ip route add 81.1.1.0/24 table vrf1 nexthop encap ip id 104541 dst 110.0.0.5 via 110.0.0.5 dev vxlan0 onlink nexthop encap ip id 104641 dst 110.0.0.6 via 110.0.0.6 dev vxlan0 onlink
ip route add 81.1.2.0/24 table vrf1 nexthop encap ip id 104551 dst 110.0.0.5 via 110.0.0.5 dev vxlan0 onlink nexthop encap ip id 104651 dst 110.0.0.6 via 110.0.0.6 dev vxlan0 onlink
ip route add 91.1.1.0/24 table vrf2 nexthop encap ip id 104541 dst 110.0.0.5 via 110.0.0.5 dev vxlan0 onlink nexthop encap ip id 104641 dst 110.0.0.6 via 110.0.0.6 dev vxlan0 onlink

ip neigh add 110.0.0.5 lladdr a6:32:1c:36:fa:21 dev vxlan0
ip neigh add 110.0.0.6 lladdr 76:e7:ab:01:b6:ed dev vxlan0

github-actions · 2022-10-13T13:24:04Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

zebra: Send RMAC along with Type-5 prefix

b8fcd79

Send RMAC along with VNI to FPM for EVPN Type-5 routes Signed-off-by: Kishore Kunal <kishorekunal01@broadcom.com>

polychaeta added the zebra label Jan 12, 2021

kishorekunal01 mentioned this pull request Jan 12, 2021

zebra: Send RMAC along with VNI to FPM for EVPN Type-5 routes #7469

Closed

kishorekunal01 mentioned this pull request Jan 12, 2021

[FRR] Added support for L3VNI for EVPN as described in the PR Azure/SONiC#437 sonic-net/sonic-buildimage#4806

Merged

sworleys self-requested a review January 12, 2021 05:50

mjstapp reviewed Jan 12, 2021

View reviewed changes

github-actions bot added the conflicts label Oct 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zebra: Send RMAC along with Type-5 prefix #7852

zebra: Send RMAC along with Type-5 prefix #7852

kishorekunal01 commented Jan 12, 2021

kishorekunal01 commented Jan 12, 2021

LabN-CI commented Jan 12, 2021

NetDEF-CI commented Jan 12, 2021

sworleys commented Jan 12, 2021

kishorekunal01 commented Jan 12, 2021

mjstapp Jan 12, 2021

kishorekunal01 Jan 12, 2021

mjstapp Jan 12, 2021

mjstapp Jan 12, 2021

donaldsharp Jan 12, 2021

donaldsharp Jan 12, 2021

kishorekunal01 Jan 12, 2021

donaldsharp Jan 12, 2021

kishorekunal01 Jan 12, 2021

donaldsharp Jan 12, 2021

kishorekunal01 Jan 12, 2021

donaldsharp Jan 13, 2021

kishorekunal01 Jan 13, 2021

donaldsharp commented Jan 20, 2021

kishorekunal01 commented Jan 20, 2021

vivek-cumulus commented Feb 2, 2021

github-actions bot commented Oct 13, 2022

zebra: Send RMAC along with Type-5 prefix #7852

Are you sure you want to change the base?

zebra: Send RMAC along with Type-5 prefix #7852

Conversation

kishorekunal01 commented Jan 12, 2021

kishorekunal01 commented Jan 12, 2021

LabN-CI commented Jan 12, 2021

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

NetDEF-CI commented Jan 12, 2021

Continuous Integration Result: SUCCESSFUL

Warnings Generated during build:

sworleys commented Jan 12, 2021

kishorekunal01 commented Jan 12, 2021

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

donaldsharp commented Jan 20, 2021

kishorekunal01 commented Jan 20, 2021

vivek-cumulus commented Feb 2, 2021

github-actions bot commented Oct 13, 2022