On Wed, Aug 17, 2022 at 8:27 AM Dumitru Ceara <dce...@redhat.com> wrote:
>
> On 8/9/22 08:46, Frode Nordahl wrote:
> > On Sat, Aug 6, 2022 at 1:49 AM Han Zhou <hz...@ovn.org> wrote:
> >>
> >>
> >>
> >> On Thu, Aug 4, 2022 at 7:09 AM Dumitru Ceara <dce...@redhat.com> wrote:
> >>>
> >>> On 7/25/22 23:34, Han Zhou wrote:
> >>>> Also remove the reset mechanism when DB is reconnected, because at DB
> >>>> reconnection the data in IDL would not reset.
> >>>>
> >>>> Signed-off-by: Han Zhou <hz...@ovn.org>
> >>>> ---
> >>>
> >>> Hi Han,
> >>>
> >>> I noticed that with this change the "VIF plugging" test starts failing
> >>> when running with conditional monitoring disabled:
> >>>
> >>> 777: ovn-controller - VIF plugging -- ovn-northd -- parallelization=no
> >>> -- ovn_monitor_all=yes ok
> >>> 775: ovn-controller - VIF plugging -- ovn-northd --
parallelization=yes
> >>> -- ovn_monitor_all=yes ok
> >>> 778: ovn-controller - VIF plugging -- ovn-northd -- parallelization=no
> >>> -- ovn_monitor_all=no FAILED (ovs-macros.at:255)
> >>> 776: ovn-controller - VIF plugging -- ovn-northd --
parallelization=yes
> >>> -- ovn_monitor_all=no FAILED (ovs-macros.at:255)
> >>
> >> I am terribly sorry for this. I guess I must have forgotten to run the
test or check the test result after adding this patch. No idea how that
could have happened :(
> >> This failure is because the new approach is more strict than before,
which requires 20 delay countdowns with real engine-updates instead of just
10 main loop iterations. Adding the ignore-startup-delay command fixes it:
> >>
> >> --- 8>< ---------------------------------------------------- ><8
--------------
> >> diff --git a/tests/ovn.at b/tests/ovn.at
> >> index 3ba6ced4e..e2b523b67 100644
> >> --- a/tests/ovn.at
> >> +++ b/tests/ovn.at
> >> @@ -31606,6 +31606,7 @@ OVS_WAIT_UNTIL([
> >> test x43 = x$(as hv1 ovs-vsctl get Interface ${iface1_uuid}
mtu_request)
> >> ])
> >>
> >> +as hv1 check ovn-appctl -t ovn-controller debug/ignore-startup-delay
> >> # Check that pointing requested-chassis somewhere else will unplug
the port
> >> check ovn-nbctl --wait=hv set Logical_Switch_Port lsp1 \
> >> options:requested-chassis=non-existent-chassis
> >> @@ -31613,6 +31614,7 @@ OVS_WAIT_UNTIL([
> >> ! as hv1 ovs-vsctl get Interface ${iface1_uuid} _uuid
> >> ])
> >>
> >> +as hv2 check ovn-appctl -t ovn-controller debug/ignore-startup-delay
> >> # Check that removing an lport will unplug it
> >> AT_CHECK([test x${iface2_uuid} = x$(as hv2 ovs-vsctl get Interface
${iface2_uuid} _uuid)], [0], [])
> >> check ovn-nbctl --wait=hv lsp-del ${lsp2_uuid}
> >>
------------------------------------------------------------------------------------
> >> I will wait for complete review comments before sending v3 for this
fix.
> >
> > I can confirm that this fixes the failing test. For rigor I also ran a
> > version of the test that artificially generated 20 updates prior to
> > attempting to unplug anything and that was also successful, and I see
> > that both the countdown and timer works as intended.
> >
> >> Thanks,
> >> Han
> >>
> >>>
> >>>> controller/ovn-controller.c | 1 -
> >>>> controller/vif-plug.c | 20 ++++++--------------
> >>>> 2 files changed, 6 insertions(+), 15 deletions(-)
> >>>>
> >>>> diff --git a/controller/ovn-controller.c
b/controller/ovn-controller.c
> >>>> index 8fc554201..8bb18fc23 100644
> >>>> --- a/controller/ovn-controller.c
> >>>> +++ b/controller/ovn-controller.c
> >>>> @@ -3874,7 +3874,6 @@ main(int argc, char *argv[])
> >>>> if (!new_ovnsb_cond_seqno) {
> >>>> VLOG_INFO("OVNSB IDL reconnected, force
recompute.");
> >>>> engine_set_force_recompute(true);
> >>>> - vif_plug_reset_idl_prime_counter();
> >>>> }
> >>>> ovnsb_cond_seqno = new_ovnsb_cond_seqno;
> >>>> }
> >>>> diff --git a/controller/vif-plug.c b/controller/vif-plug.c
> >>>> index c6fbe7e59..38348bf54 100644
> >>>> --- a/controller/vif-plug.c
> >>>> +++ b/controller/vif-plug.c
> >>>> @@ -532,22 +532,14 @@ vif_plug_handle_iface(const struct
ovsrec_interface *iface_rec,
> >>>> * completeness of the initial data downloading we need this
counter so that we
> >>>> * do not erronously unplug ports because the data is just not
loaded yet.
> >>>> */
> >>>> -#define VIF_PLUG_PRIME_IDL_COUNT_SEEED 10
> >>>> -static int vif_plug_prime_idl_count =
VIF_PLUG_PRIME_IDL_COUNT_SEEED;
> >>>> -
> >>>> -void
> >>>> -vif_plug_reset_idl_prime_counter(void)
> >>>> -{
> >>>> - vif_plug_prime_idl_count = VIF_PLUG_PRIME_IDL_COUNT_SEEED;
> >>>> -}
> >>>> -
> >>>
> >>> We should remove this from the .h file too.
> >
> > +1
> >
> >>>
> >>>> void
> >>>> vif_plug_run(struct vif_plug_ctx_in *vif_plug_ctx_in,
> >>>> struct vif_plug_ctx_out *vif_plug_ctx_out)
> >>>> {
> >>>> - if (vif_plug_prime_idl_count && --vif_plug_prime_idl_count > 0)
{
> >>>> - VLOG_DBG("vif_plug_run: vif_plug_prime_idl_count=%d, will
not unplug "
> >>>> - "ports in this iteration.",
vif_plug_prime_idl_count);
> >>>> + bool delay_plug = daemon_started_recently();
> >>>> + if (delay_plug) {
> >>>> + VLOG_DBG("vif_plug_run: daemon started recently, will not
unplug "
> >>>> + "ports in this iteration.");
> >>>> }
> >>>>
> >>>> if (!vif_plug_ctx_in->chassis_rec) {
> >>>> @@ -557,7 +549,7 @@ vif_plug_run(struct vif_plug_ctx_in
*vif_plug_ctx_in,
> >>>> OVSREC_INTERFACE_TABLE_FOR_EACH (iface_rec,
> >>>> vif_plug_ctx_in->iface_table) {
> >>>> vif_plug_handle_iface(iface_rec, vif_plug_ctx_in,
vif_plug_ctx_out,
> >>>> - !vif_plug_prime_idl_count);
> >>>> + !delay_plug);
> >>>> }
> >>>>
> >>>> struct sbrec_port_binding *target =
> >>>> @@ -573,7 +565,7 @@ vif_plug_run(struct vif_plug_ctx_in
*vif_plug_ctx_in,
> >>>> enum en_lport_type lport_type = get_lport_type(pb);
> >>>> if (lport_type == LP_VIF) {
> >>>> vif_plug_handle_lport_vif(pb, vif_plug_ctx_in,
vif_plug_ctx_out,
> >>>> - !vif_plug_prime_idl_count);
> >>>> + !delay_plug);
> >>>> }
> >>>> }
> >>>> sbrec_port_binding_index_destroy_row(target);
> >>>
> >>> Regards,
> >>> Dumitru
> >>>
> >
> > Otherwise this LGTM
> >
>
> Same here, if this is the only change to the patch, feel free to add my
> ack to v3:
>
> Acked-by: Dumitru Ceara <dce...@redhat.com>
>
Thanks! Added ACK from you both in v3.
Han
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
