>
> On Thu, May 14, 2020 at 1:12 AM Dumitru Ceara <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Han Zhou <[email protected]>
> >
> > 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?
basically the commit c0bf32d ("Manage ARP process locally in a DVR
scenario") wants to manage DVR traffic locally (e.g not send ARP
traffic through the tunnel). In particular this change is used to
distribute non-DVR traffic using geneve tunnels IIRC. I will look into
it.
Regards,
Lorenzo
>
> >
> > > ---
> > > 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
