Skip to content

Commit

Permalink
controller: fix group_table and meter_table allocation
Browse files Browse the repository at this point in the history
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: 1d6d953 ("controller: Don't artificially limit group and meter IDs to 16bit.")
Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
(cherry picked from commit 8c165db)
  • Loading branch information
simonartxavier authored and dceara committed Dec 4, 2023
1 parent 8a000cc commit acc6372
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 3 deletions.
2 changes: 0 additions & 2 deletions controller/ofctrl.c
Original file line number Diff line number Diff line change
Expand Up @@ -677,13 +677,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();
}

Expand Down
11 changes: 11 additions & 0 deletions controller/ovn-controller.c
Original file line number Diff line number Diff line change
Expand Up @@ -5673,6 +5673,17 @@ 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()) {
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()) {
Expand Down
1 change: 1 addition & 0 deletions lib/extend-table.c
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ void
ovn_extend_table_reinit(struct ovn_extend_table *table, uint32_t n_ids)
{
if (n_ids != table->n_ids) {
ovn_extend_table_clear(table, true);
id_pool_destroy(table->table_ids);
table->table_ids = id_pool_create(1, n_ids);
table->n_ids = n_ids;
Expand Down
18 changes: 17 additions & 1 deletion tests/ovn-controller.at
Original file line number Diff line number Diff line change
Expand Up @@ -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.
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"

Expand All @@ -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.
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 10.1.2.4])

# 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
Expand All @@ -2230,14 +2242,18 @@ 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
# 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

Expand Down

0 comments on commit acc6372

Please sign in to comment.