On 8/19/22 17:35, Han Zhou wrote:
> On Fri, Aug 19, 2022 at 3:06 AM Dumitru Ceara <dce...@redhat.com> wrote:
>>
>> On 8/19/22 11:10, Numan Siddique wrote:
>>> On Fri, Aug 19, 2022 at 10:42 AM Han Zhou <hz...@ovn.org> wrote:
>>>>
>>>> On Thu, Aug 18, 2022 at 5:21 PM Han Zhou <hz...@ovn.org> wrote:
>>>>>
>>>>> When handling port_binding changes, it is possible that new
>>>>> local_datapaths are added, and the fields of the local_datapath, such
> as
>>>>> localnet_port, external_ports, etc. need to be updated for the newly
>>>>> added local_datapaths.
>>>>>
>>>>> This problem doesn't happen in most cases because the changes that
>>>>> trigger the new local_datapath addition usually trigger recomputes,
>>>>> but it may not always be the case. If recompute is not triggered,
>>>>> local_datapaths are not properly updated and will result in missing
> OVS
>>>>> flows. This is more likely to happen when we delay patch interface
>>>>> deletion in the future, or if we implement patch interface I-P.
>>>>>
>>>>> Signed-off-by: Han Zhou <hz...@ovn.org>
>>>>> ---
>>>>> controller/binding.c | 37 +++++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 37 insertions(+)
>>>>>
>>>>> diff --git a/controller/binding.c b/controller/binding.c
>>>>> index efb747d52..9f04e6f35 100644
>>>>> --- a/controller/binding.c
>>>>> +++ b/controller/binding.c
>>>>> @@ -2850,6 +2850,43 @@ delete_done:
>>>>> sset_destroy(&postponed_ports);
>>>>> cleanup_claimed_port_timestamps();
>>>>>
>>>>> + if (handled) {
>>>>> + /* There may be new local datapaths added by the above
> handling,
>>>> so go
>>>>> + * through each port_binding of newly added local datapaths
> to
>>>> update
>>>>> + * related local_datapaths if needed. */
>>>>> + struct shash bridge_mappings =
>>>>> + SHASH_INITIALIZER(&bridge_mappings);
>>>>> + add_ovs_bridge_mappings(b_ctx_in->ovs_table,
>>>>> + b_ctx_in->bridge_table,
>>>>> + &bridge_mappings);
>>
>> Now that we're touching this code, we might as well factor out the code
>> that populates 'bridge_mappings'. Maybe we should store these in the
>> input context, 'b_ctx_in'. It feels awkward that we build this shash
>> for each and every port binding even though it won't change and we only
>> need it for localnet ports. It can be a different patch/follow up but I
>> think we should do it at some point.
>
> Thanks Dumitru. That's not how it was in V2 before the rebase. In V2 the
> same code in the earlier loop was moved here so that it is populated only
> once for all PBs handling in this I-P run. In V3 after the rebase, I didn't
> want to break the completeness of the function handle_updated_port(), so I
> didn't delete the logic there, but that was the inefficient part: it is
> populated for every localnet port, but it is not a problem introduced with
> this patch. It can be optimized but I don't think it would really have
> impact to performance that can be measured (unless there are huge amount of
> localnet ports updated/added in one I-P run), so I'd leave it as is for
> this patch.
>
Sounds good.
>>
>>>>> + struct tracked_datapath *t_dp;
>>>>> + HMAP_FOR_EACH (t_dp, node, b_ctx_out->tracked_dp_bindings) {
>>>>> + if (t_dp->tracked_type != TRACKED_RESOURCE_NEW) {
>>>>> + continue;
>>>>> + }
>>>>> + struct sbrec_port_binding *target =
>>>>> + sbrec_port_binding_index_init_row(
>>>>> + b_ctx_in->sbrec_port_binding_by_datapath);
>>>>> + sbrec_port_binding_index_set_datapath(target, t_dp->dp);
>>>
>>> If you see the function add_local_datapath__() in
>>> controller/local_data.c, we almost do
>>> the same. i.e iterate through all the port bindings of the datapath.
>>>
>>> Does it make sense to move the below code there ? If so, we could
>>> avoid this loop.
>>> Seems to me we can move the functions - update_ld_localnet_port(),
>>> update_ld_external_ports() and
>>> update_ld_multichassis_ports() to local_data.c
>>>
>>> What do you think ?
>>
>> +1
>
> Numan, Dumitru, it doesn't really work that way. The loop needs to be after
> all the local datapath processings of the previous loop, so that we are
> handling the complete set of local datapaths. Otherwise, for example, PB1
> is a localnet port of DP1, PB2 is a patch port of DP1. When handling PB1,
> DP1 is not added to local DPs yet, and when handling PB2, DP1 is added to
> local DPs, but it won't go back to PB1 and add it for DP1. A loop after the
> previous one is required. Just follow the same logic in binding_run.
>
Ok, I was more thinking of the fact that most of the datapath management
seems to happen in local_data.c. But OK, we can refactor after the
release if needed.
>>
>> One more thing is that with this patch we call
>> update_ld_localnet_port()/update_ld_external_ports() twice:
>> - once in handle_updated_port()
>> - and again in binding_handle_port_binding_changes()
>>
>> The first call is not needed anymore.
>
> As mentioned earlier it wasn't the case of V2, but in V3 it was on purpose
> just in case the new function handle_updated_port() is called for some
> other purpose in the future.
I'm not sure I completely agree with this. I think we can find a way to
avoid calling the same thing twice while keeping the code clean and
error-proof. But it's a refactor that can happen later.
Could you please add some TODOs for these items (maybe some XXX
comments)? Then we can follow up after the release.
Also, I do see some ovn-kubernetes tests failing with this series
applied but I'm not sure how reliable those tests are today. So this is
just a note, to keep an eye out for potential failures that might be
introduced by this series. I triggered a re-run of the failed test here:
https://github.com/dceara/ovn/runs/7924751179?check_suite_focus=true
With that in mind and with the incremental you posted below (to fix the
leak):
Acked-by: Dumitru Ceara <dce...@redhat.com>
Thanks!
>
> Thanks,
> Han
>
>>
>> Thanks,
>> Dumitru
>>
>>>
>>> Thanks
>>> Numan
>>>
>>>>> +
>>>>> + SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target,
>>>>> + b_ctx_in->sbrec_port_binding_by_datapath) {
>>>>> + enum en_lport_type lport_type = get_lport_type(pb);
>>>>> + if (lport_type == LP_LOCALNET) {
>>>>> + update_ld_localnet_port(pb, &bridge_mappings,
>>>>> + b_ctx_out->egress_ifaces,
>>>>> +
> b_ctx_out->local_datapaths);
>>>>> + } else if (lport_type == LP_EXTERNAL) {
>>>>> + update_ld_external_ports(pb,
>>>> b_ctx_out->local_datapaths);
>>>>> + } else if (pb->n_additional_chassis) {
>>>>> + update_ld_multichassis_ports(pb,
>>>>> +
>>>> b_ctx_out->local_datapaths);
>>>>> + }
>>>>> + }
>>>>> + }
>>>>> + shash_destroy(&bridge_mappings);
>>>>> + }
>>>>> +
>>>>> if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn,
>>>>> b_ctx_in->port_table,
>>>>> b_ctx_in->qos_table,
>>>>> --
>>>>> 2.30.2
>>>>>
>>>>
>>>> Oops, I missed destroying the index row. Please consider below
> incremental
>>>> patch:
>>>> ----- 8><
>>>>
> ------------------------------------------------------------------------><8
>>>> ----
>>>> diff --git a/controller/binding.c b/controller/binding.c
>>>> index 9f04e6f35..beb5c8e17 100644
>>>> --- a/controller/binding.c
>>>> +++ b/controller/binding.c
>>>> @@ -2883,6 +2883,7 @@ delete_done:
>>>>
>>>> b_ctx_out->local_datapaths);
>>>> }
>>>> }
>>>> + sbrec_port_binding_index_destroy_row(target);
>>>> }
>>>> shash_destroy(&bridge_mappings);
>>>> }
>>>>
> ------------------------------------------------------------------------------------------------
>>>> Sorry for the mistake!
>>>>
>>>> Thanks,
>>>> Han
>>>> _______________________________________________
>>>> dev mailing list
>>>> d...@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>
>>> _______________________________________________
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
