Open Stack

As pointed out by Pavel in yesterday's meeting, the refactor [1] cannot
assume that the DB transaction for IPAM operations will occur in a scope
different from the one for performing the API operation.
This is because most plugins, including ML2, use inheritance to perform
further operations on database objects, and include the call to super
within a database transaction. These operations might include, for
instance, processing extensions or setting up mappings with backend
resources.

As the IPAM driver is called in NeutronDbPluginV2, this call happens while
another transaction is typically in progress. Initiating a separate session
within the IPAM driver causes the outer transaction to fail.
I do not think there is a lot we can do about this at the moment, unless we
agree on a more pervasive refactoring:
In this case we should:
1) Assume that IPAM enabled methods in NeutronDbPluginV2 get IPAM data from
the caller (the actual plugin class in this case)
    This will ensure proper transaction scoping. Perhaps it will make also
the refactoring of NeutronDbPluginV2 slightly cleaner, but will require
explicit support for pluggable IPAM from the plugin.
    Also, this will imply that, unless we do some really hackish things,
the ability of retrying on MAC collisions will be lost (that thing does not
really work with REPEATABLE READ anyway).
2) IPAM-enable the ML2 plugin (at least). This means that IPAM calls for
allocating subnets and IPs should happen before the actual DB transaction
for creating ports or subnet is performed (in the case of a subnet we'd
also have a post-commit action for associating the IPAM subnet with a
neutron ID for future retrieval)  I am sure we will get asked to do this as
a part of the mechanism driver framework - but I'm not yet sure about how
to do that. For the time being an explicit call to the IPAM driver might be
acceptable, hopefully.

Otherwise, we'd just made the IPAM driver session aware. This implies
changes to the Pool and Subnet interface to accept an optional session
parameter.
The above works and is probably quicker from an implementation perspective.
However, doing so somehow represents a failure of the pluggable IPAM effort
as total separation between IPAM and API operation processing was one of
our goals. Also, for drivers with a remote backend, remote calls will be
made within a DB transaction, which is another thing we wanted to avoid.

And finally, there is the third option. I know IPAM contributors do not
even want to hear it... but the third option is to enjoy 6 more months to
come up with a better implementation which does not add any technical debt.
In Kilo from the IPAM side we're already introducing subnet pools, so it
won't be a total failure!

Salvatore

[1] https://review.openstack.org/#/c/153236/

On 17 March 2015 at 15:32, Salvatore Orlando <sorlando at nicira.com> wrote:

>
>
> On 17 March 2015 at 14:44, Carl Baldwin <carl at ecbaldwin.net> wrote:
>
>>
>> On Mar 15, 2015 6:42 PM, "Salvatore Orlando"
>> > * 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.
>>
>> Yes, agreed.
>>
>> > * 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.
>>
>> This is a good thing to bring up.  It is a difficult trade off.  On one
>> hand, the way it has been done makes it easy to review and see that the
>> existing implementation has not been disturbed reducing the short term
>> risk.  On the other hand, if left the way it is indefinitely, it will be a
>> maintenance burden.  Given the current timing, could we take a two-phased
>> approach?  First, merge it with duplication and immediately create a follow
>> on patch to deduplicate the code to merge when that is ready?
>>
> The problem with duplication is that it will make maintenance troubling.
> For instance if a bug is found in _test_fixed_ips the bug fixer will have
> to know that the same fix must be applied to _test_fixed_ips_for_ipam as
> well. I'm not sure we can ask contributors to fix bugs in two places. But
> if we plan to deduplicate with a follow-up patch I am on board. I know we'd
> have the cycles for that.
> Said that, the decision lies with the rest of core team (Carl's and mine
> votes do not count here!). If I were a reviewer I'd evaluate the tradeoff
> between of the benefits brought buythis new feature, the risks of the
> refactoring (which, as you say, are rather low), and the maintenance burden
> (aka technical debt) it introduces.
>
> I'm kind of sure the PTL would like to outline all of this, including
> extensive details about testing, in an etherpad so that a call could be
> made by the end of week.
> I am already taking care of that.
>
> Salvatore
>
> Carl
>>
>> __________________________________________________________________________
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe:
>> OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20150320/03b016e0/attachment.html>