On Thu, Apr 8, 2021 at 3:13 AM Aidan Shribman <[email protected]> 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 <[email protected]>
> 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
