On 12/1/23 17:25, Xavier Simonart wrote:
> Hi Dumitru
>
> Thanks for the detailed review.
> All comments make sense to me.
Thanks for checking, Xavier! I squashed the changes in and applied the
patch to main and backported it to all stable branches down to 22.03.
Regards,
Dumitru
> Thanks
> Xavier
>
> On Thu, Nov 30, 2023 at 9:13 PM Dumitru Ceara <dce...@redhat.com> wrote:
>
>> 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
