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 <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
