Done: https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/
Regards, Vladislav Odintsov > On 19 Nov 2021, at 17:55, Numan Siddique <[email protected]> wrote: > > On Fri, Nov 19, 2021 at 8:12 AM Vladislav Odintsov <[email protected] > <mailto:[email protected]>> 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/[email protected]/ >> >> <https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/> > > 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 <[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 > _______________________________________________ > dev mailing list > [email protected] <mailto:[email protected]> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
