[ovs-dev] [PATCH ovn] ovn-northd: Increase ECMP route priority with 400.

[Han Zhou]

On Thu, May 14, 2020 at 1:12 AM Dumitru Ceara <dceara at 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.bianconi at redhat.com>
> > Signed-off-by: Han Zhou <hzhou at 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
= ",
> >
>