On 8/9/22 08:46, Frode Nordahl wrote: > On Sat, Aug 6, 2022 at 1:49 AM Han Zhou <[email protected]> wrote: >> >> >> >> On Thu, Aug 4, 2022 at 7:09 AM Dumitru Ceara <[email protected]> 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 <[email protected]> >>>> --- >>> >>> 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 <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
