| Message ID | 20210408071141.4030257-1-aidan.shribman@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] ovn-nbctl: fails to delete and add lr-nat in trx | expand |
On Thu, Apr 8, 2021 at 3:13 AM Aidan Shribman <aidan.shribman@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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
yes. will add a test On Wed, Apr 21, 2021, 10:51 Numan Siddique <numans@ovn.org> wrote: > On Thu, Apr 8, 2021 at 3:13 AM Aidan Shribman <aidan.shribman@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@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@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
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); }
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@gmail.com> Fixed: 1928226 (ovn-nbctl fails to delete/add lr-nat in trx) --- utilities/ovn-nbctl.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-)