<div dir="ltr">Thanks for the update Carl!<div><br></div><div>I spent some time trying to make the refactored base plugin class with the IPAM driver.</div><div>I have been partially successful. The results are so far in [1] (it's a patch that applies on top of Pavel's topmost patch); this patch will be soon split into smaller patch which I will apply to gerrit reviews in flight.</div><div><br></div><div>I have some additional comments inline, but I would like to raise some technical points which might need attention<br></div><div><br></div><div>* the ML2 plugin overrides several methods from the base "db" class. From what I gather from unit tests results, we have not yet refactored it. I think to provide users something usable in Kilo we should ensure the ML2 plugin at least works with the IPAM driver.</div><div>* the current refactoring has ipam-driver-enabled and non-ipam-driver-enabled version of some API operations. While this the less ugly way to introduce the driver and keeping at the same time the old logic, it adds quite a bit of code duplication. I wonder if there is any effort we can make without too much yak shaving to reduce that code duplication, because in this conditions I suspect it would a hard sell to the Neutron core team.</div><div><br></div><div>Have a nice one,</div><div>Salvatore</div><div><br></div><div>[1] <a href="https://gist.github.com/salv-orlando/22e524979fe7e2d18d2d">https://gist.github.com/salv-orlando/22e524979fe7e2d18d2d</a><br><div class="gmail_extra"><br><div class="gmail_quote">On 14 March 2015 at 17:52, Carl Baldwin <span dir="ltr"><<a href="mailto:carl@ecbaldwin.net" target="_blank">carl@ecbaldwin.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Here is an update from our discussion this morning in IRC [1].  The<br>
discussion involved mainly Pavel, Salvatore, and me.<br>
<br>
We first discussed testing the integration of Salvatore's work [2] on<br>
the reference driver with Pavel's work to load a driver [3] and<br>
refactor the db_base_plugin [4].  Pavel will continue the integration<br>
efforts which he has already begun.  The final patch [4] makes the<br>
reference driver the default IPAM in Neutron so that it is being run<br>
in the check queue.  At some point before this patch merges, that will<br>
be moved to a new patch so that merging this patch will not make the<br>
new IPAM driver the default in Neutron.<br></blockquote><div><br></div><div>From a unit test perspective, it is important that we test both code with and without the IPAM driver, as we should strive to be as close as possible to 100% code coverage - in particular since the refactoring, in its current form adds quite a bit of code to neutron.db.db_base_plugin_v2 (possibly with a non negligible amount of duplicated logic).</div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Pavel's final patch [4] will merge when the tests pass with the driver<br>
as default. </blockquote><div><br></div><div>In order to avoid raising some eyebrows, it might be worth mentioning that I think you meant to write that the patch will be *ready to merge* when the tests pass with the IPAM driver as default!</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> We hope to hit this point soon.  At that point, we will<br>
create a non-voting job to run the new IPAM driver as default on all<br>
new patches.  This will give us many more test runs to work out bugs<br>
for the Kilo release and will help us to gauge the eventual readiness<br>
of the new driver to become the default IPAM in Neutron.  This latter<br>
step will not happen until the Liberty time frame.<br></blockquote><div><br></div><div>The goal for Liberty should be to deprecate the baked-in IPAM logic, aiming at its removal for M.</div><div>Non-voting jobs are useful, but mostly for the people interested in it. Other contributors tend to ignore what does not cause them a build failure. So - in my opinion - the idea should be that as soon as we're confident the IPAM driver is reliable enough, we'll switch it on either on the mysql or the postgres job.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
In Kilo, the new IPAM driver will only support greenfield deployments<br>
and will not support using multiple drivers simultaneously.  There<br>
will be no migration from the old to the new until Liberty.  </blockquote><div><br></div><div>I will leave this decision to the rest of the community, but I believe that a data migration script, similar to the one that was done for ML2, should be doable in a fairly easy way.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">However,<br>
a migration plan is required in order to eventually deprecate and<br>
remove the baked-in IPAM solution.  We will continue to evaluate use<br>
cases around using multiple drivers in the same deployment (e.g.<br>
ability to set the driver per subnet or support some sort of flavors).<br>
<br>
At the point that the non-voting check job is stable, it will be moved<br>
to voting.  When the new driver graduates to be the default driver in<br>
Neutron, the separate gate check job will no longer be necessary and<br>
will be removed.<br>
<br>
Please let me know if I left out any significant detail.<br>
<br>
Carl<br>
<br>
[1]<a href="http://eavesdrop.openstack.org/irclogs/%23openstack-neutron/%23openstack-neutron.2015-03-13.log" target="_blank">http://eavesdrop.openstack.org/irclogs/%23openstack-neutron/%23openstack-neutron.2015-03-13.log</a><br>
at 2015-03-13T15:08:15<br>
[2] <a href="https://review.openstack.org/#/c/150485/" target="_blank">https://review.openstack.org/#/c/150485/</a><br>
[3] <a href="https://review.openstack.org/#/c/147479/" target="_blank">https://review.openstack.org/#/c/147479/</a><br>
[4] <a href="https://review.openstack.org/#/c/153236/" target="_blank">https://review.openstack.org/#/c/153236/</a><br>
<div><div class="h5"><br>
On Mon, Mar 9, 2015 at 4:44 AM, Salvatore Orlando <<a href="mailto:sorlando@nicira.com">sorlando@nicira.com</a>> wrote:<br>
> Aloha everybody!<br>
><br>
> This is another long email, so here's the summary for people with 5-minute<br>
> or less attention span:<br>
><br>
> The IPAM reference driver is almost ready [1]. Unfortunately letting the<br>
> driver doing allocation pools required a few major changes, so the latest<br>
> patchset is rather big. I would like reviewers to advice on how they prefer<br>
> to split it out in multiple patches.<br>
><br>
> I also had to add an additional method to the subnet interface [2]<br>
><br>
> However, I am not sure we are doing the right thing wrt the introduction of<br>
> the driver. I think we might need to think about it more thoroughly, and<br>
> possibly introduce a concept of "allocator". Otherwise if we switch an<br>
> existing deployment to the driver, it's very likely things will just break.<br>
><br>
> So here's the detailed part.<br>
> There are only a few bits needed to complete the IPAM reference driver:<br>
> - A thorough unit test case for the newly introduced "subnet manager", which<br>
> provides db apis.<br>
> - Some more unit tests for driver behaviour<br>
> - Logic for handling allocation pools update (which pretty much needs to be<br>
> copied over from db base plugin with minor changes)<br>
> - Rework the synchronisation between the ipam object and the neutron db<br>
> object. The current one is very error prone.<br>
><br>
> While dealing with management of allocation pools, I found out that it's not<br>
> convenient for the IPAM driver to rely on Neutron's DB allocation pools.<br>
> This is a bit of a chicken and egg problem, since:<br>
> A) Neutron needs to call the driver to define allocation pools<br>
> B) The driver needs neutron to define allocation pools before creating<br>
> availability ranges<br>
> C) Neutron cannot create allocation pools if the driver has not defined them<br>
> I therefore decided, reluctantly, to add more data duplication - introducing<br>
> IPAM tables for allocation pools and availability ranges. The latter is<br>
> meant to replace Neutron's one when the IPAM driver is enabled, whereas the<br>
> former is pure data duplication. There is also an association table between<br>
> neutron subnets and ipam objects for the same subnet; I decided to do so to<br>
> not do further duplication.<br>
> I dislike this data separation; on the other hand the only viable<br>
> alternative would be to give the IPAM driver full control over neutron's<br>
> database tables for IP Allocation and Subnet allocation pools. While this is<br>
> feasible, I think it would be even worse to give code which can potentially<br>
> be 3rd party control over data structures used by the Neutron API.<br>
><br>
> As a result, the patch is different and larger. I would like to split it to<br>
> make it simpler to review, but rather than just doing that of my own accord<br>
> I would like to ask reviewers how they would prefer to have it split. At the<br>
> end of the day I'm not the one who has to spend a lot of time reviewing that<br>
> code.<br>
><br>
> Nevertheless, I think I've realised our refactoring approach is kind of<br>
> flawed, since it might work for greenfields deployments, but I don't think<br>
> it will work for brownfields deployments. Also, switching between drivers is<br>
> pretty much impossible (but we were already aware of that and we agreed to<br>
> address this in the future).<br>
> The decorator approach currently taken in [3] allows to either use the<br>
> driver or not - which means that if an operator decides to switch to the<br>
> IPAM driver, then all allocation data for existing subnets would be simply<br>
> lost, unless we provide a solution for migrating data. This is feasible and<br>
> rather trivial, but implies an additional management step.<br>
><br>
> An alternative solution would be to introduce a concept of "allocator" for<br>
> subnets. Such information should be stored in the database. It could point<br>
> to an IPAM driver or to nothing. In the latter case it would simply instruct<br>
> to use the "usual" baked-in IPAM code. This would allow us to make the<br>
> driver work on existing deployments, and pave the way for multiple drivers.<br>
> Indeed in this way, the new IPAM driver will be used for subnets created<br>
> after enabling it, whereas existing subnets will keep using the existing<br>
> logic. This should also make safe cases in which operators revert the<br>
> decision of using the IPAM driver. Additionally, administrative APIs might<br>
> be provided to migrate existing subnets to/from the driver. However, when<br>
> adopting solutions like this, it is important to pay extra care in ensuring<br>
> that the database does not start relying on the current configuration, and<br>
> therefore we need to find a way to decouple the allocator from the actual<br>
> IPAM driver, which should represent its realization.<br>
><br>
> Any feedback is very appreciated as usual.<br>
><br>
> Salvatore<br>
><br>
> [1] <a href="https://review.openstack.org/#/c/150485/" target="_blank">https://review.openstack.org/#/c/150485/</a><br>
> [2] <a href="https://review.openstack.org/#/c/150485/6/neutron/ipam/driver.py" target="_blank">https://review.openstack.org/#/c/150485/6/neutron/ipam/driver.py</a><br>
> [3]<br>
> <a href="https://review.openstack.org/#/c/153236/15/neutron/db/db_base_plugin_v2.py" target="_blank">https://review.openstack.org/#/c/153236/15/neutron/db/db_base_plugin_v2.py</a><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
</div></div>> __________________________________________________________________________<br>
> OpenStack Development Mailing List (not for usage questions)<br>
> Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
> <a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
><br>
<br>
__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</blockquote></div><br></div></div></div>