[ovs-dev] [PATCH ovn v8 2/5] northd: make connected routes have higher priority than static

Han Zhou zhouhan at gmail.com
Thu Nov 18 07:57:53 UTC 2021


Sounds good to me.

On Wed, Nov 17, 2021 at 11:53 PM Vladislav Odintsov <odivlad at gmail.com>
wrote:

> Thanks, Han.
> Please see inline.
>
> Regards,
> Vladislav Odintsov
>
> On 18 Nov 2021, at 10:26, Han Zhou <zhouhan at gmail.com> wrote:
>
> On Wed, Nov 17, 2021 at 10:10 PM Vladislav Odintsov <odivlad at gmail.com>
> wrote:
>
>
> Great, thanks.
>
> Hi @Han,
>
> I’d like you to look at the patch series too. Would you have time on it?
> If yes, could you redirect me on terms please.
>
>
> Hi Vladislav,
>
> Thanks for adding me. I am sorry that I don't think I will have enough time
> for a detailed review for this series until Nov 29. Not sure if you can
> wait that long, but I don't think my review is mandatory if Numan is
> reviewing all the patches in detail.
>
>
> It’s okay from my side to wait for Dec 2-3.
>
> I have a quick comment though, regarding the priority offset. It is
> mentioned in the commit message:
>
> Each route's prefix length has its own 'slot' in lflow prios.
> Now prefix length space is calculated using next information:
> to calculate route's priority prefixlen multiplied by 3
> + route origin offset (0 - source-based route; 1 - directly-
> connected route; 2 - static route).
>
>
> But in the code, 2 is for connected, and 1 is for static:
>
> +#define ROUTE_PRIO_OFFSET_MULTIPLIER 3
> +#define ROUTE_PRIO_OFFSET_STATIC 1
> +#define ROUTE_PRIO_OFFSET_CONNECTED 2
>
>
> I wonder which one is your intent? I'd let the static route have higher
> priority, because otherwise why would the user add the static route at all?
> But this is more of a question than a suggestion. Is there any *standard*
> behavior or similar thing that we can refer from e.g. AWS?
>
>
> It’s a typo in commit message. I’ll fix that in v9.
>
> It is done to support well-known behaviour, where directly-connected
> routes take precedence over static routes for same CIDR.
>
> To support AWS feature, where user can override "subnet" route (think,
> "connected") with a static route, additional work is needed.
> It’s not what I’m currently working on, but I thought about such use case
> and it seems that it can be easily supported by adding ability to add
> Logical_Router_Static_Route with some "override" flag, which ensures this
> static route would be installed with the highest priority.
>
> So, if no objections here or other comments for now I’ll send v9.
>
> Thanks,
> Han
>
>
> Thanks.
>
> Regards,
> Vladislav Odintsov
>
> On 18 Nov 2021, at 00:05, Numan Siddique <numans at ovn.org> wrote:
>
> On Wed, Nov 17, 2021 at 3:38 PM Vladislav Odintsov <odivlad at gmail.com
>
> <mailto:odivlad at gmail.com <odivlad at gmail.com>>> wrote:
>
>
> I’ve submitted a patch [1] with my findings.
> Also, if no comments for other my patches from this patch series, I
>
> can submit a new version.
>
> Should I?
>
>
> No comments from my side.  Perhaps you can submit another version.
>
> Numan
>
>
> 1:
>
>
> https://patchwork.ozlabs.org/project/ovn/patch/20211117203545.46142-1-odivlad@gmail.com/
> <
>
> https://patchwork.ozlabs.org/project/ovn/patch/20211117203545.46142-1-odivlad@gmail.com/
>
>
>
> Regards,
> Vladislav Odintsov
>
> On 17 Nov 2021, at 20:57, Numan Siddique <numans at ovn.org <mailto:
>
> numans at ovn.org>> wrote:
>
>
> On Wed, Nov 17, 2021 at 9:24 AM Vladislav Odintsov <odivlad at gmail.com
>
> <mailto:odivlad at gmail.com <odivlad at gmail.com>> <mailto:odivlad at gmail.com
> <odivlad at gmail.com> <mailto:
> odivlad at gmail.com>>> wrote:
>
>
> Two additions:
>
> 1. Regarding documentation for flow in lr_in_defrag section:
>
> It seems to me that documentation for it is written in a wrong
>
> section (lr_in_defrag).
>
> Since the flow is installed in lr_in_ip_routing, it should be
>
> documented there.
>
> I’ll move it if you don’t mind.
>
>
> Sure.  Thanks.  I'd suggest having a separate patch for fixing the
> documentation.
>
>
>
> 2. Documentation for other flow changes was added, but I’ve
>
> committed it to a wrong patch (#3).
>
> I’ll move documentation update between patches.
>
>
> Ack.
>
> Thanks
> Numan
>
>
> Regards,
> Vladislav Odintsov
>
> On 17 Nov 2021, at 13:51, Vladislav Odintsov <odivlad at gmail.com>
>
> wrote:
>
>
> Hi Numan,
>
> Thanks for the review.
> Sure I will fix this. Should I wait for more comments or that’s all
>
> and I can send v9?
>
>
> Regards,
> Vladislav Odintsov
>
> On 17 Nov 2021, at 05:17, Numan Siddique <numans at ovn.org> wrote:
>
> On Sat, Nov 13, 2021 at 4:44 AM Vladislav Odintsov <
>
> odivlad at gmail.com <mailto:odivlad at gmail.com <odivlad at gmail.com>>> wrote:
>
>
> With this patch routes to connected networks have higher
> priority than static routes with same ip_prefix.
>
> This brings commonly-used behaviour for routes lookup order:
> 1: longest prefix match
> 2: metric
>
> The metric has next lookup order:
> 1: connected routes
> 2: static routes
>
> Earlier static and connected routes with same ip_prefix had
> the same priority, so it was impossible to predict which one
> is used for routing decision.
>
> Each route's prefix length has its own 'slot' in lflow prios.
> Now prefix length space is calculated using next information:
> to calculate route's priority prefixlen multiplied by 3
> + route origin offset (0 - source-based route; 1 - directly-
> connected route; 2 - static route).
>
> Also, enlarge prio for generic records in lr_in_ip_routing stage
> by 10000.
>
> Signed-off-by: Vladislav Odintsov <odivlad at gmail.com>
>
>
> Hi Vladislav,
>
> Thanks for the patch.  Overall it looks good to me.  I've one
>
> comment.
>
> Looks like the documentation updated in ovn-northd.8.xml is not
>
> accurate.
>
>
> This patch modifies the flows in the lr_in_ip_routing stage but
>
> this
>
> patch doesn't update the documentation.
> Also the patch updates the documentation for the flow in
>
> lr_in_defrag
>
> stage, which seems not correct.
>
> Can you please update the documentation accurately ?
>
> Numan
>
> ---
> northd/northd.c         | 50
>
> ++++++++++++++++++++++++++++-------------
>
> northd/ovn-northd.8.xml | 12 +++++-----
> tests/ovn-northd.at     |  8 +++----
> 3 files changed, 45 insertions(+), 25 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 1e8a3457c..0d513f039 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -305,6 +305,15 @@ enum ovn_stage {
> *
> */
>
> +/*
> + * Route offsets implement logic to prioritize traffic for
>
> routes with
>
> + * same ip_prefix values:
> + *  -  connected route overrides static one;
> + *  -  static route overrides connected route. */
> +#define ROUTE_PRIO_OFFSET_MULTIPLIER 3
> +#define ROUTE_PRIO_OFFSET_STATIC 1
> +#define ROUTE_PRIO_OFFSET_CONNECTED 2
> +
> /* Returns an "enum ovn_stage" built from the arguments. */
> static enum ovn_stage
> ovn_stage_build(enum ovn_datapath_type dp_type, enum ovn_pipeline
>
> pipeline,
>
> @@ -8782,6 +8791,7 @@ struct ecmp_groups_node {
> struct in6_addr prefix;
> unsigned int plen;
> bool is_src_route;
> +    const char *origin;
> uint16_t route_count;
> struct ovs_list route_list; /* Contains ecmp_route_list_node */
> };
> @@ -8819,6 +8829,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",
>
> "");
>
> ovs_list_init(&eg->route_list);
> ecmp_groups_add_route(eg, route);
>
> @@ -8919,19 +8930,20 @@ 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)
> +                  uint16_t *priority, int ofs)
> {
> const char *dir;
> /* The priority here is calculated to implement
>
> longest-prefix-match
>
>  * routing. */
> if (is_src_route) {
>     dir = "src";
> -        *priority = plen * 2;
> +        ofs = 0;
> } else {
>     dir = "dst";
> -        *priority = (plen * 2) + 1;
> }
>
> +    *priority = (plen * ROUTE_PRIO_OFFSET_MULTIPLIER) + ofs;
> +
> if (op_inport) {
>     ds_put_format(match, "inport == %s && ",
>
> op_inport->json_key);
>
> }
> @@ -9073,7 +9085,7 @@ add_ecmp_symmetric_reply_flows(struct hmap
>
> *lflows,
>
>               out_port->lrp_networks.ea_s,
>               IN6_IS_ADDR_V4MAPPED(&route->prefix) ? "" : "xx",
>               port_ip, out_port->json_key);
> -    ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_IP_ROUTING,
>
> 300,
>
> +    ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_IP_ROUTING,
>
> 10300,
>
>                        ds_cstr(&match), ds_cstr(&actions),
>                        &st_route->header_);
>
> @@ -9103,8 +9115,10 @@ build_ecmp_route_flow(struct hmap *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;
> build_route_match(NULL, prefix_s, eg->plen, eg->is_src_route,
>
> is_ipv4,
>
> -                      &route_match, &priority);
> +                      &route_match, &priority, ofs);
> free(prefix_s);
>
> struct ds actions = DS_EMPTY_INITIALIZER;
> @@ -9180,7 +9194,7 @@ add_route(struct hmap *lflows, struct
>
> ovn_datapath *od,
>
>       const struct ovn_port *op, const char *lrp_addr_s,
>       const char *network_s, int plen, const char *gateway,
>       bool is_src_route, const struct ovsdb_idl_row *stage_hint,
> -          bool is_discard_route)
> +          bool is_discard_route, int ofs)
> {
> bool is_ipv4 = strchr(network_s, '.') ? true : false;
> struct ds match = DS_EMPTY_INITIALIZER;
> @@ -9196,7 +9210,7 @@ add_route(struct hmap *lflows, struct
>
> ovn_datapath *od,
>
>     }
> }
> build_route_match(op_inport, network_s, plen, is_src_route,
>
> is_ipv4,
>
> -                      &match, &priority);
> +                      &match, &priority, ofs);
>
> struct ds common_actions = DS_EMPTY_INITIALIZER;
> struct ds actions = DS_EMPTY_INITIALIZER;
> @@ -9256,10 +9270,15 @@ build_static_route_flow(struct hmap
>
> *lflows, struct ovn_datapath *od,
>
>     }
> }
>
> +    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 : out_port->od,
>
> out_port,
>
>           lrp_addr_s, prefix_s, route_->plen, route->nexthop,
> -              route_->is_src_route, &route->header_,
>
> route_->is_discard_route);
>
> +              route_->is_src_route, &route->header_,
>
> route_->is_discard_route,
>
> +              ofs);
>
> free(prefix_s);
> }
> @@ -10672,14 +10691,14 @@ build_ip_routing_flows_for_lrouter_port(
>         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,
>
> -                      &op->nbrp->header_, false);
> +                      &op->nbrp->header_, false,
>
> ROUTE_PRIO_OFFSET_CONNECTED);
>
>     }
>
>     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,
>
> -                      &op->nbrp->header_, false);
> +                      &op->nbrp->header_, false,
>
> ROUTE_PRIO_OFFSET_CONNECTED);
>
>     }
> } else if (lsp_is_router(op->nbsp)) {
>     struct ovn_port *peer = ovn_port_get_peer(ports, op);
> @@ -10702,7 +10721,8 @@ build_ip_routing_flows_for_lrouter_port(
>
> peer->lrp_networks.ipv4_addrs[0].addr_s,
>
>                           laddrs->ipv4_addrs[k].network_s,
>                           laddrs->ipv4_addrs[k].plen, NULL,
>
> false,
>
> -                              &peer->nbrp->header_, false);
> +                              &peer->nbrp->header_, false,
> +                              ROUTE_PRIO_OFFSET_CONNECTED);
>             }
>         }
>     }
> @@ -10773,7 +10793,7 @@ build_mcast_lookup_flows_for_lrouter(
>     /* Drop IPv6 multicast traffic that shouldn't be forwarded,
>      * i.e., router solicitation and router advertisement.
>      */
> -        ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 550,
> +        ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 10550,
>                   "nd_rs || nd_ra", "drop;");
>     if (!od->mcast_info.rtr.relay) {
>         return;
> @@ -10801,7 +10821,7 @@ build_mcast_lookup_flows_for_lrouter(
>         }
>         ds_put_format(actions, "outport = \"%s\"; ip.ttl--;
>
> next;",
>
>                       igmp_group->mcgroup.name);
> -            ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING,
>
> 500,
>
> +            ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING,
>
> 10500,
>
>                       ds_cstr(match), ds_cstr(actions));
>     }
>
> @@ -10809,7 +10829,7 @@ build_mcast_lookup_flows_for_lrouter(
>      * ports. Otherwise drop any multicast traffic.
>      */
>     if (od->mcast_info.rtr.flood_static) {
> -            ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING,
>
> 450,
>
> +            ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING,
>
> 10450,
>
>                       "ip4.mcast || ip6.mcast",
>                       "clone { "
>                             "outport = \""MC_STATIC"\"; "
> @@ -10817,7 +10837,7 @@ build_mcast_lookup_flows_for_lrouter(
>                             "next; "
>                       "};");
>     } else {
> -            ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING,
>
> 450,
>
> +            ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING,
>
> 10450,
>
>                       "ip4.mcast || ip6.mcast", "drop;");
>     }
> }
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index fb67395e3..4f3a9d5e3 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -2945,12 +2945,12 @@ icmp6 {
>
> <p>
>   If ECMP routes with symmetric reply are configured in the
> -      <code>OVN_Northbound</code> database for a gateway router,
>
> a priority-300
>
> -      flow is added for each router port on which symmetric
>
> replies are
>
> -      configured. The matching logic for these ports essentially
>
> reverses the
>
> -      configured logic of the ECMP route. So for instance, a
>
> route with a
>
> -      destination routing policy will instead match if the
>
> source IP address
>
> -      matches the static route's prefix. The flow uses the action
> +      <code>OVN_Northbound</code> database for a gateway router,
>
> a
>
> +      priority-10300 flow is added for each router port on which
>
> symmetric
>
> +      replies are configured. The matching logic for these ports
>
> essentially
>
> +      reverses the configured logic of the ECMP route. So for
>
> instance, a route
>
> +      with a destination routing policy will instead match if
>
> the source IP
>
>


More information about the dev mailing list