On 6/6/25 5:48 PM, Aleksandr Smirnov wrote:
> Remove wait-before-clear option because lflow upload
> is delayed with that fix
> 'controller: Delay initial flow upload to OVS.'
>
> Signed-off-by: Aleksandr Smirnov <alekssmirnov@k2.cloud>
> Acked-by: Numan Siddique <num...@ovn.org>
> ---
Hi Aleksandr,
> v3: Call daemon_started_recently() to detect
> load completion.
> v2: Addressed Numan's comments.
> ---
> NEWS | 3 +
> controller/ofctrl.c | 34 +---------
> controller/ovn-controller.8.xml | 34 ----------
> tests/ovn-controller.at | 106 --------------------------------
> 4 files changed, 4 insertions(+), 173 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 4d84a456a..34f85a598 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -26,6 +26,9 @@ Post v25.03.0
> - Add a new logical switch option - enable-stateless-acl-lb with default
> value of false. This option should be set to true for logical switches
> with stateless ACL to work with load balancer.
> + - An ovn-controller option wait-before-clear is no longer supported.
Nit: s/An ovn-controller option/The ovn-controller option/
I guess we should use the actual option name here.
> + It will be ignored if used. An ovn-controller will automatically care
> + about proper delay before clearing lflow.
>
In general, I guess I'd rephrase this as follows:
- The ovn-controller option ovn-ofctrl-wait-before-clear is no longer
supported. It will be ignored if used. ovn-controller will
automatically care about proper delay before clearing lflow.
Both comments above are minor and can be addressed when merging the
patches so we probably don't need a v4. Similar to patch 1/2 I'd like
to also hear from Numan on this before merging the patch.
In any case:
Acked-by: Dumitru Ceara <dce...@redhat.com>
Regards,
Dumitru
> OVN v25.03.0 - 07 Mar 2025
> --------------------------
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 304b9bbc8..b0b051e77 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -352,14 +352,6 @@ 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;
> -
> -/* The time when the state S_WAIT_BEFORE_CLEAR should complete.
> - * If the timer is not started yet, it is set to 0. */
> -static long long int wait_before_clear_expire = 0;
> -
> /* Transaction IDs for messages in flight to the switch. */
> static ovs_be32 xid, xid2;
>
> @@ -642,26 +634,9 @@ 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)) {
> + if (wait_before_clear_proceed) {
> state = S_CLEAR_FLOWS;
> - return;
> }
> -
> - if (!wait_before_clear_expire) {
> - /* Start the timer. */
> - wait_before_clear_expire = time_msec() + wait_before_clear_time;
> - }
> - poll_timer_wait_until(wait_before_clear_expire);
> }
>
> static void
> @@ -837,13 +812,6 @@ ofctrl_run(const char *conn_target, int probe_interval,
> const struct ovsrec_open_vswitch *cfg =
> ovsrec_open_vswitch_table_first(ovs_table);
> ovs_assert(cfg);
> - unsigned int _wait_before_clear_time =
> - smap_get_uint(&cfg->external_ids, "ovn-ofctrl-wait-before-clear", 0);
> - if (_wait_before_clear_time != wait_before_clear_time) {
> - VLOG_INFO("ofctrl-wait-before-clear is now %u ms (was %u ms)",
> - _wait_before_clear_time, wait_before_clear_time);
> - wait_before_clear_time = _wait_before_clear_time;
> - }
>
> bool progress = true;
> for (int i = 0; progress && i < 50; i++) {
> diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
> index 5007f5f80..3f2cf5bae 100644
> --- a/controller/ovn-controller.8.xml
> +++ b/controller/ovn-controller.8.xml
> @@ -294,40 +294,6 @@
> The default value is considered false if this option is not defined.
> </dd>
>
> - <dt><code>external_ids:ovn-ofctrl-wait-before-clear</code></dt>
> - <dd>
> - The time, in milliseconds, to wait before clearing flows in OVS after
> - OpenFlow connection/reconnection during <code>ovn-controller</code>
> - initialization. The purpose of this wait is to give time for
> - <code>ovn-controller</code> to compute the new flows before clearing
> - existing ones, to avoid data plane down time during
> - <code>ovn-controller</code> restart/upgrade at large scale
> - environments where recomputing the flows takes more than a few
> seconds
> - or even longer. It is difficult for <code>ovn-controller</code>
> - to determine when the new flows computing is completed, because of
> the
> - dynamics in the cloud environments, which is why this configuration
> is
> - provided for users to adjust based on the scale of the environment.
> By
> - default, it is 0, which means clearing existing flows without
> waiting.
> - Not setting the value, or setting it too small, may result in data
> - plane down time during upgrade/restart, while setting it too big may
> - result in unnecessary extra control plane latency of applying new
> - changes of CMS during upgrade/restart. In most cases, a slightly
> bigger
> - value is not harmful, because the extra control plane latency happens
> - only once during the OpenFlow connection. To get a reasonable range
> of
> - the value setting, it is recommended to run the below commands on a
> - node in the target environment and then set this configuration to
> twice
> - the value of <code>Maximum</code> shown in the output of the second
> - command.
> - <ul>
> - <li>
> - <code>ovn-appctl -t ovn-controller inc-engine/recompute</code>
> - </li>
> - <li>
> - <code>ovn-appctl -t ovn-controller stopwatch/show
> flow-generation</code>
> - </li>
> - </ul>
> - </dd>
> -
> <dt><code>external_ids:ovn-enable-lflow-cache</code></dt>
> <dd>
> The boolean flag indicates if <code>ovn-controller</code> should
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 2e7ea5cb8..1e34d7ddf 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -2348,112 +2348,6 @@ OVS_WAIT_UNTIL([
> OVN_CLEANUP([hv1])
> AT_CLEANUP
>
> -AT_SETUP([ovn-controller - ofctrl wait before clearing flows])
> -
> -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
> -# There should be 2 group IDs allocated
> -AT_CHECK([ovn-appctl -t ovn-controller group-table-list | awk '{print $2}' |
> sort | uniq | wc -l], [0], [2
> -])
> -
> -# Stop ovn-controller
> -OVS_APP_EXIT_AND_WAIT([ovn-controller])
> -
> -# Set 5 seconds wait time before clearing OVS flows.
> -check ovs-vsctl set open . external_ids:ovn-ofctrl-wait-before-clear=5000
> -
> -# The old OVS flows should remain (this is regardless of the configuration)
> -AT_CHECK([ovs-ofctl dump-flows br-int | grep -F 10.1.2.3], [0], [ignore])
> -
> -# We should have 2 flows with groups.
> -AT_CHECK([ovs-ofctl dump-flows br-int | grep group -c], [0], [2
> -])
> -
> -# Make a change to the ls1-lp1's IP
> -check ovn-nbctl --wait=sb lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01
> 10.1.2.4"
> -
> -# Start ovn-controller, which should compute new flows but not apply them
> -# until the wait time is completed.
> -start_daemon ovn-controller
> -
> -# Wait for octrl to run - it will handle the wait-before-clear
> -OVS_WAIT_UNTIL([grep -q 'wait-before-clear' hv1/ovn-controller.log])
> -
> -# Check that there is no flow using 10.1.2.4 except the lb one (using
> 2.2.2.2)
> -OVS_WAIT_UNTIL([test 0 = $(ovs-ofctl dump-flows br-int | grep -F 10.1.2.4 |
> grep -cvF 2.2.2.2)])
> -
> -# Check in the middle of the wait.
> -# Wait for ovn-controller to claim the port as it might cause some lflow_run.
> -OVS_WAIT_UNTIL([test 2 = $(grep -c 'Claiming lport ls1-lp1 for this chassis'
> hv1/ovn-controller.log)])
> -
> -lflow_run_1=$(ovn-appctl -t ovn-controller coverage/read-counter lflow_run)
> -AT_CHECK([ovs-ofctl dump-flows br-int | grep -F 10.1.2.3], [0], [ignore])
> -
> -# We should have 2 flows with groups.
> -AT_CHECK([ovs-ofctl dump-flows br-int | grep group -c], [0], [2
> -])
> -
> -sleep 5
> -
> -# Check after the wait
> -OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep -F 10.1.2.4 | grep -vF
> 2.2.2.2])
> -
> -# We should have 2 flows with groups.
> -AT_CHECK([ovs-ofctl dump-flows br-int | grep group -c], [0], [2
> -])
> -lflow_run_2=$(ovn-appctl -t ovn-controller coverage/read-counter lflow_run)
> -
> -# Verify that the flow compute completed during the wait (after the wait it
> -# merely installs the new flows).
> -AT_CHECK_UNQUOTED([echo $lflow_run_1], [0], [$lflow_run_2
> -])
> -
> -# Restart OVS this time. Flows should be reinstalled without waiting.
> -# Set the wait-before-clear to a large value (60s) to make the test more
> reliable.
> -check ovs-vsctl set open . external_ids:ovn-ofctrl-wait-before-clear=60000
> -check ovn-nbctl --wait=hv sync
> -
> -OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
> -start_daemon ovs-vswitchd --enable-dummy=system -vvconn -vofproto_dpif
> -vunixctl
> -
> -# Flow should be installed without waiting for another 60s.
> -OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep -F 10.1.2.4 | grep -vF
> 2.2.2.2])
> -
> -check ovn-nbctl --wait=hv lb-add lb3 3.3.3.3 10.1.2.5 \
> --- ls-lb-add ls1 lb3
> -
> -# There should be 3 group IDs allocated (this is to ensure the group ID
> -# allocation is correct after ofctrl state reset.
> -AT_CHECK([ovn-appctl -t ovn-controller group-table-list | awk '{print $2}' |
> sort | uniq | wc -l], [0], [3
> -])
> -
> -# We should have 3 flows with groups.
> -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])
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
