On Fri, Sep 9, 2022 at 4:41 AM Dumitru Ceara <[email protected]> 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 <[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. > > > > 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 until some changes, although seem to be unrelated, do not work as expected. Thanks, Han > >> > >>>> 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
