On Fri, Nov 19, 2021 at 8:12 AM Vladislav Odintsov <odiv...@gmail.com> wrote: > > Hi Numan, > > yes, it’s ready. But I’ve based it on the commit from this patch [1]. > Can you please take a look on it and apply if it’s okay. > Then I can send patch series without conflicting changes. > > Or I can send it with that patch as a part of patchset. > > 1: > https://patchwork.ozlabs.org/project/ovn/patch/20211117203545.46142-1-odiv...@gmail.com/
I'd suggest if you can include [1] as part of the patchset. Numan > > Regards, > Vladislav Odintsov > > > On 18 Nov 2021, at 19:50, Numan Siddique <num...@ovn.org> wrote: > > > > 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 <zhou...@gmail.com> wrote: > >> > >> Sounds good to me. > >> > >> On Wed, Nov 17, 2021 at 11:53 PM Vladislav Odintsov <odiv...@gmail.com> > >> wrote: > >> > >>> Thanks, Han. > >>> Please see inline. > >>> > >>> Regards, > >>> Vladislav Odintsov > >>> > >>> On 18 Nov 2021, at 10:26, Han Zhou <zhou...@gmail.com> wrote: > >>> > >>> On Wed, Nov 17, 2021 at 10:10 PM Vladislav Odintsov <odiv...@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 <num...@ovn.org> wrote: > >>> > >>> On Wed, Nov 17, 2021 at 3:38 PM Vladislav Odintsov <odiv...@gmail.com > >>> > >>> <mailto:odiv...@gmail.com <odiv...@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-odiv...@gmail.com/ > >>> < > >>> > >>> https://patchwork.ozlabs.org/project/ovn/patch/20211117203545.46142-1-odiv...@gmail.com/ > >>> > >>> > >>> > >>> Regards, > >>> Vladislav Odintsov > >>> > >>> On 17 Nov 2021, at 20:57, Numan Siddique <num...@ovn.org <mailto: > >>> > >>> num...@ovn.org>> wrote: > >>> > >>> > >>> On Wed, Nov 17, 2021 at 9:24 AM Vladislav Odintsov <odiv...@gmail.com > >>> > >>> <mailto:odiv...@gmail.com <odiv...@gmail.com>> <mailto:odiv...@gmail.com > >>> <odiv...@gmail.com> <mailto: > >>> odiv...@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 <odiv...@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 <num...@ovn.org> wrote: > >>> > >>> On Sat, Nov 13, 2021 at 4:44 AM Vladislav Odintsov < > >>> > >>> odiv...@gmail.com <mailto:odiv...@gmail.com <odiv...@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 <odiv...@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 > >> d...@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > > dev mailing list > > d...@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
