[ovs-dev] [RFC ovn] northd: set max priority for automatic routes

Han Zhou hzhou at ovn.org
Tue Feb 16 07:46:55 UTC 2021


On Wed, Feb 10, 2021 at 2:17 PM Lorenzo Bianconi <
lorenzo.bianconi at redhat.com> wrote:
>
> Increase priority for automatic routes (routes created assigning IP
> addresses to OVN logical router interfaces) in order to always prefer them
> over static routes since the router has a direct link to the destination
> address (possible use-case can be found here [0]).
>
> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1891516
>

Hi Lorenzo, Tim,

While the patch may solve the problem in the bug report, I think there is
something more fundamental to be discussed. The problem is caused by
ovn-k8s's use of "src-ip" in static routes which overrides the
direct-connected route. I think the implementation of "src-ip" support in
the static route is somehow flawed. The priorities of the flows generated
by static routes are calculated according to the prefix length, so that
longest prefix routes are prioritised when there are multiple route
matches, which is correct when comparing matches among "dst-ip" routes or
among "src-ip" routes, but is not correct between "dst-ip" and "src-ip"
routes. Comparison of prefix length between these two types of static
routes doesn't make sense, because they match by different fields (src-ip
v.s. dst-ip). For example, when there are static routes:
1. 192.168.0.0/24 via 100.64.0.1 src-ip
2. 10.0.0.0/20 via 100.64.0.2 dst-ip

In this example, a packet from 192.168.0.1 to 10.0.0.1 matches both routes,
but it is unreasonable to say it should follow the 1st route just because
it has longer prefix length. Instead, we should prioritize one type over
the other. It seems in physical router implementation policy based routing
always has higher priority than destination routing, so we should probably
enforce it in a similar way in OVN, i.e. set "src-ip" flows with higher
priority than all the "dst-ip" flows. In fact, the policy routing table
already supported this behavior because it is examined before the static
route table.

Since the "src-ip" feature in the static route table is flawed, and can be
replaced by the policy routing table, I'd suggest to deprecate it. For
correctness, users (like ovn-k8s) should use the policy routing table
instead for the src-ip based routing rules. Users have full control of how
they want the packets to be routed. For the use case mentioned in the bug
report, it should have two rules in the policy routing table:

100 ip.dst == 100.64.0.0/16 accept # for directly connected destination,
skip the src-ip rules
90   ip.src == 10.244.0.0/24 nexthop 100.64.0.1 # src-ip rules

Would this better satisfy the need of ovn-k8s?
If the above is agreed, the priority change of directly connected routes
from this patch is irrelevant to the problem reported in the bug, because
policy routing rules are examined before the static routing table anyway,
so no matter how high the route priority is, it wouldn't matter. In
addition, it seems to me no real use cases would benefit from this change,
but I could be wrong and please correct me if so.

Thanks,
Han

> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> ---
>  northd/ovn-northd.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index b2b5f6a1b..dc8706f2f 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -7994,18 +7994,23 @@ build_route_prefix_s(const struct in6_addr
*prefix, unsigned int plen)
>
>  static void
>  build_route_match(const struct ovn_port *op_inport, const char
*network_s,
> -                  int plen, bool is_src_route, bool is_ipv4, struct ds
*match,
> -                  uint16_t *priority)
> +                  int plen, bool is_src_route, bool is_ipv4, bool
automatic,
> +                  struct ds *match, uint16_t *priority)
>  {
> +    int prefix_len = plen;
>      const char *dir;
>      /* The priority here is calculated to implement longest-prefix-match
> -     * routing. */
> +     * routing. Automatic routes have max priority */
> +    if (automatic) {
> +        prefix_len = is_ipv4 ? 32 : 128;
> +        prefix_len++;
> +    }
>      if (is_src_route) {
>          dir = "src";
> -        *priority = plen * 2;
> +        *priority = prefix_len * 2;
>      } else {
>          dir = "dst";
> -        *priority = (plen * 2) + 1;
> +        *priority = (prefix_len * 2) + 1;
>      }
>
>      if (op_inport) {
> @@ -8172,7 +8177,7 @@ build_ecmp_route_flow(struct hmap *lflows, struct
ovn_datapath *od,
>
>      char *prefix_s = build_route_prefix_s(&eg->prefix, eg->plen);
>      build_route_match(NULL, prefix_s, eg->plen, eg->is_src_route,
is_ipv4,
> -                      &route_match, &priority);
> +                      false, &route_match, &priority);
>      free(prefix_s);
>
>      struct ds actions = DS_EMPTY_INITIALIZER;
> @@ -8246,7 +8251,7 @@ build_ecmp_route_flow(struct hmap *lflows, struct
ovn_datapath *od,
>  static void
>  add_route(struct hmap *lflows, const struct ovn_port *op,
>            const char *lrp_addr_s, const char *network_s, int plen,
> -          const char *gateway, bool is_src_route,
> +          const char *gateway, bool is_src_route, bool automatic,
>            const struct ovsdb_idl_row *stage_hint)
>  {
>      bool is_ipv4 = strchr(network_s, '.') ? true : false;
> @@ -8263,7 +8268,7 @@ add_route(struct hmap *lflows, const struct
ovn_port *op,
>          }
>      }
>      build_route_match(op_inport, network_s, plen, is_src_route, is_ipv4,
> -                      &match, &priority);
> +                      automatic, &match, &priority);
>
>      struct ds common_actions = DS_EMPTY_INITIALIZER;
>      ds_put_format(&common_actions, REG_ECMP_GROUP_ID" = 0; %s = ",
> @@ -8319,7 +8324,7 @@ build_static_route_flow(struct hmap *lflows, struct
ovn_datapath *od,
>
>      char *prefix_s = build_route_prefix_s(&route_->prefix, route_->plen);
>      add_route(lflows, out_port, lrp_addr_s, prefix_s, route_->plen,
> -              route->nexthop, route_->is_src_route,
> +              route->nexthop, route_->is_src_route, false,
>                &route->header_);
>
>      free(prefix_s);
> @@ -9389,14 +9394,14 @@ build_ip_routing_flows_for_lrouter_port(
>              add_route(lflows, 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,
> -                      &op->nbrp->header_);
> +                      true, &op->nbrp->header_);
>          }
>
>          for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
>              add_route(lflows, 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,
> -                      &op->nbrp->header_);
> +                      true, &op->nbrp->header_);
>          }
>      }
>  }
> --
> 2.29.2
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list