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/[email protected]/ Regards, Vladislav Odintsov > On 18 Nov 2021, at 19:50, Numan Siddique <[email protected]> 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 <[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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
