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 ? > 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. BTW: to get the patches upstream, do you need any extra information or confirmation from me ? 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
