On Fri, Aug 19, 2022 at 12:40 PM Dumitru Ceara <dce...@redhat.com> wrote:
>
> 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.
>
I would argue that calling the same thing twice itself shouldn't be a
concern as long as the "same thing" is idempotent and efficient. It is all
about enforcing the desired state. It is in theory the same as:
if (a != <something>) { // it is a common practice to ignore this check.
a = <something>
}
I believe there are many examples in this project and others that there are
many functions that may be called this way. What really matters is to
ensure that the functions are idempotent when we do this.
In this particular case, both the update_ld_localnet_port and
update_ld_external_ports are idempotent, and calling each of them just one
more (at most) time is nothing in terms of performance of ovn-controller,
so I am not worrying anything here. I could simply remove the handling in
the function and it is still correct, but I would worry (a little) for
doing that. I'd add a TODO if I did remove it from the function: xxx: we
don't call xyz here because all the callers of this function would call xyz
later for this PB.
> 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 again for the careful review!
Han
> 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
