[ovs-dev,v2] ovn-sbctl: Fix removal of Chassis_Private record on chassis-del.

After commit 4adc10f58127e, the ovn-controller will populate both
Chassis and Chassis_Private tables on registration.

The `ovn-sbctl chassis-del` command exists so that an administrator
can manually remove a chassis record when needed.

Before this patch any existing Chassis_Private record would be left
behind, and this may cause problems for other automation or CMS
software.

Fixes: 4adc10f58127 ("Avoid nb_cfg update notification flooding")
Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
---
 tests/ovn-sbctl.at    | 22 ++++++++++++++++
 utilities/ovn-sbctl.c | 58 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)

On Wed, Dec 14, 2022 at 12:11 PM Frode Nordahl <frode.nordahl@canonical.com>
wrote:
>
> After commit 4adc10f58127e, the ovn-controller will populate both
> Chassis and Chassis_Private tables on registration.
>
> The `ovn-sbctl chassis-del` command exists so that an administrator
> can manually remove a chassis record when needed.
>
> Before this patch any existing Chassis_Private record would be left
> behind, and this may cause problems for other automation or CMS
> software.
>
> Fixes: 4adc10f58127 ("Avoid nb_cfg update notification flooding")
> Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
> ---
>  tests/ovn-sbctl.at    | 22 ++++++++++++++++
>  utilities/ovn-sbctl.c | 58 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 80 insertions(+)
>
> diff --git a/tests/ovn-sbctl.at b/tests/ovn-sbctl.at
> index 1d1eee23e..19ac55c80 100644
> --- a/tests/ovn-sbctl.at
> +++ b/tests/ovn-sbctl.at
> @@ -93,6 +93,28 @@ AT_CHECK([ovn-sbctl -f csv -d bare --no-headings
--columns ip,type list encap |
>  1.2.3.5,vxlan
>  ])
>
> +AT_CHECK([ovn-sbctl chassis-add ch2 geneve 2.3.4.5])
> +ch2_uuid=$(ovn-sbctl -d bare --no-headings --columns _uuid find chassis
name=ch2)
> +ovn-sbctl create Chassis_Private name=ch2 chassis=$ch2_uuid
> +check_row_count Chassis_Private 1
> +
> +AT_CHECK([ovn-sbctl -f csv -d bare --no-headings --columns ip,type list
encap | sort],
> +         [0], [dnl
> +1.2.3.5,geneve
> +1.2.3.5,stt
> +1.2.3.5,vxlan
> +2.3.4.5,geneve
> +])
> +
> +AT_CHECK([ovn-sbctl chassis-del ch2])
> +AT_CHECK([ovn-sbctl -f csv -d bare --no-headings --columns ip,type list
encap | sort],
> +         [0], [dnl
> +1.2.3.5,geneve
> +1.2.3.5,stt
> +1.2.3.5,vxlan
> +])
> +check_row_count Chassis_Private 0
> +
>  as ovn-sb
>  OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>  as
> diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
> index 4d7e7d670..37760805d 100644
> --- a/utilities/ovn-sbctl.c
> +++ b/utilities/ovn-sbctl.c
> @@ -164,6 +164,8 @@ struct sbctl_context {
>      bool cache_valid;
>      /* Maps from chassis name to struct sbctl_chassis. */
>      struct shash chassis;
> +    /* Maps from chassis name to struct sbctl_chassis_private. */
> +    struct shash chassis_private;
>      /* Maps from lport name to struct sbctl_port_binding. */
>      struct shash port_bindings;
>  };
> @@ -179,6 +181,10 @@ struct sbctl_chassis {
>      const struct sbrec_chassis *ch_cfg;
>  };
>
> +struct sbctl_chassis_private {
> +    const struct sbrec_chassis_private *ch_priv;
> +};
> +
>  struct sbctl_port_binding {
>      const struct sbrec_port_binding *bd_cfg;
>  };
> @@ -193,6 +199,7 @@ sbctl_context_invalidate_cache(struct ctl_context
*ctx)
>      }
>      sbctl_ctx->cache_valid = false;
>      shash_destroy_free_data(&sbctl_ctx->chassis);
> +    shash_destroy_free_data(&sbctl_ctx->chassis_private);
>      shash_destroy_free_data(&sbctl_ctx->port_bindings);
>  }
>
> @@ -207,11 +214,13 @@ sbctl_context_get(struct ctl_context *ctx)
>      }
>
>      const struct sbrec_chassis *chassis_rec;
> +    const struct sbrec_chassis_private *chassis_private_rec;
>      const struct sbrec_port_binding *port_binding_rec;
>      struct sset chassis, port_bindings;
>
>      sbctl_ctx->cache_valid = true;
>      shash_init(&sbctl_ctx->chassis);
> +    shash_init(&sbctl_ctx->chassis_private);
>      shash_init(&sbctl_ctx->port_bindings);
>      sset_init(&chassis);
>      SBREC_CHASSIS_FOR_EACH(chassis_rec, ctx->idl) {
> @@ -229,6 +238,25 @@ sbctl_context_get(struct ctl_context *ctx)
>      }
>      sset_destroy(&chassis);
>
> +    sset_init(&chassis);
> +    SBREC_CHASSIS_PRIVATE_FOR_EACH (chassis_private_rec, ctx->idl) {
> +        struct sbctl_chassis_private *ch_priv;
> +
> +        if (!sset_add(&chassis, chassis_private_rec->name)) {
> +            VLOG_WARN("database contains duplicate private record for "
> +                      "chassis named (%s)",
> +                      chassis_rec->name);
> +            continue;
> +        }
> +
> +        ch_priv = xmalloc(sizeof *ch_priv);
> +        ch_priv->ch_priv = chassis_private_rec;
> +        shash_add(&sbctl_ctx->chassis_private,
> +                  chassis_private_rec->name,
> +                  ch_priv);
> +    }
> +    sset_destroy(&chassis);
> +
>      sset_init(&port_bindings);
>      SBREC_PORT_BINDING_FOR_EACH(port_binding_rec, ctx->idl) {
>          struct sbctl_port_binding *bd;
> @@ -280,6 +308,22 @@ find_chassis(struct ctl_context *ctx, const char
*name, bool must_exist)
>      return sbctl_ch;
>  }
>
> +static struct sbctl_chassis_private *
> +find_chassis_private(struct ctl_context *ctx,
> +                     const char *name,
> +                     bool must_exist)
> +{
> +    struct sbctl_context *sbctl_ctx = sbctl_context_get(ctx);
> +    struct sbctl_chassis_private *sbctl_ch_priv = shash_find_data(
> +        &sbctl_ctx->chassis_private, name);
> +
> +    if (must_exist && !sbctl_ch_priv) {
> +        ctl_error(ctx, "no private record for chassis named %s", name);
> +    }
> +
> +    return sbctl_ch_priv;
> +}
> +
>  static struct sbctl_port_binding *
>  find_port_binding(struct ctl_context *ctx, const char *name, bool
must_exist)
>  {
> @@ -299,6 +343,8 @@ pre_get_info(struct ctl_context *ctx)
>      ovsdb_idl_add_column(ctx->idl, &sbrec_chassis_col_name);
>      ovsdb_idl_add_column(ctx->idl, &sbrec_chassis_col_encaps);
>
> +    ovsdb_idl_add_column(ctx->idl, &sbrec_chassis_private_col_name);
> +
>      ovsdb_idl_add_column(ctx->idl, &sbrec_encap_col_type);
>      ovsdb_idl_add_column(ctx->idl, &sbrec_encap_col_ip);
>
> @@ -432,6 +478,18 @@ cmd_chassis_del(struct ctl_context *ctx)
>              for (i = 0; i < sbctl_ch->ch_cfg->n_encaps; i++) {
>                  sbrec_encap_delete(sbctl_ch->ch_cfg->encaps[i]);
>              }
> +
> +            struct sbctl_chassis_private *sbctl_ch_priv;
> +            sbctl_ch_priv = find_chassis_private(ctx, ctx->argv[1],
false);
> +            if (sbctl_ch_priv) {
> +                if (sbctl_ch_priv->ch_priv) {
> +                    sbrec_chassis_private_delete(sbctl_ch_priv->ch_priv);
> +                }
> +                shash_find_and_delete(&sbctl_ctx->chassis_private,
> +                                      ctx->argv[1]);
> +                free(sbctl_ch_priv);
> +            }
> +
>              sbrec_chassis_delete(sbctl_ch->ch_cfg);
>          }
>          shash_find_and_delete(&sbctl_ctx->chassis, ctx->argv[1]);
> --
> 2.37.2
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Thanks Frode. This makes sense. In the beginning I wondered that
chassis-add and chassis-del's behavior should match, but then I realized
that it doesn't make much sense for chassis-add to add the private record
that is only for the nb_cfg purpose, which is only useful if there is a
real chassis, which would register and create the private record by itself.
So I think it is good to keep chassis-add untouched (otherwise it would in
fact break some of my simulation tests), unless there are new requirements
in the future.
On the other hand, the chassis-del command is useful not just for testing
purposes but also for operators, so it should take care of deleting the
private record, if it exists.
This patch behaves exactly as expected. So, applied to the main branch.

Regards,
Han