Open Stack

Hi,

one other comment inline :

On Wed, Feb 5, 2014 at 5:01 PM, Robert Kukura <rkukura at redhat.com> wrote:
> On 02/05/2014 09:10 AM, Henry Gessau wrote:
>> Bob, this is fantastic, I really appreciate all the detail. A couple of
>> questions ...
>>
>> On Wed, Feb 05, at 2:16 am, Robert Kukura <rkukura at redhat.com> wrote:
>>
>>> A couple of interrelated issues with the ML2 plugin's port binding have
>>> been discussed over the past several months in the weekly ML2 meetings.
>>> These effect drivers being implemented for icehouse, and therefore need
>>> to be addressed in icehouse:
>>>
>>> * MechanismDrivers need detailed information about all binding changes,
>>> including unbinding on port deletion
>>> (https://bugs.launchpad.net/neutron/+bug/1276395)
>>> * MechanismDrivers' bind_port() methods are currently called inside
>>> transactions, but in some cases need to make remote calls to controllers
>>> or devices (https://bugs.launchpad.net/neutron/+bug/1276391)
>>> * Semantics of concurrent port binding need to be defined if binding is
>>> moved outside the triggering transaction.
>>>
>>> I've taken the action of writing up a unified proposal for resolving
>>> these issues, which follows...
>>>
>>> 1) An original_bound_segment property will be added to PortContext. When
>>> the MechanismDriver update_port_precommit() and update_port_postcommit()
>>> methods are called and a binding previously existed (whether its being
>>> torn down or not), this property will provide access to the network
>>> segment used by the old binding. In these same cases, the portbinding
>>> extension attributes (such as binding:vif_type) for the old binding will
>>> be available via the PortContext.original property. It may be helpful to
>>> also add bound_driver and original_bound_driver properties to
>>> PortContext that behave similarly to bound_segment and
>>> original_bound_segment.
>>>
>>> 2) The MechanismDriver.bind_port() method will no longer be called from
>>> within a transaction. This will allow drivers to make remote calls on
>>> controllers or devices from within this method without holding a DB
>>> transaction open during those calls. Drivers can manage their own
>>> transactions within bind_port() if needed, but need to be aware that
>>> these are independent from the transaction that triggered binding, and
>>> concurrent changes to the port could be occurring.
>>>
>>> 3) Binding will only occur after the transaction that triggers it has
>>> been completely processed and committed. That initial transaction will
>>> unbind the port if necessary. Four cases for the initial transaction are
>>> possible:
>>>
>>> 3a) In a port create operation, whether the binding:host_id is supplied
>>> or not, all drivers' port_create_precommit() methods will be called, the
>>> initial transaction will be committed, and all drivers'
>>> port_create_postcommit() methods will be called. The drivers will see
>>> this as creation of a new unbound port, with PortContext properties as
>>> shown. If a value for binding:host_id was supplied, binding will occur
>>> afterwards as described in 4 below.
>>>
>>> PortContext.original: None
>>> PortContext.original_bound_segment: None
>>> PortContext.original_bound_driver: None
>>> PortContext.current['binding:host_id']: supplied value or None
>>> PortContext.current['binding:vif_type']: 'unbound'
>>> PortContext.bound_segment: None
>>> PortContext.bound_driver: None
>>>
>>> 3b) Similarly, in a port update operation on a previously unbound port,
>>> all drivers' port_update_precommit() and port_update_postcommit()
>>> methods will be called, with PortContext properies as shown. If a value
>>> for binding:host_id was supplied, binding will occur afterwards as
>>> described in 4 below.
>>>
>>> PortContext.original['binding:host_id']: previous value or None
>>> PortContext.original['binding:vif_type']: 'unbound' or 'binding_failed'
>>> PortContext.original_bound_segment: None
>>> PortContext.original_bound_driver: None
>>> PortContext.current['binding:host_id']: current value or None
>>> PortContext.current['binding:vif_type']: 'unbound'
>>> PortContext.bound_segment: None
>>> PortContext.bound_driver: None
>>>
>>> 3c) In a port update operation on a previously bound port that does not
>>> trigger unbinding or rebinding, all drivers' update_port_precommit() and
>>> update_port_postcommit() methods will be called with PortContext
>>> properties reflecting unchanged binding states as shown.
>>>
>>> PortContext.original['binding:host_id']: previous value
>>> PortContext.original['binding:vif_type']: previous value
>>> PortContext.original_bound_segment: previous value
>>> PortContext.original_bound_driver: previous value
>>> PortContext.current['binding:host_id']: previous value
>>> PortContext.current['binding:vif_type']: previous value
>>> PortContext.bound_segment: previous value
>>> PortContext.bound_driver: previous value
>>>
>>> 3d) In a the port update operation on a previously bound port that does
>>> trigger unbinding or rebinding, all drivers' update_port_precommit() and
>>> update_port_postcommit() methods will be called with PortContext
>>> properties reflecting the previously bound and currently unbound binding
>>> states as shown. If a value for binding:host_id was supplied, binding
>>> will occur afterwards as described in 4 below.
>>>
>>> PortContext.original['binding:host_id']: previous value
>>> PortContext.original['binding:vif_type']: previous value
>>> PortContext.original_bound_segment: previous value
>>> PortContext.original_bound_driver: previous value
>>> PortContext.current['binding:host_id']: new or current value
>>> PortContext.current['binding:vif_type']: 'unbound'
>>> PortContext.bound_segment: None
>>> PortContext.bound_driver: None
>>>
>>> 4) If a port create or update operation triggers binding or rebinding,
>>> it is attempted after the initial transaction is processed and committed
>>> as described in 3 above. The binding process itself is just as before,
>>> except it happens after and outside the transaction. Since binding now
>>> occurs outside the transaction, its possible that multiple threads or
>>> processes could concurrently attempt to bind the same port, although
>>> this is should be a rare occurrence. Rather than trying to prevent this
>>> with some sort of distributed lock or complicated state machine,
>>> concurrent attempts to bind are allowed to proceed in parallel. When a
>>> thread completes its attempt to bind (either successfully or
>>> unsuccessfully) it then performs a second transaction to update the DB
>>> with the result of its binding attempt. When doing so, it checks to see
>>> if some other thread has already committed relevant changes to the port
>>> between the two transactions. There are three possible cases:
>>>
>>> 4a) If the thread's binding attempt succeeded, and no other thread has
>>> committed either a new binding or changes that invalidate this thread's
>>> new binding between the two transactions, the thread commits its own
>>> binding results, calling all drivers' update_port_precommit() and
>>> update_port_postcommit() methods with PortContext properties reflecting
>>> the new binding as shown. It then returns the updated port dictionary to
>>> the caller.
>>>
>>> PortContext.original['binding:host_id']: previous value
>>> PortContext.original['binding:vif_type']: 'unbound'
>>> PortContext.original_bound_segment: None
>>> PortContext.original_bound_driver: None
>>> PortContext.current['binding:host_id']: previous value
>>
>> Are you not expecting/allowing the host_id to change in this scenario? Why?
>
> Correct. This thread's initial transaction has already committed update
> of binding:host_id if that is what's triggering this thread to bind. If
> another thread committed a change to binding:host_id while this thread
> was binding, that is covered in 4c.
>
>>
>>> PortContext.current['binding:vif_type']: new value
>>> PortContext.bound_segment: new value
>>> PortContext.bound_driver: new value
>>>
>>> 4b) If the thread's binding attempt either succeeded or failed, but some
>>> other thread has committed a new successful binding between the two
>>> transactions, the thread returns a port dictionary with attributes based
>>> on the DB state from the new transaction, including the other thread's
>>> binding and any other port state changes. No further calls to mechanism
>>> drivers are needed here since they are the responsibility of the other
>>> thread that bound the port.
>>>
>>> 4c) If some other thread committed changes to the port's
>>> binding-relevant state but has not committed a successful binding, then
>>> this thread attempts to bind again using that updated state, repeating 4.

