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.
>>> + 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
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.
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
