Thanks for the review and sorry for the late response. On Tue, Aug 23, 2022 at 5:22 AM Dumitru Ceara <[email protected]> wrote: > > 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.") > 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 <[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? 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 "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
