On Wed, Jan 09, 2013 at 04:12:48AM +0400, Alexander V. Chernikov wrote: > Version 7 :) > Changes > * Fix bug with using 0/0 and :: in aggregation protocol > * Fix mandatory lists not working before second rehash > * Improve docs and configuration example a bit > * Some code cleanups > > Thanks to Robers Hass for his bugreport and testing.
Hello I would like to merge the aggregator, but i have three related conceptual problems with your patch: (1) About third of the code is related to BGP attribute processing according to RFC 4271 9.2.2.2 (esp. AS_PATH merging and AS_SET construction) for 'save attributes' option, but this behavior is IMHO mostly useless, as such behavior is deprecated, RFC 6472 recommends that AS_SETs should not be generated and there is a RFC draft [*] that even prohibits them. As this feature adds significant and unnecessary complexity, i would like to remove it. [*] http://tools.ietf.org/html/draft-kumari-deprecate-as-set-confed-set-01 (2) If i understand it correctly, your aggregator keeps a copy of all received and aggregated routes in an internal fib, which is useful for implementing (1), but unnecessarily if you just want to originate an aggregate route if there are some matching routes (or required number of mandatory routes). In that case you just need some counters for summary routes and flags for mandatory routes. Current implementation is especially memory wasteful in the common case 'aggregate full BGP feed to get a default route'. (3) Generated aggregate routes have attrs->proto->proto == proto_agg, but attrs->source == RTS_BGP. What is a reason for that? It does not make much sense to me, because it does not have true BGP route behavior (for example rte_better would not compare it with others because of attrs->proto->proto) so i see no reason why not to define RTS_AGGREGATOR and use it. Even if we would like to generate BGP-style aggregated routes with BGP_AGGREGATOR and BGP_ATOMIC_AGGR attributes. There are some minor issues (e.g. there is probably a bug in bgp_update_sumroute() in BGP_ATOMIC_AGGR generation - even if not received from aggregated routes, this attribute should be generated if BGP routes with non-empty AS_PATH are aggregated and 'save attributes' is disabled), but these probably aren't problematic. If you do not have strong objections, i would remove (1), do some readjustment for (2), fix some minor issues and merge it. Question is whether there is any need to have optional keeping a copy of child routes in (2) for some sophisticated aggregation modes like (1), but without (1), i currently don't see the need. -- Elen sila lumenn' omentielvo Ondrej 'SanTiago' Zajicek (email: [email protected]) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
signature.asc
Description: Digital signature
