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
