[ovs-dev] [PATCH ovn] utilities: nbctl: add --may-exist parameter to meter-add cmd

Numan Siddique numans at ovn.org
Tue Nov 2 21:59:56 UTC 2021


On Wed, Oct 27, 2021 at 10:54 AM Lorenzo Bianconi
<lorenzo.bianconi at redhat.com> wrote:
>
> Add --may-exist support to meter-add command in order to update
> meter parameters if it has been already created
>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> ---
>  tests/ovn-nbctl.at    |  6 +++--
>  tests/ovn-northd.at   | 11 +++++++++-
>  utilities/ovn-nbctl.c | 51 ++++++++++++++++++++++++-------------------
>  3 files changed, 43 insertions(+), 25 deletions(-)
>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 9b80ae410..a8946fef8 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -371,6 +371,8 @@ dnl Add duplicate meter name
>  AT_CHECK([ovn-nbctl meter-add meter1 drop 10 kbps], [1], [], [stderr])
>  AT_CHECK([grep 'already exists' stderr], [0], [ignore])
>
> +AT_CHECK([ovn-nbctl --may-exist meter-add meter1 drop 11 kbps])
> +
>  dnl Add reserved meter name
>  AT_CHECK([ovn-nbctl meter-add __meter1 drop 10 kbps], [1], [], [stderr])
>  AT_CHECK([grep 'reserved' stderr], [0], [ignore])
> @@ -396,7 +398,7 @@ AT_CHECK([ovn-nbctl meter-add meter5 drop 10 100010111111 kbps], [1], [],
>
>  AT_CHECK([ovn-nbctl meter-list], [0], [dnl
>  meter1: bands:
> -  drop: 10 kbps
> +  drop: 11 kbps
>  meter2: bands:
>    drop: 3 kbps, 2 kb burst
>  meter3: bands:
> @@ -409,7 +411,7 @@ dnl Delete a single meter.
>  AT_CHECK([ovn-nbctl meter-del meter2])
>  AT_CHECK([ovn-nbctl meter-list], [0], [dnl
>  meter1: bands:
> -  drop: 10 kbps
> +  drop: 11 kbps
>  meter3: bands:
>    drop: 100 kbps, 200 kb burst
>  meter4: (fair) bands:
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index e2b9924b6..ca1b8a117 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -3235,7 +3235,16 @@ event-elb: meter0
>
>  AT_CHECK([ovn-sbctl list logical_flow | grep trigger_event -A 2 | grep -q meter0])
>
> -check ovn-nbctl --wait=hv meter-add meter1 drop 200 pktps 10
> +check ovn-nbctl --wait=hv meter-add meter1 drop 300 pktps 10
> +AT_CHECK([ovn-nbctl meter-list |grep meter1 -A 1], [0], [dnl
> +meter1: bands:
> +  drop: 300 pktps, 10 packet burst
> +])
> +check ovn-nbctl --wait=hv --may-exist meter-add meter1 drop 200 pktps 10
> +AT_CHECK([ovn-nbctl meter-list |grep meter1 -A 1], [0], [dnl
> +meter1: bands:
> +  drop: 200 pktps, 10 packet burst
> +])
>  check ovn-nbctl --wait=hv lr-copp-add r0 arp meter1
>  AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
>  arp: meter1
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index b04b24d4b..1f71cae46 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -2585,19 +2585,6 @@ meter_cmp(const void *meter1_, const void *meter2_)
>      return strcmp(meter1->name, meter2->name);
>  }
>
> -static void
> -nbctl_pre_meter_list(struct ctl_context *ctx)
> -{
> -    ovsdb_idl_add_column(ctx->idl, &nbrec_meter_col_name);
> -    ovsdb_idl_add_column(ctx->idl, &nbrec_meter_col_fair);
> -    ovsdb_idl_add_column(ctx->idl, &nbrec_meter_col_bands);
> -    ovsdb_idl_add_column(ctx->idl, &nbrec_meter_col_unit);
> -
> -    ovsdb_idl_add_column(ctx->idl, &nbrec_meter_band_col_action);
> -    ovsdb_idl_add_column(ctx->idl, &nbrec_meter_band_col_rate);
> -    ovsdb_idl_add_column(ctx->idl, &nbrec_meter_band_col_burst_size);
> -}
> -

Hi Lorenzo,

Thanks for the patch.  I applied this patch to the main branch with one change.

I kept the function nbctl_pre_meter_list() AS IS even though
nbctl_pre_meter_add()
are exactly the same.   I felt it's a bit odd to call
nbctl_pre_meter_add() in the meter-list pre check command.

Thanks
Numan



>  static void
>  nbctl_meter_list(struct ctl_context *ctx)
>  {
> @@ -2650,18 +2637,34 @@ static void
>  nbctl_pre_meter_add(struct ctl_context *ctx)
>  {
>      ovsdb_idl_add_column(ctx->idl, &nbrec_meter_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_meter_col_fair);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_meter_col_bands);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_meter_col_unit);
> +
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_meter_band_col_action);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_meter_band_col_rate);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_meter_band_col_burst_size);
>  }
>
>  static void
>  nbctl_meter_add(struct ctl_context *ctx)
>  {
> -    const struct nbrec_meter *meter;
> +    const struct nbrec_meter *meter = NULL, *iter;
> +    struct nbrec_meter_band *band = NULL;
>
>      const char *name = ctx->argv[1];
> -    NBREC_METER_FOR_EACH (meter, ctx->idl) {
> -        if (!strcmp(meter->name, name)) {
> -            ctl_error(ctx, "meter with name \"%s\" already exists", name);
> -            return;
> +    NBREC_METER_FOR_EACH (iter, ctx->idl) {
> +        if (!strcmp(iter->name, name)) {
> +            if (!shash_find(&ctx->options, "--may-exist")) {
> +                ctl_error(ctx, "meter with name \"%s\" already exists", name);
> +                return;
> +            } else {
> +                meter = iter;
> +                if (meter->n_bands) {
> +                    band = meter->bands[0];
> +                }
> +                break;
> +            }
>          }
>      }
>
> @@ -2699,13 +2702,17 @@ nbctl_meter_add(struct ctl_context *ctx)
>      }
>
>      /* Create the band.  We only support adding a single band. */
> -    struct nbrec_meter_band *band = nbrec_meter_band_insert(ctx->txn);
> +    if (!band) {
> +        band = nbrec_meter_band_insert(ctx->txn);
> +    }
>      nbrec_meter_band_set_action(band, action);
>      nbrec_meter_band_set_rate(band, rate);
>      nbrec_meter_band_set_burst_size(band, burst);
>
>      /* Create the meter. */
> -    meter = nbrec_meter_insert(ctx->txn);
> +    if (!meter) {
> +        meter = nbrec_meter_insert(ctx->txn);
> +    }
>      nbrec_meter_set_name(meter, name);
>      nbrec_meter_set_unit(meter, unit);
>      nbrec_meter_set_bands(meter, &band, 1);
> @@ -6853,10 +6860,10 @@ static const struct ctl_command_syntax nbctl_commands[] = {
>
>      /* meter commands. */
>      { "meter-add", 4, 5, "NAME ACTION RATE UNIT [BURST]", nbctl_pre_meter_add,
> -      nbctl_meter_add, NULL, "--fair", RW },
> +      nbctl_meter_add, NULL, "--fair,--may-exist", RW },
>      { "meter-del", 0, 1, "[NAME]", nbctl_pre_meter_del, nbctl_meter_del,
>        NULL, "", RW },
> -    { "meter-list", 0, 0, "", nbctl_pre_meter_list, nbctl_meter_list,
> +    { "meter-list", 0, 0, "", nbctl_pre_meter_add, nbctl_meter_list,
>        NULL, "", RO },
>
>      /* logical switch port commands. */
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list