At Thu, 8 Dec 2016 15:28:42 +0900,
Iwase Yusuke wrote:
> 
> Hi Kakuma-San,
> 
> 
> On 2016年12月08日 15:06, fumihiko kakuma wrote:
> > On Mon,  7 Nov 2016 16:04:23 +0900
> > IWASE Yusuke <iwase.yusu...@gmail.com> wrote:
> > 
> >> This patch enables BGPSpeaker to set the capability for IPv6 unicast
> >> address family.
> >>
> >> Signed-off-by: IWASE Yusuke <iwase.yusu...@gmail.com>
> >> ---
> >>  ryu/services/protocols/bgp/bgp_sample_conf.py | 32 
> >> ++++++++++++++++++++++-----
> >>  ryu/services/protocols/bgp/bgpspeaker.py      | 26 +++++++++-------------
> >>  ryu/tests/integrated/common/ryubgp.py         | 11 +++++----
> >>  3 files changed, 42 insertions(+), 27 deletions(-)
> >>
> > 
> > snip
> > 
> >> diff --git a/ryu/services/protocols/bgp/bgpspeaker.py 
> >> b/ryu/services/protocols/bgp/bgpspeaker.py
> >> index 4934e41..9e30d3e 100644
> >> --- a/ryu/services/protocols/bgp/bgpspeaker.py
> >> +++ b/ryu/services/protocols/bgp/bgpspeaker.py
> >> @@ -64,6 +64,7 @@ from ryu.services.protocols.bgp.rtconf.base import 
> >> CAP_FOUR_OCTET_AS_NUMBER
> >>  from ryu.services.protocols.bgp.rtconf.base import MULTI_EXIT_DISC
> >>  from ryu.services.protocols.bgp.rtconf.base import SITE_OF_ORIGINS
> >>  from ryu.services.protocols.bgp.rtconf.neighbors import 
> >> DEFAULT_CAP_MBGP_IPV4
> >> +from ryu.services.protocols.bgp.rtconf.neighbors import 
> >> DEFAULT_CAP_MBGP_IPV6
> >>  from ryu.services.protocols.bgp.rtconf.neighbors import 
> >> DEFAULT_CAP_MBGP_VPNV4
> >>  from ryu.services.protocols.bgp.rtconf.neighbors import 
> >> DEFAULT_CAP_MBGP_VPNV6
> >>  from ryu.services.protocols.bgp.rtconf.neighbors import 
> >> DEFAULT_CAP_MBGP_EVPN
> >> @@ -289,6 +290,7 @@ class BGPSpeaker(object):
> >>  
> >>      def neighbor_add(self, address, remote_as,
> >>                       enable_ipv4=DEFAULT_CAP_MBGP_IPV4,
> >> +                     enable_ipv6=DEFAULT_CAP_MBGP_IPV6,
> >>                       enable_vpnv4=DEFAULT_CAP_MBGP_VPNV4,
> >>                       enable_vpnv6=DEFAULT_CAP_MBGP_VPNV6,
> >>                       enable_evpn=DEFAULT_CAP_MBGP_EVPN,
> >> @@ -313,6 +315,9 @@ class BGPSpeaker(object):
> >>          ``enable_ipv4`` enables IPv4 address family for this
> >>          neighbor. The default is True.
> >>  
> >> +        ``enable_ipv6`` enables IPv6 address family for this
> >> +        neighbor. The default is False.
> >> +
> >>          ``enable_vpnv4`` enables VPNv4 address family for this
> >>          neighbor. The default is False.
> >>  
> >> @@ -371,23 +376,12 @@ class BGPSpeaker(object):
> >>              CONNECT_MODE: connect_mode,
> >>              CAP_ENHANCED_REFRESH: enable_enhanced_refresh,
> >>              CAP_FOUR_OCTET_AS_NUMBER: enable_four_octet_as_number,
> >> +            CAP_MBGP_IPV4: enable_ipv4,
> >> +            CAP_MBGP_IPV6: enable_ipv6,
> >> +            CAP_MBGP_VPNV4: enable_vpnv4,
> >> +            CAP_MBGP_VPNV6: enable_vpnv6,
> >> +            CAP_MBGP_EVPN: enable_evpn,
> >>          }
> >> -        # v6 advertisement is available with only v6 peering
> >> -        if netaddr.valid_ipv4(address):
> >> -            bgp_neighbor[CAP_MBGP_IPV4] = enable_ipv4
> >> -            bgp_neighbor[CAP_MBGP_IPV6] = False
> >> -            bgp_neighbor[CAP_MBGP_VPNV4] = enable_vpnv4
> >> -            bgp_neighbor[CAP_MBGP_VPNV6] = enable_vpnv6
> >> -            bgp_neighbor[CAP_MBGP_EVPN] = enable_evpn
> >> -        elif netaddr.valid_ipv6(address):
> >> -            bgp_neighbor[CAP_MBGP_IPV4] = False
> >> -            bgp_neighbor[CAP_MBGP_IPV6] = True
> >> -            bgp_neighbor[CAP_MBGP_VPNV4] = False
> >> -            bgp_neighbor[CAP_MBGP_VPNV6] = False
> >> -            bgp_neighbor[CAP_MBGP_EVPN] = enable_evpn
> >> -        else:
> >> -            # FIXME: should raise an exception
> >> -            pass
> >>  
> >>          if multi_exit_disc:
> >>              bgp_neighbor[MULTI_EXIT_DISC] = multi_exit_disc
> > 
> > Prior to this patch, CAP_MBGP_IPV6 capability is advertised to BGP
> > peer when ipv6 transport is used for a BGP connection. So, v6 BGP on
> > v6 transport worked automagically.
> > Now the CAP_MBGP_IPV6 advertisement is controlled by the new
> > enable_ipv6 argument, existing applications[1], which don't know the new
> > argument, fail to advertise IPv6 routes.
> > 
> > I think this patch is in the right direction, but I wish I could
> > receive some warning of such an API change.
> 
> Thank you for pointing out.
> 
> How have we better to notify changes?
> Describing NOTE more explicitly on commit logs?
> Or reporting on OpenStack lauchpad?

