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 <dce...@redhat.com> wrote:
>>
>> Hi Han,
>>
>> On 8/23/22 06:44, Han Zhou wrote:
>>> On Tue, Jul 5, 2022 at 3:41 PM Han Zhou <hz...@ovn.org> 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 <hz...@ovn.org>
>>>> ---
>>>> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
