On Tue, 2025-02-25 at 00:01 +0100, Dumitru Ceara wrote:
> It makes more sense to make these static (internal) in the module
> where
> they're actually used. They use "public" APIs to add advertised
> dynamic
> routes.
>
> Fixes: cd4ad2f56179 ("northd: Redistribution of NAT/LB routes.")
> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
I have no objections to moving these functions out of northd.c, so
overall this can have Acked-by me. However, since we are at it,
wouldn't it make sense to also move out the `build_parsed_routes` (and
related functions), as they too, seem to be only used from `en-
northd.c`.
Thanks,
Martin.
> ---
> northd/en-advertised-route-sync.c | 233
> ++++++++++++++++++++++++++++++
> northd/northd.c | 231 ----------------------------
> -
> northd/northd.h | 14 --
> 3 files changed, 233 insertions(+), 245 deletions(-)
>
> diff --git a/northd/en-advertised-route-sync.c b/northd/en-
> advertised-route-sync.c
> index 794a87cb3b..48a0b05deb 100644
> --- a/northd/en-advertised-route-sync.c
> +++ b/northd/en-advertised-route-sync.c
> @@ -20,7 +20,9 @@
> #include "northd.h"
>
> #include "en-advertised-route-sync.h"
> +#include "en-lr-nat.h"
> #include "en-lr-stateful.h"
> +#include "lb.h"
> #include "lib/stopwatch-names.h"
> #include "openvswitch/hmap.h"
> #include "ovn-util.h"
> @@ -163,6 +165,237 @@ en_advertised_route_sync_run(struct engine_node
> *node, void *data OVS_UNUSED)
> engine_set_node_state(node, EN_UPDATED);
> }
>
> +/* This function adds new parsed route for each entry in lr_nat
> record
> + * to "routes". Logical port of the route is set to "advertising_op"
> and
> + * tracked port is set to NAT's distributed gw port. If NAT doesn't
> have
> + * DGP (for example if it's set on gateway router), no tracked port
> will
> + * be set.*/
> +static void
> +build_nat_parsed_route_for_port(const struct ovn_port
> *advertising_op,
> + const struct lr_nat_record *lr_nat,
> + const struct hmap *ls_ports,
> + struct hmap *routes)
> +{
> + const struct ovn_datapath *advertising_od = advertising_op->od;
> +
> + for (size_t i = 0; i < lr_nat->n_nat_entries; i++) {
> + const struct ovn_nat *nat = &lr_nat->nat_entries[i];
> + if (!nat->is_valid) {
> + continue;
> + }
> +
> + int plen = nat_entry_is_v6(nat) ? 128 : 32;
> + struct in6_addr prefix;
> + ip46_parse(nat->nb->external_ip, &prefix);
> +
> + const struct ovn_port *tracked_port =
> + nat->is_distributed
> + ? ovn_port_find(ls_ports, nat->nb->logical_port)
> + : nat->l3dgw_port;
> + parsed_route_add(advertising_od, NULL, &prefix, plen, false,
> + nat->nb->external_ip, advertising_op, 0,
> false,
> + false, NULL, ROUTE_SOURCE_NAT, &nat->nb-
> >header_,
> + tracked_port, routes);
> + }
> +}
> +
> +/* Generate parsed routes for NAT external IPs in lr_nat, for each
> ovn port
> + * in "od" that has enabled redistribution of NAT adresses.*/
> +static void
> +build_nat_parsed_routes(const struct ovn_datapath *od,
> + const struct lr_nat_record *lr_nat,
> + const struct hmap *ls_ports,
> + struct hmap *routes)
> +{
> + const struct ovn_port *op;
> + HMAP_FOR_EACH (op, dp_node, &od->ports) {
> + if (!drr_mode_NAT_is_set(op->dynamic_routing_redistribute))
> {
> + continue;
> + }
> +
> + build_nat_parsed_route_for_port(op, lr_nat, ls_ports,
> routes);
> + }
> +}
> +
> +/* Similar to build_nat_parsed_routes, this function generates
> parsed routes
> + * for nat records in neighboring routers. For each ovn port in "od"
> that has
> + * enabled redistribution of NAT adresses, look up their neighbors
> (either
> + * directly routers, or routers connected through common LS) and
> advertise
> + * thier external NAT IPs too.*/
> +static void
> +build_nat_connected_parsed_routes(
> + const struct ovn_datapath *od,
> + const struct lr_stateful_table *lr_stateful_table,
> + const struct hmap *ls_ports,
> + struct hmap *routes)
> +{
> + const struct ovn_port *op;
> + HMAP_FOR_EACH (op, dp_node, &od->ports) {
> + if (!drr_mode_NAT_is_set(op->dynamic_routing_redistribute))
> {
> + continue;
> + }
> +
> + if (!op->peer) {
> + continue;
> + }
> +
> + struct ovn_datapath *peer_od = op->peer->od;
> + ovs_assert(peer_od->nbs || peer_od->nbr);
> +
> + const struct ovn_port *peer_port = NULL;
> + /* This is a directly connected LR peer. */
> + if (peer_od->nbr) {
> + peer_port = op->peer;
> +
> + const struct lr_stateful_record *peer_lr_stateful =
> + lr_stateful_table_find_by_index(lr_stateful_table,
> + peer_od->index);
> + if (!peer_lr_stateful) {
> + continue;
> + }
> +
> + /* Advertise peer's NAT routes via the local port too.
> */
> + build_nat_parsed_route_for_port(op, peer_lr_stateful-
> >lrnat_rec,
> + ls_ports, routes);
> + return;
> + }
> +
> + /* This peer is LSP, we need to check all connected router
> ports
> + * for NAT.*/
> + for (size_t i = 0; i < peer_od->n_router_ports; i++) {
> + peer_port = peer_od->router_ports[i]->peer;
> + if (peer_port == op) {
> + /* Skip advertising router. */
> + continue;
> + }
> +
> + const struct lr_stateful_record *peer_lr_stateful =
> + lr_stateful_table_find_by_index(lr_stateful_table,
> + peer_port->od-
> >index);
> + if (!peer_lr_stateful) {
> + continue;
> + }
> +
> + /* Advertise peer's NAT routes via the local port too.
> */
> + build_nat_parsed_route_for_port(op, peer_lr_stateful-
> >lrnat_rec,
> + ls_ports, routes);
> + }
> + }
> +}
> +
> +/* This function adds new parsed route for each IP in lb_ips to
> "routes".*/
> +static void
> +build_lb_parsed_route_for_port(const struct ovn_port
> *advertising_op,
> + const struct ovn_port *tracked_port,
> + const struct ovn_lb_ip_set *lb_ips,
> + struct hmap *routes)
> +{
> + const struct ovn_datapath *advertising_od = advertising_op->od;
> +
> + const char *ip_address;
> + SSET_FOR_EACH (ip_address, &lb_ips->ips_v4) {
> + struct in6_addr prefix;
> + ip46_parse(ip_address, &prefix);
> + parsed_route_add(advertising_od, NULL, &prefix, 32, false,
> + ip_address, advertising_op, 0, false,
> false,
> + NULL, ROUTE_SOURCE_LB, &advertising_op-
> >nbrp->header_,
> + tracked_port, routes);
> + }
> + SSET_FOR_EACH (ip_address, &lb_ips->ips_v6) {
> + struct in6_addr prefix;
> + ip46_parse(ip_address, &prefix);
> + parsed_route_add(advertising_od, NULL, &prefix, 128, false,
> + ip_address, advertising_op, 0, false,
> false,
> + NULL, ROUTE_SOURCE_LB, &advertising_op-
> >nbrp->header_,
> + tracked_port, routes);
> + }
> +}
> +
> +/* Similar to build_lb_parsed_routes, this function generates parsed
> routes
> + * for LB VIPs of neighboring routers. For each ovn port in "od"
> that has
> + * enabled redistribution of LB VIPs, look up their neighbors
> (either
> + * directly routers, or routers connected through common LS) and
> advertise
> + * thier LB VIPs too.*/
> +static void
> +build_lb_connected_parsed_routes(
> + const struct ovn_datapath *od,
> + const struct lr_stateful_table *lr_stateful_table,
> + struct hmap *routes)
> +{
> + const struct ovn_port *op;
> + HMAP_FOR_EACH (op, dp_node, &od->ports) {
> + if (!drr_mode_LB_is_set(op->dynamic_routing_redistribute)) {
> + continue;
> + }
> +
> + if (!op->peer) {
> + continue;
> + }
> +
> + struct ovn_datapath *peer_od = op->peer->od;
> + ovs_assert(peer_od->nbs || peer_od->nbr);
> +
> + const struct lr_stateful_record *lr_stateful_rec;
> + const struct ovn_port *peer_port = NULL;
> + /* This is directly connected LR peer. */
> + if (peer_od->nbr) {
> + lr_stateful_rec = lr_stateful_table_find_by_index(
> + lr_stateful_table, peer_od->index);
> + peer_port = op->peer;
> + build_lb_parsed_route_for_port(op, peer_port,
> + lr_stateful_rec->lb_ips,
> routes);
> + return;
> + }
> +
> + /* This peer is LSP, we need to check all connected router
> ports for
> + * LBs.*/
> + for (size_t i = 0; i < peer_od->n_router_ports; i++) {
> + peer_port = peer_od->router_ports[i]->peer;
> + if (peer_port == op) {
> + /* no need to check for LBs on ovn_port that
> initiated this
> + * function.*/
> + continue;
> + }
> + lr_stateful_rec = lr_stateful_table_find_by_index(
> + lr_stateful_table, peer_port->od->index);
> +
> + build_lb_parsed_route_for_port(op, peer_port,
> + lr_stateful_rec->lb_ips,
> routes);
> + }
> + }
> +}
> +
> +static void
> +build_lb_parsed_routes(const struct ovn_datapath *od,
> + const struct ovn_lb_ip_set *lb_ips,
> + struct hmap *routes)
> +{
> + const struct ovn_port *op;
> + HMAP_FOR_EACH (op, dp_node, &od->ports) {
> + if (!drr_mode_LB_is_set(op->dynamic_routing_redistribute)) {
> + continue;
> + }
> +
> + /* Traffic processed by a load balancer is:
> + * - handled by the chassis where a gateway router is bound
> + * OR
> + * - always redirected to a distributed gateway router port
> + *
> + * Advertise the LB IPs via all 'op' if this is a gateway
> router or
> + * throuh all DGPs of this distributed router otherwise. */
> + struct ovn_port *op_ = NULL;
> + size_t n_tracked_ports = !od->is_gw_router ? od-
> >n_l3dgw_ports : 1;
> + struct ovn_port **tracked_ports = !od->is_gw_router
> + ? od->l3dgw_ports
> + : &op_;
> +
> + for (size_t i = 0; i < n_tracked_ports; i++) {
> + build_lb_parsed_route_for_port(op, tracked_ports[i],
> lb_ips,
> + routes);
> + }
> + }
> +}
> +
> void *
> en_dynamic_routes_init(struct engine_node *node OVS_UNUSED,
> struct engine_arg *arg OVS_UNUSED)
> diff --git a/northd/northd.c b/northd/northd.c
> index a91e48ac26..fdeb1d4670 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -11323,237 +11323,6 @@ build_parsed_routes(const struct
> ovn_datapath *od, const struct hmap *lr_ports,
> }
> }
>
> -/* This function adds new parsed route for each entry in lr_nat
> record
> - * to "routes". Logical port of the route is set to "advertising_op"
> and
> - * tracked port is set to NAT's distributed gw port. If NAT doesn't
> have
> - * DGP (for example if it's set on gateway router), no tracked port
> will
> - * be set.*/
> -static void
> -build_nat_parsed_route_for_port(const struct ovn_port
> *advertising_op,
> - const struct lr_nat_record *lr_nat,
> - const struct hmap *ls_ports,
> - struct hmap *routes)
> -{
> - const struct ovn_datapath *advertising_od = advertising_op->od;
> -
> - for (size_t i = 0; i < lr_nat->n_nat_entries; i++) {
> - const struct ovn_nat *nat = &lr_nat->nat_entries[i];
> - if (!nat->is_valid) {
> - continue;
> - }
> -
> - int plen = nat_entry_is_v6(nat) ? 128 : 32;
> - struct in6_addr prefix;
> - ip46_parse(nat->nb->external_ip, &prefix);
> -
> - const struct ovn_port *tracked_port =
> - nat->is_distributed
> - ? ovn_port_find(ls_ports, nat->nb->logical_port)
> - : nat->l3dgw_port;
> - parsed_route_add(advertising_od, NULL, &prefix, plen, false,
> - nat->nb->external_ip, advertising_op, 0,
> false,
> - false, NULL, ROUTE_SOURCE_NAT, &nat->nb-
> >header_,
> - tracked_port, routes);
> - }
> -}
> -
> -/* Generate parsed routes for NAT external IPs in lr_nat, for each
> ovn port
> - * in "od" that has enabled redistribution of NAT adresses.*/
> -void
> -build_nat_parsed_routes(const struct ovn_datapath *od,
> - const struct lr_nat_record *lr_nat,
> - const struct hmap *ls_ports,
> - struct hmap *routes)
> -{
> - const struct ovn_port *op;
> - HMAP_FOR_EACH (op, dp_node, &od->ports) {
> - if (!drr_mode_NAT_is_set(op->dynamic_routing_redistribute))
> {
> - continue;
> - }
> -
> - build_nat_parsed_route_for_port(op, lr_nat, ls_ports,
> routes);
> - }
> -}
> -
> -/* Similar to build_nat_parsed_routes, this function generates
> parsed routes
> - * for nat records in neighboring routers. For each ovn port in "od"
> that has
> - * enabled redistribution of NAT adresses, look up their neighbors
> (either
> - * directly routers, or routers connected through common LS) and
> advertise
> - * thier external NAT IPs too.*/
> -void
> -build_nat_connected_parsed_routes(
> - const struct ovn_datapath *od,
> - const struct lr_stateful_table *lr_stateful_table,
> - const struct hmap *ls_ports,
> - struct hmap *routes)
> -{
> - const struct ovn_port *op;
> - HMAP_FOR_EACH (op, dp_node, &od->ports) {
> - if (!drr_mode_NAT_is_set(op->dynamic_routing_redistribute))
> {
> - continue;
> - }
> -
> - if (!op->peer) {
> - continue;
> - }
> -
> - struct ovn_datapath *peer_od = op->peer->od;
> - ovs_assert(peer_od->nbs || peer_od->nbr);
> -
> - const struct ovn_port *peer_port = NULL;
> - /* This is a directly connected LR peer. */
> - if (peer_od->nbr) {
> - peer_port = op->peer;
> -
> - const struct lr_stateful_record *peer_lr_stateful =
> - lr_stateful_table_find_by_index(lr_stateful_table,
> - peer_od->index);
> - if (!peer_lr_stateful) {
> - continue;
> - }
> -
> - /* Advertise peer's NAT routes via the local port too.
> */
> - build_nat_parsed_route_for_port(op, peer_lr_stateful-
> >lrnat_rec,
> - ls_ports, routes);
> - return;
> - }
> -
> - /* This peer is LSP, we need to check all connected router
> ports
> - * for NAT.*/
> - for (size_t i = 0; i < peer_od->n_router_ports; i++) {
> - peer_port = peer_od->router_ports[i]->peer;
> - if (peer_port == op) {
> - /* Skip advertising router. */
> - continue;
> - }
> -
> - const struct lr_stateful_record *peer_lr_stateful =
> - lr_stateful_table_find_by_index(lr_stateful_table,
> - peer_port->od-
> >index);
> - if (!peer_lr_stateful) {
> - continue;
> - }
> -
> - /* Advertise peer's NAT routes via the local port too.
> */
> - build_nat_parsed_route_for_port(op, peer_lr_stateful-
> >lrnat_rec,
> - ls_ports, routes);
> - }
> - }
> -}
> -
> -/* This function adds new parsed route for each IP in lb_ips to
> "routes".*/
> -static void
> -build_lb_parsed_route_for_port(const struct ovn_port
> *advertising_op,
> - const struct ovn_port *tracked_port,
> - const struct ovn_lb_ip_set *lb_ips,
> - struct hmap *routes)
> -{
> - const struct ovn_datapath *advertising_od = advertising_op->od;
> -
> - const char *ip_address;
> - SSET_FOR_EACH (ip_address, &lb_ips->ips_v4) {
> - struct in6_addr prefix;
> - ip46_parse(ip_address, &prefix);
> - parsed_route_add(advertising_od, NULL, &prefix, 32, false,
> - ip_address, advertising_op, 0, false,
> false,
> - NULL, ROUTE_SOURCE_LB, &advertising_op-
> >nbrp->header_,
> - tracked_port, routes);
> - }
> - SSET_FOR_EACH (ip_address, &lb_ips->ips_v6) {
> - struct in6_addr prefix;
> - ip46_parse(ip_address, &prefix);
> - parsed_route_add(advertising_od, NULL, &prefix, 128, false,
> - ip_address, advertising_op, 0, false,
> false,
> - NULL, ROUTE_SOURCE_LB, &advertising_op-
> >nbrp->header_,
> - tracked_port, routes);
> - }
> -}
> -
> -/* Similar to build_lb_parsed_routes, this function generates parsed
> routes
> - * for LB VIPs of neighboring routers. For each ovn port in "od"
> that has
> - * enabled redistribution of LB VIPs, look up their neighbors
> (either
> - * directly routers, or routers connected through common LS) and
> advertise
> - * thier LB VIPs too.*/
> -void
> -build_lb_connected_parsed_routes(
> - const struct ovn_datapath *od,
> - const struct lr_stateful_table *lr_stateful_table,
> - struct hmap *routes)
> -{
> - const struct ovn_port *op;
> - HMAP_FOR_EACH (op, dp_node, &od->ports) {
> - if (!drr_mode_LB_is_set(op->dynamic_routing_redistribute)) {
> - continue;
> - }
> -
> - if (!op->peer) {
> - continue;
> - }
> -
> - struct ovn_datapath *peer_od = op->peer->od;
> - ovs_assert(peer_od->nbs || peer_od->nbr);
> -
> - const struct lr_stateful_record *lr_stateful_rec;
> - const struct ovn_port *peer_port = NULL;
> - /* This is directly connected LR peer. */
> - if (peer_od->nbr) {
> - lr_stateful_rec = lr_stateful_table_find_by_index(
> - lr_stateful_table, peer_od->index);
> - peer_port = op->peer;
> - build_lb_parsed_route_for_port(op, peer_port,
> - lr_stateful_rec->lb_ips,
> routes);
> - return;
> - }
> -
> - /* This peer is LSP, we need to check all connected router
> ports for
> - * LBs.*/
> - for (size_t i = 0; i < peer_od->n_router_ports; i++) {
> - peer_port = peer_od->router_ports[i]->peer;
> - if (peer_port == op) {
> - /* no need to check for LBs on ovn_port that
> initiated this
> - * function.*/
> - continue;
> - }
> - lr_stateful_rec = lr_stateful_table_find_by_index(
> - lr_stateful_table, peer_port->od->index);
> -
> - build_lb_parsed_route_for_port(op, peer_port,
> - lr_stateful_rec->lb_ips,
> routes);
> - }
> - }
> -}
> -
> -void
> -build_lb_parsed_routes(const struct ovn_datapath *od,
> - const struct ovn_lb_ip_set *lb_ips,
> - struct hmap *routes)
> -{
> - const struct ovn_port *op;
> - HMAP_FOR_EACH (op, dp_node, &od->ports) {
> - if (!drr_mode_LB_is_set(op->dynamic_routing_redistribute)) {
> - continue;
> - }
> -
> - /* Traffic processed by a load balancer is:
> - * - handled by the chassis where a gateway router is bound
> - * OR
> - * - always redirected to a distributed gateway router port
> - *
> - * Advertise the LB IPs via all 'op' if this is a gateway
> router or
> - * throuh all DGPs of this distributed router otherwise. */
> - struct ovn_port *op_ = NULL;
> - size_t n_tracked_ports = !od->is_gw_router ? od-
> >n_l3dgw_ports : 1;
> - struct ovn_port **tracked_ports = !od->is_gw_router
> - ? od->l3dgw_ports
> - : &op_;
> -
> - for (size_t i = 0; i < n_tracked_ports; i++) {
> - build_lb_parsed_route_for_port(op, tracked_ports[i],
> lb_ips,
> - routes);
> - }
> - }
> -
> -}
> struct ecmp_route_list_node {
> struct ovs_list list_node;
> uint16_t id; /* starts from 1 */
> diff --git a/northd/northd.h b/northd/northd.h
> index de0d924c5f..12fccd7751 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -833,20 +833,6 @@ void route_policies_destroy(struct
> route_policies_data *);
> void build_parsed_routes(const struct ovn_datapath *, const struct
> hmap *,
> const struct hmap *, struct hmap *, struct
> simap *,
> struct hmap *);
> -void build_nat_parsed_routes(const struct ovn_datapath *,
> - const struct lr_nat_record *,
> - const struct hmap *ls_ports,
> - struct hmap *routes);
> -void build_nat_connected_parsed_routes(const struct ovn_datapath *,
> - const struct
> lr_stateful_table *,
> - const struct hmap *ls_ports,
> - struct hmap *routes);
> -void build_lb_parsed_routes(const struct ovn_datapath *,
> - const struct ovn_lb_ip_set *,
> - struct hmap *);
> -void build_lb_connected_parsed_routes(const struct ovn_datapath *,
> - const struct lr_stateful_table
> *,
> - struct hmap *routes);
> uint32_t get_route_table_id(struct simap *, const char *);
> void routes_init(struct routes_data *);
> void routes_destroy(struct routes_data *);
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
