On Tue, Mar 18, 2025 at 6:08 AM Aleksandr Smirnov <alekssmirnov@k2.cloud> 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>
Hi Aleksandr,
Sorry for the delay in reviewing this. I was a little concerned about
this patch series initially
when I saw the patch commit message. After reviewing this patch, my
concerns are cleared
and I think it's a smart approach.
I tested it locally with a pretty big sized OVN SB DB. ovn-controller
installs around 400k openflow rules.
Without this patch, ovn-controller takes around 10 seconds to add
back all the flows when it is restarted.
With this patch, I don't see any delay. The existing flows are
cleared and added back without any delay.
I'd definitely like to get opinions from other maintainers too if they
have any concerns.
@Han Zhou As you added the option - ovn-ofctrl-wait-before-clear
option can you please take a look at it ?
From my side
Acked-by: Numan Siddque <num...@ovn.org>
Numan
> ---
> 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 bc8acddf1..2623fc758 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -6384,7 +6384,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 efb0a1741..812616711 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -2463,6 +2463,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])
>
> --
> 2.47.0
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
