[ovs-dev] [PATCH ovn] ovn-controller: Fix I-P for SB Port_Binding and OVS Interface.

Dumitru Ceara dceara at redhat.com
Thu Jun 11 08:31:23 UTC 2020

On 6/10/20 7:58 PM, Numan Siddique wrote:
> On Wed, Jun 10, 2020 at 10:16 PM Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>> wrote:
>     The commit that introduced incremental processing for Port_Binding and
>     OVS Interface in the I-P runtime_data node covered most cases but two
>     were missed:
>     1. If a Port_Binding was already claimed by the local hypervisor when
>     ovn-controller starts, binding_handle_port_binding_changes doesn't
>     correctly set the "changed" variable causing en_runtime_data node to
>     go to EN_VALID instead of EN_UPDATED. Due to this update_sb_monitors()
>     is skipped in that run and ovn-controller does not register for
>     updates regarding the datapath containing the Port_Binding.
>     2. If a Port_Binding was already claimed by the local hypervisor when
>     ovn-controller starts, but the underlying OVS interface was removed in
>     the meantime, handle_updated_vif_lport() would fail the assertion that a
>     local_binding should exist in memory.
>     To address the first issue, we now explicitly track changes to the
>     binding
>     context local_lport and local_lport_ids sets. If these change during
>     incremental processing of the runtime_data OVS_Interface and
>     SB_Port_Binding input nodes then the runtime_data node should change
>     state to EN_UPDATED.
>     For the second issue, we now allow the case when a stale port_binding is
>     released.
>     Also, added an explicit non_vif_ports_changed variable to
>     binding_ctx_out to track if other types of Port_Bindings
>     have been changed in the current run. This kind of update should also
>     cause runtime_data to move to EN_UPDATED such that update_sb_monitors()
>     gets executed.
> Thanks Dumitru  for fixing this. I applied this patch to master and
> branch-20.06.

Thanks Numan!

> When other types of Port_Bindings get updated, I don't think there is a
> need to
> call update_sb_monitors(). Because we don't do conditional monitoring on
> other type of Port_Bindings and the fact that 'non_vif_ports_changed'
> gets set
> to true, indicate that the ovn-controller received the updates for other
> type of
> Port_Bindings.

Actually, at least for some non-vif port bindings like LP_PATCH we need
to call update_sb_monitors() because local_datapaths might be updated.
Some SB records (e.g., Logical_Flow) are filtered on datapath id and if
we don't call update_sb_monitors() SB won't send them to us.

It's true that we might have to refine in which cases we need to set
non_vif_ports_changed. For LP_PATCH for example we don't really need to
because patch port bindings are also added to local_lport_ids so
local_lport_ids_changed will be true. But for now I just decided to set
it in all places where we had "*changed = true" for non-vif port binding


> I'll rebase my I-P patch series and submit v12.
> Thanks
> Numan

More information about the dev mailing list