On 11/30/23 17:53, Xavier Simonart wrote:
> The group_table and meter_table are initialized in ovn-controller, with n_ids
> = 0.
> Then they are re-initialized in ofctrl, with proper number of n_ids, in state
> S_CLEAR_FLOWS.
> However, nothing prevented to start adding flows (by adding logical ports)
> using groups
> before ofctrl reaches this state. This was causing some wrong flows (missing
> group in the flow).
>
> With this patch, as soon as the feature rconn is available, i.e. before
> adding flows, those table
> are properly re-initialized.
>
> This issue is usually not visible in main and recent branches ci, since
> "Wait for new ovn-controllers to connect to Southbound." as this was slowing
> down the moment when
> the test started to add ports.
> This was causing the following test to fail:
> "ECMP static routes -- ovn-northd -- parallelization=yes --
> ovn_monitor_all=yes".
>
> Fixes: 1d6d953bf883 ("controller: Don't artificially limit group and meter
> IDs to 16bit.")
> Signed-off-by: Xavier Simonart <xsimo...@redhat.com>
>
> ---
> v2: - Updated based on Dumitru's feedback i.e. call ovn_extend_table_clear
> within ovn_extend_table_reinit
> and do not call ovn_extend_table_reinit anymore from ofctrl
> - Modified existing test case (ofctrl wait before clearing flows) to
> catch this error
> ---
Thanks, Xavier; it looks OK overall, I have a few minor comments below
though.
> controller/ofctrl.c | 2 --
> controller/ovn-controller.c | 8 ++++++++
> lib/extend-table.c | 1 +
> tests/ovn-controller.at | 18 +++++++++++++++++-
> 4 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index c6b1272ba..7aac0128b 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -675,13 +675,11 @@ run_S_CLEAR_FLOWS(void)
> /* Clear existing groups, to match the state of the switch. */
> if (groups) {
> ovn_extend_table_clear(groups, true);
> - ovn_extend_table_reinit(groups, ovs_feature_max_select_groups_get());
> }
>
> /* Clear existing meters, to match the state of the switch. */
> if (meters) {
> ovn_extend_table_clear(meters, true);
> - ovn_extend_table_reinit(meters, ovs_feature_max_meters_get());
> ofctrl_meter_bands_clear();
> }
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 44605eb4e..98d95b1f3 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -5654,6 +5654,14 @@ main(int argc, char *argv[])
> br_int ? br_int->name : NULL)) {
> VLOG_INFO("OVS feature set changed, force recompute.");
> engine_set_force_recompute(true);
> + if (ovs_feature_set_discovered()) {
> + lflow_output_data =
> + engine_get_internal_data(&en_lflow_output);
lflow_output_data is used in multiple different ways in this file and
that's not that easy to follow IMO.
I think I'd just use another local variable here.
> + ovn_extend_table_reinit(&lflow_output_data->group_table,
> + ovs_feature_max_select_groups_get());
Maybe it's a sign of too much nesting but we should properly align this.
> + ovn_extend_table_reinit(&lflow_output_data->meter_table,
> + ovs_feature_max_meters_get());
> + }
Does this look better to you?
if (ovs_feature_set_discovered()) {
uint32_t max_groups = ovs_feature_max_select_groups_get();
uint32_t max_meters = ovs_feature_max_meters_get();
struct ed_type_lflow_output *lflow_out_data =
engine_get_internal_data(&en_lflow_output);
ovn_extend_table_reinit(&lflow_out_data->group_table,
max_groups);
ovn_extend_table_reinit(&lflow_out_data->meter_table,
max_meters);
}
> }
>
> if (br_int && ovs_feature_set_discovered()) {
> diff --git a/lib/extend-table.c b/lib/extend-table.c
> index 7f2358778..87828772c 100644
> --- a/lib/extend-table.c
> +++ b/lib/extend-table.c
> @@ -47,6 +47,7 @@ ovn_extend_table_init(struct ovn_extend_table *table, const
> char *table_name,
> void
> ovn_extend_table_reinit(struct ovn_extend_table *table, uint32_t n_ids)
> {
> + ovn_extend_table_clear(table, true);
This should be done only if the number of IDs changed. So we
should move it inside the "if" below.
> if (n_ids != table->n_ids) {
> id_pool_destroy(table->table_ids);
> table->table_ids = id_pool_create(1, n_ids);
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 5acb380db..1f826b21c 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -2202,6 +2202,10 @@ OVS_APP_EXIT_AND_WAIT([ovn-controller])
> # The old OVS flows should remain (this is regardless of the configuration)
> AT_CHECK([ovs-ofctl dump-flows br-int | grep 10.1.2.3], [0], [ignore])
>
> +# We should have 2 flows with groups
Missing '.' at end of sentence.
> +AT_CHECK([ovs-ofctl dump-flows br-int | grep group | wc -l], [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"
>
> @@ -2214,10 +2218,18 @@ sleep 2
> lflow_run_1=$(ovn-appctl -t ovn-controller coverage/read-counter lflow_run)
> AT_CHECK([ovs-ofctl dump-flows br-int | grep 10.1.2.3], [0], [ignore])
>
> +# We should have 2 flows with groups
Missing '.' at end of sentence.
> +AT_CHECK([ovs-ofctl dump-flows br-int | grep group | wc -l], [0], [2
grep -c
> +])
> +
> sleep 5
>
> # Check after the wait
> OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep 10.1.2.4])
> +
> +# We should have 2 flows with groups
Missing '.' at end of sentence.
> +AT_CHECK([ovs-ofctl dump-flows br-int | grep group | wc -l], [0], [2
grep -c
> +])
> 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
> @@ -2230,7 +2242,7 @@ OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
> start_daemon ovs-vswitchd --enable-dummy=system -vvconn -vofproto_dpif
> -vunixctl
> OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep 10.1.2.4])
>
> -check ovn-nbctl --wait=hv lb-add lb3 2.2.2.2 10.1.2.5 \
> +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
> @@ -2238,6 +2250,10 @@ check ovn-nbctl --wait=hv lb-add lb3 2.2.2.2 10.1.2.5 \
> 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
Missing '.' at end of sentence.
> +AT_CHECK([ovs-ofctl dump-flows br-int | grep group | wc -l], [0], [3
grep -c
> +])
> +
> OVN_CLEANUP([hv1])
> AT_CLEANUP
>
If these minor changes look OK to you I can squash them in when applying
the patch.
Thanks,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
