On 3/11/25 4:35 PM, Numan Siddique wrote:
> On Mon, Mar 10, 2025 at 12:22 PM Dumitru Ceara <dce...@redhat.com> wrote:
>>
>> On 2/28/25 10:40 PM, num...@ovn.org wrote:
>>> From: Numan Siddique <num...@ovn.org>
>>>
>>> This flag is added to the external_ids column of SB Datapath_binding
>>> table only if a logical switch meets the following
>>> criteria:
>>> - has one or more localnet ports.
>>> - has one or more router ports and all its peers are distributed
>>> gateway ports.
>>>
>>> ovn-controller in the following patch makes use of this flag.
>>>
>>> Signed-off-by: Numan Siddique <num...@ovn.org>
>>> ---
>>
>> Hi Numan,
>>
>>> northd/northd.c | 31 +++++++++++++++++++++---
>>> northd/northd.h | 4 +++
>>> tests/ovn-northd.at | 59 +++++++++++++++++++++++++++++++++++++++++++++
>>
>> I think we should document this new external_id in ovn-sb.xml in the
>> "OVN_Northbound Relationship" section. It's not that obvious what it
>> does just from the name, in my opinion.
>
> Ack.
>
>
>>
>>> 3 files changed, 90 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/northd/northd.c b/northd/northd.c
>>> index 488bfdf697..821a653b71 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -781,6 +781,15 @@ ovn_datapath_update_external_ids(struct ovn_datapath
>>> *od)
>>> smap_add_format(&ids, "fdb_age_threshold",
>>> "%u", age_threshold);
>>> }
>>> +
>>> + /* Add the flag 'only_dgp_peer_ports=true' to the SB datapath's
>>> + * external_ids if
>>> + * - It has (a) localnet port(s) and
>>> + * - All its logical router ports' peers are DGPs. */
>>> + if (od->n_router_ports && od->n_router_ports ==
>>> od->n_peer_dgw_ports
>>> + && od->n_localnet_ports) {
>>> + smap_add(&ids, "only_dgp_peer_ports", "true");
>>
>> Nit: this feels a bit inaccurate. As the comment itself says the switch
>> has N+M non-VIF ports out of which N are peers of DGPs and M are
>> localnet. I don't have a better suggestion however. I was thinking of
>> "needs_peer_datapaths" but that's also a bit ugly. If we can't find a
>> better name we can leave it as "only_dgp_peer_ports"
>>
>
> Yeah, I couldn't find a better name. Initially I thought of using
> "pure_provider_switch" to
> indicate this logical switch has only dgp ports and localnet ports.
> But I didn't like the name :)
>
I think I like that one more but OK. :)
>
> I'll keep thinking for a better name.
>>> + }
>>> }
>>>
>>> /* Set snat-ct-zone */
>>> @@ -867,6 +876,20 @@ parse_dynamic_routing_redistribute(
>>> return out;
>>> }
>>>
>>> +static void
>>> +update_datapaths_external_ids(struct ovn_datapaths *ls_datapaths,
>>> + struct ovn_datapaths *lr_datapaths)
>>> +{
>>> + struct ovn_datapath *od;
>>> + HMAP_FOR_EACH (od, key_node, &ls_datapaths->datapaths) {
>>> + ovn_datapath_update_external_ids(od);
>>> + }
>>> +
>>> + HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
>>> + ovn_datapath_update_external_ids(od);
>>> + }
>>> +}
>>> +
>>> static void
>>> join_datapaths(const struct nbrec_logical_switch_table *nbrec_ls_table,
>>> const struct nbrec_logical_router_table *nbrec_lr_table,
>>> @@ -919,7 +942,6 @@ join_datapaths(const struct nbrec_logical_switch_table
>>> *nbrec_ls_table,
>>> od->nbs = nbs;
>>> ovs_list_remove(&od->list);
>>> ovs_list_push_back(both, &od->list);
>>> - ovn_datapath_update_external_ids(od);
>>> } else {
>>> od = ovn_datapath_create(datapaths, &nbs->header_.uuid,
>>> nbs, NULL, NULL);
>>> @@ -947,7 +969,6 @@ join_datapaths(const struct nbrec_logical_switch_table
>>> *nbrec_ls_table,
>>> od->nbr = nbr;
>>> ovs_list_remove(&od->list);
>>> ovs_list_push_back(both, &od->list);
>>> - ovn_datapath_update_external_ids(od);
>>> } else {
>>> /* Can't happen! */
>>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
>>> 1);
>>> @@ -1121,11 +1142,9 @@ build_datapaths(struct ovsdb_idl_txn *ovnsb_txn,
>>> if (od->sb->tunnel_key != od->tunnel_key) {
>>> sbrec_datapath_binding_set_tunnel_key(od->sb, od->tunnel_key);
>>> }
>>> - ovn_datapath_update_external_ids(od);
>>> }
>>> LIST_FOR_EACH (od, list, &nb_only) {
>>> od->sb = sbrec_datapath_binding_insert(ovnsb_txn);
>>> - ovn_datapath_update_external_ids(od);
>>> sbrec_datapath_binding_set_tunnel_key(od->sb, od->tunnel_key);
>>> }
>>> ovn_destroy_tnlids(&dp_tnlids);
>>> @@ -2580,6 +2599,8 @@ join_logical_ports(const struct
>>> sbrec_port_binding_table *sbrec_pb_table,
>>> /* Only used for the router type LSP whose peer is l3dgw_port
>>> */
>>> op->peer->enable_router_port_acl = smap_get_bool(
>>> &op->peer->nbsp->options, "enable_router_port_acl",
>>> false);
>>> +
>>> + op->peer->od->n_peer_dgw_ports++;
>>> }
>>> }
>>>
>>> @@ -18832,6 +18853,8 @@ ovnnb_db_run(struct northd_input *input_data,
>>> input_data->sbrec_ha_chassis_grp_by_name,
>>> &data->ls_datapaths.datapaths,
>>> &data->lr_datapaths.datapaths,
>>> &data->ls_ports, &data->lr_ports);
>>> + update_datapaths_external_ids(&data->ls_datapaths,
>>> + &data->lr_datapaths);
>>> build_lb_port_related_data(ovnsb_txn,
>>> input_data->sbrec_service_monitor_table,
>>> input_data->svc_monitor_mac,
>>> diff --git a/northd/northd.h b/northd/northd.h
>>> index 1a7afe9027..0f7d4c3432 100644
>>> --- a/northd/northd.h
>>> +++ b/northd/northd.h
>>> @@ -373,6 +373,10 @@ struct ovn_datapath {
>>> size_t n_router_ports;
>>> size_t n_allocated_router_ports;
>>>
>>> + /* Indicates the number of router port's peers which are distributed
>>> + * gateway ports (DGPs). */
>>> + size_t n_peer_dgw_ports;
>>> +
>>> struct hmap port_tnlids;
>>> uint32_t port_key_hint;
>>>
>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>>> index ee3103d44a..3ec2afa588 100644
>>> --- a/tests/ovn-northd.at
>>> +++ b/tests/ovn-northd.at
>>> @@ -14656,6 +14656,65 @@ check rbr_match_custom
>>>
>>> AT_CLEANUP
>>>
>>> +OVN_FOR_EACH_NORTHD_NO_HV([
>>> +AT_SETUP([ovn-northd -- only_dgp_peer_ports flag in SB datapath_binding])
>>> +ovn_start
>>> +
>>> +check ovn-nbctl ls-add sw0
>>> +check ovn-nbctl --wait=sb sync
>>> +
>>> +# Check that there is no option "only_dgp_peer_ports" in the
>>> +# SB.datapath_binding's external_ids.
>>> +AT_CHECK([ovn-sbctl get datapath_binding sw0
>>> external_ids:only_dgp_peer_ports], [1], [ignore], [ignore])
>>> +
>>> +# only_dgp_peer_ports flag will be set in the SB:datapath_binding's
>>> external_ids
>>> +# only if the logical switch has
>>> +# - one or more localnet ports.
>>> +# - All its router ports' peeers are DGPs if one or more router
>>> +# ports are present.
>>> +
>>> +check ovn-nbctl --wait=sb lsp-add sw0 ln-sw0 -- lsp-set-type ln-sw0
>>> localnet
>>> +
>>> +AT_CHECK([ovn-sbctl get datapath_binding sw0
>>> external_ids:only_dgp_peer_ports], [1], [ignore], [ignore])
>>> +
>>> +check ovn-nbctl lsp-add sw0 sw0-lr0 -- lsp-set-type sw0-lr0 router
>>> +check ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0
>>> +
>>> +AT_CHECK([ovn-sbctl get datapath_binding sw0
>>> external_ids:only_dgp_peer_ports], [1], [ignore], [ignore])
>>> +
>>> +check ovn-nbctl lr-add lr0
>>> +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw0 00:00:02:01:02:03
>>> 172.16.1.1/24
>>> +
>>> +AT_CHECK([ovn-sbctl get datapath_binding sw0
>>> external_ids:only_dgp_peer_ports], [1], [ignore], [ignore])
>>> +
>>> +check ovn-nbctl --wait=sb lrp-set-gateway-chassis lr0-sw0 ovn-gw-1
>>> +AT_CHECK([ovn-sbctl get datapath_binding sw0
>>> external_ids:only_dgp_peer_ports], [0], [dnl
>>> +"true"
>>> +])
>>
>> Nit: we could use fetch_column.
> I tried fetch_column, but it didn't seem to work for external_ids:<key> .
> I'll see if I can use it. Maybe I was doing something wrong.
>
Maybe it's easier with check_row_count? We do the following in some tests:
check_row_count nb:NAT 1 options:stateless=true
> Thanks
> Numan
>
>>
>>> +
>>> +check ovn-nbctl lsp-add sw0 sw0-lr1 -- lsp-set-type sw0-lr1 router
>>> +check ovn-nbctl --wait=sb lsp-set-options sw0-lr1 router-port=lr1-sw0
>>> +check ovn-nbctl lr-add lr1
>>> +check ovn-nbctl --wait=sb lrp-add lr1 lr1-sw0 00:00:02:01:02:03
>>> 172.16.1.1/24
>>> +
>>> +AT_CHECK([ovn-sbctl get datapath_binding sw0
>>> external_ids:only_dgp_peer_ports], [1], [ignore], [ignore])
>>> +
>>> +check ovn-nbctl --wait=sb lrp-set-gateway-chassis lr1-sw0 ovn-gw-1
>>> +AT_CHECK([ovn-sbctl get datapath_binding sw0
>>> external_ids:only_dgp_peer_ports], [0], [dnl
>>> +"true"
>>> +])
>>
>> Nit: we could use fetch_column.
>>
>>> +
>>> +# Just add a normal port.
>>> +check ovn-nbctl --wait=sb lsp-add sw0 sw0p1
>>> +
>>> +check ovn-nbctl --wait=sb lrp-set-gateway-chassis lr1-sw0 ovn-gw-1
>>> +AT_CHECK([ovn-sbctl get datapath_binding sw0
>>> external_ids:only_dgp_peer_ports], [0], [dnl
>>> +"true"
>>> +])
>>
>> Nit: we could use fetch_column.
>>
>>> +
>>> +AT_CLEANUP
>>> +])
>>> +
>>> OVN_FOR_EACH_NORTHD_NO_HV([
>>> AT_SETUP([ACL mismatched tiers])
>>> ovn_start
>>
>> Regards,
>> Dumitru
>>
>>
>
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
