On 6/9/25 10:42 AM, Smirnov Aleksandr (K2 Cloud) wrote: > Hi Dumitru, >
Hi Aleksandr, > Since Felix's update was merged I repost my patch again: > https://patchwork.ozlabs.org/project/ovn/list/?series=459843 > > It's only changed one line: > Replace sequence number comparison with call of daemon_started_recently() > This change should resolve all concerns about possible fall into > infinite waiting. > > Could you please continue with review? > Sorry for the delay, I've added this series to my TODO list and will try to review it tomorrow. Regards, Dumitru > Thank you, > > Alexander > > On 6/2/25 3:18 PM, Dumitru Ceara wrote: >> On 6/2/25 2:01 PM, Smirnov Aleksandr (K2 Cloud) wrote: >>> Hello Dumitru, >>> >> Hi Aleksandr, >> >>> I want to ask you and all guys to temporary stop this review. I think I >>> need to wait until Felix update merged. >>> (https://patchwork.ozlabs.org/project/ovn/patch/67010dca0a8dcc390e873349d662c9163e01050f.1746623091.git.felix.huettner@stackit.cloud/) >>> >> Thanks for letting us know. I moved the whole series to "Deferred" in >> patchwork: >> >> https://patchwork.ozlabs.org/project/ovn/list/?series=455751&state=* >> >> Please feel free to repost once Felix's changes are merged. >> >>> After that I will rely on new behavior of daemon_started_recently() >>> function. Apart of my approach Felix does two extra guard checks (by >>> time and by number of loop runs). This must remove all worries raised >>> against my code which only relies on seqnum comparison. Thank you, >> Sounds good, thanks again for working on this! >> >> Regards, >> Dumitru >> >>> Alexander >>> >>> On 5/21/25 11:47 AM, Dumitru Ceara wrote: >>>> On 5/7/25 1:05 PM, Aleksandr Smirnov wrote: >>>>> Delay initial flow upload to OVS until all monitored updates received. >>>>> This is a kind of replacement of the wait-before-clear parameter. >>>>> Instead of waiting a certain amount of time we will wait >>>>> until the final monitored update comes from SB DB. >>>>> An event we watch in the controller's main loop is current condition >>>>> sequence number compared vs expected condition sequence number. >>>>> If they are not equal we still have to receive updates in response >>>>> to recent monitor condition change request. This check makes sense >>>>> only in code that lies after update_sb_monitors() function call. >>>>> >>>>> Note, this update will only work if wait-before-clear == 0, >>>>> i.e. you can still rely on wait-before-clear behavior. >>>>> >>>>> Signed-off-by: Aleksandr Smirnov <alekssmirnov@k2.cloud> >>>>> Acked-by: Numan Siddique <num...@ovn.org> >>>>> --- >>>> Hi Aleksandr, >>>> >>>> Thanks for the new revision! I think however that it would be great if >>>> you could reply to Ilya's questions/comments from v1. In case you >>>> missed it: >>>> >>>> https://mail.openvswitch.org/pipermail/ovs-dev/2025-May/423314.html >>>> >>>> Thanks, >>>> Dumitru >>>> >>>>> controller/ofctrl.c | 29 ++++++++++++++++- >>>>> controller/ofctrl.h | 3 +- >>>>> controller/ovn-controller.c | 4 ++- >>>>> tests/ovn-controller.at | 62 +++++++++++++++++++++++++++++++++++++ >>>>> 4 files changed, 95 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/controller/ofctrl.c b/controller/ofctrl.c >>>>> index 4a3d35b97..304b9bbc8 100644 >>>>> --- a/controller/ofctrl.c >>>>> +++ b/controller/ofctrl.c >>>>> @@ -349,6 +349,9 @@ static uint64_t cur_cfg; >>>>> /* Current state. */ >>>>> static enum ofctrl_state state; >>>>> >>>>> +/* Release wait before clear stage. */ >>>>> +static bool wait_before_clear_proceed = false; >>>>> + >>>>> /* The time (ms) to stay in the state S_WAIT_BEFORE_CLEAR. Read from >>>>> * external_ids: ovn-ofctrl-wait-before-clear. */ >>>>> static unsigned int wait_before_clear_time = 0; >>>>> @@ -446,6 +449,7 @@ run_S_NEW(void) >>>>> struct ofpbuf *buf = ofpraw_alloc(OFPRAW_NXT_TLV_TABLE_REQUEST, >>>>> rconn_get_version(swconn), 0); >>>>> xid = queue_msg(buf); >>>>> + wait_before_clear_proceed = false; >>>>> state = S_TLV_TABLE_REQUESTED; >>>>> } >>>>> >>>>> @@ -638,6 +642,14 @@ error: >>>>> static void >>>>> run_S_WAIT_BEFORE_CLEAR(void) >>>>> { >>>>> + if (wait_before_clear_time == 0) { >>>>> + if (wait_before_clear_proceed) { >>>>> + state = S_CLEAR_FLOWS; >>>>> + } >>>>> + >>>>> + return; >>>>> + } >>>>> + >>>>> if (!wait_before_clear_time || >>>>> (wait_before_clear_expire && >>>>> time_msec() >= wait_before_clear_expire)) { >>>>> @@ -2695,11 +2707,26 @@ ofctrl_put(struct ovn_desired_flow_table >>>>> *lflow_table, >>>>> uint64_t req_cfg, >>>>> bool lflows_changed, >>>>> bool pflows_changed, >>>>> - struct tracked_acl_ids *tracked_acl_ids) >>>>> + struct tracked_acl_ids *tracked_acl_ids, >>>>> + bool monitor_cond_complete) >>>>> { >>>>> static bool skipped_last_time = false; >>>>> static uint64_t old_req_cfg = 0; >>>>> bool need_put = false; >>>>> + >>>>> + if (state == S_WAIT_BEFORE_CLEAR) { >>>>> + /* If no more monitored condition changes expected >>>>> + Release wait before clear stage and skip >>>>> + over poll wait. */ >>>>> + if (monitor_cond_complete) { >>>>> + wait_before_clear_proceed = true; >>>>> + poll_immediate_wake(); >>>>> + } >>>>> + >>>>> + skipped_last_time = true; >>>>> + return; >>>>> + } >>>>> + >>>>> if (lflows_changed || pflows_changed || skipped_last_time || >>>>> ofctrl_initial_clear) { >>>>> need_put = true; >>>>> diff --git a/controller/ofctrl.h b/controller/ofctrl.h >>>>> index d1ee69cb0..76e2fbece 100644 >>>>> --- a/controller/ofctrl.h >>>>> +++ b/controller/ofctrl.h >>>>> @@ -68,7 +68,8 @@ void ofctrl_put(struct ovn_desired_flow_table >>>>> *lflow_table, >>>>> uint64_t nb_cfg, >>>>> bool lflow_changed, >>>>> bool pflow_changed, >>>>> - struct tracked_acl_ids *tracked_acl_ids); >>>>> + struct tracked_acl_ids *tracked_acl_ids, >>>>> + bool monitor_cond_complete); >>>>> bool ofctrl_has_backlog(void); >>>>> void ofctrl_wait(void); >>>>> void ofctrl_destroy(void); >>>>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >>>>> index 13a566d26..2a90f8de2 100644 >>>>> --- a/controller/ovn-controller.c >>>>> +++ b/controller/ovn-controller.c >>>>> @@ -6436,7 +6436,9 @@ main(int argc, char *argv[]) >>>>> ofctrl_seqno_get_req_cfg(), >>>>> >>>>> engine_node_changed(&en_lflow_output), >>>>> >>>>> engine_node_changed(&en_pflow_output), >>>>> - tracked_acl_ids); >>>>> + tracked_acl_ids, >>>>> + ovnsb_cond_seqno >>>>> + == ovnsb_expected_cond_seqno); >>>>> stopwatch_stop(OFCTRL_PUT_STOPWATCH_NAME, >>>>> time_msec()); >>>>> } >>>>> stopwatch_start(OFCTRL_SEQNO_RUN_STOPWATCH_NAME, >>>>> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at >>>>> index a878a440b..a646381b7 100644 >>>>> --- a/tests/ovn-controller.at >>>>> +++ b/tests/ovn-controller.at >>>>> @@ -2455,6 +2455,68 @@ AT_CHECK([ovs-ofctl dump-flows br-int | grep group >>>>> -c], [0], [3 >>>>> OVN_CLEANUP([hv1]) >>>>> AT_CLEANUP >>>>> >>>>> +OVN_FOR_EACH_NORTHD([ >>>>> +AT_SETUP([ovn-controller - ofctrl delay until all monitored updates >>>>> come]) >>>>> + >>>>> +# Prepare testing configuration >>>>> +ovn_start >>>>> + >>>>> +net_add n1 >>>>> +sim_add hv1 >>>>> +as hv1 >>>>> +check ovs-vsctl add-br br-phys >>>>> +ovn_attach n1 br-phys 192.168.0.1 >>>>> +check ovs-vsctl -- add-port br-int hv1-vif1 -- \ >>>>> + set interface hv1-vif1 external-ids:iface-id=ls1-lp1 >>>>> + >>>>> +check ovn-nbctl ls-add ls1 >>>>> + >>>>> +check ovn-nbctl lsp-add ls1 ls1-lp1 \ >>>>> +-- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01 10.1.2.3" >>>>> + >>>>> +check ovn-nbctl lb-add lb1 1.1.1.1 10.1.2.3 \ >>>>> +-- ls-lb-add ls1 lb1 >>>>> + >>>>> +check ovn-nbctl lb-add lb2 2.2.2.2 10.1.2.4 \ >>>>> +-- ls-lb-add ls1 lb2 >>>>> + >>>>> +check ovn-nbctl --wait=hv sync >>>>> + >>>>> +# Stop ovn-controller >>>>> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) >>>>> + >>>>> +# Sine we test initial flow upload controller is restarted. >>>>> +# Clear log file and start controller. >>>>> +rm -f hv1/ovn-controller.log >>>>> +start_daemon ovn-controller -vfile:jsonrpc:dbg -vfile:ofctrl:dbg >>>>> + >>>>> +# Monitor log file until flow finally uploaded to OVS >>>>> +OVS_WAIT_UNTIL([grep -q 'Setting lport.*in OVS' hv1/ovn-controller.log]) >>>>> + >>>>> +# Analyse log file, select records about: >>>>> +# 1. monitor_cond changes made for SB DB (message class is 'jsonrpc') >>>>> +# 2. 'clearing all flows' message which is issued after 'wait before >>>>> +# clear' stage released (message class is 'ofctrl') >>>>> +# >>>>> +# We expect here that all monitoring condition changes should be made >>>>> before >>>>> +# OVS flow cleared / uploaded. >>>>> +# For now all monitoring updates comes in three iterations: initial, >>>>> +# direct dps, indirect dps that corresponds to >>>>> +# three messages of type 1 followed by one message of type 2 >>>>> +# >>>>> +# For monitor-all=true one message of type 1 followed by one message of >>>>> type 2 >>>>> +# >>>>> +# Then we cut off message class and take first letter >>>>> +# (j for jsonrpc and o for ofctrl) >>>>> +# >>>>> +call_seq=`grep -E \ >>>>> + "(clearing all flows)|(monitor_cond.*South)" \ >>>>> + hv1/ovn-controller.log | cut -d "|" -f 3- | cut -b 1 | tr -d '\n'` >>>>> +AT_CHECK([echo $call_seq | grep -qE "^j+o$"], [0]) >>>>> + >>>>> +OVN_CLEANUP([hv1]) >>>>> +AT_CLEANUP >>>>> +]) >>>>> >>>>> AT_SETUP([ovn-controller - check ovn-chassis-mac-mappings]) >>>>> >>> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
