[ovs-dev] [PATCH ovn v2] ovn-nbctl: fails to delete and add lr-nat in trx

[Aidan Shribman]

yes. will add a test

On Wed, Apr 21, 2021, 10:51 Numan Siddique <numans at ovn.org> wrote:

> On Thu, Apr 8, 2021 at 3:13 AM Aidan Shribman <aidan.shribman at gmail.com>
> wrote:
> >
> > The delete operation did not update the internal state cuasing the add
> > operation to wrongly believe that the lr-nat entry is still present and
> > thus can't be added.
> >
> > Below is the sequance is failed before and now passes:
> >
> > ***
> > echo "add switch"
> > ovn-nbctl ls-del ls0 2>/dev/null
> > ovn-nbctl ls-add ls0
> > ovn-nbctl ls-list
> > ovn-nbctl show ls0
> > ovn-nbctl lsp-del lsp0 2>/dev/null
> > ovn-nbctl lsp-add ls0 lsp0 00:00:00:00:00:01 10.0.0.0/24
> > ovn-nbctl lsp-list ls0
> >
> > echo "add router"
> > ovn-nbctl ls-del lr0 2>/dev/null
> > ovn-nbctl lr-add lr0
> > ovn-nbctl lr-list
> > ovn-nbctl show lr0
> >
> > echo "add nat"
> > ovn-nbctl lr-nat-del lr0 2>/dev/null
> > ovn-nbctl lr-nat-add lr0 dnat_and_snat 192.168.0.3 10.0.0.3 \
> >         lsp0 f0:00:00:00:00:03
> > ovn-nbctl lr-nat-list lr0
> >
> > echo "FIXED: delete nat (all of type) + add nat"
> > ovn-nbctl lr-nat-del lr0 dnat_and_snat -- \
> >         --may-exist lr-nat-add lr0 dnat_and_snat 192.168.0.3 10.0.0.3 \
> >         lsp0 f0:00:00:00:00:03
> > ovn-nbctl lr-nat-list lr0
> >
> > echo "FIXED: delete nat (all of external_ip) + add"
> > ovn-nbctl lr-nat-del lr0 dnat_and_snat 192.168.0.3 -- \
> >         --may-exist lr-nat-add lr0 dnat_and_snat 192.168.0.3 10.0.0.3 \
> >         lsp0 f0:00:00:00:00:03
> > ovn-nbctl lr-nat-list lr0
> > ***
> >
> > Signed-off-by: Aidan Shribman <aidan.shribman at gmail.com>
> > Fixed: 1928226 (ovn-nbctl fails to delete/add lr-nat in trx)
>
> Hi Aidan,
>
> I haven't reviewed the code yet. But looking into the commit message,
> I think you can add a test case for this fix ? Can you please add the test
> case
> and submit v3 ?
>
> Thanks
> Numan
>
> > ---
> >  utilities/ovn-nbctl.c | 33 ++++++++++++++++++---------------
> >  1 file changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> > index 184058356..db1ac4f5b 100644
> > --- a/utilities/ovn-nbctl.c
> > +++ b/utilities/ovn-nbctl.c
> > @@ -4531,11 +4531,18 @@ nbctl_lr_nat_del(struct ctl_context *ctx)
> >
> >      if (ctx->argc == 3) {
> >          /*Deletes all NATs with the specified type. */
> > +        struct nbrec_nat **keep_nat = xmalloc(lr->n_nat * sizeof
> *keep_nat);
> > +        size_t n_keep_nat = 0;
> >          for (size_t i = 0; i < lr->n_nat; i++) {
> > -            if (!strcmp(nat_type, lr->nat[i]->type)) {
> > -                nbrec_logical_router_update_nat_delvalue(lr,
> lr->nat[i]);
> > +            if (strcmp(nat_type, lr->nat[i]->type)) {
> > +                keep_nat[n_keep_nat++] = lr->nat[i];
> >              }
> >          }
> > +        if (n_keep_nat < lr->n_nat) {
> > +            nbrec_logical_router_verify_nat(lr);
> > +            nbrec_logical_router_set_nat(lr, keep_nat, n_keep_nat);
> > +        }
> > +        free(keep_nat);
> >          return;
> >      }
> >
> > @@ -4547,31 +4554,27 @@ nbctl_lr_nat_del(struct ctl_context *ctx)
> >
> >      int is_snat = !strcmp("snat", nat_type);
> >      /* Remove the matching NAT. */
> > +    struct nbrec_nat **keep_nat = xmalloc(lr->n_nat * sizeof *keep_nat);
> > +    size_t n_keep_nat = 0;
> >      for (size_t i = 0; i < lr->n_nat; i++) {
> >          struct nbrec_nat *nat = lr->nat[i];
> > -        bool should_return = false;
> >          char *old_ip = normalize_prefix_str(is_snat
> >                                              ? nat->logical_ip
> >                                              : nat->external_ip);
> > -        if (!old_ip) {
> > -            continue;
> > -        }
> > -        if (!strcmp(nat_type, nat->type) && !strcmp(nat_ip, old_ip)) {
> > -            nbrec_logical_router_update_nat_delvalue(lr, nat);
> > -            should_return = true;
> > +        if (!old_ip || strcmp(nat_type, nat->type) || strcmp(nat_ip,
> old_ip)) {
> > +            keep_nat[n_keep_nat++] = lr->nat[i];
> >          }
> >          free(old_ip);
> > -        if (should_return) {
> > -            goto cleanup;
> > -        }
> >      }
> >
> > -    if (must_exist) {
> > +    if (n_keep_nat < lr->n_nat) {
> > +        nbrec_logical_router_verify_nat(lr);
> > +        nbrec_logical_router_set_nat(lr, keep_nat, n_keep_nat);
> > +    } else if (must_exist) {
> >          ctl_error(ctx, "no matching NAT with the type (%s) and %s (%s)",
> >                    nat_type, is_snat ? "logical_ip" : "external_ip",
> nat_ip);
> >      }
> > -
> > -cleanup:
> > +    free(keep_nat);
> >      free(nat_ip);
> >  }
> >
> > --
> > 2.25.1
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>