When nova will create a port, the port returned will be unbound. Nova
gets port info (vif_type, port dict) by calling port-list with the
neutron client, but the port could potentially not be bound yet
Don't you think that binding the port to a vif_type outside the
create_port could potentially create such a race conditions?

>>>
>>> 5) Port deletion no longer does anything special to unbind the port. All
>>> drivers' delete_port_precommit() and delete_port_postcommit() methods
>>> are called with PortContext properties reflecting the binding state
>>> before deletion as shown.
>>>
>>> PortContext.original: None
>>> PortContext.original_bound_segment: None
>>> PortContext.original_bound_driver: None
>>> PortContext.current['binding:host_id']: previous value or None
>>> PortContext.current['binding:vif_type']: previous value
>>> PortContext.bound_segment: previous value
>>> PortContext.bound_driver: previous value
>>
>> Could this part of the port deletion also be done by port update?
>
> I had considered that, but that would then involve two separate
> transacitons - 1st the port update to unbind and then the port delete.
> Some other thread might rebind the port between the two, so a retry loop
> would be needed. This loop is not needed if the same transaction unbinds
> and deletes the port.
>
> -Bob
>
>>
>>>
>>> 6) In order to ensure successful bindings are created and returned
>>> whenever possible, the get port and get ports operations also attempt to
>>> bind the port as in 4 above when binding:host_id is available but there
>>> is no existing successful binding in the DB.
>>>
>>> 7) We can either eliminate MechanismDriver.unbind_port(), or call it on
>>> the previously bound driver within the transaction in 3d and 5 above. If
>>> we do keep it, the old binding state must be consistently reflected in
>>> the PortContext as either current or original state, TBD. Since all
>>> drivers see unbinding as a port update where current_bound_segment is
>>> None and original_bound_segment is not None, calling unbind_port() seems
>>> redundant.
>>>
>>> 8) If bindings shouldn't spontaneously become invalid, maybe we can
>>> eliminate MechanismDriver.validate_bound_port().
>>>
>>>
>>> I've provided a lot of details, and the above may seem complicated. But
>>> I think its actually much more consistent and predictable than the
>>> current port binding code, and implementation should be straightforward.
>>>
>>> -Bob
>>
>> _______________________________________________
>> OpenStack-dev mailing list
>> OpenStack-dev at lists.openstack.org
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev