WIP: BGP agents with Ryu

[tfukushima]

This is re-submitted version of #119. The patches are rebased on top of bp/bgp-dynamic-routing instead of bp/bgp-dynamic-routing.

All issues mentioned in #119 are fixed.

[tfukushima]

@jdevesa, @ryu25ish I replaced the previous dependency with ryu>=3.12 since Ryu 3.12, which resolved the issue I had with Ryu 3.11, has been released.

[tfukushima]

@jdevesa

bgp speaker driver class should be informed also in the dr_agent.ini file and then loaded using the importutils module. Check out this as example. https://github.com/openstack/neutron/blob/master/neutron/agent/l3_agent.py#L406

Although I already modified it, what is the advantage of this? I guess if we decouple the class to be instantiated from the logic, we can make it pluggable like dependency injection in Java. However, to do that we have to make it sure that all pluggable BGP speakers have the same interface. For instance, the following code assumes it has neighbor_add but it's not necessarily true. Even if another BGP speaker appeared, there is no guarantee that it has the same interface as Ryu's one.

https://github.com/tfukushima/neutron/commit/545db527d30a3207903c909c0c2c65fbdeaabc1a#diff-5cc370b45e67b91a58dc58806859ffc1R116

If importutils.import_object retrieves the singleton object, it makes some sense but it doesn't. It creates an instance every time.

https://github.com/openstack/neutron/blob/8307be7a9ef83c66eee8f2e6816aed8774df74d1/neutron/openstack/common/importutils.py#L24-L38

If we can assume Ryu is the only BGP speaker supported by our dynamic routing implementation, things go easier and we can simply add Ryu to the dependency. Otherwise we need to switch which package is installed depending on the BGP speaker specified in dr_agent.ini.

EDIT: I'll hold the dynamic dependency resolution based on the configuration if the users activate the dynamic routing because it depends on how we deal with this problem.

[devvesa]

Hi Taku,

yes, they have to have the same interface. We have to create 'driver' object that it is abstracted of the implementation and call these interfaces method. Since we are offering only create/remove peers (call them peers, not neighbours) and create/remove networks, the interface will not be too much Ryu specific, but quite standard for any BGP speaker. It is not so hard to create a new layer! :) Try to copy how L3 agent does it: https://github.com/openstack/neutron/blob/master/neutron/agent/l3_agent.py#L405

I don't see the problem of creating a new instance of BGP speaker every time we initialize the agent... The DRagent will be a single instance per process... so, even if it were a singleton, that would need to be created every time.

I'm not sure if we can assume the BGP Ryu will be the only one. We are developing the 'core' of the agent, so we have to make the life easier for everyone that want to ship OpenStack. Imagine a service company like Mirantis or RedHat takes Juno , but does not like Ryu BGP. They have to be able to develop their own implementation of the driver. Even they don't push their implementation in upstream, they should be able to replace Ryu easier in their distribution. They don't want to wait until Kilo to have something more flexible. If we don't provide a common interface (the driver one in the first paragraph) and an easy way to instantiate their implementation (using the *.ini file), they will reject our code.

I hope I explained well my thoughts...

On 4 August 2014 07:21, Taku Fukushima notifications@github.com wrote:

@jdevesa https://github.com/jdevesa

bgp speaker driver class should be informed also in the dr_agent.ini file and then loaded using the importutils module. Check out this as example.

https://github.com/openstack/neutron/blob/master/neutron/agent/l3_agent.py#L406

Although I already modified it, what is the advantage of this? I guess if we decouple the class to be instantiated from the logic, we can make it pluggable like dependency injection in Java. However, to do that we have to make it sure that all pluggable BGP speakers have the same interface. For instance, the following code assumes it has neighbor_add but it's not necessarily true. Even if another BGP speaker appeared, there is no guarantee

tfukushima@545db52#diff-5cc370b45e67b91a58dc58806859ffc1R116 https://github.com/tfukushima/neutron/commit/545db527d30a3207903c909c0c2c65fbdeaabc1a#diff-5cc370b45e67b91a58dc58806859ffc1R116

If importutils.import_object retrieves the singleton object, it makes some sense but it doesn't. It creates an instance every time.

https://github.com/openstack/neutron/blob/8307be7a9ef83c66eee8f2e6816aed8774df74d1/neutron/openstack/common/importutils.py#L24-L38

If we can assume Ryu is the only BGP speaker supported by our dynamic routing implementation, things go easier and we can simply add Ryu to the dependency. Otherwise we need to switch which package is installed depending on the BGP speaker specified in dr_agent.ini.

— Reply to this email directly or view it on GitHub https://github.com/midokura/neutron/pull/120#issuecomment-51018560.

Jaume Devesa Software Engineer at Midokura

[tfukushima]

@jdevesa OK, now I get why you called it "driver". Let me look at L3 agent first and then think how to implement it. I think we need to assume the users would put the accurate class name in dr_agent.ini. Otherwise we can't assume that the specified class has the correct interface.

The most important part would be error handling or the fallback. I'd add the following code.

1. If the BGP driver class is specified in dr_agent.ini, try to import it as the BGP speaker with neutron.openstack.common.importutils.import_object
   • If the interface is not correctly defined in the class, the error was caused by the method invocation import
     • This error would be triggered on loading the class, which is defined using abc
   • The default value would be set by the wrapper of Ryu's BGPSpeaker and go to case 3
2. If the specified class can't be imported, ImportError is thrown and the agent stops
   • In case the users specified the incorrect classes or they made typos
3. If the BGP driver class is not specified, it fallbacks to the default implementation with Ryu
   • In this class, say RyuBGPDriver, the methods defined in the interface are declared

EDIT: I would add the base class for the drivers as BGPDriver defining the methods as the interface with abc and NotImplementedError.

[devvesa]

Everything you say makes sense to me. Thanks Taku!

[tfukushima]

@jdevesa I added the simple driver. Please take a look at it.

I'm still thinking how I can add the dynamic dependency resolution.

[devvesa]

Looking at the L2 plugins, they don't seem to add anything... just let you know that you need that dependency... I'll look in more detail as well.

[devvesa]

Sorry Taku, I forgot to merge it yesterday.