On Thu, May 14, 2020 at 1:12 AM Dumitru Ceara <dce...@redhat.com> wrote: > > On 5/13/20 11:51 PM, Han Zhou wrote: > > In commit c0bf32d the priorities of the regular routes were increased by > > 400, but ECMP routes were not updated. This patch fixes it. Since > > for ECMP routes the outport is unknown (there can be many different > > outports) the condition check of whether the outport is distributed > > gateway port is skipped. > > > > Fixes: c0bf32d ("Manage ARP process locally in a DVR scenario") > > Cc: Lorenzo Bianconi <lorenzo.bianc...@redhat.com> > > Signed-off-by: Han Zhou <hz...@ovn.org> > > Hi Han, > > Should we also add a test for this scenario to avoid further regressions? > > Thanks, > Dumitru
Thanks Dumitru for the review. I thought it twice and it seems this fix doesn't solve the whole problem. In commit c0bf32d ("Manage ARP process locally in a DVR scenario"), it introduced different sets of route priorities. If the route's output port is distributed gateway port, it is 400 lower than the routes with same prefix length but next hop is a regular router port. The longest prefix match would not work properly for routing. This violates the basic principle for the default routing behavior. For example: route1: 10.0.0.0/24 nexthop 192.168.0.2 via lrp1 (distributed gateway port) route2: 10.0.0.0/16 nexthop 192.168.100.2 via lrp2 (regular router port) The user would expected route1 to override route2, because it is has longer prefix match. However, the commit c0bf32d results in 10.0.0.0/16 taking effect, because the priority now is (400 + 33) v.s. 49. table=9 (lr_in_ip_routing ), priority=49 , match=(ip4.dst == 10.0.0.0/24), action=(ip.ttl--; reg8[0..15] = 0; reg0 = 192.168.0.2; reg1 = 192.168.0.1; eth.src = aa:aa:aa:aa:aa:01; outport = "lrp1"; flags.loopback = 1; next;) table=9 (lr_in_ip_routing ), priority=433 , match=(ip4.dst == 10.0.0.0/16), action=(ip.ttl--; reg8[0..15] = 0; reg0 = 192.168.100.2; reg1 = 192.168.100.1; eth.src = aa:aa:aa:aa:aa:02; outport = "lrp2"; flags.loopback = 1; next;) So I wonder if we should change the solution of the commit c0bf32d, instead of just increasing the ECMP routes priority only. I don't completely understand the problem that commit was trying to solve. Is there an example that describes the scenario in more detail and the reason why the route priorities have to be different? > > > --- > > northd/ovn-northd.8.xml | 3 ++- > > northd/ovn-northd.c | 22 ++++++++++++---------- > > 2 files changed, 14 insertions(+), 11 deletions(-) > > > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > > index 8f224b0..b0475d0 100644 > > --- a/northd/ovn-northd.8.xml > > +++ b/northd/ovn-northd.8.xml > > @@ -2601,7 +2601,8 @@ next; > > ids <var>MID1</var>, <var>MID2</var>, ..., a logical flow with match > > in CIDR notation <code>ip4.dst == <var>N</var>/<var>M</var></code>, > > or <code>ip6.dst == <var>N</var>/<var>M</var></code>, whose priority > > - is the integer value of <var>M</var>, has the following actions: > > + is <code>400</code> + the integer value of <var>M</var>, has the > > + following actions: > > </p> > > > > <pre> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index b25152d..c02e305 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -7337,7 +7337,8 @@ build_route_prefix_s(const struct v46_ip *prefix, unsigned int plen) > > } > > > > static void > > -build_route_match(const struct ovn_port *op_inport, const char *network_s, > > +build_route_match(const struct ovn_port *op_inport, > > + const struct ovn_port *op_outport, const char *network_s, > > int plen, bool is_src_route, bool is_ipv4, struct ds *match, > > uint16_t *priority) > > { > > @@ -7351,6 +7352,13 @@ build_route_match(const struct ovn_port *op_inport, const char *network_s, > > dir = "dst"; > > *priority = (plen * 2) + 1; > > } > > + /* traffic for internal IPs of logical switch ports must be sent to > > + * the gw controller through the overlay tunnels > > + */ > > + if (!op_outport || > > + (op_outport->nbrp && !op_outport->nbrp->n_gateway_chassis)) { > > + priority += DROUTE_PRIO; > > + } > > > > if (op_inport) { > > ds_put_format(match, "inport == %s && ", op_inport->json_key); > > @@ -7434,8 +7442,8 @@ build_ecmp_route_flow(struct hmap *lflows, struct ovn_datapath *od, > > uint16_t priority; > > > > char *prefix_s = build_route_prefix_s(&eg->prefix, eg->plen); > > - build_route_match(NULL, prefix_s, eg->plen, eg->is_src_route, is_ipv4, > > - &match, &priority); > > + build_route_match(NULL, NULL, prefix_s, eg->plen, eg->is_src_route, > > + is_ipv4, &match, &priority); > > free(prefix_s); > > > > struct ds actions = DS_EMPTY_INITIALIZER; > > @@ -7548,14 +7556,8 @@ add_route(struct hmap *lflows, const struct ovn_port *op, > > op_inport = op; > > } > > } > > - build_route_match(op_inport, network_s, plen, is_src_route, is_ipv4, > > + build_route_match(op_inport, op, network_s, plen, is_src_route, is_ipv4, > > &match, &priority); > > - /* traffic for internal IPs of logical switch ports must be sent to > > - * the gw controller through the overlay tunnels > > - */ > > - if (op->nbrp && !op->nbrp->n_gateway_chassis) { > > - priority += DROUTE_PRIO; > > - } > > > > struct ds actions = DS_EMPTY_INITIALIZER; > > ds_put_format(&actions, "ip.ttl--; "REG_ECMP_GROUP_ID" = 0; %sreg0 = ", > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev