Hi Albert,
On 2017年02月04日 23:46, Albert Siersema wrote: > Iwase, > >> 1. Pylint claims the "format()" style argument for LOG.info() like: >> [W1202(logging-format-interpolation), >> Peer._extract_and_handle_mpbgp_new_paths] Use % formatting in logging >> functions and pass the % parameters as arguments >> I prefer to change '%s' style like in my patch. >> >> 2. If we do logging with "info" level, the messages for allowing as-in >> will be displayed for every UPDATE messages. >> I have changed to "debug" level in my patch. > > Thanks for point that out, gave me a chance to read up on > W1202(logging-format-interpolation). > And of course, it should be debug. > > > >>> What are your thoughts on checking a maximum number of local ASN >>> occurrences in the path instead of this boolean toggle ? >>> Any preferences there ? >> >> You mean like the following image? >> no strong preference I have. >> >> # ryu/services/protocols/bgp/bgpspeaker.py >> ...(snip)... >> class BGPSpeaker(object): >> def __init__(self, as_number, router_id, >> ... >> allow_local_as_in_count=0): >> ... >> >> ``allow_local_as_in_count`` maximum number of local AS number >> occurrences in AS_PATH. >> This option is useful for the leaf/spine architecture with the >> shared AS number, for example. >> """ >> ...(snip)... >> >> # ryu/lib/packet/bgp.py >> ...(snip)... >> def has_local_as(self, local_as, max_count=0): >> """Check if *local_as* is already present on path list.""" >> _count = 0 >> for as_path_seg in self.value: >> _count += list(as_path_seg).count(local_as) >> return _count <= max_count >> ...(snip)... > > > Yes, exactly like that. I've been thinking of mixing a False/None with an > integer count for 'allow_local_as_in' (compared to the Cisco configuration > item, see below), but in the end, I think it's much cleaner to actually state > in the code that we're dealing with a count plus a reasonable default which > disallows the local ASN in the path. > Should I change to code to reflect these thoughts and re-submit the patch ? If you want to change and reflect them, you can re-submit patches with versioning like: [PATCH v2] [PATCH v2 N/N] These notation helps us for finding the latest one. > >> I'm not sure how Cisco or other routers are handling this feature, >> no strong preference I have. > > At least Cisco used a boolean value at first, then amended it with a number > (default=3). > You can either leave out allowas-in (as in False) or include it in the > configuration with a default value of 3. > I guess it's an extra level of checks to allow the local ASN in the path but > not if it appears too often, which could indeed indicate a loop. Either is fine for me. I guess "True/False" style is obvious and simple, but when introducing "count" option, it causes increasing the number of arguments. If "count=0" can indicate "allowas-in=False" implicitly, I prefer to omit "True/False" style argument and add only "count" style. + ``allow_local_as_in_count`` maximum number of local AS number + occurrences in AS_PATH. + This option is useful for the leaf/spine architecture with the + shared AS number, for example. + The default is 0 and means "local AS number is not allowed in + AS_PATH". + To allow local AS, 3 is recommended (Cisco's default). > > > > BTW: to get the patches upstream, do you need any extra information or > confirmation from me ? Not mandatory, I guess. But, I would appreciate it if you could reply your test/check result on my posted patches! Thanks, Iwase > > Thanks, > Albert > > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot _______________________________________________ Ryu-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/ryu-devel
