Re: [ovs-dev] [PATCH ovn 2/3] ovn-controller: Track OVSDB column open_vswitch:external_ids.
On Fri, Sep 9, 2022 at 4:41 AM Dumitru Ceara wrote:
>
> On 9/9/22 07:26, Han Zhou wrote:
> > Thanks for the review and sorry for the late response.
> >
> > On Tue, Aug 23, 2022 at 5:22 AM Dumitru Ceara wrote:
> >>
> >> Hi Han,
> >>
> >> On 8/23/22 06:44, Han Zhou wrote:
> >>> On Tue, Jul 5, 2022 at 3:41 PM Han Zhou wrote:
>
> The column was not tracked before while it should. The column
includes
> many ovn-controller global configurations that could impact the way
> flows are computed. It worked before because lots of the
configurations
> are also populated to OVN-SB DB's chassis table, and as a result the
SB
> DB change would notify back to the ovn-controller itself, thus
> triggering a recompute to make the configure change effective.
However,
> this is not the way it is supposed to work. We should track the
> open_vswitch:external_ids column directly, and the I-P engine would
> immediately recompute and apply the change.
> >>
> >> This part seems good to me. To nit-pick I'd add:
> >>
> >> Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental
engine -
> > quiet mode.")
> >>
> > Ack
> >
>
> This patch also adjust the order of adding tracked and untracked
> > columns
> to monitoring, to workaround a problem in OVS IDL that could end up
> overwriting the track flag. A XXX comment is added for future
> improvement.
>
> >>
> >> I would argue that the problem is not that the IDL overwrites the track
> >> flag but that ovn-controller registers to monitor the same column in
> >> multiple ways.
> >>
> > Maybe. The IDL document didn't define this clearly, but it seems to me
more
> > natural to add the bit flags without clearing existing ones when calling
> > ovsdb_idl_add_column() after ovsdb_idl_track_add_column(), because the
> > names don't indicate they are mutually exclusive. If a user really
wants to
> > untrack a monitored column, it would be better to do so through a
separate
> > API such as "ovsdb_idl_untrack_column()". The ovsdb_idl_add_column() was
> > implemented before the tracking support so it seemed more like a small
> > omission when adding support for tracking.
> >
>
> Ok, this is also a valid way to look at it.
>
> >> Unfortunately, I think at this point we should aggregate all the idl
> >> registering inside the ctrl_register_ovs_idl() function and not do it
> >> from other modules. In my opinion it's becoming complicated to follow
> >> especially because all the DB I-P nodes (OVS_NODES, SB_NODES) are
> >> defined in ovn-controller.c.
> >
> > I agree it is a little complicated, but the intention of maintaining the
> > usage of OVS tables and columns from each module is still valid.
Although
> > it is straightforward to add in the ovn-controller.c for a new column
to be
> > monitored/tracked, it would be more difficult when removing. For
example,
> > if a column is not used anymore in physical.c, we could simply remove
the
> > monitoring from physical_register_ovs_idl() and not worry about other
> > modules. However, if we put them all in one place in ovn-controller.c,
we
> > will need to check for every module and make sure none of them are using
> > the column before removing it. Of course, this approach may not work if
the
> > ovsdb_idl_add_column() is overwriting the flags.
> >
> > It is true that OVS_NODES are defined in ovn-controller.c but they don't
> > really define which columns are monitored.
> >
> > So, for this bug fix, I was trying to keep the original approach, with
the
> > assumption that the ovsdb_idl_add_column will be updated to keep the
> > tracking flag (although not urgent).
> >
> >>
> >> In any case, shall we split out this second part into a separate patch?
> >
> > Yes, it is better to split this part to a separate patch that solves the
> > order problem, but it will be the first patch and the real change "track
> > open_vswitch:external_ids" will depend on it. So we still need to make a
> > decision whether it is just an order change or aggregating monitoring. I
> > will send v2 when we make the decision. Please let me know if you still
> > prefer the "aggregation" approach with the above considerations.
> >
>
> Let's go for changing of the order like you initially suggested because
> it seems like we'd get a smaller patch.
>
Sounds good. I sent v2 with two separate patches with the comments
addressed (sorry that I forgot to add notes for v2).
https://patchwork.ozlabs.org/project/ovn/list/?series=317680
> But I think we need to revisit this in the future. For example the
> 'ovsrec_port_col_external_ids' column is tracked in ovn-controller.c but
> also in encaps.c and physical.c. Shouldn't we remove it from
> ovn-controller.c? I didn't check closely but it seems to me that quite
> a few others are not really used in ovn-controller.c and could just be
> removed from ctrl_register_ovs_idl().
>
I agree. We should revisit in the future. Sometimes the problems are not
exposed un
Re: [ovs-dev] [PATCH ovn 2/3] ovn-controller: Track OVSDB column open_vswitch:external_ids.
On 9/9/22 07:26, Han Zhou wrote:
> Thanks for the review and sorry for the late response.
>
> On Tue, Aug 23, 2022 at 5:22 AM Dumitru Ceara wrote:
>>
>> Hi Han,
>>
>> On 8/23/22 06:44, Han Zhou wrote:
>>> On Tue, Jul 5, 2022 at 3:41 PM Han Zhou wrote:
The column was not tracked before while it should. The column includes
many ovn-controller global configurations that could impact the way
flows are computed. It worked before because lots of the configurations
are also populated to OVN-SB DB's chassis table, and as a result the SB
DB change would notify back to the ovn-controller itself, thus
triggering a recompute to make the configure change effective. However,
this is not the way it is supposed to work. We should track the
open_vswitch:external_ids column directly, and the I-P engine would
immediately recompute and apply the change.
>>
>> This part seems good to me. To nit-pick I'd add:
>>
>> Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine -
> quiet mode.")
>>
> Ack
>
This patch also adjust the order of adding tracked and untracked
> columns
to monitoring, to workaround a problem in OVS IDL that could end up
overwriting the track flag. A XXX comment is added for future
improvement.
>>
>> I would argue that the problem is not that the IDL overwrites the track
>> flag but that ovn-controller registers to monitor the same column in
>> multiple ways.
>>
> Maybe. The IDL document didn't define this clearly, but it seems to me more
> natural to add the bit flags without clearing existing ones when calling
> ovsdb_idl_add_column() after ovsdb_idl_track_add_column(), because the
> names don't indicate they are mutually exclusive. If a user really wants to
> untrack a monitored column, it would be better to do so through a separate
> API such as "ovsdb_idl_untrack_column()". The ovsdb_idl_add_column() was
> implemented before the tracking support so it seemed more like a small
> omission when adding support for tracking.
>
Ok, this is also a valid way to look at it.
>> Unfortunately, I think at this point we should aggregate all the idl
>> registering inside the ctrl_register_ovs_idl() function and not do it
>> from other modules. In my opinion it's becoming complicated to follow
>> especially because all the DB I-P nodes (OVS_NODES, SB_NODES) are
>> defined in ovn-controller.c.
>
> I agree it is a little complicated, but the intention of maintaining the
> usage of OVS tables and columns from each module is still valid. Although
> it is straightforward to add in the ovn-controller.c for a new column to be
> monitored/tracked, it would be more difficult when removing. For example,
> if a column is not used anymore in physical.c, we could simply remove the
> monitoring from physical_register_ovs_idl() and not worry about other
> modules. However, if we put them all in one place in ovn-controller.c, we
> will need to check for every module and make sure none of them are using
> the column before removing it. Of course, this approach may not work if the
> ovsdb_idl_add_column() is overwriting the flags.
>
> It is true that OVS_NODES are defined in ovn-controller.c but they don't
> really define which columns are monitored.
>
> So, for this bug fix, I was trying to keep the original approach, with the
> assumption that the ovsdb_idl_add_column will be updated to keep the
> tracking flag (although not urgent).
>
>>
>> In any case, shall we split out this second part into a separate patch?
>
> Yes, it is better to split this part to a separate patch that solves the
> order problem, but it will be the first patch and the real change "track
> open_vswitch:external_ids" will depend on it. So we still need to make a
> decision whether it is just an order change or aggregating monitoring. I
> will send v2 when we make the decision. Please let me know if you still
> prefer the "aggregation" approach with the above considerations.
>
Let's go for changing of the order like you initially suggested because
it seems like we'd get a smaller patch.
But I think we need to revisit this in the future. For example the
'ovsrec_port_col_external_ids' column is tracked in ovn-controller.c but
also in encaps.c and physical.c. Shouldn't we remove it from
ovn-controller.c? I didn't check closely but it seems to me that quite
a few others are not really used in ovn-controller.c and could just be
removed from ctrl_register_ovs_idl().
>>
Signed-off-by: Han Zhou
---
controller/ovn-controller.c | 28 +---
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 69615308e..dacef0204 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -922,23 +922,19 @@ static void
ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
{
/* We do not monitor a
Re: [ovs-dev] [PATCH ovn 2/3] ovn-controller: Track OVSDB column open_vswitch:external_ids.
Thanks for the review and sorry for the late response.
On Tue, Aug 23, 2022 at 5:22 AM Dumitru Ceara wrote:
>
> Hi Han,
>
> On 8/23/22 06:44, Han Zhou wrote:
> > On Tue, Jul 5, 2022 at 3:41 PM Han Zhou wrote:
> >>
> >> The column was not tracked before while it should. The column includes
> >> many ovn-controller global configurations that could impact the way
> >> flows are computed. It worked before because lots of the configurations
> >> are also populated to OVN-SB DB's chassis table, and as a result the SB
> >> DB change would notify back to the ovn-controller itself, thus
> >> triggering a recompute to make the configure change effective. However,
> >> this is not the way it is supposed to work. We should track the
> >> open_vswitch:external_ids column directly, and the I-P engine would
> >> immediately recompute and apply the change.
>
> This part seems good to me. To nit-pick I'd add:
>
> Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine -
quiet mode.")
>
Ack
> >>
> >> This patch also adjust the order of adding tracked and untracked
columns
> >> to monitoring, to workaround a problem in OVS IDL that could end up
> >> overwriting the track flag. A XXX comment is added for future
> >> improvement.
> >>
>
> I would argue that the problem is not that the IDL overwrites the track
> flag but that ovn-controller registers to monitor the same column in
> multiple ways.
>
Maybe. The IDL document didn't define this clearly, but it seems to me more
natural to add the bit flags without clearing existing ones when calling
ovsdb_idl_add_column() after ovsdb_idl_track_add_column(), because the
names don't indicate they are mutually exclusive. If a user really wants to
untrack a monitored column, it would be better to do so through a separate
API such as "ovsdb_idl_untrack_column()". The ovsdb_idl_add_column() was
implemented before the tracking support so it seemed more like a small
omission when adding support for tracking.
> Unfortunately, I think at this point we should aggregate all the idl
> registering inside the ctrl_register_ovs_idl() function and not do it
> from other modules. In my opinion it's becoming complicated to follow
> especially because all the DB I-P nodes (OVS_NODES, SB_NODES) are
> defined in ovn-controller.c.
I agree it is a little complicated, but the intention of maintaining the
usage of OVS tables and columns from each module is still valid. Although
it is straightforward to add in the ovn-controller.c for a new column to be
monitored/tracked, it would be more difficult when removing. For example,
if a column is not used anymore in physical.c, we could simply remove the
monitoring from physical_register_ovs_idl() and not worry about other
modules. However, if we put them all in one place in ovn-controller.c, we
will need to check for every module and make sure none of them are using
the column before removing it. Of course, this approach may not work if the
ovsdb_idl_add_column() is overwriting the flags.
It is true that OVS_NODES are defined in ovn-controller.c but they don't
really define which columns are monitored.
So, for this bug fix, I was trying to keep the original approach, with the
assumption that the ovsdb_idl_add_column will be updated to keep the
tracking flag (although not urgent).
>
> In any case, shall we split out this second part into a separate patch?
Yes, it is better to split this part to a separate patch that solves the
order problem, but it will be the first patch and the real change "track
open_vswitch:external_ids" will depend on it. So we still need to make a
decision whether it is just an order change or aggregating monitoring. I
will send v2 when we make the decision. Please let me know if you still
prefer the "aggregation" approach with the above considerations.
>
> >> Signed-off-by: Han Zhou
> >> ---
> >> controller/ovn-controller.c | 28 +---
> >> 1 file changed, 17 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> >> index 69615308e..dacef0204 100644
> >> --- a/controller/ovn-controller.c
> >> +++ b/controller/ovn-controller.c
> >> @@ -922,23 +922,19 @@ static void
> >> ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> >> {
> >> /* We do not monitor all tables by default, so modules must
register
> >> - * their interest explicitly. */
> >> + * their interest explicitly.
> >> + * XXX: when there are overlapping columns monitored by different
> > modules,
>
> Isn't "when the same column is monitored in different modes by different
> modules" more accurate?
Absolutely.
Thanks,
Han
>
> Regards,
> Dumitru
>
> >> + * there is a chance that "track" flag added by
> > ovsdb_idl_track_add_column
> >> + * by one module being overwritten by a following
> > ovsdb_idl_add_column by
> >> + * another module. Before this is fixed in OVSDB IDL, we need to
be
> > careful
> >> + * about the order so that the "tra
Re: [ovs-dev] [PATCH ovn 2/3] ovn-controller: Track OVSDB column open_vswitch:external_ids.
Hi Han,
On 8/23/22 06:44, Han Zhou wrote:
> On Tue, Jul 5, 2022 at 3:41 PM Han Zhou wrote:
>>
>> The column was not tracked before while it should. The column includes
>> many ovn-controller global configurations that could impact the way
>> flows are computed. It worked before because lots of the configurations
>> are also populated to OVN-SB DB's chassis table, and as a result the SB
>> DB change would notify back to the ovn-controller itself, thus
>> triggering a recompute to make the configure change effective. However,
>> this is not the way it is supposed to work. We should track the
>> open_vswitch:external_ids column directly, and the I-P engine would
>> immediately recompute and apply the change.
This part seems good to me. To nit-pick I'd add:
Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine - quiet
mode.")
>>
>> This patch also adjust the order of adding tracked and untracked columns
>> to monitoring, to workaround a problem in OVS IDL that could end up
>> overwriting the track flag. A XXX comment is added for future
>> improvement.
>>
I would argue that the problem is not that the IDL overwrites the track
flag but that ovn-controller registers to monitor the same column in
multiple ways.
Unfortunately, I think at this point we should aggregate all the idl
registering inside the ctrl_register_ovs_idl() function and not do it
from other modules. In my opinion it's becoming complicated to follow
especially because all the DB I-P nodes (OVS_NODES, SB_NODES) are
defined in ovn-controller.c.
In any case, shall we split out this second part into a separate patch?
>> Signed-off-by: Han Zhou
>> ---
>> controller/ovn-controller.c | 28 +---
>> 1 file changed, 17 insertions(+), 11 deletions(-)
>>
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index 69615308e..dacef0204 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -922,23 +922,19 @@ static void
>> ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
>> {
>> /* We do not monitor all tables by default, so modules must register
>> - * their interest explicitly. */
>> + * their interest explicitly.
>> + * XXX: when there are overlapping columns monitored by different
> modules,
Isn't "when the same column is monitored in different modes by different
modules" more accurate?
Regards,
Dumitru
>> + * there is a chance that "track" flag added by
> ovsdb_idl_track_add_column
>> + * by one module being overwritten by a following
> ovsdb_idl_add_column by
>> + * another module. Before this is fixed in OVSDB IDL, we need to be
> careful
>> + * about the order so that the "track" calls are after the
> "non-track"
>> + * calls. */
>> ovsdb_idl_add_table(ovs_idl, &ovsrec_table_open_vswitch);
>> -ovsdb_idl_add_column(ovs_idl, &ovsrec_open_vswitch_col_external_ids);
>> ovsdb_idl_add_column(ovs_idl, &ovsrec_open_vswitch_col_other_config);
>> ovsdb_idl_add_column(ovs_idl, &ovsrec_open_vswitch_col_bridges);
>> ovsdb_idl_add_column(ovs_idl, &ovsrec_open_vswitch_col_datapaths);
>> ovsdb_idl_add_table(ovs_idl, &ovsrec_table_interface);
>> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_name);
>> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_bfd);
>> -ovsdb_idl_track_add_column(ovs_idl,
> &ovsrec_interface_col_bfd_status);
>> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_type);
>> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_options);
>> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_ofport);
>> ovsdb_idl_add_table(ovs_idl, &ovsrec_table_port);
>> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_name);
>> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_interfaces);
>> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_external_ids);
>> ovsdb_idl_add_table(ovs_idl, &ovsrec_table_bridge);
>> ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_ports);
>> ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_name);
>> @@ -958,6 +954,16 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
>> bfd_register_ovs_idl(ovs_idl);
>> physical_register_ovs_idl(ovs_idl);
>> vif_plug_register_ovs_idl(ovs_idl);
>> +ovsdb_idl_track_add_column(ovs_idl,
> &ovsrec_open_vswitch_col_external_ids);
>> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_name);
>> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_bfd);
>> +ovsdb_idl_track_add_column(ovs_idl,
> &ovsrec_interface_col_bfd_status);
>> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_type);
>> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_options);
>> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_ofport);
>> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_name);
>> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_in
Re: [ovs-dev] [PATCH ovn 2/3] ovn-controller: Track OVSDB column open_vswitch:external_ids.
On Tue, Jul 5, 2022 at 3:41 PM Han Zhou wrote:
>
> The column was not tracked before while it should. The column includes
> many ovn-controller global configurations that could impact the way
> flows are computed. It worked before because lots of the configurations
> are also populated to OVN-SB DB's chassis table, and as a result the SB
> DB change would notify back to the ovn-controller itself, thus
> triggering a recompute to make the configure change effective. However,
> this is not the way it is supposed to work. We should track the
> open_vswitch:external_ids column directly, and the I-P engine would
> immediately recompute and apply the change.
>
> This patch also adjust the order of adding tracked and untracked columns
> to monitoring, to workaround a problem in OVS IDL that could end up
> overwriting the track flag. A XXX comment is added for future
> improvement.
>
> Signed-off-by: Han Zhou
> ---
> controller/ovn-controller.c | 28 +---
> 1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 69615308e..dacef0204 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -922,23 +922,19 @@ static void
> ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> {
> /* We do not monitor all tables by default, so modules must register
> - * their interest explicitly. */
> + * their interest explicitly.
> + * XXX: when there are overlapping columns monitored by different
modules,
> + * there is a chance that "track" flag added by
ovsdb_idl_track_add_column
> + * by one module being overwritten by a following
ovsdb_idl_add_column by
> + * another module. Before this is fixed in OVSDB IDL, we need to be
careful
> + * about the order so that the "track" calls are after the
"non-track"
> + * calls. */
> ovsdb_idl_add_table(ovs_idl, &ovsrec_table_open_vswitch);
> -ovsdb_idl_add_column(ovs_idl, &ovsrec_open_vswitch_col_external_ids);
> ovsdb_idl_add_column(ovs_idl, &ovsrec_open_vswitch_col_other_config);
> ovsdb_idl_add_column(ovs_idl, &ovsrec_open_vswitch_col_bridges);
> ovsdb_idl_add_column(ovs_idl, &ovsrec_open_vswitch_col_datapaths);
> ovsdb_idl_add_table(ovs_idl, &ovsrec_table_interface);
> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_name);
> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_bfd);
> -ovsdb_idl_track_add_column(ovs_idl,
&ovsrec_interface_col_bfd_status);
> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_type);
> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_options);
> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_ofport);
> ovsdb_idl_add_table(ovs_idl, &ovsrec_table_port);
> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_name);
> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_interfaces);
> -ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_external_ids);
> ovsdb_idl_add_table(ovs_idl, &ovsrec_table_bridge);
> ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_ports);
> ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_name);
> @@ -958,6 +954,16 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> bfd_register_ovs_idl(ovs_idl);
> physical_register_ovs_idl(ovs_idl);
> vif_plug_register_ovs_idl(ovs_idl);
> +ovsdb_idl_track_add_column(ovs_idl,
&ovsrec_open_vswitch_col_external_ids);
> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_name);
> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_bfd);
> +ovsdb_idl_track_add_column(ovs_idl,
&ovsrec_interface_col_bfd_status);
> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_type);
> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_options);
> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_ofport);
> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_name);
> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_interfaces);
> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_external_ids);
> }
>
> #define SB_NODES \
> --
> 2.30.2
>
A kind reminder for this patch, which can be reviewed and applied
independent of the rest of the series.
Thanks,
Han
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/3] ovn-controller: Track OVSDB column open_vswitch:external_ids.
Bleep bloop. Greetings Han Zhou, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Comment with 'xxx' marker #36 FILE: controller/ovn-controller.c:926: * XXX: when there are overlapping columns monitored by different modules, Lines checked: 80, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 2/3] ovn-controller: Track OVSDB column open_vswitch:external_ids.
The column was not tracked before while it should. The column includes
many ovn-controller global configurations that could impact the way
flows are computed. It worked before because lots of the configurations
are also populated to OVN-SB DB's chassis table, and as a result the SB
DB change would notify back to the ovn-controller itself, thus
triggering a recompute to make the configure change effective. However,
this is not the way it is supposed to work. We should track the
open_vswitch:external_ids column directly, and the I-P engine would
immediately recompute and apply the change.
This patch also adjust the order of adding tracked and untracked columns
to monitoring, to workaround a problem in OVS IDL that could end up
overwriting the track flag. A XXX comment is added for future
improvement.
Signed-off-by: Han Zhou
---
controller/ovn-controller.c | 28 +---
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 69615308e..dacef0204 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -922,23 +922,19 @@ static void
ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
{
/* We do not monitor all tables by default, so modules must register
- * their interest explicitly. */
+ * their interest explicitly.
+ * XXX: when there are overlapping columns monitored by different modules,
+ * there is a chance that "track" flag added by ovsdb_idl_track_add_column
+ * by one module being overwritten by a following ovsdb_idl_add_column by
+ * another module. Before this is fixed in OVSDB IDL, we need to be careful
+ * about the order so that the "track" calls are after the "non-track"
+ * calls. */
ovsdb_idl_add_table(ovs_idl, &ovsrec_table_open_vswitch);
-ovsdb_idl_add_column(ovs_idl, &ovsrec_open_vswitch_col_external_ids);
ovsdb_idl_add_column(ovs_idl, &ovsrec_open_vswitch_col_other_config);
ovsdb_idl_add_column(ovs_idl, &ovsrec_open_vswitch_col_bridges);
ovsdb_idl_add_column(ovs_idl, &ovsrec_open_vswitch_col_datapaths);
ovsdb_idl_add_table(ovs_idl, &ovsrec_table_interface);
-ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_name);
-ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_bfd);
-ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_bfd_status);
-ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_type);
-ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_options);
-ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_ofport);
ovsdb_idl_add_table(ovs_idl, &ovsrec_table_port);
-ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_name);
-ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_interfaces);
-ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_external_ids);
ovsdb_idl_add_table(ovs_idl, &ovsrec_table_bridge);
ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_ports);
ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_name);
@@ -958,6 +954,16 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
bfd_register_ovs_idl(ovs_idl);
physical_register_ovs_idl(ovs_idl);
vif_plug_register_ovs_idl(ovs_idl);
+ovsdb_idl_track_add_column(ovs_idl, &ovsrec_open_vswitch_col_external_ids);
+ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_name);
+ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_bfd);
+ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_bfd_status);
+ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_type);
+ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_options);
+ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_ofport);
+ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_name);
+ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_interfaces);
+ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_external_ids);
}
#define SB_NODES \
--
2.30.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
