Hi Han,
On 8/23/22 06:44, Han Zhou wrote:
> On Tue, Jul 5, 2022 at 3:41 PM Han Zhou <[email protected]> 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 <[email protected]>
>> ---
>> 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_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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
