[ovs-dev,v2,08/32] northd: Move connected routes to route engine.

previously connected routes (so routes based on the networks of the
LRPs) where handled separately from static routes.

In order to later announce routes from ovn to the fabric we need to have
a central overview over all routes we want to announce. This includes
routes based on Logical_Router_Static_Routes as well as the networks on
Logical_Router_Port.

With this change the "routes" engine node will output a complete list of
all routes in the system. This also serves as a future integration point
to integrate routes learned from outside of OVN.

Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
---
 northd/northd.c | 270 ++++++++++++++++++++++++++++++------------------
 northd/northd.h |  11 +-
 2 files changed, 178 insertions(+), 103 deletions(-)

> previously connected routes (so routes based on the networks of the
> LRPs) where handled separately from static routes.
> 
> In order to later announce routes from ovn to the fabric we need to have
> a central overview over all routes we want to announce. This includes
> routes based on Logical_Router_Static_Routes as well as the networks on
> Logical_Router_Port.
> 
> With this change the "routes" engine node will output a complete list of
> all routes in the system. This also serves as a future integration point
> to integrate routes learned from outside of OVN.

Hi Felix,

This patch needs a respin. Some nits inline.

Regards,
Lorenzo

> 
> Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> ---
>  northd/northd.c | 270 ++++++++++++++++++++++++++++++------------------
>  northd/northd.h |  11 +-
>  2 files changed, 178 insertions(+), 103 deletions(-)
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index 1265e3c81..90ddc2acb 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -11071,6 +11071,20 @@ parsed_route_lookup(struct hmap *routes, size_t hash,
>              continue;
>          }
>  
> +        if (pr->source != new_pr->source) {
> +            continue;
> +        }
> +
> +        if (!pr->nexthop != !new_pr->nexthop) {
> +            continue;

why not if (pr->nexthop != new_pr->nexthop) ?

> +        }
> +
> +        if (pr->nexthop &&
> +                memcmp(pr->nexthop, new_pr->nexthop,
> +                       sizeof(struct in6_addr))) {

I guess we can use ipv6_addr_equals() here.

> +            continue;
> +        }
> +
>          if (memcmp(&pr->prefix, &new_pr->prefix, sizeof(struct in6_addr))) {
>              continue;
>          }
> @@ -11111,6 +11125,9 @@ parsed_route_lookup(struct hmap *routes, size_t hash,
>  
>  static void
>  parsed_route_free(struct parsed_route *pr) {
> +    if (pr->nexthop) {

you can remove if () here

> +        free(pr->nexthop);
> +    }
>      if (pr->lrp_addr_s) {
>          free(pr->lrp_addr_s);
>      }
> @@ -11119,31 +11136,77 @@ parsed_route_free(struct parsed_route *pr) {
>  }
>  
>  static void
> -parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> +parsed_route_add(const struct ovn_datapath *od,
> +                 struct in6_addr *nexthop,
> +                 const struct in6_addr prefix,
> +                 unsigned int plen,
> +                 bool is_discard_route,
> +                 const char *lrp_addr_s,
> +                 const struct ovn_port *out_port,
> +                 const struct nbrec_logical_router_static_route *route,
> +                 uint32_t route_table_id,
> +                 bool is_src_route,
> +                 bool ecmp_symmetric_reply,
> +                 enum route_source source,
> +                 struct hmap *routes)
> +{
> +
> +    struct parsed_route *new_pr = xzalloc(sizeof *new_pr);
> +    new_pr->prefix = prefix;
> +    new_pr->plen = plen;
> +    new_pr->nexthop = nexthop;
> +    new_pr->route_table_id = route_table_id;
> +    new_pr->is_src_route = is_src_route;
> +    new_pr->hash = route_hash(new_pr);
> +    new_pr->nbr = od->nbr;
> +    new_pr->ecmp_symmetric_reply = ecmp_symmetric_reply;
> +    new_pr->is_discard_route = is_discard_route;
> +    if (!is_discard_route) {
> +        new_pr->lrp_addr_s = xstrdup(lrp_addr_s);
> +    }
> +    new_pr->out_port = out_port;
> +    new_pr->source = source;
> +    new_pr->route = route;
> +
> +    size_t hash = uuid_hash(&od->key);
> +    struct parsed_route *pr = parsed_route_lookup(routes, hash, new_pr);
> +    if (!pr) {
> +        hmap_insert(routes, &new_pr->key_node, hash);
> +    } else {
> +        pr->stale = false;
> +        parsed_route_free(new_pr);
> +    }
> +}
> +
> +static void
> +parsed_routes_add_static(struct ovn_datapath *od, const struct hmap *lr_ports,
>                    const struct nbrec_logical_router_static_route *route,
>                    const struct hmap *bfd_connections,
>                    struct hmap *routes, struct simap *route_tables,
>                    struct hmap *bfd_active_connections)
>  {
>      /* Verify that the next hop is an IP address with an all-ones mask. */
> -    struct in6_addr nexthop;
> +    struct in6_addr *nexthop = NULL;

if we allow parsed_route_add() to make a private copy of it we can avoid a
pointer for nexthop here and make the patch more readable (e.g. remove all
free(nexthop))

>      unsigned int plen;
>      bool is_discard_route = !strcmp(route->nexthop, "discard");
>      bool valid_nexthop = route->nexthop[0] && !is_discard_route;
>      if (valid_nexthop) {
> -        if (!ip46_parse_cidr(route->nexthop, &nexthop, &plen)) {
> +        nexthop = xmalloc(sizeof(*nexthop));
> +        if (!ip46_parse_cidr(route->nexthop, nexthop, &plen)) {
>              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>              VLOG_WARN_RL(&rl, "bad 'nexthop' %s in static route "
>                           UUID_FMT, route->nexthop,
>                           UUID_ARGS(&route->header_.uuid));
> +            free(nexthop);
>              return;
>          }
> -        if ((IN6_IS_ADDR_V4MAPPED(&nexthop) && plen != 32) ||
> -            (!IN6_IS_ADDR_V4MAPPED(&nexthop) && plen != 128)) {
> +        if ((IN6_IS_ADDR_V4MAPPED(nexthop) && plen != 32) ||
> +            (!IN6_IS_ADDR_V4MAPPED(nexthop) && plen != 128)) {
>              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>              VLOG_WARN_RL(&rl, "bad next hop mask %s in static route "
>                           UUID_FMT, route->nexthop,
>                           UUID_ARGS(&route->header_.uuid));
> +            free(nexthop);
>              return;
>          }
>      }
> @@ -11155,17 +11218,19 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
>          VLOG_WARN_RL(&rl, "bad 'ip_prefix' %s in static route "
>                       UUID_FMT, route->ip_prefix,
>                       UUID_ARGS(&route->header_.uuid));
> +        free(nexthop);
>          return;
>      }
>  
>      /* Verify that ip_prefix and nexthop have same address familiy. */
>      if (valid_nexthop) {
> -        if (IN6_IS_ADDR_V4MAPPED(&prefix) != IN6_IS_ADDR_V4MAPPED(&nexthop)) {
> +        if (IN6_IS_ADDR_V4MAPPED(&prefix) != IN6_IS_ADDR_V4MAPPED(nexthop)) {
>              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>              VLOG_WARN_RL(&rl, "Address family doesn't match between 'ip_prefix'"
>                           " %s and 'nexthop' %s in static route "UUID_FMT,
>                           route->ip_prefix, route->nexthop,
>                           UUID_ARGS(&route->header_.uuid));
> +            free(nexthop);
>              return;
>          }
>      }
> @@ -11177,6 +11242,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
>          !find_static_route_outport(od, lr_ports, route,
>                                     IN6_IS_ADDR_V4MAPPED(&prefix),
>                                     &lrp_addr_s, &out_port)) {
> +        free(nexthop);
>          return;
>      }
>  
> @@ -11186,6 +11252,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
>                                                    nb_bt->logical_port,
>                                                    nb_bt->dst_ip);
>          if (!bfd_e) {
> +            free(nexthop);
>              return;
>          }
>  
> @@ -11205,36 +11272,58 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
>  
>  
>          if (!strcmp(bfd_sr->status, "down")) {
> +            free(nexthop);
>              return;
>          }
>      }
>  
> -    struct parsed_route *new_pr = xzalloc(sizeof *new_pr);
> -    new_pr->prefix = prefix;
> -    new_pr->plen = plen;
> -    new_pr->route_table_id = get_route_table_id(route_tables,
> -                                                route->route_table);
> -    new_pr->is_src_route = (route->policy &&
> -                            !strcmp(route->policy, "src-ip"));
> -    new_pr->hash = route_hash(new_pr);
> -    new_pr->route = route;
> -    new_pr->nbr = od->nbr;
> -    new_pr->ecmp_symmetric_reply = smap_get_bool(&route->options,
> -                                                 "ecmp_symmetric_reply",
> -                                                 false);
> -    new_pr->is_discard_route = is_discard_route;
> -    if (!is_discard_route) {
> -        new_pr->lrp_addr_s = xstrdup(lrp_addr_s);
> -    }
> -    new_pr->out_port = out_port;
> +    uint32_t route_table_id = get_route_table_id(route_tables,
> +                                        route->route_table);
> +    bool is_src_route = (route->policy && !strcmp(route->policy, "src-ip"));
> +    bool ecmp_symmetric_reply = smap_get_bool(&route->options,
> +                                         "ecmp_symmetric_reply",
> +                                         false);
>  
> -    size_t hash = uuid_hash(&od->key);
> -    struct parsed_route *pr = parsed_route_lookup(routes, hash, new_pr);
> -    if (!pr) {
> -        hmap_insert(routes, &new_pr->key_node, hash);
> +    enum route_source source;
> +    if (!strcmp(smap_get_def(&route->options, "origin", ""),
> +                                 ROUTE_ORIGIN_CONNECTED)) {
> +        source = ROUTE_SOURCE_CONNECTED;
>      } else {
> -        pr->stale = false;
> -        parsed_route_free(new_pr);
> +        source = ROUTE_SOURCE_STATIC;
> +    }
> +
> +    parsed_route_add(od, nexthop, prefix, plen, is_discard_route, lrp_addr_s,
> +                      out_port, route, route_table_id, is_src_route,
> +                      ecmp_symmetric_reply, source,
> +                      routes);
> +}
> +
> +static void
> +parsed_routes_add_connected(struct ovn_datapath *od, const struct ovn_port *op,
> +                            struct hmap *routes)
> +{
> +    for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> +        const struct ipv4_netaddr *addr = &op->lrp_networks.ipv4_addrs[i];
> +        struct in6_addr prefix;
> +        ip46_parse(addr->network_s, &prefix);
> +
> +        parsed_route_add(od, NULL, prefix, addr->plen,
> +                          false, addr->addr_s, op,
> +                          NULL, 0, false,
> +                          false, ROUTE_SOURCE_CONNECTED,
> +                          routes);
> +    }
> +
> +    for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> +        const struct ipv6_netaddr *addr = &op->lrp_networks.ipv6_addrs[i];
> +        struct in6_addr prefix;
> +        ip46_parse(addr->network_s, &prefix);
> +
> +        parsed_route_add(od, NULL, prefix, addr->plen,
> +                          false, addr->addr_s, op,
> +                          NULL, 0, false,
> +                          false, ROUTE_SOURCE_CONNECTED,
> +                          routes);
>      }
>  }
>  
> @@ -11252,9 +11341,14 @@ build_parsed_routes(struct ovn_datapath *od, const struct hmap *lr_ports,
>      }
>  
>      for (int i = 0; i < od->nbr->n_static_routes; i++) {
> -        parsed_routes_add(od, lr_ports, od->nbr->static_routes[i],
> -                          bfd_connections, routes, route_tables,
> -                          bfd_active_connections);
> +        parsed_routes_add_static(od, lr_ports, od->nbr->static_routes[i],
> +                                 bfd_connections, routes, route_tables,
> +                                 bfd_active_connections);
> +    }
> +
> +    const struct ovn_port *op;
> +    HMAP_FOR_EACH (op, dp_node, &od->ports) {
> +        parsed_routes_add_connected(od, op, routes);
>      }
>  
>      HMAP_FOR_EACH_SAFE (pr, key_node, routes) {
> @@ -11279,7 +11373,7 @@ struct ecmp_groups_node {
>      struct in6_addr prefix;
>      unsigned int plen;
>      bool is_src_route;
> -    const char *origin;
> +    enum route_source source;
>      uint32_t route_table_id;
>      uint16_t route_count;
>      struct ovs_list route_list; /* Contains ecmp_route_list_node */
> @@ -11318,7 +11412,7 @@ ecmp_groups_add(struct hmap *ecmp_groups,
>      eg->prefix = route->prefix;
>      eg->plen = route->plen;
>      eg->is_src_route = route->is_src_route;
> -    eg->origin = smap_get_def(&route->route->options, "origin", "");
> +    eg->source = route->source;
>      eg->route_table_id = route->route_table_id;
>      ovs_list_init(&eg->route_list);
>      ecmp_groups_add_route(eg, route);
> @@ -11382,6 +11476,7 @@ unique_routes_remove(struct hmap *unique_routes,
>          if (ipv6_addr_equals(&route->prefix, &ur->route->prefix) &&
>              route->plen == ur->route->plen &&
>              route->is_src_route == ur->route->is_src_route &&
> +            route->source == ur->route->source &&
>              route->route_table_id == ur->route->route_table_id) {
>              hmap_remove(unique_routes, &ur->hmap_node);
>              const struct parsed_route *existed_route = ur->route;
> @@ -11515,7 +11610,7 @@ static void
>  add_ecmp_symmetric_reply_flows(struct lflow_table *lflows,
>                                 struct ovn_datapath *od,
>                                 const char *port_ip,
> -                               struct ovn_port *out_port,
> +                               const struct ovn_port *out_port,
>                                 const struct parsed_route *route,
>                                 struct ds *route_match,
>                                 struct lflow_ref *lflow_ref)
> @@ -11609,6 +11704,19 @@ add_ecmp_symmetric_reply_flows(struct lflow_table *lflows,
>      ds_destroy(&ecmp_reply);
>  }
>  
> +static int
> +route_source_to_offset(enum route_source source)
> +{
> +    switch (source) {
> +        case ROUTE_SOURCE_CONNECTED:
> +            return ROUTE_PRIO_OFFSET_CONNECTED;
> +        case ROUTE_SOURCE_STATIC:
> +            return ROUTE_PRIO_OFFSET_STATIC;
> +        default:
> +            OVS_NOT_REACHED();
> +    }
> +}
> +
>  static void
>  build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
>                        struct ecmp_groups_node *eg, struct lflow_ref *lflow_ref)
> @@ -11620,8 +11728,7 @@ build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
>      struct ds route_match = DS_EMPTY_INITIALIZER;
>  
>      char *prefix_s = build_route_prefix_s(&eg->prefix, eg->plen);
> -    int ofs = !strcmp(eg->origin, ROUTE_ORIGIN_CONNECTED) ?
> -        ROUTE_PRIO_OFFSET_CONNECTED: ROUTE_PRIO_OFFSET_STATIC;
> +    int ofs = route_source_to_offset(eg->source);
>      build_route_match(NULL, eg->route_table_id, prefix_s, eg->plen,
>                        eg->is_src_route, is_ipv4, &route_match, &priority, ofs);
>      free(prefix_s);
> @@ -11675,13 +11782,14 @@ build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
>                        REG_ECMP_MEMBER_ID" == %"PRIu16,
>                        eg->id, er->id);
>          ds_clear(&actions);
> -        ds_put_format(&actions, "%s = %s; "
> +        ds_put_format(&actions, "%s = ",
> +                      is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6);
> +        ipv6_format_mapped(route_->nexthop, &actions);
> +        ds_put_format(&actions, "; "
>                        "%s = %s; "
>                        "eth.src = %s; "
>                        "outport = %s; "
>                        "next;",
> -                      is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6,
> -                      route->nexthop,
>                        is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6,
>                        route_->lrp_addr_s,
>                        route_->out_port->lrp_networks.ea_s,
> @@ -11699,17 +11807,19 @@ build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
>  static void
>  add_route(struct lflow_table *lflows, struct ovn_datapath *od,
>            const struct ovn_port *op, const char *lrp_addr_s,
> -          const char *network_s, int plen, const char *gateway,
> +          const char *network_s, int plen, const struct in6_addr *gateway,
>            bool is_src_route, const uint32_t rtb_id,
>            const struct sset *bfd_ports,
>            const struct ovsdb_idl_row *stage_hint, bool is_discard_route,
> -          int ofs, struct lflow_ref *lflow_ref)
> +          enum route_source source, struct lflow_ref *lflow_ref)
>  {
>      bool is_ipv4 = strchr(network_s, '.') ? true : false;
>      struct ds match = DS_EMPTY_INITIALIZER;
>      uint16_t priority;
>      const struct ovn_port *op_inport = NULL;
>  
> +    int ofs = route_source_to_offset(source);
> +
>      /* IPv6 link-local addresses must be scoped to the local router port. */
>      if (!is_ipv4) {
>          struct in6_addr network;
> @@ -11728,8 +11838,8 @@ add_route(struct lflow_table *lflows, struct ovn_datapath *od,
>      } else {
>          ds_put_format(&common_actions, REG_ECMP_GROUP_ID" = 0; %s = ",
>                        is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6);
> -        if (gateway && gateway[0]) {
> -            ds_put_cstr(&common_actions, gateway);
> +        if (gateway) {
> +            ipv6_format_mapped(gateway, &common_actions);
>          } else {
>              ds_put_format(&common_actions, "ip%s.dst", is_ipv4 ? "4" : "6");
>          }
> @@ -11764,23 +11874,20 @@ add_route(struct lflow_table *lflows, struct ovn_datapath *od,
>  }
>  
>  static void
> -build_static_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> +build_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
>                          const struct parsed_route *route_,
>                          const struct sset *bfd_ports,
>                          struct lflow_ref *lflow_ref)
>  {
>      const struct nbrec_logical_router_static_route *route = route_->route;
>  
> -    int ofs = !strcmp(smap_get_def(&route->options, "origin", ""),
> -                      ROUTE_ORIGIN_CONNECTED) ? ROUTE_PRIO_OFFSET_CONNECTED
> -                                              : ROUTE_PRIO_OFFSET_STATIC;
> -
>      char *prefix_s = build_route_prefix_s(&route_->prefix, route_->plen);
>      add_route(lflows, route_->is_discard_route ? od : route_->out_port->od,
>                route_->out_port, route_->lrp_addr_s, prefix_s,
> -              route_->plen, route->nexthop, route_->is_src_route,
> -              route_->route_table_id, bfd_ports, &route->header_,
> -              route_->is_discard_route, ofs, lflow_ref);
> +              route_->plen, route_->nexthop, route_->is_src_route,
> +              route_->route_table_id, bfd_ports,
> +              route ? &route->header_ : &route_->out_port->nbrp->header_,
> +              route_->is_discard_route, route_->source, lflow_ref);
>  
>      free(prefix_s);
>  }
> @@ -13509,51 +13616,8 @@ build_ip_routing_pre_flows_for_lrouter(struct ovn_datapath *od,
>                    REG_ROUTE_TABLE_ID" = 0; next;", lflow_ref);
>  }
>  
> -/* Logical router ingress table IP_ROUTING : IP Routing.
> - *
> - * A packet that arrives at this table is an IP packet that should be
> - * routed to the address in 'ip[46].dst'.
> - *
> - * For regular routes without ECMP, table IP_ROUTING sets outport to the
> - * correct output port, eth.src to the output port's MAC address, and
> - * REG_NEXT_HOP_IPV4/REG_NEXT_HOP_IPV6 to the next-hop IP address
> - * (leaving 'ip[46].dst', the packet’s final destination, unchanged), and
> - * advances to the next table.
> - *
> - * For ECMP routes, i.e. multiple routes with same policy and prefix, table
> - * IP_ROUTING remembers ECMP group id and selects a member id, and advances
> - * to table IP_ROUTING_ECMP, which sets outport, eth.src and
> - * REG_NEXT_HOP_IPV4/REG_NEXT_HOP_IPV6 for the selected ECMP member.
> - *
> - * This function adds routes for directly connected subnets configured on the
> - * LRP 'op'.
> - */
>  static void
> -build_ip_routing_flows_for_lrp(struct ovn_port *op,
> -                               const struct sset *bfd_ports,
> -                               struct lflow_table *lflows,
> -                               struct lflow_ref *lflow_ref)
> -{
> -    ovs_assert(op->nbrp);
> -    for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> -        add_route(lflows, op->od, op, op->lrp_networks.ipv4_addrs[i].addr_s,
> -                  op->lrp_networks.ipv4_addrs[i].network_s,
> -                  op->lrp_networks.ipv4_addrs[i].plen, NULL, false, 0,
> -                  bfd_ports, &op->nbrp->header_, false,
> -                  ROUTE_PRIO_OFFSET_CONNECTED, lflow_ref);
> -    }
> -
> -    for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> -        add_route(lflows, op->od, op, op->lrp_networks.ipv6_addrs[i].addr_s,
> -                  op->lrp_networks.ipv6_addrs[i].network_s,
> -                  op->lrp_networks.ipv6_addrs[i].plen, NULL, false, 0,
> -                  bfd_ports, &op->nbrp->header_, false,
> -                  ROUTE_PRIO_OFFSET_CONNECTED, lflow_ref);
> -    }
> -}
> -
> -static void
> -build_static_route_flows_for_lrouter(
> +build_route_flows_for_lrouter(
>          struct ovn_datapath *od, struct lflow_table *lflows,
>          struct hmap *parsed_routes,
>          struct simap *route_tables, const struct sset *bfd_ports,
> @@ -13580,6 +13644,10 @@ build_static_route_flows_for_lrouter(
>      struct parsed_route *route;
>      HMAP_FOR_EACH_WITH_HASH (route, key_node, uuid_hash(&od->key),
>                               parsed_routes) {
> +        if (route->source == ROUTE_SOURCE_CONNECTED) {
> +            unique_routes_add(&unique_routes, route);
> +            continue;
> +        }
>          group = ecmp_groups_find(&ecmp_groups, route);
>          if (group) {
>              ecmp_groups_add_route(group, route);
> @@ -13608,7 +13676,7 @@ build_static_route_flows_for_lrouter(
>      }
>      const struct unique_routes_node *ur;
>      HMAP_FOR_EACH (ur, hmap_node, &unique_routes) {
> -        build_static_route_flow(lflows, od, ur->route,
> +        build_route_flow(lflows, od, ur->route,
>                                  bfd_ports, lflow_ref);
>      }
>      ecmp_groups_destroy(&ecmp_groups);
> @@ -16852,7 +16920,7 @@ build_routable_flows_for_router_port(
>                                laddrs->ipv4_addrs[k].network_s,
>                                laddrs->ipv4_addrs[k].plen, NULL, false, 0,
>                                bfd_ports, &router_port->nbrp->header_,
> -                              false, ROUTE_PRIO_OFFSET_CONNECTED,
> +                              false, ROUTE_SOURCE_CONNECTED,
>                                lrp->stateful_lflow_ref);
>                  }
>              }
> @@ -17094,7 +17162,7 @@ build_lswitch_and_lrouter_iterate_by_lr(struct ovn_datapath *od,
>                                             lsi->meter_groups, NULL);
>      build_ND_RA_flows_for_lrouter(od, lsi->lflows, NULL);
>      build_ip_routing_pre_flows_for_lrouter(od, lsi->lflows, NULL);
> -    build_static_route_flows_for_lrouter(od, lsi->lflows,
> +    build_route_flows_for_lrouter(od, lsi->lflows,
>                                           lsi->parsed_routes, lsi->route_tables,
>                                           lsi->bfd_ports, NULL);
>      build_mcast_lookup_flows_for_lrouter(od, lsi->lflows, &lsi->match,
> @@ -17167,8 +17235,6 @@ build_lswitch_and_lrouter_iterate_by_lrp(struct ovn_port *op,
>                                            &lsi->actions, op->lflow_ref);
>      build_neigh_learning_flows_for_lrouter_port(op, lsi->lflows, &lsi->match,
>                                                  &lsi->actions, op->lflow_ref);
> -    build_ip_routing_flows_for_lrp(op, lsi->bfd_ports,
> -                                   lsi->lflows, op->lflow_ref);
>      build_ND_RA_flows_for_lrouter_port(op, lsi->lflows, &lsi->match,
>                                         &lsi->actions, lsi->meter_groups,
>                                         op->lflow_ref);
> diff --git a/northd/northd.h b/northd/northd.h
> index f701abd45..eb669f734 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -696,10 +696,18 @@ struct ovn_port {
>      struct lflow_ref *stateful_lflow_ref;
>  };
>  
> +enum route_source {
> +    /* the route is directly connected to the logical router */
> +    ROUTE_SOURCE_CONNECTED,

I know naming is hard but maybe ROUTE_SOURCE_LOCAL?

> +    /* the route is derived from a northbound static route entry */
> +    ROUTE_SOURCE_STATIC,
> +};
> +
>  struct parsed_route {
>      struct hmap_node key_node;
>      struct in6_addr prefix;
>      unsigned int plen;
> +    struct in6_addr *nexthop; /* NULL for ROUTE_SOURCE_CONNECTED */
>      bool is_src_route;
>      uint32_t route_table_id;
>      uint32_t hash;
> @@ -708,8 +716,9 @@ struct parsed_route {
>      bool is_discard_route;
>      const struct nbrec_logical_router *nbr;
>      bool stale;
> +    enum route_source source;
>      char *lrp_addr_s;
> -    struct ovn_port *out_port;
> +    const struct ovn_port *out_port;

can you add this in the patch where you added out_port pointer?

>  };
>  
>  void ovnnb_db_run(struct northd_input *input_data,
> -- 
> 2.47.0
> 
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Thu, Nov 14, 2024 at 11:10:39AM +0100, Lorenzo Bianconi wrote:
> > previously connected routes (so routes based on the networks of the
> > LRPs) where handled separately from static routes.
> > 
> > In order to later announce routes from ovn to the fabric we need to have
> > a central overview over all routes we want to announce. This includes
> > routes based on Logical_Router_Static_Routes as well as the networks on
> > Logical_Router_Port.
> > 
> > With this change the "routes" engine node will output a complete list of
> > all routes in the system. This also serves as a future integration point
> > to integrate routes learned from outside of OVN.
> 
> Hi Felix,
> 
> This patch needs a respin. Some nits inline.

Hi Lorenzo,

thanks for the review

> 
> Regards,
> Lorenzo
> 
> > 
> > Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> > ---
> >  northd/northd.c | 270 ++++++++++++++++++++++++++++++------------------
> >  northd/northd.h |  11 +-
> >  2 files changed, 178 insertions(+), 103 deletions(-)
> > 
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 1265e3c81..90ddc2acb 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -11071,6 +11071,20 @@ parsed_route_lookup(struct hmap *routes, size_t hash,
> >              continue;
> >          }
> >  
> > +        if (pr->source != new_pr->source) {
> > +            continue;
> > +        }
> > +
> > +        if (!pr->nexthop != !new_pr->nexthop) {
> > +            continue;
> 
> why not if (pr->nexthop != new_pr->nexthop) ?

We would then compare the pointers of pr->nexthop to new_pr->nexthop
instead of checking if either both of them are null or non-null.
I'll add a comment to clarify this.

> 
> > +        }
> > +
> > +        if (pr->nexthop &&
> > +                memcmp(pr->nexthop, new_pr->nexthop,
> > +                       sizeof(struct in6_addr))) {
> 
> I guess we can use ipv6_addr_equals() here.

Thanks fixed.

> 
> > +            continue;
> > +        }
> > +
> >          if (memcmp(&pr->prefix, &new_pr->prefix, sizeof(struct in6_addr))) {
> >              continue;
> >          }
> > @@ -11111,6 +11125,9 @@ parsed_route_lookup(struct hmap *routes, size_t hash,
> >  
> >  static void
> >  parsed_route_free(struct parsed_route *pr) {
> > +    if (pr->nexthop) {
> 
> you can remove if () here

Thanks fixed.

> 
> > +        free(pr->nexthop);
> > +    }
> >      if (pr->lrp_addr_s) {
> >          free(pr->lrp_addr_s);
> >      }
> > @@ -11119,31 +11136,77 @@ parsed_route_free(struct parsed_route *pr) {
> >  }
> >  
> >  static void
> > -parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> > +parsed_route_add(const struct ovn_datapath *od,
> > +                 struct in6_addr *nexthop,
> > +                 const struct in6_addr prefix,
> > +                 unsigned int plen,
> > +                 bool is_discard_route,
> > +                 const char *lrp_addr_s,
> > +                 const struct ovn_port *out_port,
> > +                 const struct nbrec_logical_router_static_route *route,
> > +                 uint32_t route_table_id,
> > +                 bool is_src_route,
> > +                 bool ecmp_symmetric_reply,
> > +                 enum route_source source,
> > +                 struct hmap *routes)
> > +{
> > +
> > +    struct parsed_route *new_pr = xzalloc(sizeof *new_pr);
> > +    new_pr->prefix = prefix;
> > +    new_pr->plen = plen;
> > +    new_pr->nexthop = nexthop;
> > +    new_pr->route_table_id = route_table_id;
> > +    new_pr->is_src_route = is_src_route;
> > +    new_pr->hash = route_hash(new_pr);
> > +    new_pr->nbr = od->nbr;
> > +    new_pr->ecmp_symmetric_reply = ecmp_symmetric_reply;
> > +    new_pr->is_discard_route = is_discard_route;
> > +    if (!is_discard_route) {
> > +        new_pr->lrp_addr_s = xstrdup(lrp_addr_s);
> > +    }
> > +    new_pr->out_port = out_port;
> > +    new_pr->source = source;
> > +    new_pr->route = route;
> > +
> > +    size_t hash = uuid_hash(&od->key);
> > +    struct parsed_route *pr = parsed_route_lookup(routes, hash, new_pr);
> > +    if (!pr) {
> > +        hmap_insert(routes, &new_pr->key_node, hash);
> > +    } else {
> > +        pr->stale = false;
> > +        parsed_route_free(new_pr);
> > +    }
> > +}
> > +
> > +static void
> > +parsed_routes_add_static(struct ovn_datapath *od, const struct hmap *lr_ports,
> >                    const struct nbrec_logical_router_static_route *route,
> >                    const struct hmap *bfd_connections,
> >                    struct hmap *routes, struct simap *route_tables,
> >                    struct hmap *bfd_active_connections)
> >  {
> >      /* Verify that the next hop is an IP address with an all-ones mask. */
> > -    struct in6_addr nexthop;
> > +    struct in6_addr *nexthop = NULL;
> 
> if we allow parsed_route_add() to make a private copy of it we can avoid a
> pointer for nexthop here and make the patch more readable (e.g. remove all
> free(nexthop))

I choose this way because on a static route the nexthop field might not
be set. OVN currently accepts this as a valid static route and then
uses "ip.dst" as nexthop value (see add_route in northd.c).

If we don't use a pointer here then parsed_route_add() would need to
check based on some magic address to then treat it as NULL. This magic
address (even though it might be invalid nexthop) would then be
unavailable for the user to set. So i think this would cause a behaviour
change.

Therefor i decided to stick to the current pointer approach.

> 
> >      unsigned int plen;
> >      bool is_discard_route = !strcmp(route->nexthop, "discard");
> >      bool valid_nexthop = route->nexthop[0] && !is_discard_route;
> >      if (valid_nexthop) {
> > -        if (!ip46_parse_cidr(route->nexthop, &nexthop, &plen)) {
> > +        nexthop = xmalloc(sizeof(*nexthop));
> > +        if (!ip46_parse_cidr(route->nexthop, nexthop, &plen)) {
> >              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> >              VLOG_WARN_RL(&rl, "bad 'nexthop' %s in static route "
> >                           UUID_FMT, route->nexthop,
> >                           UUID_ARGS(&route->header_.uuid));
> > +            free(nexthop);
> >              return;
> >          }
> > -        if ((IN6_IS_ADDR_V4MAPPED(&nexthop) && plen != 32) ||
> > -            (!IN6_IS_ADDR_V4MAPPED(&nexthop) && plen != 128)) {
> > +        if ((IN6_IS_ADDR_V4MAPPED(nexthop) && plen != 32) ||
> > +            (!IN6_IS_ADDR_V4MAPPED(nexthop) && plen != 128)) {
> >              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> >              VLOG_WARN_RL(&rl, "bad next hop mask %s in static route "
> >                           UUID_FMT, route->nexthop,
> >                           UUID_ARGS(&route->header_.uuid));
> > +            free(nexthop);
> >              return;
> >          }
> >      }
> > @@ -11155,17 +11218,19 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> >          VLOG_WARN_RL(&rl, "bad 'ip_prefix' %s in static route "
> >                       UUID_FMT, route->ip_prefix,
> >                       UUID_ARGS(&route->header_.uuid));
> > +        free(nexthop);
> >          return;
> >      }
> >  
> >      /* Verify that ip_prefix and nexthop have same address familiy. */
> >      if (valid_nexthop) {
> > -        if (IN6_IS_ADDR_V4MAPPED(&prefix) != IN6_IS_ADDR_V4MAPPED(&nexthop)) {
> > +        if (IN6_IS_ADDR_V4MAPPED(&prefix) != IN6_IS_ADDR_V4MAPPED(nexthop)) {
> >              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> >              VLOG_WARN_RL(&rl, "Address family doesn't match between 'ip_prefix'"
> >                           " %s and 'nexthop' %s in static route "UUID_FMT,
> >                           route->ip_prefix, route->nexthop,
> >                           UUID_ARGS(&route->header_.uuid));
> > +            free(nexthop);
> >              return;
> >          }
> >      }
> > @@ -11177,6 +11242,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> >          !find_static_route_outport(od, lr_ports, route,
> >                                     IN6_IS_ADDR_V4MAPPED(&prefix),
> >                                     &lrp_addr_s, &out_port)) {
> > +        free(nexthop);
> >          return;
> >      }
> >  
> > @@ -11186,6 +11252,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> >                                                    nb_bt->logical_port,
> >                                                    nb_bt->dst_ip);
> >          if (!bfd_e) {
> > +            free(nexthop);
> >              return;
> >          }
> >  
> > @@ -11205,36 +11272,58 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> >  
> >  
> >          if (!strcmp(bfd_sr->status, "down")) {
> > +            free(nexthop);
> >              return;
> >          }
> >      }
> >  
> > -    struct parsed_route *new_pr = xzalloc(sizeof *new_pr);
> > -    new_pr->prefix = prefix;
> > -    new_pr->plen = plen;
> > -    new_pr->route_table_id = get_route_table_id(route_tables,
> > -                                                route->route_table);
> > -    new_pr->is_src_route = (route->policy &&
> > -                            !strcmp(route->policy, "src-ip"));
> > -    new_pr->hash = route_hash(new_pr);
> > -    new_pr->route = route;
> > -    new_pr->nbr = od->nbr;
> > -    new_pr->ecmp_symmetric_reply = smap_get_bool(&route->options,
> > -                                                 "ecmp_symmetric_reply",
> > -                                                 false);
> > -    new_pr->is_discard_route = is_discard_route;
> > -    if (!is_discard_route) {
> > -        new_pr->lrp_addr_s = xstrdup(lrp_addr_s);
> > -    }
> > -    new_pr->out_port = out_port;
> > +    uint32_t route_table_id = get_route_table_id(route_tables,
> > +                                        route->route_table);
> > +    bool is_src_route = (route->policy && !strcmp(route->policy, "src-ip"));
> > +    bool ecmp_symmetric_reply = smap_get_bool(&route->options,
> > +                                         "ecmp_symmetric_reply",
> > +                                         false);
> >  
> > -    size_t hash = uuid_hash(&od->key);
> > -    struct parsed_route *pr = parsed_route_lookup(routes, hash, new_pr);
> > -    if (!pr) {
> > -        hmap_insert(routes, &new_pr->key_node, hash);
> > +    enum route_source source;
> > +    if (!strcmp(smap_get_def(&route->options, "origin", ""),
> > +                                 ROUTE_ORIGIN_CONNECTED)) {
> > +        source = ROUTE_SOURCE_CONNECTED;
> >      } else {
> > -        pr->stale = false;
> > -        parsed_route_free(new_pr);
> > +        source = ROUTE_SOURCE_STATIC;
> > +    }
> > +
> > +    parsed_route_add(od, nexthop, prefix, plen, is_discard_route, lrp_addr_s,
> > +                      out_port, route, route_table_id, is_src_route,
> > +                      ecmp_symmetric_reply, source,
> > +                      routes);
> > +}
> > +
> > +static void
> > +parsed_routes_add_connected(struct ovn_datapath *od, const struct ovn_port *op,
> > +                            struct hmap *routes)
> > +{
> > +    for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> > +        const struct ipv4_netaddr *addr = &op->lrp_networks.ipv4_addrs[i];
> > +        struct in6_addr prefix;
> > +        ip46_parse(addr->network_s, &prefix);
> > +
> > +        parsed_route_add(od, NULL, prefix, addr->plen,
> > +                          false, addr->addr_s, op,
> > +                          NULL, 0, false,
> > +                          false, ROUTE_SOURCE_CONNECTED,
> > +                          routes);
> > +    }
> > +
> > +    for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> > +        const struct ipv6_netaddr *addr = &op->lrp_networks.ipv6_addrs[i];
> > +        struct in6_addr prefix;
> > +        ip46_parse(addr->network_s, &prefix);
> > +
> > +        parsed_route_add(od, NULL, prefix, addr->plen,
> > +                          false, addr->addr_s, op,
> > +                          NULL, 0, false,
> > +                          false, ROUTE_SOURCE_CONNECTED,
> > +                          routes);
> >      }
> >  }
> >  
> > @@ -11252,9 +11341,14 @@ build_parsed_routes(struct ovn_datapath *od, const struct hmap *lr_ports,
> >      }
> >  
> >      for (int i = 0; i < od->nbr->n_static_routes; i++) {
> > -        parsed_routes_add(od, lr_ports, od->nbr->static_routes[i],
> > -                          bfd_connections, routes, route_tables,
> > -                          bfd_active_connections);
> > +        parsed_routes_add_static(od, lr_ports, od->nbr->static_routes[i],
> > +                                 bfd_connections, routes, route_tables,
> > +                                 bfd_active_connections);
> > +    }
> > +
> > +    const struct ovn_port *op;
> > +    HMAP_FOR_EACH (op, dp_node, &od->ports) {
> > +        parsed_routes_add_connected(od, op, routes);
> >      }
> >  
> >      HMAP_FOR_EACH_SAFE (pr, key_node, routes) {
> > @@ -11279,7 +11373,7 @@ struct ecmp_groups_node {
> >      struct in6_addr prefix;
> >      unsigned int plen;
> >      bool is_src_route;
> > -    const char *origin;
> > +    enum route_source source;
> >      uint32_t route_table_id;
> >      uint16_t route_count;
> >      struct ovs_list route_list; /* Contains ecmp_route_list_node */
> > @@ -11318,7 +11412,7 @@ ecmp_groups_add(struct hmap *ecmp_groups,
> >      eg->prefix = route->prefix;
> >      eg->plen = route->plen;
> >      eg->is_src_route = route->is_src_route;
> > -    eg->origin = smap_get_def(&route->route->options, "origin", "");
> > +    eg->source = route->source;
> >      eg->route_table_id = route->route_table_id;
> >      ovs_list_init(&eg->route_list);
> >      ecmp_groups_add_route(eg, route);
> > @@ -11382,6 +11476,7 @@ unique_routes_remove(struct hmap *unique_routes,
> >          if (ipv6_addr_equals(&route->prefix, &ur->route->prefix) &&
> >              route->plen == ur->route->plen &&
> >              route->is_src_route == ur->route->is_src_route &&
> > +            route->source == ur->route->source &&
> >              route->route_table_id == ur->route->route_table_id) {
> >              hmap_remove(unique_routes, &ur->hmap_node);
> >              const struct parsed_route *existed_route = ur->route;
> > @@ -11515,7 +11610,7 @@ static void
> >  add_ecmp_symmetric_reply_flows(struct lflow_table *lflows,
> >                                 struct ovn_datapath *od,
> >                                 const char *port_ip,
> > -                               struct ovn_port *out_port,
> > +                               const struct ovn_port *out_port,
> >                                 const struct parsed_route *route,
> >                                 struct ds *route_match,
> >                                 struct lflow_ref *lflow_ref)
> > @@ -11609,6 +11704,19 @@ add_ecmp_symmetric_reply_flows(struct lflow_table *lflows,
> >      ds_destroy(&ecmp_reply);
> >  }
> >  
> > +static int
> > +route_source_to_offset(enum route_source source)
> > +{
> > +    switch (source) {
> > +        case ROUTE_SOURCE_CONNECTED:
> > +            return ROUTE_PRIO_OFFSET_CONNECTED;
> > +        case ROUTE_SOURCE_STATIC:
> > +            return ROUTE_PRIO_OFFSET_STATIC;
> > +        default:
> > +            OVS_NOT_REACHED();
> > +    }
> > +}
> > +
> >  static void
> >  build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> >                        struct ecmp_groups_node *eg, struct lflow_ref *lflow_ref)
> > @@ -11620,8 +11728,7 @@ build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> >      struct ds route_match = DS_EMPTY_INITIALIZER;
> >  
> >      char *prefix_s = build_route_prefix_s(&eg->prefix, eg->plen);
> > -    int ofs = !strcmp(eg->origin, ROUTE_ORIGIN_CONNECTED) ?
> > -        ROUTE_PRIO_OFFSET_CONNECTED: ROUTE_PRIO_OFFSET_STATIC;
> > +    int ofs = route_source_to_offset(eg->source);
> >      build_route_match(NULL, eg->route_table_id, prefix_s, eg->plen,
> >                        eg->is_src_route, is_ipv4, &route_match, &priority, ofs);
> >      free(prefix_s);
> > @@ -11675,13 +11782,14 @@ build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> >                        REG_ECMP_MEMBER_ID" == %"PRIu16,
> >                        eg->id, er->id);
> >          ds_clear(&actions);
> > -        ds_put_format(&actions, "%s = %s; "
> > +        ds_put_format(&actions, "%s = ",
> > +                      is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6);
> > +        ipv6_format_mapped(route_->nexthop, &actions);
> > +        ds_put_format(&actions, "; "
> >                        "%s = %s; "
> >                        "eth.src = %s; "
> >                        "outport = %s; "
> >                        "next;",
> > -                      is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6,
> > -                      route->nexthop,
> >                        is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6,
> >                        route_->lrp_addr_s,
> >                        route_->out_port->lrp_networks.ea_s,
> > @@ -11699,17 +11807,19 @@ build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> >  static void
> >  add_route(struct lflow_table *lflows, struct ovn_datapath *od,
> >            const struct ovn_port *op, const char *lrp_addr_s,
> > -          const char *network_s, int plen, const char *gateway,
> > +          const char *network_s, int plen, const struct in6_addr *gateway,
> >            bool is_src_route, const uint32_t rtb_id,
> >            const struct sset *bfd_ports,
> >            const struct ovsdb_idl_row *stage_hint, bool is_discard_route,
> > -          int ofs, struct lflow_ref *lflow_ref)
> > +          enum route_source source, struct lflow_ref *lflow_ref)
> >  {
> >      bool is_ipv4 = strchr(network_s, '.') ? true : false;
> >      struct ds match = DS_EMPTY_INITIALIZER;
> >      uint16_t priority;
> >      const struct ovn_port *op_inport = NULL;
> >  
> > +    int ofs = route_source_to_offset(source);
> > +
> >      /* IPv6 link-local addresses must be scoped to the local router port. */
> >      if (!is_ipv4) {
> >          struct in6_addr network;
> > @@ -11728,8 +11838,8 @@ add_route(struct lflow_table *lflows, struct ovn_datapath *od,
> >      } else {
> >          ds_put_format(&common_actions, REG_ECMP_GROUP_ID" = 0; %s = ",
> >                        is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6);
> > -        if (gateway && gateway[0]) {
> > -            ds_put_cstr(&common_actions, gateway);
> > +        if (gateway) {
> > +            ipv6_format_mapped(gateway, &common_actions);
> >          } else {
> >              ds_put_format(&common_actions, "ip%s.dst", is_ipv4 ? "4" : "6");
> >          }
> > @@ -11764,23 +11874,20 @@ add_route(struct lflow_table *lflows, struct ovn_datapath *od,
> >  }
> >  
> >  static void
> > -build_static_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> > +build_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> >                          const struct parsed_route *route_,
> >                          const struct sset *bfd_ports,
> >                          struct lflow_ref *lflow_ref)
> >  {
> >      const struct nbrec_logical_router_static_route *route = route_->route;
> >  
> > -    int ofs = !strcmp(smap_get_def(&route->options, "origin", ""),
> > -                      ROUTE_ORIGIN_CONNECTED) ? ROUTE_PRIO_OFFSET_CONNECTED
> > -                                              : ROUTE_PRIO_OFFSET_STATIC;
> > -
> >      char *prefix_s = build_route_prefix_s(&route_->prefix, route_->plen);
> >      add_route(lflows, route_->is_discard_route ? od : route_->out_port->od,
> >                route_->out_port, route_->lrp_addr_s, prefix_s,
> > -              route_->plen, route->nexthop, route_->is_src_route,
> > -              route_->route_table_id, bfd_ports, &route->header_,
> > -              route_->is_discard_route, ofs, lflow_ref);
> > +              route_->plen, route_->nexthop, route_->is_src_route,
> > +              route_->route_table_id, bfd_ports,
> > +              route ? &route->header_ : &route_->out_port->nbrp->header_,
> > +              route_->is_discard_route, route_->source, lflow_ref);
> >  
> >      free(prefix_s);
> >  }
> > @@ -13509,51 +13616,8 @@ build_ip_routing_pre_flows_for_lrouter(struct ovn_datapath *od,
> >                    REG_ROUTE_TABLE_ID" = 0; next;", lflow_ref);
> >  }
> >  
> > -/* Logical router ingress table IP_ROUTING : IP Routing.
> > - *
> > - * A packet that arrives at this table is an IP packet that should be
> > - * routed to the address in 'ip[46].dst'.
> > - *
> > - * For regular routes without ECMP, table IP_ROUTING sets outport to the
> > - * correct output port, eth.src to the output port's MAC address, and
> > - * REG_NEXT_HOP_IPV4/REG_NEXT_HOP_IPV6 to the next-hop IP address
> > - * (leaving 'ip[46].dst', the packet’s final destination, unchanged), and
> > - * advances to the next table.
> > - *
> > - * For ECMP routes, i.e. multiple routes with same policy and prefix, table
> > - * IP_ROUTING remembers ECMP group id and selects a member id, and advances
> > - * to table IP_ROUTING_ECMP, which sets outport, eth.src and
> > - * REG_NEXT_HOP_IPV4/REG_NEXT_HOP_IPV6 for the selected ECMP member.
> > - *
> > - * This function adds routes for directly connected subnets configured on the
> > - * LRP 'op'.
> > - */
> >  static void
> > -build_ip_routing_flows_for_lrp(struct ovn_port *op,
> > -                               const struct sset *bfd_ports,
> > -                               struct lflow_table *lflows,
> > -                               struct lflow_ref *lflow_ref)
> > -{
> > -    ovs_assert(op->nbrp);
> > -    for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> > -        add_route(lflows, op->od, op, op->lrp_networks.ipv4_addrs[i].addr_s,
> > -                  op->lrp_networks.ipv4_addrs[i].network_s,
> > -                  op->lrp_networks.ipv4_addrs[i].plen, NULL, false, 0,
> > -                  bfd_ports, &op->nbrp->header_, false,
> > -                  ROUTE_PRIO_OFFSET_CONNECTED, lflow_ref);
> > -    }
> > -
> > -    for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> > -        add_route(lflows, op->od, op, op->lrp_networks.ipv6_addrs[i].addr_s,
> > -                  op->lrp_networks.ipv6_addrs[i].network_s,
> > -                  op->lrp_networks.ipv6_addrs[i].plen, NULL, false, 0,
> > -                  bfd_ports, &op->nbrp->header_, false,
> > -                  ROUTE_PRIO_OFFSET_CONNECTED, lflow_ref);
> > -    }
> > -}
> > -
> > -static void
> > -build_static_route_flows_for_lrouter(
> > +build_route_flows_for_lrouter(
> >          struct ovn_datapath *od, struct lflow_table *lflows,
> >          struct hmap *parsed_routes,
> >          struct simap *route_tables, const struct sset *bfd_ports,
> > @@ -13580,6 +13644,10 @@ build_static_route_flows_for_lrouter(
> >      struct parsed_route *route;
> >      HMAP_FOR_EACH_WITH_HASH (route, key_node, uuid_hash(&od->key),
> >                               parsed_routes) {
> > +        if (route->source == ROUTE_SOURCE_CONNECTED) {
> > +            unique_routes_add(&unique_routes, route);
> > +            continue;
> > +        }
> >          group = ecmp_groups_find(&ecmp_groups, route);
> >          if (group) {
> >              ecmp_groups_add_route(group, route);
> > @@ -13608,7 +13676,7 @@ build_static_route_flows_for_lrouter(
> >      }
> >      const struct unique_routes_node *ur;
> >      HMAP_FOR_EACH (ur, hmap_node, &unique_routes) {
> > -        build_static_route_flow(lflows, od, ur->route,
> > +        build_route_flow(lflows, od, ur->route,
> >                                  bfd_ports, lflow_ref);
> >      }
> >      ecmp_groups_destroy(&ecmp_groups);
> > @@ -16852,7 +16920,7 @@ build_routable_flows_for_router_port(
> >                                laddrs->ipv4_addrs[k].network_s,
> >                                laddrs->ipv4_addrs[k].plen, NULL, false, 0,
> >                                bfd_ports, &router_port->nbrp->header_,
> > -                              false, ROUTE_PRIO_OFFSET_CONNECTED,
> > +                              false, ROUTE_SOURCE_CONNECTED,
> >                                lrp->stateful_lflow_ref);
> >                  }
> >              }
> > @@ -17094,7 +17162,7 @@ build_lswitch_and_lrouter_iterate_by_lr(struct ovn_datapath *od,
> >                                             lsi->meter_groups, NULL);
> >      build_ND_RA_flows_for_lrouter(od, lsi->lflows, NULL);
> >      build_ip_routing_pre_flows_for_lrouter(od, lsi->lflows, NULL);
> > -    build_static_route_flows_for_lrouter(od, lsi->lflows,
> > +    build_route_flows_for_lrouter(od, lsi->lflows,
> >                                           lsi->parsed_routes, lsi->route_tables,
> >                                           lsi->bfd_ports, NULL);
> >      build_mcast_lookup_flows_for_lrouter(od, lsi->lflows, &lsi->match,
> > @@ -17167,8 +17235,6 @@ build_lswitch_and_lrouter_iterate_by_lrp(struct ovn_port *op,
> >                                            &lsi->actions, op->lflow_ref);
> >      build_neigh_learning_flows_for_lrouter_port(op, lsi->lflows, &lsi->match,
> >                                                  &lsi->actions, op->lflow_ref);
> > -    build_ip_routing_flows_for_lrp(op, lsi->bfd_ports,
> > -                                   lsi->lflows, op->lflow_ref);
> >      build_ND_RA_flows_for_lrouter_port(op, lsi->lflows, &lsi->match,
> >                                         &lsi->actions, lsi->meter_groups,
> >                                         op->lflow_ref);
> > diff --git a/northd/northd.h b/northd/northd.h
> > index f701abd45..eb669f734 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -696,10 +696,18 @@ struct ovn_port {
> >      struct lflow_ref *stateful_lflow_ref;
> >  };
> >  
> > +enum route_source {
> > +    /* the route is directly connected to the logical router */
> > +    ROUTE_SOURCE_CONNECTED,
> 
> I know naming is hard but maybe ROUTE_SOURCE_LOCAL?

The idea behind this name was to have it similar to what physical
routers use. There also the terms "connected" and "static" are used.
But if you prefer i can still change it.

> 
> > +    /* the route is derived from a northbound static route entry */
> > +    ROUTE_SOURCE_STATIC,
> > +};
> > +
> >  struct parsed_route {
> >      struct hmap_node key_node;
> >      struct in6_addr prefix;
> >      unsigned int plen;
> > +    struct in6_addr *nexthop; /* NULL for ROUTE_SOURCE_CONNECTED */
> >      bool is_src_route;
> >      uint32_t route_table_id;
> >      uint32_t hash;
> > @@ -708,8 +716,9 @@ struct parsed_route {
> >      bool is_discard_route;
> >      const struct nbrec_logical_router *nbr;
> >      bool stale;
> > +    enum route_source source;
> >      char *lrp_addr_s;
> > -    struct ovn_port *out_port;
> > +    const struct ovn_port *out_port;
> 
> can you add this in the patch where you added out_port pointer?

Yes will do that.

Thanks for the review
Felix

> 
> >  };
> >  
> >  void ovnnb_db_run(struct northd_input *input_data,
> > -- 
> > 2.47.0
> > 
> > 
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev


> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Mon, Nov 18, 2024 at 12:11 PM Felix Huettner via dev
<ovs-dev@openvswitch.org> wrote:
>
> On Thu, Nov 14, 2024 at 11:10:39AM +0100, Lorenzo Bianconi wrote:
> > > previously connected routes (so routes based on the networks of the
> > > LRPs) where handled separately from static routes.
> > >
> > > In order to later announce routes from ovn to the fabric we need to have
> > > a central overview over all routes we want to announce. This includes
> > > routes based on Logical_Router_Static_Routes as well as the networks on
> > > Logical_Router_Port.

While I can see you have a use case that requires a central overview
of all routes, it is not required for all use cases. For the use cases
not requiring it, this approach appears like it would introduce
duplication of data?

`northd` already has access to information on LRPs for flow
generation, and the `ovn-controller` already has access to information
on LRPs in the Port_Binding table. Is that not sufficient to announce
connected routes?

Also, when using Netlink plugin for route exchange, the IP/MAC
addresses of LRPs can be replicated in VRFs in the system which will
already give the routing protocol daemon the information it needs to
redistribute connected routes.

--
Frode Nordahl

> > >
> > > With this change the "routes" engine node will output a complete list of
> > > all routes in the system. This also serves as a future integration point
> > > to integrate routes learned from outside of OVN.
> >
> > Hi Felix,
> >
> > This patch needs a respin. Some nits inline.
>
> Hi Lorenzo,
>
> thanks for the review
>
> >
> > Regards,
> > Lorenzo
> >
> > >
> > > Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> > > ---
> > >  northd/northd.c | 270 ++++++++++++++++++++++++++++++------------------
> > >  northd/northd.h |  11 +-
> > >  2 files changed, 178 insertions(+), 103 deletions(-)
> > >
> > > diff --git a/northd/northd.c b/northd/northd.c
> > > index 1265e3c81..90ddc2acb 100644
> > > --- a/northd/northd.c
> > > +++ b/northd/northd.c
> > > @@ -11071,6 +11071,20 @@ parsed_route_lookup(struct hmap *routes, size_t hash,
> > >              continue;
> > >          }
> > >
> > > +        if (pr->source != new_pr->source) {
> > > +            continue;
> > > +        }
> > > +
> > > +        if (!pr->nexthop != !new_pr->nexthop) {
> > > +            continue;
> >
> > why not if (pr->nexthop != new_pr->nexthop) ?
>
> We would then compare the pointers of pr->nexthop to new_pr->nexthop
> instead of checking if either both of them are null or non-null.
> I'll add a comment to clarify this.
>
> >
> > > +        }
> > > +
> > > +        if (pr->nexthop &&
> > > +                memcmp(pr->nexthop, new_pr->nexthop,
> > > +                       sizeof(struct in6_addr))) {
> >
> > I guess we can use ipv6_addr_equals() here.
>
> Thanks fixed.
>
> >
> > > +            continue;
> > > +        }
> > > +
> > >          if (memcmp(&pr->prefix, &new_pr->prefix, sizeof(struct in6_addr))) {
> > >              continue;
> > >          }
> > > @@ -11111,6 +11125,9 @@ parsed_route_lookup(struct hmap *routes, size_t hash,
> > >
> > >  static void
> > >  parsed_route_free(struct parsed_route *pr) {
> > > +    if (pr->nexthop) {
> >
> > you can remove if () here
>
> Thanks fixed.
>
> >
> > > +        free(pr->nexthop);
> > > +    }
> > >      if (pr->lrp_addr_s) {
> > >          free(pr->lrp_addr_s);
> > >      }
> > > @@ -11119,31 +11136,77 @@ parsed_route_free(struct parsed_route *pr) {
> > >  }
> > >
> > >  static void
> > > -parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> > > +parsed_route_add(const struct ovn_datapath *od,
> > > +                 struct in6_addr *nexthop,
> > > +                 const struct in6_addr prefix,
> > > +                 unsigned int plen,
> > > +                 bool is_discard_route,
> > > +                 const char *lrp_addr_s,
> > > +                 const struct ovn_port *out_port,
> > > +                 const struct nbrec_logical_router_static_route *route,
> > > +                 uint32_t route_table_id,
> > > +                 bool is_src_route,
> > > +                 bool ecmp_symmetric_reply,
> > > +                 enum route_source source,
> > > +                 struct hmap *routes)
> > > +{
> > > +
> > > +    struct parsed_route *new_pr = xzalloc(sizeof *new_pr);
> > > +    new_pr->prefix = prefix;
> > > +    new_pr->plen = plen;
> > > +    new_pr->nexthop = nexthop;
> > > +    new_pr->route_table_id = route_table_id;
> > > +    new_pr->is_src_route = is_src_route;
> > > +    new_pr->hash = route_hash(new_pr);
> > > +    new_pr->nbr = od->nbr;
> > > +    new_pr->ecmp_symmetric_reply = ecmp_symmetric_reply;
> > > +    new_pr->is_discard_route = is_discard_route;
> > > +    if (!is_discard_route) {
> > > +        new_pr->lrp_addr_s = xstrdup(lrp_addr_s);
> > > +    }
> > > +    new_pr->out_port = out_port;
> > > +    new_pr->source = source;
> > > +    new_pr->route = route;
> > > +
> > > +    size_t hash = uuid_hash(&od->key);
> > > +    struct parsed_route *pr = parsed_route_lookup(routes, hash, new_pr);
> > > +    if (!pr) {
> > > +        hmap_insert(routes, &new_pr->key_node, hash);
> > > +    } else {
> > > +        pr->stale = false;
> > > +        parsed_route_free(new_pr);
> > > +    }
> > > +}
> > > +
> > > +static void
> > > +parsed_routes_add_static(struct ovn_datapath *od, const struct hmap *lr_ports,
> > >                    const struct nbrec_logical_router_static_route *route,
> > >                    const struct hmap *bfd_connections,
> > >                    struct hmap *routes, struct simap *route_tables,
> > >                    struct hmap *bfd_active_connections)
> > >  {
> > >      /* Verify that the next hop is an IP address with an all-ones mask. */
> > > -    struct in6_addr nexthop;
> > > +    struct in6_addr *nexthop = NULL;
> >
> > if we allow parsed_route_add() to make a private copy of it we can avoid a
> > pointer for nexthop here and make the patch more readable (e.g. remove all
> > free(nexthop))
>
> I choose this way because on a static route the nexthop field might not
> be set. OVN currently accepts this as a valid static route and then
> uses "ip.dst" as nexthop value (see add_route in northd.c).
>
> If we don't use a pointer here then parsed_route_add() would need to
> check based on some magic address to then treat it as NULL. This magic
> address (even though it might be invalid nexthop) would then be
> unavailable for the user to set. So i think this would cause a behaviour
> change.
>
> Therefor i decided to stick to the current pointer approach.
>
> >
> > >      unsigned int plen;
> > >      bool is_discard_route = !strcmp(route->nexthop, "discard");
> > >      bool valid_nexthop = route->nexthop[0] && !is_discard_route;
> > >      if (valid_nexthop) {
> > > -        if (!ip46_parse_cidr(route->nexthop, &nexthop, &plen)) {
> > > +        nexthop = xmalloc(sizeof(*nexthop));
> > > +        if (!ip46_parse_cidr(route->nexthop, nexthop, &plen)) {
> > >              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > >              VLOG_WARN_RL(&rl, "bad 'nexthop' %s in static route "
> > >                           UUID_FMT, route->nexthop,
> > >                           UUID_ARGS(&route->header_.uuid));
> > > +            free(nexthop);
> > >              return;
> > >          }
> > > -        if ((IN6_IS_ADDR_V4MAPPED(&nexthop) && plen != 32) ||
> > > -            (!IN6_IS_ADDR_V4MAPPED(&nexthop) && plen != 128)) {
> > > +        if ((IN6_IS_ADDR_V4MAPPED(nexthop) && plen != 32) ||
> > > +            (!IN6_IS_ADDR_V4MAPPED(nexthop) && plen != 128)) {
> > >              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > >              VLOG_WARN_RL(&rl, "bad next hop mask %s in static route "
> > >                           UUID_FMT, route->nexthop,
> > >                           UUID_ARGS(&route->header_.uuid));
> > > +            free(nexthop);
> > >              return;
> > >          }
> > >      }
> > > @@ -11155,17 +11218,19 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> > >          VLOG_WARN_RL(&rl, "bad 'ip_prefix' %s in static route "
> > >                       UUID_FMT, route->ip_prefix,
> > >                       UUID_ARGS(&route->header_.uuid));
> > > +        free(nexthop);
> > >          return;
> > >      }
> > >
> > >      /* Verify that ip_prefix and nexthop have same address familiy. */
> > >      if (valid_nexthop) {
> > > -        if (IN6_IS_ADDR_V4MAPPED(&prefix) != IN6_IS_ADDR_V4MAPPED(&nexthop)) {
> > > +        if (IN6_IS_ADDR_V4MAPPED(&prefix) != IN6_IS_ADDR_V4MAPPED(nexthop)) {
> > >              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > >              VLOG_WARN_RL(&rl, "Address family doesn't match between 'ip_prefix'"
> > >                           " %s and 'nexthop' %s in static route "UUID_FMT,
> > >                           route->ip_prefix, route->nexthop,
> > >                           UUID_ARGS(&route->header_.uuid));
> > > +            free(nexthop);
> > >              return;
> > >          }
> > >      }
> > > @@ -11177,6 +11242,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> > >          !find_static_route_outport(od, lr_ports, route,
> > >                                     IN6_IS_ADDR_V4MAPPED(&prefix),
> > >                                     &lrp_addr_s, &out_port)) {
> > > +        free(nexthop);
> > >          return;
> > >      }
> > >
> > > @@ -11186,6 +11252,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> > >                                                    nb_bt->logical_port,
> > >                                                    nb_bt->dst_ip);
> > >          if (!bfd_e) {
> > > +            free(nexthop);
> > >              return;
> > >          }
> > >
> > > @@ -11205,36 +11272,58 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> > >
> > >
> > >          if (!strcmp(bfd_sr->status, "down")) {
> > > +            free(nexthop);
> > >              return;
> > >          }
> > >      }
> > >
> > > -    struct parsed_route *new_pr = xzalloc(sizeof *new_pr);
> > > -    new_pr->prefix = prefix;
> > > -    new_pr->plen = plen;
> > > -    new_pr->route_table_id = get_route_table_id(route_tables,
> > > -                                                route->route_table);
> > > -    new_pr->is_src_route = (route->policy &&
> > > -                            !strcmp(route->policy, "src-ip"));
> > > -    new_pr->hash = route_hash(new_pr);
> > > -    new_pr->route = route;
> > > -    new_pr->nbr = od->nbr;
> > > -    new_pr->ecmp_symmetric_reply = smap_get_bool(&route->options,
> > > -                                                 "ecmp_symmetric_reply",
> > > -                                                 false);
> > > -    new_pr->is_discard_route = is_discard_route;
> > > -    if (!is_discard_route) {
> > > -        new_pr->lrp_addr_s = xstrdup(lrp_addr_s);
> > > -    }
> > > -    new_pr->out_port = out_port;
> > > +    uint32_t route_table_id = get_route_table_id(route_tables,
> > > +                                        route->route_table);
> > > +    bool is_src_route = (route->policy && !strcmp(route->policy, "src-ip"));
> > > +    bool ecmp_symmetric_reply = smap_get_bool(&route->options,
> > > +                                         "ecmp_symmetric_reply",
> > > +                                         false);
> > >
> > > -    size_t hash = uuid_hash(&od->key);
> > > -    struct parsed_route *pr = parsed_route_lookup(routes, hash, new_pr);
> > > -    if (!pr) {
> > > -        hmap_insert(routes, &new_pr->key_node, hash);
> > > +    enum route_source source;
> > > +    if (!strcmp(smap_get_def(&route->options, "origin", ""),
> > > +                                 ROUTE_ORIGIN_CONNECTED)) {
> > > +        source = ROUTE_SOURCE_CONNECTED;
> > >      } else {
> > > -        pr->stale = false;
> > > -        parsed_route_free(new_pr);
> > > +        source = ROUTE_SOURCE_STATIC;
> > > +    }
> > > +
> > > +    parsed_route_add(od, nexthop, prefix, plen, is_discard_route, lrp_addr_s,
> > > +                      out_port, route, route_table_id, is_src_route,
> > > +                      ecmp_symmetric_reply, source,
> > > +                      routes);
> > > +}
> > > +
> > > +static void
> > > +parsed_routes_add_connected(struct ovn_datapath *od, const struct ovn_port *op,
> > > +                            struct hmap *routes)
> > > +{
> > > +    for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> > > +        const struct ipv4_netaddr *addr = &op->lrp_networks.ipv4_addrs[i];
> > > +        struct in6_addr prefix;
> > > +        ip46_parse(addr->network_s, &prefix);
> > > +
> > > +        parsed_route_add(od, NULL, prefix, addr->plen,
> > > +                          false, addr->addr_s, op,
> > > +                          NULL, 0, false,
> > > +                          false, ROUTE_SOURCE_CONNECTED,
> > > +                          routes);
> > > +    }
> > > +
> > > +    for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> > > +        const struct ipv6_netaddr *addr = &op->lrp_networks.ipv6_addrs[i];
> > > +        struct in6_addr prefix;
> > > +        ip46_parse(addr->network_s, &prefix);
> > > +
> > > +        parsed_route_add(od, NULL, prefix, addr->plen,
> > > +                          false, addr->addr_s, op,
> > > +                          NULL, 0, false,
> > > +                          false, ROUTE_SOURCE_CONNECTED,
> > > +                          routes);
> > >      }
> > >  }
> > >
> > > @@ -11252,9 +11341,14 @@ build_parsed_routes(struct ovn_datapath *od, const struct hmap *lr_ports,
> > >      }
> > >
> > >      for (int i = 0; i < od->nbr->n_static_routes; i++) {
> > > -        parsed_routes_add(od, lr_ports, od->nbr->static_routes[i],
> > > -                          bfd_connections, routes, route_tables,
> > > -                          bfd_active_connections);
> > > +        parsed_routes_add_static(od, lr_ports, od->nbr->static_routes[i],
> > > +                                 bfd_connections, routes, route_tables,
> > > +                                 bfd_active_connections);
> > > +    }
> > > +
> > > +    const struct ovn_port *op;
> > > +    HMAP_FOR_EACH (op, dp_node, &od->ports) {
> > > +        parsed_routes_add_connected(od, op, routes);
> > >      }
> > >
> > >      HMAP_FOR_EACH_SAFE (pr, key_node, routes) {
> > > @@ -11279,7 +11373,7 @@ struct ecmp_groups_node {
> > >      struct in6_addr prefix;
> > >      unsigned int plen;
> > >      bool is_src_route;
> > > -    const char *origin;
> > > +    enum route_source source;
> > >      uint32_t route_table_id;
> > >      uint16_t route_count;
> > >      struct ovs_list route_list; /* Contains ecmp_route_list_node */
> > > @@ -11318,7 +11412,7 @@ ecmp_groups_add(struct hmap *ecmp_groups,
> > >      eg->prefix = route->prefix;
> > >      eg->plen = route->plen;
> > >      eg->is_src_route = route->is_src_route;
> > > -    eg->origin = smap_get_def(&route->route->options, "origin", "");
> > > +    eg->source = route->source;
> > >      eg->route_table_id = route->route_table_id;
> > >      ovs_list_init(&eg->route_list);
> > >      ecmp_groups_add_route(eg, route);
> > > @@ -11382,6 +11476,7 @@ unique_routes_remove(struct hmap *unique_routes,
> > >          if (ipv6_addr_equals(&route->prefix, &ur->route->prefix) &&
> > >              route->plen == ur->route->plen &&
> > >              route->is_src_route == ur->route->is_src_route &&
> > > +            route->source == ur->route->source &&
> > >              route->route_table_id == ur->route->route_table_id) {
> > >              hmap_remove(unique_routes, &ur->hmap_node);
> > >              const struct parsed_route *existed_route = ur->route;
> > > @@ -11515,7 +11610,7 @@ static void
> > >  add_ecmp_symmetric_reply_flows(struct lflow_table *lflows,
> > >                                 struct ovn_datapath *od,
> > >                                 const char *port_ip,
> > > -                               struct ovn_port *out_port,
> > > +                               const struct ovn_port *out_port,
> > >                                 const struct parsed_route *route,
> > >                                 struct ds *route_match,
> > >                                 struct lflow_ref *lflow_ref)
> > > @@ -11609,6 +11704,19 @@ add_ecmp_symmetric_reply_flows(struct lflow_table *lflows,
> > >      ds_destroy(&ecmp_reply);
> > >  }
> > >
> > > +static int
> > > +route_source_to_offset(enum route_source source)
> > > +{
> > > +    switch (source) {
> > > +        case ROUTE_SOURCE_CONNECTED:
> > > +            return ROUTE_PRIO_OFFSET_CONNECTED;
> > > +        case ROUTE_SOURCE_STATIC:
> > > +            return ROUTE_PRIO_OFFSET_STATIC;
> > > +        default:
> > > +            OVS_NOT_REACHED();
> > > +    }
> > > +}
> > > +
> > >  static void
> > >  build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> > >                        struct ecmp_groups_node *eg, struct lflow_ref *lflow_ref)
> > > @@ -11620,8 +11728,7 @@ build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> > >      struct ds route_match = DS_EMPTY_INITIALIZER;
> > >
> > >      char *prefix_s = build_route_prefix_s(&eg->prefix, eg->plen);
> > > -    int ofs = !strcmp(eg->origin, ROUTE_ORIGIN_CONNECTED) ?
> > > -        ROUTE_PRIO_OFFSET_CONNECTED: ROUTE_PRIO_OFFSET_STATIC;
> > > +    int ofs = route_source_to_offset(eg->source);
> > >      build_route_match(NULL, eg->route_table_id, prefix_s, eg->plen,
> > >                        eg->is_src_route, is_ipv4, &route_match, &priority, ofs);
> > >      free(prefix_s);
> > > @@ -11675,13 +11782,14 @@ build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> > >                        REG_ECMP_MEMBER_ID" == %"PRIu16,
> > >                        eg->id, er->id);
> > >          ds_clear(&actions);
> > > -        ds_put_format(&actions, "%s = %s; "
> > > +        ds_put_format(&actions, "%s = ",
> > > +                      is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6);
> > > +        ipv6_format_mapped(route_->nexthop, &actions);
> > > +        ds_put_format(&actions, "; "
> > >                        "%s = %s; "
> > >                        "eth.src = %s; "
> > >                        "outport = %s; "
> > >                        "next;",
> > > -                      is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6,
> > > -                      route->nexthop,
> > >                        is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6,
> > >                        route_->lrp_addr_s,
> > >                        route_->out_port->lrp_networks.ea_s,
> > > @@ -11699,17 +11807,19 @@ build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> > >  static void
> > >  add_route(struct lflow_table *lflows, struct ovn_datapath *od,
> > >            const struct ovn_port *op, const char *lrp_addr_s,
> > > -          const char *network_s, int plen, const char *gateway,
> > > +          const char *network_s, int plen, const struct in6_addr *gateway,
> > >            bool is_src_route, const uint32_t rtb_id,
> > >            const struct sset *bfd_ports,
> > >            const struct ovsdb_idl_row *stage_hint, bool is_discard_route,
> > > -          int ofs, struct lflow_ref *lflow_ref)
> > > +          enum route_source source, struct lflow_ref *lflow_ref)
> > >  {
> > >      bool is_ipv4 = strchr(network_s, '.') ? true : false;
> > >      struct ds match = DS_EMPTY_INITIALIZER;
> > >      uint16_t priority;
> > >      const struct ovn_port *op_inport = NULL;
> > >
> > > +    int ofs = route_source_to_offset(source);
> > > +
> > >      /* IPv6 link-local addresses must be scoped to the local router port. */
> > >      if (!is_ipv4) {
> > >          struct in6_addr network;
> > > @@ -11728,8 +11838,8 @@ add_route(struct lflow_table *lflows, struct ovn_datapath *od,
> > >      } else {
> > >          ds_put_format(&common_actions, REG_ECMP_GROUP_ID" = 0; %s = ",
> > >                        is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6);
> > > -        if (gateway && gateway[0]) {
> > > -            ds_put_cstr(&common_actions, gateway);
> > > +        if (gateway) {
> > > +            ipv6_format_mapped(gateway, &common_actions);
> > >          } else {
> > >              ds_put_format(&common_actions, "ip%s.dst", is_ipv4 ? "4" : "6");
> > >          }
> > > @@ -11764,23 +11874,20 @@ add_route(struct lflow_table *lflows, struct ovn_datapath *od,
> > >  }
> > >
> > >  static void
> > > -build_static_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> > > +build_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> > >                          const struct parsed_route *route_,
> > >                          const struct sset *bfd_ports,
> > >                          struct lflow_ref *lflow_ref)
> > >  {
> > >      const struct nbrec_logical_router_static_route *route = route_->route;
> > >
> > > -    int ofs = !strcmp(smap_get_def(&route->options, "origin", ""),
> > > -                      ROUTE_ORIGIN_CONNECTED) ? ROUTE_PRIO_OFFSET_CONNECTED
> > > -                                              : ROUTE_PRIO_OFFSET_STATIC;
> > > -
> > >      char *prefix_s = build_route_prefix_s(&route_->prefix, route_->plen);
> > >      add_route(lflows, route_->is_discard_route ? od : route_->out_port->od,
> > >                route_->out_port, route_->lrp_addr_s, prefix_s,
> > > -              route_->plen, route->nexthop, route_->is_src_route,
> > > -              route_->route_table_id, bfd_ports, &route->header_,
> > > -              route_->is_discard_route, ofs, lflow_ref);
> > > +              route_->plen, route_->nexthop, route_->is_src_route,
> > > +              route_->route_table_id, bfd_ports,
> > > +              route ? &route->header_ : &route_->out_port->nbrp->header_,
> > > +              route_->is_discard_route, route_->source, lflow_ref);
> > >
> > >      free(prefix_s);
> > >  }
> > > @@ -13509,51 +13616,8 @@ build_ip_routing_pre_flows_for_lrouter(struct ovn_datapath *od,
> > >                    REG_ROUTE_TABLE_ID" = 0; next;", lflow_ref);
> > >  }
> > >
> > > -/* Logical router ingress table IP_ROUTING : IP Routing.
> > > - *
> > > - * A packet that arrives at this table is an IP packet that should be
> > > - * routed to the address in 'ip[46].dst'.
> > > - *
> > > - * For regular routes without ECMP, table IP_ROUTING sets outport to the
> > > - * correct output port, eth.src to the output port's MAC address, and
> > > - * REG_NEXT_HOP_IPV4/REG_NEXT_HOP_IPV6 to the next-hop IP address
> > > - * (leaving 'ip[46].dst', the packet’s final destination, unchanged), and
> > > - * advances to the next table.
> > > - *
> > > - * For ECMP routes, i.e. multiple routes with same policy and prefix, table
> > > - * IP_ROUTING remembers ECMP group id and selects a member id, and advances
> > > - * to table IP_ROUTING_ECMP, which sets outport, eth.src and
> > > - * REG_NEXT_HOP_IPV4/REG_NEXT_HOP_IPV6 for the selected ECMP member.
> > > - *
> > > - * This function adds routes for directly connected subnets configured on the
> > > - * LRP 'op'.
> > > - */
> > >  static void
> > > -build_ip_routing_flows_for_lrp(struct ovn_port *op,
> > > -                               const struct sset *bfd_ports,
> > > -                               struct lflow_table *lflows,
> > > -                               struct lflow_ref *lflow_ref)
> > > -{
> > > -    ovs_assert(op->nbrp);
> > > -    for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> > > -        add_route(lflows, op->od, op, op->lrp_networks.ipv4_addrs[i].addr_s,
> > > -                  op->lrp_networks.ipv4_addrs[i].network_s,
> > > -                  op->lrp_networks.ipv4_addrs[i].plen, NULL, false, 0,
> > > -                  bfd_ports, &op->nbrp->header_, false,
> > > -                  ROUTE_PRIO_OFFSET_CONNECTED, lflow_ref);
> > > -    }
> > > -
> > > -    for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> > > -        add_route(lflows, op->od, op, op->lrp_networks.ipv6_addrs[i].addr_s,
> > > -                  op->lrp_networks.ipv6_addrs[i].network_s,
> > > -                  op->lrp_networks.ipv6_addrs[i].plen, NULL, false, 0,
> > > -                  bfd_ports, &op->nbrp->header_, false,
> > > -                  ROUTE_PRIO_OFFSET_CONNECTED, lflow_ref);
> > > -    }
> > > -}
> > > -
> > > -static void
> > > -build_static_route_flows_for_lrouter(
> > > +build_route_flows_for_lrouter(
> > >          struct ovn_datapath *od, struct lflow_table *lflows,
> > >          struct hmap *parsed_routes,
> > >          struct simap *route_tables, const struct sset *bfd_ports,
> > > @@ -13580,6 +13644,10 @@ build_static_route_flows_for_lrouter(
> > >      struct parsed_route *route;
> > >      HMAP_FOR_EACH_WITH_HASH (route, key_node, uuid_hash(&od->key),
> > >                               parsed_routes) {
> > > +        if (route->source == ROUTE_SOURCE_CONNECTED) {
> > > +            unique_routes_add(&unique_routes, route);
> > > +            continue;
> > > +        }
> > >          group = ecmp_groups_find(&ecmp_groups, route);
> > >          if (group) {
> > >              ecmp_groups_add_route(group, route);
> > > @@ -13608,7 +13676,7 @@ build_static_route_flows_for_lrouter(
> > >      }
> > >      const struct unique_routes_node *ur;
> > >      HMAP_FOR_EACH (ur, hmap_node, &unique_routes) {
> > > -        build_static_route_flow(lflows, od, ur->route,
> > > +        build_route_flow(lflows, od, ur->route,
> > >                                  bfd_ports, lflow_ref);
> > >      }
> > >      ecmp_groups_destroy(&ecmp_groups);
> > > @@ -16852,7 +16920,7 @@ build_routable_flows_for_router_port(
> > >                                laddrs->ipv4_addrs[k].network_s,
> > >                                laddrs->ipv4_addrs[k].plen, NULL, false, 0,
> > >                                bfd_ports, &router_port->nbrp->header_,
> > > -                              false, ROUTE_PRIO_OFFSET_CONNECTED,
> > > +                              false, ROUTE_SOURCE_CONNECTED,
> > >                                lrp->stateful_lflow_ref);
> > >                  }
> > >              }
> > > @@ -17094,7 +17162,7 @@ build_lswitch_and_lrouter_iterate_by_lr(struct ovn_datapath *od,
> > >                                             lsi->meter_groups, NULL);
> > >      build_ND_RA_flows_for_lrouter(od, lsi->lflows, NULL);
> > >      build_ip_routing_pre_flows_for_lrouter(od, lsi->lflows, NULL);
> > > -    build_static_route_flows_for_lrouter(od, lsi->lflows,
> > > +    build_route_flows_for_lrouter(od, lsi->lflows,
> > >                                           lsi->parsed_routes, lsi->route_tables,
> > >                                           lsi->bfd_ports, NULL);
> > >      build_mcast_lookup_flows_for_lrouter(od, lsi->lflows, &lsi->match,
> > > @@ -17167,8 +17235,6 @@ build_lswitch_and_lrouter_iterate_by_lrp(struct ovn_port *op,
> > >                                            &lsi->actions, op->lflow_ref);
> > >      build_neigh_learning_flows_for_lrouter_port(op, lsi->lflows, &lsi->match,
> > >                                                  &lsi->actions, op->lflow_ref);
> > > -    build_ip_routing_flows_for_lrp(op, lsi->bfd_ports,
> > > -                                   lsi->lflows, op->lflow_ref);
> > >      build_ND_RA_flows_for_lrouter_port(op, lsi->lflows, &lsi->match,
> > >                                         &lsi->actions, lsi->meter_groups,
> > >                                         op->lflow_ref);
> > > diff --git a/northd/northd.h b/northd/northd.h
> > > index f701abd45..eb669f734 100644
> > > --- a/northd/northd.h
> > > +++ b/northd/northd.h
> > > @@ -696,10 +696,18 @@ struct ovn_port {
> > >      struct lflow_ref *stateful_lflow_ref;
> > >  };
> > >
> > > +enum route_source {
> > > +    /* the route is directly connected to the logical router */
> > > +    ROUTE_SOURCE_CONNECTED,
> >
> > I know naming is hard but maybe ROUTE_SOURCE_LOCAL?
>
> The idea behind this name was to have it similar to what physical
> routers use. There also the terms "connected" and "static" are used.
> But if you prefer i can still change it.
>
> >
> > > +    /* the route is derived from a northbound static route entry */
> > > +    ROUTE_SOURCE_STATIC,
> > > +};
> > > +
> > >  struct parsed_route {
> > >      struct hmap_node key_node;
> > >      struct in6_addr prefix;
> > >      unsigned int plen;
> > > +    struct in6_addr *nexthop; /* NULL for ROUTE_SOURCE_CONNECTED */
> > >      bool is_src_route;
> > >      uint32_t route_table_id;
> > >      uint32_t hash;
> > > @@ -708,8 +716,9 @@ struct parsed_route {
> > >      bool is_discard_route;
> > >      const struct nbrec_logical_router *nbr;
> > >      bool stale;
> > > +    enum route_source source;
> > >      char *lrp_addr_s;
> > > -    struct ovn_port *out_port;
> > > +    const struct ovn_port *out_port;
> >
> > can you add this in the patch where you added out_port pointer?
>
> Yes will do that.
>
> Thanks for the review
> Felix
>
> >
> > >  };
> > >
> > >  void ovnnb_db_run(struct northd_input *input_data,
> > > --
> > > 2.47.0
> > >
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Mon, Nov 18, 2024 at 12:50:36PM +0100, Frode Nordahl wrote:
> On Mon, Nov 18, 2024 at 12:11 PM Felix Huettner via dev
> <ovs-dev@openvswitch.org> wrote:
> >
> > On Thu, Nov 14, 2024 at 11:10:39AM +0100, Lorenzo Bianconi wrote:
> > > > previously connected routes (so routes based on the networks of the
> > > > LRPs) where handled separately from static routes.
> > > >
> > > > In order to later announce routes from ovn to the fabric we need to have
> > > > a central overview over all routes we want to announce. This includes
> > > > routes based on Logical_Router_Static_Routes as well as the networks on
> > > > Logical_Router_Port.

Hi Frode,

thanks for the review.

> 
> While I can see you have a use case that requires a central overview
> of all routes, it is not required for all use cases. For the use cases
> not requiring it, this approach appears like it would introduce
> duplication of data?
> 
> `northd` already has access to information on LRPs for flow
> generation, and the `ovn-controller` already has access to information
> on LRPs in the Port_Binding table. Is that not sufficient to announce
> connected routes?

I would agree that this is sufficient for connected routes (as long as
you don't want to advertise per-host routes).

However since needed some logic for static routes, connected-routes as
host routes and for learning routes i wanted to keep things uniform.
While this creates such large changes to northd i believe it makes the
whole concept more understandable later as all routing information comes
from a single source.

Thanks
Felix

> 
> Also, when using Netlink plugin for route exchange, the IP/MAC
> addresses of LRPs can be replicated in VRFs in the system which will
> already give the routing protocol daemon the information it needs to
> redistribute connected routes.
> 
> --
> Frode Nordahl
> 
> > > >
> > > > With this change the "routes" engine node will output a complete list of
> > > > all routes in the system. This also serves as a future integration point
> > > > to integrate routes learned from outside of OVN.
> > >
> > > Hi Felix,
> > >
> > > This patch needs a respin. Some nits inline.
> >
> > Hi Lorenzo,
> >
> > thanks for the review
> >
> > >
> > > Regards,
> > > Lorenzo
> > >
> > > >
> > > > Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> > > > ---
> > > >  northd/northd.c | 270 ++++++++++++++++++++++++++++++------------------
> > > >  northd/northd.h |  11 +-
> > > >  2 files changed, 178 insertions(+), 103 deletions(-)
> > > >
> > > > diff --git a/northd/northd.c b/northd/northd.c
> > > > index 1265e3c81..90ddc2acb 100644
> > > > --- a/northd/northd.c
> > > > +++ b/northd/northd.c
> > > > @@ -11071,6 +11071,20 @@ parsed_route_lookup(struct hmap *routes, size_t hash,
> > > >              continue;
> > > >          }
> > > >
> > > > +        if (pr->source != new_pr->source) {
> > > > +            continue;
> > > > +        }
> > > > +
> > > > +        if (!pr->nexthop != !new_pr->nexthop) {
> > > > +            continue;
> > >
> > > why not if (pr->nexthop != new_pr->nexthop) ?
> >
> > We would then compare the pointers of pr->nexthop to new_pr->nexthop
> > instead of checking if either both of them are null or non-null.
> > I'll add a comment to clarify this.
> >
> > >
> > > > +        }
> > > > +
> > > > +        if (pr->nexthop &&
> > > > +                memcmp(pr->nexthop, new_pr->nexthop,
> > > > +                       sizeof(struct in6_addr))) {
> > >
> > > I guess we can use ipv6_addr_equals() here.
> >
> > Thanks fixed.
> >
> > >
> > > > +            continue;
> > > > +        }
> > > > +
> > > >          if (memcmp(&pr->prefix, &new_pr->prefix, sizeof(struct in6_addr))) {
> > > >              continue;
> > > >          }
> > > > @@ -11111,6 +11125,9 @@ parsed_route_lookup(struct hmap *routes, size_t hash,
> > > >
> > > >  static void
> > > >  parsed_route_free(struct parsed_route *pr) {
> > > > +    if (pr->nexthop) {
> > >
> > > you can remove if () here
> >
> > Thanks fixed.
> >
> > >
> > > > +        free(pr->nexthop);
> > > > +    }
> > > >      if (pr->lrp_addr_s) {
> > > >          free(pr->lrp_addr_s);
> > > >      }
> > > > @@ -11119,31 +11136,77 @@ parsed_route_free(struct parsed_route *pr) {
> > > >  }
> > > >
> > > >  static void
> > > > -parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> > > > +parsed_route_add(const struct ovn_datapath *od,
> > > > +                 struct in6_addr *nexthop,
> > > > +                 const struct in6_addr prefix,
> > > > +                 unsigned int plen,
> > > > +                 bool is_discard_route,
> > > > +                 const char *lrp_addr_s,
> > > > +                 const struct ovn_port *out_port,
> > > > +                 const struct nbrec_logical_router_static_route *route,
> > > > +                 uint32_t route_table_id,
> > > > +                 bool is_src_route,
> > > > +                 bool ecmp_symmetric_reply,
> > > > +                 enum route_source source,
> > > > +                 struct hmap *routes)
> > > > +{
> > > > +
> > > > +    struct parsed_route *new_pr = xzalloc(sizeof *new_pr);
> > > > +    new_pr->prefix = prefix;
> > > > +    new_pr->plen = plen;
> > > > +    new_pr->nexthop = nexthop;
> > > > +    new_pr->route_table_id = route_table_id;
> > > > +    new_pr->is_src_route = is_src_route;
> > > > +    new_pr->hash = route_hash(new_pr);
> > > > +    new_pr->nbr = od->nbr;
> > > > +    new_pr->ecmp_symmetric_reply = ecmp_symmetric_reply;
> > > > +    new_pr->is_discard_route = is_discard_route;
> > > > +    if (!is_discard_route) {
> > > > +        new_pr->lrp_addr_s = xstrdup(lrp_addr_s);
> > > > +    }
> > > > +    new_pr->out_port = out_port;
> > > > +    new_pr->source = source;
> > > > +    new_pr->route = route;
> > > > +
> > > > +    size_t hash = uuid_hash(&od->key);
> > > > +    struct parsed_route *pr = parsed_route_lookup(routes, hash, new_pr);
> > > > +    if (!pr) {
> > > > +        hmap_insert(routes, &new_pr->key_node, hash);
> > > > +    } else {
> > > > +        pr->stale = false;
> > > > +        parsed_route_free(new_pr);
> > > > +    }
> > > > +}
> > > > +
> > > > +static void
> > > > +parsed_routes_add_static(struct ovn_datapath *od, const struct hmap *lr_ports,
> > > >                    const struct nbrec_logical_router_static_route *route,
> > > >                    const struct hmap *bfd_connections,
> > > >                    struct hmap *routes, struct simap *route_tables,
> > > >                    struct hmap *bfd_active_connections)
> > > >  {
> > > >      /* Verify that the next hop is an IP address with an all-ones mask. */
> > > > -    struct in6_addr nexthop;
> > > > +    struct in6_addr *nexthop = NULL;
> > >
> > > if we allow parsed_route_add() to make a private copy of it we can avoid a
> > > pointer for nexthop here and make the patch more readable (e.g. remove all
> > > free(nexthop))
> >
> > I choose this way because on a static route the nexthop field might not
> > be set. OVN currently accepts this as a valid static route and then
> > uses "ip.dst" as nexthop value (see add_route in northd.c).
> >
> > If we don't use a pointer here then parsed_route_add() would need to
> > check based on some magic address to then treat it as NULL. This magic
> > address (even though it might be invalid nexthop) would then be
> > unavailable for the user to set. So i think this would cause a behaviour
> > change.
> >
> > Therefor i decided to stick to the current pointer approach.
> >
> > >
> > > >      unsigned int plen;
> > > >      bool is_discard_route = !strcmp(route->nexthop, "discard");
> > > >      bool valid_nexthop = route->nexthop[0] && !is_discard_route;
> > > >      if (valid_nexthop) {
> > > > -        if (!ip46_parse_cidr(route->nexthop, &nexthop, &plen)) {
> > > > +        nexthop = xmalloc(sizeof(*nexthop));
> > > > +        if (!ip46_parse_cidr(route->nexthop, nexthop, &plen)) {
> > > >              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > > >              VLOG_WARN_RL(&rl, "bad 'nexthop' %s in static route "
> > > >                           UUID_FMT, route->nexthop,
> > > >                           UUID_ARGS(&route->header_.uuid));
> > > > +            free(nexthop);
> > > >              return;
> > > >          }
> > > > -        if ((IN6_IS_ADDR_V4MAPPED(&nexthop) && plen != 32) ||
> > > > -            (!IN6_IS_ADDR_V4MAPPED(&nexthop) && plen != 128)) {
> > > > +        if ((IN6_IS_ADDR_V4MAPPED(nexthop) && plen != 32) ||
> > > > +            (!IN6_IS_ADDR_V4MAPPED(nexthop) && plen != 128)) {
> > > >              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > > >              VLOG_WARN_RL(&rl, "bad next hop mask %s in static route "
> > > >                           UUID_FMT, route->nexthop,
> > > >                           UUID_ARGS(&route->header_.uuid));
> > > > +            free(nexthop);
> > > >              return;
> > > >          }
> > > >      }
> > > > @@ -11155,17 +11218,19 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> > > >          VLOG_WARN_RL(&rl, "bad 'ip_prefix' %s in static route "
> > > >                       UUID_FMT, route->ip_prefix,
> > > >                       UUID_ARGS(&route->header_.uuid));
> > > > +        free(nexthop);
> > > >          return;
> > > >      }
> > > >
> > > >      /* Verify that ip_prefix and nexthop have same address familiy. */
> > > >      if (valid_nexthop) {
> > > > -        if (IN6_IS_ADDR_V4MAPPED(&prefix) != IN6_IS_ADDR_V4MAPPED(&nexthop)) {
> > > > +        if (IN6_IS_ADDR_V4MAPPED(&prefix) != IN6_IS_ADDR_V4MAPPED(nexthop)) {
> > > >              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > > >              VLOG_WARN_RL(&rl, "Address family doesn't match between 'ip_prefix'"
> > > >                           " %s and 'nexthop' %s in static route "UUID_FMT,
> > > >                           route->ip_prefix, route->nexthop,
> > > >                           UUID_ARGS(&route->header_.uuid));
> > > > +            free(nexthop);
> > > >              return;
> > > >          }
> > > >      }
> > > > @@ -11177,6 +11242,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> > > >          !find_static_route_outport(od, lr_ports, route,
> > > >                                     IN6_IS_ADDR_V4MAPPED(&prefix),
> > > >                                     &lrp_addr_s, &out_port)) {
> > > > +        free(nexthop);
> > > >          return;
> > > >      }
> > > >
> > > > @@ -11186,6 +11252,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> > > >                                                    nb_bt->logical_port,
> > > >                                                    nb_bt->dst_ip);
> > > >          if (!bfd_e) {
> > > > +            free(nexthop);
> > > >              return;
> > > >          }
> > > >
> > > > @@ -11205,36 +11272,58 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> > > >
> > > >
> > > >          if (!strcmp(bfd_sr->status, "down")) {
> > > > +            free(nexthop);
> > > >              return;
> > > >          }
> > > >      }
> > > >
> > > > -    struct parsed_route *new_pr = xzalloc(sizeof *new_pr);
> > > > -    new_pr->prefix = prefix;
> > > > -    new_pr->plen = plen;
> > > > -    new_pr->route_table_id = get_route_table_id(route_tables,
> > > > -                                                route->route_table);
> > > > -    new_pr->is_src_route = (route->policy &&
> > > > -                            !strcmp(route->policy, "src-ip"));
> > > > -    new_pr->hash = route_hash(new_pr);
> > > > -    new_pr->route = route;
> > > > -    new_pr->nbr = od->nbr;
> > > > -    new_pr->ecmp_symmetric_reply = smap_get_bool(&route->options,
> > > > -                                                 "ecmp_symmetric_reply",
> > > > -                                                 false);
> > > > -    new_pr->is_discard_route = is_discard_route;
> > > > -    if (!is_discard_route) {
> > > > -        new_pr->lrp_addr_s = xstrdup(lrp_addr_s);
> > > > -    }
> > > > -    new_pr->out_port = out_port;
> > > > +    uint32_t route_table_id = get_route_table_id(route_tables,
> > > > +                                        route->route_table);
> > > > +    bool is_src_route = (route->policy && !strcmp(route->policy, "src-ip"));
> > > > +    bool ecmp_symmetric_reply = smap_get_bool(&route->options,
> > > > +                                         "ecmp_symmetric_reply",
> > > > +                                         false);
> > > >
> > > > -    size_t hash = uuid_hash(&od->key);
> > > > -    struct parsed_route *pr = parsed_route_lookup(routes, hash, new_pr);
> > > > -    if (!pr) {
> > > > -        hmap_insert(routes, &new_pr->key_node, hash);
> > > > +    enum route_source source;
> > > > +    if (!strcmp(smap_get_def(&route->options, "origin", ""),
> > > > +                                 ROUTE_ORIGIN_CONNECTED)) {
> > > > +        source = ROUTE_SOURCE_CONNECTED;
> > > >      } else {
> > > > -        pr->stale = false;
> > > > -        parsed_route_free(new_pr);
> > > > +        source = ROUTE_SOURCE_STATIC;
> > > > +    }
> > > > +
> > > > +    parsed_route_add(od, nexthop, prefix, plen, is_discard_route, lrp_addr_s,
> > > > +                      out_port, route, route_table_id, is_src_route,
> > > > +                      ecmp_symmetric_reply, source,
> > > > +                      routes);
> > > > +}
> > > > +
> > > > +static void
> > > > +parsed_routes_add_connected(struct ovn_datapath *od, const struct ovn_port *op,
> > > > +                            struct hmap *routes)
> > > > +{
> > > > +    for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> > > > +        const struct ipv4_netaddr *addr = &op->lrp_networks.ipv4_addrs[i];
> > > > +        struct in6_addr prefix;
> > > > +        ip46_parse(addr->network_s, &prefix);
> > > > +
> > > > +        parsed_route_add(od, NULL, prefix, addr->plen,
> > > > +                          false, addr->addr_s, op,
> > > > +                          NULL, 0, false,
> > > > +                          false, ROUTE_SOURCE_CONNECTED,
> > > > +                          routes);
> > > > +    }
> > > > +
> > > > +    for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> > > > +        const struct ipv6_netaddr *addr = &op->lrp_networks.ipv6_addrs[i];
> > > > +        struct in6_addr prefix;
> > > > +        ip46_parse(addr->network_s, &prefix);
> > > > +
> > > > +        parsed_route_add(od, NULL, prefix, addr->plen,
> > > > +                          false, addr->addr_s, op,
> > > > +                          NULL, 0, false,
> > > > +                          false, ROUTE_SOURCE_CONNECTED,
> > > > +                          routes);
> > > >      }
> > > >  }
> > > >
> > > > @@ -11252,9 +11341,14 @@ build_parsed_routes(struct ovn_datapath *od, const struct hmap *lr_ports,
> > > >      }
> > > >
> > > >      for (int i = 0; i < od->nbr->n_static_routes; i++) {
> > > > -        parsed_routes_add(od, lr_ports, od->nbr->static_routes[i],
> > > > -                          bfd_connections, routes, route_tables,
> > > > -                          bfd_active_connections);
> > > > +        parsed_routes_add_static(od, lr_ports, od->nbr->static_routes[i],
> > > > +                                 bfd_connections, routes, route_tables,
> > > > +                                 bfd_active_connections);
> > > > +    }
> > > > +
> > > > +    const struct ovn_port *op;
> > > > +    HMAP_FOR_EACH (op, dp_node, &od->ports) {
> > > > +        parsed_routes_add_connected(od, op, routes);
> > > >      }
> > > >
> > > >      HMAP_FOR_EACH_SAFE (pr, key_node, routes) {
> > > > @@ -11279,7 +11373,7 @@ struct ecmp_groups_node {
> > > >      struct in6_addr prefix;
> > > >      unsigned int plen;
> > > >      bool is_src_route;
> > > > -    const char *origin;
> > > > +    enum route_source source;
> > > >      uint32_t route_table_id;
> > > >      uint16_t route_count;
> > > >      struct ovs_list route_list; /* Contains ecmp_route_list_node */
> > > > @@ -11318,7 +11412,7 @@ ecmp_groups_add(struct hmap *ecmp_groups,
> > > >      eg->prefix = route->prefix;
> > > >      eg->plen = route->plen;
> > > >      eg->is_src_route = route->is_src_route;
> > > > -    eg->origin = smap_get_def(&route->route->options, "origin", "");
> > > > +    eg->source = route->source;
> > > >      eg->route_table_id = route->route_table_id;
> > > >      ovs_list_init(&eg->route_list);
> > > >      ecmp_groups_add_route(eg, route);
> > > > @@ -11382,6 +11476,7 @@ unique_routes_remove(struct hmap *unique_routes,
> > > >          if (ipv6_addr_equals(&route->prefix, &ur->route->prefix) &&
> > > >              route->plen == ur->route->plen &&
> > > >              route->is_src_route == ur->route->is_src_route &&
> > > > +            route->source == ur->route->source &&
> > > >              route->route_table_id == ur->route->route_table_id) {
> > > >              hmap_remove(unique_routes, &ur->hmap_node);
> > > >              const struct parsed_route *existed_route = ur->route;
> > > > @@ -11515,7 +11610,7 @@ static void
> > > >  add_ecmp_symmetric_reply_flows(struct lflow_table *lflows,
> > > >                                 struct ovn_datapath *od,
> > > >                                 const char *port_ip,
> > > > -                               struct ovn_port *out_port,
> > > > +                               const struct ovn_port *out_port,
> > > >                                 const struct parsed_route *route,
> > > >                                 struct ds *route_match,
> > > >                                 struct lflow_ref *lflow_ref)
> > > > @@ -11609,6 +11704,19 @@ add_ecmp_symmetric_reply_flows(struct lflow_table *lflows,
> > > >      ds_destroy(&ecmp_reply);
> > > >  }
> > > >
> > > > +static int
> > > > +route_source_to_offset(enum route_source source)
> > > > +{
> > > > +    switch (source) {
> > > > +        case ROUTE_SOURCE_CONNECTED:
> > > > +            return ROUTE_PRIO_OFFSET_CONNECTED;
> > > > +        case ROUTE_SOURCE_STATIC:
> > > > +            return ROUTE_PRIO_OFFSET_STATIC;
> > > > +        default:
> > > > +            OVS_NOT_REACHED();
> > > > +    }
> > > > +}
> > > > +
> > > >  static void
> > > >  build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> > > >                        struct ecmp_groups_node *eg, struct lflow_ref *lflow_ref)
> > > > @@ -11620,8 +11728,7 @@ build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> > > >      struct ds route_match = DS_EMPTY_INITIALIZER;
> > > >
> > > >      char *prefix_s = build_route_prefix_s(&eg->prefix, eg->plen);
> > > > -    int ofs = !strcmp(eg->origin, ROUTE_ORIGIN_CONNECTED) ?
> > > > -        ROUTE_PRIO_OFFSET_CONNECTED: ROUTE_PRIO_OFFSET_STATIC;
> > > > +    int ofs = route_source_to_offset(eg->source);
> > > >      build_route_match(NULL, eg->route_table_id, prefix_s, eg->plen,
> > > >                        eg->is_src_route, is_ipv4, &route_match, &priority, ofs);
> > > >      free(prefix_s);
> > > > @@ -11675,13 +11782,14 @@ build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> > > >                        REG_ECMP_MEMBER_ID" == %"PRIu16,
> > > >                        eg->id, er->id);
> > > >          ds_clear(&actions);
> > > > -        ds_put_format(&actions, "%s = %s; "
> > > > +        ds_put_format(&actions, "%s = ",
> > > > +                      is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6);
> > > > +        ipv6_format_mapped(route_->nexthop, &actions);
> > > > +        ds_put_format(&actions, "; "
> > > >                        "%s = %s; "
> > > >                        "eth.src = %s; "
> > > >                        "outport = %s; "
> > > >                        "next;",
> > > > -                      is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6,
> > > > -                      route->nexthop,
> > > >                        is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6,
> > > >                        route_->lrp_addr_s,
> > > >                        route_->out_port->lrp_networks.ea_s,
> > > > @@ -11699,17 +11807,19 @@ build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> > > >  static void
> > > >  add_route(struct lflow_table *lflows, struct ovn_datapath *od,
> > > >            const struct ovn_port *op, const char *lrp_addr_s,
> > > > -          const char *network_s, int plen, const char *gateway,
> > > > +          const char *network_s, int plen, const struct in6_addr *gateway,
> > > >            bool is_src_route, const uint32_t rtb_id,
> > > >            const struct sset *bfd_ports,
> > > >            const struct ovsdb_idl_row *stage_hint, bool is_discard_route,
> > > > -          int ofs, struct lflow_ref *lflow_ref)
> > > > +          enum route_source source, struct lflow_ref *lflow_ref)
> > > >  {
> > > >      bool is_ipv4 = strchr(network_s, '.') ? true : false;
> > > >      struct ds match = DS_EMPTY_INITIALIZER;
> > > >      uint16_t priority;
> > > >      const struct ovn_port *op_inport = NULL;
> > > >
> > > > +    int ofs = route_source_to_offset(source);
> > > > +
> > > >      /* IPv6 link-local addresses must be scoped to the local router port. */
> > > >      if (!is_ipv4) {
> > > >          struct in6_addr network;
> > > > @@ -11728,8 +11838,8 @@ add_route(struct lflow_table *lflows, struct ovn_datapath *od,
> > > >      } else {
> > > >          ds_put_format(&common_actions, REG_ECMP_GROUP_ID" = 0; %s = ",
> > > >                        is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6);
> > > > -        if (gateway && gateway[0]) {
> > > > -            ds_put_cstr(&common_actions, gateway);
> > > > +        if (gateway) {
> > > > +            ipv6_format_mapped(gateway, &common_actions);
> > > >          } else {
> > > >              ds_put_format(&common_actions, "ip%s.dst", is_ipv4 ? "4" : "6");
> > > >          }
> > > > @@ -11764,23 +11874,20 @@ add_route(struct lflow_table *lflows, struct ovn_datapath *od,
> > > >  }
> > > >
> > > >  static void
> > > > -build_static_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> > > > +build_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> > > >                          const struct parsed_route *route_,
> > > >                          const struct sset *bfd_ports,
> > > >                          struct lflow_ref *lflow_ref)
> > > >  {
> > > >      const struct nbrec_logical_router_static_route *route = route_->route;
> > > >
> > > > -    int ofs = !strcmp(smap_get_def(&route->options, "origin", ""),
> > > > -                      ROUTE_ORIGIN_CONNECTED) ? ROUTE_PRIO_OFFSET_CONNECTED
> > > > -                                              : ROUTE_PRIO_OFFSET_STATIC;
> > > > -
> > > >      char *prefix_s = build_route_prefix_s(&route_->prefix, route_->plen);
> > > >      add_route(lflows, route_->is_discard_route ? od : route_->out_port->od,
> > > >                route_->out_port, route_->lrp_addr_s, prefix_s,
> > > > -              route_->plen, route->nexthop, route_->is_src_route,
> > > > -              route_->route_table_id, bfd_ports, &route->header_,
> > > > -              route_->is_discard_route, ofs, lflow_ref);
> > > > +              route_->plen, route_->nexthop, route_->is_src_route,
> > > > +              route_->route_table_id, bfd_ports,
> > > > +              route ? &route->header_ : &route_->out_port->nbrp->header_,
> > > > +              route_->is_discard_route, route_->source, lflow_ref);
> > > >
> > > >      free(prefix_s);
> > > >  }
> > > > @@ -13509,51 +13616,8 @@ build_ip_routing_pre_flows_for_lrouter(struct ovn_datapath *od,
> > > >                    REG_ROUTE_TABLE_ID" = 0; next;", lflow_ref);
> > > >  }
> > > >
> > > > -/* Logical router ingress table IP_ROUTING : IP Routing.
> > > > - *
> > > > - * A packet that arrives at this table is an IP packet that should be
> > > > - * routed to the address in 'ip[46].dst'.
> > > > - *
> > > > - * For regular routes without ECMP, table IP_ROUTING sets outport to the
> > > > - * correct output port, eth.src to the output port's MAC address, and
> > > > - * REG_NEXT_HOP_IPV4/REG_NEXT_HOP_IPV6 to the next-hop IP address
> > > > - * (leaving 'ip[46].dst', the packet’s final destination, unchanged), and
> > > > - * advances to the next table.
> > > > - *
> > > > - * For ECMP routes, i.e. multiple routes with same policy and prefix, table
> > > > - * IP_ROUTING remembers ECMP group id and selects a member id, and advances
> > > > - * to table IP_ROUTING_ECMP, which sets outport, eth.src and
> > > > - * REG_NEXT_HOP_IPV4/REG_NEXT_HOP_IPV6 for the selected ECMP member.
> > > > - *
> > > > - * This function adds routes for directly connected subnets configured on the
> > > > - * LRP 'op'.
> > > > - */
> > > >  static void
> > > > -build_ip_routing_flows_for_lrp(struct ovn_port *op,
> > > > -                               const struct sset *bfd_ports,
> > > > -                               struct lflow_table *lflows,
> > > > -                               struct lflow_ref *lflow_ref)
> > > > -{
> > > > -    ovs_assert(op->nbrp);
> > > > -    for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> > > > -        add_route(lflows, op->od, op, op->lrp_networks.ipv4_addrs[i].addr_s,
> > > > -                  op->lrp_networks.ipv4_addrs[i].network_s,
> > > > -                  op->lrp_networks.ipv4_addrs[i].plen, NULL, false, 0,
> > > > -                  bfd_ports, &op->nbrp->header_, false,
> > > > -                  ROUTE_PRIO_OFFSET_CONNECTED, lflow_ref);
> > > > -    }
> > > > -
> > > > -    for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> > > > -        add_route(lflows, op->od, op, op->lrp_networks.ipv6_addrs[i].addr_s,
> > > > -                  op->lrp_networks.ipv6_addrs[i].network_s,
> > > > -                  op->lrp_networks.ipv6_addrs[i].plen, NULL, false, 0,
> > > > -                  bfd_ports, &op->nbrp->header_, false,
> > > > -                  ROUTE_PRIO_OFFSET_CONNECTED, lflow_ref);
> > > > -    }
> > > > -}
> > > > -
> > > > -static void
> > > > -build_static_route_flows_for_lrouter(
> > > > +build_route_flows_for_lrouter(
> > > >          struct ovn_datapath *od, struct lflow_table *lflows,
> > > >          struct hmap *parsed_routes,
> > > >          struct simap *route_tables, const struct sset *bfd_ports,
> > > > @@ -13580,6 +13644,10 @@ build_static_route_flows_for_lrouter(
> > > >      struct parsed_route *route;
> > > >      HMAP_FOR_EACH_WITH_HASH (route, key_node, uuid_hash(&od->key),
> > > >                               parsed_routes) {
> > > > +        if (route->source == ROUTE_SOURCE_CONNECTED) {
> > > > +            unique_routes_add(&unique_routes, route);
> > > > +            continue;
> > > > +        }
> > > >          group = ecmp_groups_find(&ecmp_groups, route);
> > > >          if (group) {
> > > >              ecmp_groups_add_route(group, route);
> > > > @@ -13608,7 +13676,7 @@ build_static_route_flows_for_lrouter(
> > > >      }
> > > >      const struct unique_routes_node *ur;
> > > >      HMAP_FOR_EACH (ur, hmap_node, &unique_routes) {
> > > > -        build_static_route_flow(lflows, od, ur->route,
> > > > +        build_route_flow(lflows, od, ur->route,
> > > >                                  bfd_ports, lflow_ref);
> > > >      }
> > > >      ecmp_groups_destroy(&ecmp_groups);
> > > > @@ -16852,7 +16920,7 @@ build_routable_flows_for_router_port(
> > > >                                laddrs->ipv4_addrs[k].network_s,
> > > >                                laddrs->ipv4_addrs[k].plen, NULL, false, 0,
> > > >                                bfd_ports, &router_port->nbrp->header_,
> > > > -                              false, ROUTE_PRIO_OFFSET_CONNECTED,
> > > > +                              false, ROUTE_SOURCE_CONNECTED,
> > > >                                lrp->stateful_lflow_ref);
> > > >                  }
> > > >              }
> > > > @@ -17094,7 +17162,7 @@ build_lswitch_and_lrouter_iterate_by_lr(struct ovn_datapath *od,
> > > >                                             lsi->meter_groups, NULL);
> > > >      build_ND_RA_flows_for_lrouter(od, lsi->lflows, NULL);
> > > >      build_ip_routing_pre_flows_for_lrouter(od, lsi->lflows, NULL);
> > > > -    build_static_route_flows_for_lrouter(od, lsi->lflows,
> > > > +    build_route_flows_for_lrouter(od, lsi->lflows,
> > > >                                           lsi->parsed_routes, lsi->route_tables,
> > > >                                           lsi->bfd_ports, NULL);
> > > >      build_mcast_lookup_flows_for_lrouter(od, lsi->lflows, &lsi->match,
> > > > @@ -17167,8 +17235,6 @@ build_lswitch_and_lrouter_iterate_by_lrp(struct ovn_port *op,
> > > >                                            &lsi->actions, op->lflow_ref);
> > > >      build_neigh_learning_flows_for_lrouter_port(op, lsi->lflows, &lsi->match,
> > > >                                                  &lsi->actions, op->lflow_ref);
> > > > -    build_ip_routing_flows_for_lrp(op, lsi->bfd_ports,
> > > > -                                   lsi->lflows, op->lflow_ref);
> > > >      build_ND_RA_flows_for_lrouter_port(op, lsi->lflows, &lsi->match,
> > > >                                         &lsi->actions, lsi->meter_groups,
> > > >                                         op->lflow_ref);
> > > > diff --git a/northd/northd.h b/northd/northd.h
> > > > index f701abd45..eb669f734 100644
> > > > --- a/northd/northd.h
> > > > +++ b/northd/northd.h
> > > > @@ -696,10 +696,18 @@ struct ovn_port {
> > > >      struct lflow_ref *stateful_lflow_ref;
> > > >  };
> > > >
> > > > +enum route_source {
> > > > +    /* the route is directly connected to the logical router */
> > > > +    ROUTE_SOURCE_CONNECTED,
> > >
> > > I know naming is hard but maybe ROUTE_SOURCE_LOCAL?
> >
> > The idea behind this name was to have it similar to what physical
> > routers use. There also the terms "connected" and "static" are used.
> > But if you prefer i can still change it.
> >
> > >
> > > > +    /* the route is derived from a northbound static route entry */
> > > > +    ROUTE_SOURCE_STATIC,
> > > > +};
> > > > +
> > > >  struct parsed_route {
> > > >      struct hmap_node key_node;
> > > >      struct in6_addr prefix;
> > > >      unsigned int plen;
> > > > +    struct in6_addr *nexthop; /* NULL for ROUTE_SOURCE_CONNECTED */
> > > >      bool is_src_route;
> > > >      uint32_t route_table_id;
> > > >      uint32_t hash;
> > > > @@ -708,8 +716,9 @@ struct parsed_route {
> > > >      bool is_discard_route;
> > > >      const struct nbrec_logical_router *nbr;
> > > >      bool stale;
> > > > +    enum route_source source;
> > > >      char *lrp_addr_s;
> > > > -    struct ovn_port *out_port;
> > > > +    const struct ovn_port *out_port;
> > >
> > > can you add this in the patch where you added out_port pointer?
> >
> > Yes will do that.
> >
> > Thanks for the review
> > Felix
> >
> > >
> > > >  };
> > > >
> > > >  void ovnnb_db_run(struct northd_input *input_data,
> > > > --
> > > > 2.47.0
> > > >
> > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > dev@openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev