evpn: Add ETag and IPAddress to tableKey() for EVPNMacIPAdvertisementRoute

[taitov]

This pull request fixes incorrect index generation for EVPN routing table. before: RD+MAC after: RD+MACLEN+MAC+ETAG+IPLEN+IP

https://datatracker.ietf.org/doc/html/rfc7432#section-7.2

[taitov]

@Tuetuopay Hi! Look at this PR, please

[Tuetuopay]

Hi,

Yup that's indeed the proper way to do it, which I failed to do at the time. Did you see that I had a PR open for the same thing, albeit unoptimized? (https://github.com/osrg/gobgp/pull/2804). It goes a bit further by renaming fields to something that makes sense. I have another follow-up that does the same binary optimization, but even stronger by avoiding allocations (as I measured allocs+gc could introduce up to 20% overhead in route ingestion).

Also, you PR misses pretty much all place where the index is manually crafted instead of using tableKey, because you did not change the code that accesses Table.macIndex. Please see my PR for all those places.

FWIW the PR I linked #2804, along with #2805, and the binary optimization, have been running in prod for the past two weeks in the EVPN instance I observed issues: both quadratic performance (the root for the original incorrect patch) and lost routes (what you try to fix here).

Thanks for the ping :)

[taitov]

Yes, i see your #2804. And our changes conflict :) My changes are only in the correct index generation in binary format (with test coverage). Which should be much faster than calling nlri.String().

[Tuetuopay]

Sorry for the delay, I was in vacation away from the computer.

Indeed yours is much faster, but is not correct. It does not update the macIndex hash map, that indexes routes based on their destination mac for mac mobility handling. However with my PR now merged, yours is now correct.

However, I'll open another one that does it even better than yours, limiting allocations to a single one for the whole call. If you want to do it be my guest! I'm basically doing the following:

• add EncodeToBytes([]byte) to RouteDistinguisherInterface that directly writes to the target slice without allocating (like Serialize() does, which relies on this now)
• do it for all five evpn route type supported by gobgp

[fujita]

merging https://github.com/osrg/gobgp/pull/2805 makes evpn users happy? Then I can close this?

[Tuetuopay]

merging #2805 makes evpn users happy? Then I can close this?

Well #2805 would make at least me very happy as it is an issue we are hitting in production!

As for this PR, it fixes the same issues #2804 does, but with some optimizations sprinkled on top. I think it is up to @taitov to say whether he wants to keep this one open for the optimization or not. Given that I plan to open the same kind of optimization MR but a bit more complete (all 5 evpn route types, less allocations, copy instead of append), it is up to them :)

[fujita]

Understood, I merged #2805.