[ovs-dev] [PATCH ovn v8 2/5] northd: make connected routes have higher priority than static
Numan Siddique
numans at ovn.org
Thu Nov 18 16:50:22 UTC 2021
Hi Vladislav,
Once v9 is ready, please submit them. I'll take a look.
Thanks
Numan
On Thu, Nov 18, 2021 at 2:58 AM Han Zhou <zhouhan at gmail.com> wrote:
>
> 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
> >
> >
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list