The commit message just says "enable to set ipv6 capability".
It is very difficult to know that one needs to explicitly set ipv6
capability from this commit. IMO, the commit message should have said
"you need to explicitly set enable_ipv6 when using ipv6 peers" or
somesuch.

For maintaining API compatibility, having one or two release cycles of
deprecation period, which raises warning messages if deprecated APIs
are used, will help users.  OTOH, deprecation cycles slow down
depelopment and I am not aware of which policy ryu should take.

> Then, for quick-fix, how about setting IPv6 capability enable when
> neighbor IP address is valid IPv6 address automatically?
> 
> $ git diff
> diff --git a/ryu/services/protocols/bgp/bgpspeaker.py 
> b/ryu/services/protocols/bgp/bgpspeaker.py
> index c56f8dd..fed47bf 100644
> --- a/ryu/services/protocols/bgp/bgpspeaker.py
> +++ b/ryu/services/protocols/bgp/bgpspeaker.py
> @@ -372,6 +372,9 @@ class BGPSpeaker(object):
>          CONNECT_MODE_BOTH use both methods.
>          The default is CONNECT_MODE_BOTH.
>          """
> +        # For the backward compatibility
> +        if netaddr.valid_ipv6(address):
> +            enable_ipv6 = True
>          bgp_neighbor = {
>              neighbors.IP_ADDRESS: address,
>              neighbors.REMOTE_AS: remote_as,
> 
> 
> Thanks,
> Iwase
> 
> > 
> > 
> > [1] e.g. neutron-dynamic-routing on openstack
> > https://github.com/openstack/neutron-dynamic-routing/blob/master/neutron_dynamic_routing/services/bgp/agent/driver/ryu/driver.py#L126
> > 
> > Thanks,
> > kakuma
> > 
> 
> ------------------------------------------------------------------------------
> Developer Access Program for Intel Xeon Phi Processors
> Access to Intel Xeon Phi processor-based developer platforms.
> With one year of Intel Parallel Studio XE.
> Training and support from Colfax.
> Order your platform today.http://sdm.link/xeonphi
> _______________________________________________
> Ryu-devel mailing list
> Ryu-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/ryu-devel

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/xeonphi
_______________________________________________
Ryu-devel mailing list
Ryu-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ryu-devel

Reply via email to