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

[Dumitru Ceara]

On 6/11/20 10:39 AM, Numan Siddique wrote:
> 
> 
> On Thu, Jun 11, 2020 at 2:01 PM Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>> wrote:
> 
>     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>
>     > <mailto: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
>     updates.
> 
> 
> Agree. This patch makes sense.
> 
> I'll refine it further in the I-P patches to set only for those which
> are required.
> Or maybe even if we set for all, it should not cause any flow
> computation in the flow output stage
> if the tracked datapaths are empty.
> 
> Thanks
> Numan
> 

Looking forward to it, thanks!