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

[Numan Siddique]

On Thu, Jun 11, 2020 at 2:01 PM Dumitru Ceara <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>> 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




> Regards,
> Dumitru
>
> >
> > I'll rebase my I-P patch series and submit v12.
> >
> > Thanks
> > Numan
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>