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 <[email protected]> wrote: > > Sounds good to me. > > On Wed, Nov 17, 2021 at 11:53 PM Vladislav Odintsov <[email protected]> > wrote: > > > Thanks, Han. > > Please see inline. > > > > Regards, > > Vladislav Odintsov > > > > On 18 Nov 2021, at 10:26, Han Zhou <[email protected]> wrote: > > > > On Wed, Nov 17, 2021 at 10:10 PM Vladislav Odintsov <[email protected]> > > 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 <[email protected]> wrote: > > > > On Wed, Nov 17, 2021 at 3:38 PM Vladislav Odintsov <[email protected] > > > > <mailto:[email protected] <[email protected]>>> 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/[email protected]/ > > < > > > > https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ > > > > > > > > Regards, > > Vladislav Odintsov > > > > On 17 Nov 2021, at 20:57, Numan Siddique <[email protected] <mailto: > > > > [email protected]>> wrote: > > > > > > On Wed, Nov 17, 2021 at 9:24 AM Vladislav Odintsov <[email protected] > > > > <mailto:[email protected] <[email protected]>> <mailto:[email protected] > > <[email protected]> <mailto: > > [email protected]>>> 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 <[email protected]> > > > > 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 <[email protected]> wrote: > > > > On Sat, Nov 13, 2021 at 4:44 AM Vladislav Odintsov < > > > > [email protected] <mailto:[email protected] <[email protected]>>> 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 <[email protected]> > > > > > > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
