Hi Albert,

On 2017年02月02日 22:39, Albert Siersema wrote:
> Hey Iwase,
> 
>> BTW, we are using Git for indexing versions.
>> Would you send patches in the git format?
> 
> Sure, I prefer git anyway :) I diff-ed it because I consulted:
>   https://github.com/osrg/ryu/blob/master/CONTRIBUTING.rst
> and the linked: 
> https://kernel.org/doc/html/latest/process/submitting-patches.html
> (btw your link to kernel.org in CONTRIBUTING.rst is out of date)

Thank you for pointing out.
We need to update CONTRIBUTING.rst along with our style, I guess...


> Granted I did forget the pep8 step :( Ran "./run_tests.sh -p" this time. 
> Which btw doesn't complain about lines being too long ?! Fixed these by hand 
> tho.

Oops, you are right.
I didn't notice that, because I'm also using PyCharm IDE and check
pep8 via this editor.


> 
> Anyway, maybe CONTRIBUTING.rst should be updated with the excellent howto you 
> sent me ?

Yes, I agree with you!


> 
>> e.g.)
>>   $ git clone https://github.com/osrg/ryu.git
>>   $ cd ryu
>>   Create a branch, if needed.
>>   $ git checkout -b <your branch>
>>   <... some modifications ...>
>>   $ git add <modified files>
>>   $ git commit -s
>>   Format patches. ("N" means the number of commits to be included)
>>   $ git format-patch HEAD~N
>>   Then, you can use *.patch for submitting!
>>   By Mailer or "git send-email" command like:
>>   $ git send-email --to="[email protected]" *.patch
>>
>> I attached the patch created like the above.
>> Could you review and test my patch?
>> FYI, if you can use Git, applying patches is easy:
>> e.g.)
>>   Save *.patch files and
>>   $ git am *.patch
> 
> I'll check your patch.
> In the meantime, here is a run-test-ed and git version of my patch.

Thanks!

FYI, this is just my taste, I modified the following at 2 point.

diff --git a/ryu/services/protocols/bgp/peer.py 
b/ryu/services/protocols/bgp/peer.py
index 8bf96d6..95b6c37 100644
--- a/ryu/services/protocols/bgp/peer.py
+++ b/ryu/services/protocols/bgp/peer.py
@@ -1623,9 +1623,13 @@ class Peer(Source, Sink, NeighborConfListener, Activity):
         aspath = umsg_pattrs.get(BGP_ATTR_TYPE_AS_PATH)
         # Check if AS_PATH has loops.
         if aspath.has_local_as(self.local_as):
-            LOG.error('Update message AS_PATH has loops. Ignoring this'
-                      ' UPDATE. %s', update_msg)
-            return
+            if self._common_conf.allow_local_as_in:
+                LOG.info('Allowing update message AS_PATH with own AS {}'.
+                         format(update_msg))
+            else:
+                LOG.error('Update message AS_PATH has loops. Ignoring this'
+                          ' UPDATE. %s', update_msg)
+                return
 
         next_hop = update_msg.get_path_attr(BGP_ATTR_TYPE_NEXT_HOP).value
         # Nothing to do if we do not have any new NLRIs in this message.

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.


> 
> 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?

# 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)...


I'm not sure how Cisco or other routers are handling this feature,
no strong preference I have.


Thanks,
Iwase


> 
> Cheers,
> 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

Reply via email to