On Tue, Mar 11, 2025 at 11:39 AM Dumitru Ceara <dce...@redhat.com> wrote:
>
> 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. :)
If "pure_provider_switch" is fine with you, then I'm fine with it.
I'll change it in the next version.
I initially liked it. But I was not sure if others would be fine with
it or not :)
Numan
>
> >
> > 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
