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

[Han Zhou]

On Fri, May 15, 2020 at 9:01 AM Lorenzo Bianconi <
lorenzo.bianconi at redhat.com> wrote:
>
> > > 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>
> >
> > [...]
> >
> > > 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?
> >
> > Hi Han,
> >
> > this morning Dumitru and me worked trying to find a solution for this
issue.
> > We though to restore the original configuration in table 9
> > and add a new table used only to take care of FIP/DVR traffic.
> > Does it work for you?
> > I sent a RFC with the basic implementation. I tested it and it works
fine
> > regarding to FIP traffic. I will work on unitest waiting for feedbacks.
> >
> > Regards,
> > Lorenzo
>
> ops I forgot, this is the patch:
>
https://patchwork.ozlabs.org/project/openvswitch/patch/a99d46b275a10a6d8278434f7cefa0466bb78af5.1589557561.git.lorenzo.bianconi@redhat.com/
>
> Regards,
> Lorenzo
>

That's great. Thanks Lorenzo. I will abandon this patch.

> >
> > >
> > > >
> > > > > ---
> > > > >  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
> > > = ",
> > > > >
> > > >
>
>