| Message ID | 20231130165344.3240652-1-xsimonar@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | [ovs-dev,v2] controller: fix group_table and meter_table allocation | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
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 <xsimonar@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
Hi Dumitru Thanks for the detailed review. All comments make sense to me. Thanks Xavier On Thu, Nov 30, 2023 at 9:13 PM Dumitru Ceara <dceara@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 <xsimonar@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 > >
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 <dceara@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 <xsimonar@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 >> >> >
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); + ovn_extend_table_reinit(&lflow_output_data->group_table, + ovs_feature_max_select_groups_get()); + ovn_extend_table_reinit(&lflow_output_data->meter_table, + ovs_feature_max_meters_get()); + } } 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); 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 +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 +AT_CHECK([ovs-ofctl dump-flows br-int | grep group | wc -l], [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 | wc -l], [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 @@ -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 +AT_CHECK([ovs-ofctl dump-flows br-int | grep group | wc -l], [0], [3 +]) + OVN_CLEANUP([hv1]) AT_CLEANUP
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 <xsimonar@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 --- 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(-)