[ovs-dev,ACL,Meters,4/7] ovn: Add Meter and Meter_Band tables to the NB and SB databases.

Add support for configuring meters through the Meter and Meter_Band
tables in the Northbound database.  This commit also has ovn-northd
sync those tables between the Northbound and Southbound databases.

Add support for configuring meters with ovn-nbctl.

Signed-off-by: Justin Pettit <jpettit@ovn.org>
---
 ovn/northd/ovn-northd.c       | 145 ++++++++++++++++++++++++++++++++++
 ovn/ovn-nb.ovsschema          |  33 +++++++-
 ovn/ovn-nb.xml                |  80 +++++++++++++++++++
 ovn/ovn-sb.ovsschema          |  27 ++++++-
 ovn/ovn-sb.xml                |  72 +++++++++++++++++
 ovn/utilities/ovn-nbctl.8.xml |  50 ++++++++++++
 ovn/utilities/ovn-nbctl.c     | 143 +++++++++++++++++++++++++++++++++
 tests/ovn-nbctl.at            |  58 ++++++++++++++
 8 files changed, 604 insertions(+), 4 deletions(-)

Bleep bloop.  Greetings Justin Pettit, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 80 characters long (recommended limit is 79)
#217 FILE: ovn/ovn-nb.ovsschema:202:
                                          "enum": ["set", ["kbps", "pktps"]]}}},

WARNING: Line is 80 characters long (recommended limit is 79)
#358 FILE: ovn/ovn-sb.ovsschema:105:
                                          "enum": ["set", ["kbps", "pktps"]]}}},

WARNING: Line is 129 characters long (recommended limit is 79)
#473 FILE: ovn/utilities/ovn-nbctl.8.xml:177:
        <dt><code>meter-add</code> <var>name</var> <var>unit</var> <var>action</var> <var>rate</var> [<var>burst_size</var>]</dt>

WARNING: Line lacks whitespace around operator
#533 FILE: ovn/utilities/ovn-nbctl.c:500:
  meter-add NAME UNIT ACTION RATE [BURST_SIZE]\n\

WARNING: Line lacks whitespace around operator
#535 FILE: ovn/utilities/ovn-nbctl.c:502:
  meter-del [NAME]          remove meters\n\

WARNING: Line lacks whitespace around operator
#536 FILE: ovn/utilities/ovn-nbctl.c:503:
  meter-list                print meters\n\

Lines checked: 764, Warnings: 6, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot

On Sun, Jul 29, 2018 at 11:46:35PM -0700, Justin Pettit wrote:
> Add support for configuring meters through the Meter and Meter_Band
> tables in the Northbound database.  This commit also has ovn-northd
> sync those tables between the Northbound and Southbound databases.
> 
> Add support for configuring meters with ovn-nbctl.
> 
> Signed-off-by: Justin Pettit <jpettit@ovn.org>

Thanks for the patches!

band_cmp() uses the form "a > b ? -1 : 1" a couple of times.  I believe
that this will sort 'a' and 'b' into descending order.  Is that what you
want?

(Oh, actually I see it doesn't matter, it just needs to be consistent.)

In ovn-[ns]b.xml, for consistency with the rest of the tables,
title="..." in <table> start tags should not include the word "table"
and maybe should give better descriptions.

The Meter_Band table says the following.  I'm not sure what a "relative
rate limit" is.  It kind of sounds like it would be an additive one,
where if you have a first band with a rate of 500 Mbps then the second
one with a rate of 250 Mbps would effectively be 500 + 250 = 750 Mbps.
But I don't think you really mean that; I think it's actually an
absolute rate.  Maybe by relative you just mean that the unit is
specified by the Meter.

    <column name="rate">
      <p>
        The relative rate limit for this band, in kilobits per second or
        bits per second, depending on whether the parent <ref table="Meter"/>
        entry's <ref column="unit" table="Meter"/> column specified
        <code>kbps</code> or <code>pktps</code>.
      </p>
    </column>

The documentation for "meter-add" uses two different names burst_size
and burst_rate for the same argument.  (Maybe just "burst" would be a
better name.)

The documentation writes "__" for two underscores, but it's a little
hard to tell in most fonts whether there are one or two underscores, so
you might add (two underscores) to make it clear.

The command
        meter-add name unit action rate [burst]
might be a little more natural if it were more like
        meter-add name action rate [burst]
where "rate" was to be supplied as, e.g. 1000kbps or 50000pktps.

Possibly the "list" output could be a little more human friendly in the
same way, e.g.:

    meter1:
      drop: 10 kbps
    meter3:
      drop: 100 kbps, 200 kb burst

but it's not a big deal either way.

If the burst size is truly optional, it might be nice to make it
optional in the schema, so that it could be omitted instead of set to
0.  The schema documentation for Meter_Band doesn't say that it's
optional; it probably should explain what it means to set it to 0.

Acked-by: Ben Pfaff <blp@ovn.org>

Hi Justin,

I took a look through the patch series, and this is the only one that I 
had some immediate feedback on.

First, it would be nice if we could refer to Meters by name when issuing 
DB commands. For instance `ovn-nbctl add Meter <meter_name> bands <band 
UUID>`.

Second, I noticed that the algorithm for computing southbound meter 
bands can result in a larger number of bands than is in the northbound 
table. For instance, you could issue the following:

ovn-nbctl --id=@id create Meter_Band action=drop rate=1000 -- \
create Meter name=foo unit=kbps band=@id -- \
create Meter name=bar unit=kbps band=@id

In the northbound database, you'll have one entry in the meter_band 
table. In the southbound database, you'll have two entries in the 
meter_band table. The algorithm does not take into account that multiple 
northbound meters may refer to the same meter_band. I'm not sure how big 
an issue this is (or really if it is an issue), but it surprised me a 
bit when I was playing around with it.


On 07/30/2018 02:46 AM, Justin Pettit wrote:
> Add support for configuring meters through the Meter and Meter_Band
> tables in the Northbound database.  This commit also has ovn-northd
> sync those tables between the Northbound and Southbound databases.
> 
> Add support for configuring meters with ovn-nbctl.
> 
> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> ---
>   ovn/northd/ovn-northd.c       | 145 ++++++++++++++++++++++++++++++++++
>   ovn/ovn-nb.ovsschema          |  33 +++++++-
>   ovn/ovn-nb.xml                |  80 +++++++++++++++++++
>   ovn/ovn-sb.ovsschema          |  27 ++++++-
>   ovn/ovn-sb.xml                |  72 +++++++++++++++++
>   ovn/utilities/ovn-nbctl.8.xml |  50 ++++++++++++
>   ovn/utilities/ovn-nbctl.c     | 143 +++++++++++++++++++++++++++++++++
>   tests/ovn-nbctl.at            |  58 ++++++++++++++
>   8 files changed, 604 insertions(+), 4 deletions(-)
> 
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 04a072ba8de7..45557170edc8 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -6606,6 +6606,140 @@ sync_port_groups(struct northd_context *ctx)
>       shash_destroy(&sb_port_groups);
>   }
>   
> +struct band_entry {
> +    int64_t rate;
> +    int64_t burst_size;
> +    const char *action;
> +};
> +
> +static int
> +band_cmp(const void *band1_, const void *band2_)
> +{
> +    const struct band_entry *band1p = band1_;
> +    const struct band_entry *band2p = band2_;
> +
> +    if (band1p->rate != band2p->rate) {
> +        return band1p->rate > band2p->rate ? -1 : 1;
> +    } else if (band1p->burst_size != band2p->burst_size) {
> +        return band1p->burst_size > band2p->burst_size ? -1 : 1;
> +    } else {
> +        return strcmp(band1p->action, band2p->action);
> +    }
> +}
> +
> +static bool
> +bands_need_update(const struct nbrec_meter *nb_meter,
> +                  const struct sbrec_meter *sb_meter)
> +{
> +    if (nb_meter->n_bands != sb_meter->n_bands) {
> +        return true;
> +    }
> +
> +    /* A single band is the most common scenario, so speed up that
> +     * check. */
> +    if (nb_meter->n_bands == 1) {
> +        struct nbrec_meter_band *nb_band = nb_meter->bands[0];
> +        struct sbrec_meter_band *sb_band = sb_meter->bands[0];
> +
> +        return !(nb_band->rate == sb_band->rate
> +                 && nb_band->burst_size == sb_band->burst_size
> +                 && !strcmp(sb_band->action, nb_band->action));
> +    }
> +
> +    /* Place the Northbound entries in sorted order. */
> +    struct band_entry *nb_bands;
> +    nb_bands = xmalloc(sizeof *nb_bands * nb_meter->n_bands);
> +    for (size_t i = 0; i < nb_meter->n_bands; i++) {
> +        struct nbrec_meter_band *nb_band = nb_meter->bands[i];
> +
> +        nb_bands[i].rate = nb_band->rate;
> +        nb_bands[i].burst_size = nb_band->burst_size;
> +        nb_bands[i].action = nb_band->action;
> +    }
> +    qsort(nb_bands, nb_meter->n_bands, sizeof *nb_bands, band_cmp);
> +
> +    /* Place the Southbound entries in sorted order. */
> +    struct band_entry *sb_bands;
> +    sb_bands = xmalloc(sizeof *sb_bands * sb_meter->n_bands);
> +    for (size_t i = 0; i < sb_meter->n_bands; i++) {
> +        struct sbrec_meter_band *sb_band = sb_meter->bands[i];
> +
> +        sb_bands[i].rate = sb_band->rate;
> +        sb_bands[i].burst_size = sb_band->burst_size;
> +        sb_bands[i].action = sb_band->action;
> +    }
> +    qsort(sb_bands, sb_meter->n_bands, sizeof *sb_bands, band_cmp);
> +
> +    bool need_update = false;
> +    for (size_t i = 0; i < nb_meter->n_bands; i++) {
> +        if (nb_bands[i].rate != sb_bands[i].rate
> +            || nb_bands[i].burst_size != sb_bands[i].burst_size
> +            || strcmp(nb_bands[i].action, nb_bands[i].action)) {
> +            need_update = true;
> +            goto done;
> +        }
> +    }
> +
> +done:
> +    free(nb_bands);
> +    free(sb_bands);
> +
> +    return need_update;
> +}
> +
> +/* Each entry in the Meter and Meter_Band tables in OVN_Northbound have
> + * a corresponding entries in the Meter and Meter_Band tables in
> + * OVN_Southbound.
> + */
> +static void
> +sync_meters(struct northd_context *ctx)
> +{
> +    struct shash sb_meters = SHASH_INITIALIZER(&sb_meters);
> +
> +    const struct sbrec_meter *sb_meter;
> +    SBREC_METER_FOR_EACH (sb_meter, ctx->ovnsb_idl) {
> +        shash_add(&sb_meters, sb_meter->name, sb_meter);
> +    }
> +
> +    const struct nbrec_meter *nb_meter;
> +    NBREC_METER_FOR_EACH (nb_meter, ctx->ovnnb_idl) {
> +        bool new_sb_meter = false;
> +
> +        sb_meter = shash_find_and_delete(&sb_meters, nb_meter->name);
> +        if (!sb_meter) {
> +            sb_meter = sbrec_meter_insert(ctx->ovnsb_txn);
> +            sbrec_meter_set_name(sb_meter, nb_meter->name);
> +            new_sb_meter = true;
> +        }
> +
> +        if (new_sb_meter || bands_need_update(nb_meter, sb_meter)) {
> +            struct sbrec_meter_band **sb_bands;
> +            sb_bands = xcalloc(nb_meter->n_bands, sizeof *sb_bands);
> +            for (size_t i = 0; i < nb_meter->n_bands; i++) {
> +                const struct nbrec_meter_band *nb_band = nb_meter->bands[i];
> +
> +                sb_bands[i] = sbrec_meter_band_insert(ctx->ovnsb_txn);
> +
> +                sbrec_meter_band_set_action(sb_bands[i], nb_band->action);
> +                sbrec_meter_band_set_rate(sb_bands[i], nb_band->rate);
> +                sbrec_meter_band_set_burst_size(sb_bands[i],
> +                                                nb_band->burst_size);
> +            }
> +            sbrec_meter_set_bands(sb_meter, sb_bands, nb_meter->n_bands);
> +            free(sb_bands);
> +        }
> +
> +        sbrec_meter_set_unit(sb_meter, nb_meter->unit);
> +    }
> +
> +    struct shash_node *node, *next;
> +    SHASH_FOR_EACH_SAFE (node, next, &sb_meters) {
> +        sbrec_meter_delete(node->data);
> +        shash_delete(&sb_meters, node);
> +    }
> +    shash_destroy(&sb_meters);
> +}
> +
>   /*
>    * struct 'dns_info' is used to sync the DNS records between OVN Northbound db
>    * and Southbound db.
> @@ -6726,6 +6860,7 @@ ovnnb_db_run(struct northd_context *ctx,
>   
>       sync_address_sets(ctx);
>       sync_port_groups(ctx);
> +    sync_meters(ctx);
>       sync_dns_entries(ctx, &datapaths);
>   
>       struct ovn_port_group *pg, *next_pg;
> @@ -7351,6 +7486,16 @@ main(int argc, char *argv[])
>                          &sbrec_rbac_permission_col_insert_delete);
>       add_column_noalert(ovnsb_idl_loop.idl, &sbrec_rbac_permission_col_update);
>   
> +    ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_meter);
> +    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_col_name);
> +    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_col_unit);
> +    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_col_bands);
> +
> +    ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_meter_band);
> +    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_band_col_action);
> +    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_band_col_rate);
> +    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_band_col_burst_size);
> +
>       ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_chassis);
>       ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_chassis_col_nb_cfg);
>       ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_chassis_col_name);
> diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
> index 8e6ddec4662f..9a0d8ec70514 100644
> --- a/ovn/ovn-nb.ovsschema
> +++ b/ovn/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>   {
>       "name": "OVN_Northbound",
> -    "version": "5.11.0",
> -    "cksum": "1149260021 18713",
> +    "version": "5.12.0",
> +    "cksum": "2812995200 20238",
>       "tables": {
>           "NB_Global": {
>               "columns": {
> @@ -195,6 +195,35 @@
>                       "type": {"key": "string", "value": "string",
>                                "min": 0, "max": "unlimited"}}},
>               "isRoot": false},
> +        "Meter": {
> +            "columns": {
> +                "name": {"type": "string"},
> +                "unit": {"type": {"key": {"type": "string",
> +                                          "enum": ["set", ["kbps", "pktps"]]}}},
> +                "bands": {"type": {"key": {"type": "uuid",
> +                                           "refTable": "Meter_Band",
> +                                           "refType": "strong"},
> +                                   "min": 1,
> +                                   "max": "unlimited"}},
> +                "external_ids": {
> +                    "type": {"key": "string", "value": "string",
> +                             "min": 0, "max": "unlimited"}}},
> +            "indexes": [["name"]],
> +            "isRoot": true},
> +        "Meter_Band": {
> +            "columns": {
> +                "action": {"type": {"key": {"type": "string",
> +                                            "enum": ["set", ["drop"]]}}},
> +                "rate": {"type": {"key": {"type": "integer",
> +                                          "minInteger": 1,
> +                                          "maxInteger": 4294967295}}},
> +                "burst_size": {"type": {"key": {"type": "integer",
> +                                                "minInteger": 0,
> +                                                "maxInteger": 4294967295}}},
> +                "external_ids": {
> +                    "type": {"key": "string", "value": "string",
> +                             "min": 0, "max": "unlimited"}}},
> +            "isRoot": false},
>           "Logical_Router": {
>               "columns": {
>                   "name": {"type": "string"},
> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> index e4e72b27cf36..1feb2af52027 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -1356,6 +1356,86 @@
>       </column>
>     </table>
>   
> +  <table name="Meter" title="Meter table">
> +    <p>
> +      Each row in this table represents a meter that can be used for QoS or
> +      rate-limiting.
> +    </p>
> +
> +    <column name="name">
> +      <p>
> +        A name for this meter.
> +      </p>
> +
> +      <p>
> +        Names that begin with "__" are reserved for OVN internal use and should
> +        not be added manually.
> +      </p>
> +    </column>
> +
> +    <column name="unit">
> +      <p>
> +        The unit for <ref column="rate" table="Meter_Band"/> and
> +        <ref column="burst_rate" table="Meter_Band"/> parameters in
> +        the <ref column="bands"/> entry.  <code>kbps</code> specifies
> +        kilobits per second, and <code>pktps</code> specifies packets
> +        per second.
> +      </p>
> +    </column>
> +
> +    <column name="bands">
> +      <p>
> +        The bands associated with this meter.  Each band specifies a
> +        rate above which the band is to take the action
> +        <code>action</code>.  If multiple bands' rates are exceeded,
> +        then the band with the highest rate among the exceeded bands is
> +        selected.
> +      </p>
> +    </column>
> +
> +    <column name="external_ids">
> +      See <em>External IDs</em> at the beginning of this document.
> +    </column>
> +  </table>
> +
> +  <table name="Meter_Band" title="Meter_Band table">
> +    <p>
> +      Each row in this table represents a meter band which specifies the
> +      rate above which the configured action should be applied.  These bands
> +      are referenced by the <ref column="bands" table="Meter"/> column in
> +      the <ref table="Meter"/> table.
> +    </p>
> +
> +    <column name="action">
> +      <p>
> +        The action to execute when this band matches.  The only supported
> +        action is <code>drop</code>.
> +      </p>
> +    </column>
> +
> +    <column name="rate">
> +      <p>
> +        The relative rate limit for this band, in kilobits per second or
> +        bits per second, depending on whether the parent <ref table="Meter"/>
> +        entry's <ref column="unit" table="Meter"/> column specified
> +        <code>kbps</code> or <code>pktps</code>.
> +      </p>
> +    </column>
> +
> +    <column name="burst_size">
> +      <p>
> +        The maximum burst allowed for the band in kilobits or packets,
> +        depending on whether <code>kbps</code> or <code>pktps</code> was
> +        selected in the parent <ref table="Meter"/> entry's
> +        <ref column="unit" table="Meter"/> column.
> +      </p>
> +    </column>
> +
> +    <column name="external_ids">
> +      See <em>External IDs</em> at the beginning of this document.
> +    </column>
> +  </table>
> +
>     <table name="Logical_Router_Port" title="L3 logical router port">
>       <p>
>         A port within an L3 logical router.
> diff --git a/ovn/ovn-sb.ovsschema b/ovn/ovn-sb.ovsschema
> index 9e271d433246..ad6ad3b71da0 100644
> --- a/ovn/ovn-sb.ovsschema
> +++ b/ovn/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
>   {
>       "name": "OVN_Southbound",
> -    "version": "1.15.0",
> -    "cksum": "1839738004 13639",
> +    "version": "1.16.0",
> +    "cksum": "3046632234 14844",
>       "tables": {
>           "SB_Global": {
>               "columns": {
> @@ -98,6 +98,29 @@
>               "indexes": [["datapath", "tunnel_key"],
>                           ["datapath", "name"]],
>               "isRoot": true},
> +        "Meter": {
> +            "columns": {
> +                "name": {"type": "string"},
> +                "unit": {"type": {"key": {"type": "string",
> +                                          "enum": ["set", ["kbps", "pktps"]]}}},
> +                "bands": {"type": {"key": {"type": "uuid",
> +                                           "refTable": "Meter_Band",
> +                                           "refType": "strong"},
> +                                   "min": 1,
> +                                   "max": "unlimited"}}},
> +            "indexes": [["name"]],
> +            "isRoot": true},
> +        "Meter_Band": {
> +            "columns": {
> +                "action": {"type": {"key": {"type": "string",
> +                                            "enum": ["set", ["drop"]]}}},
> +                "rate": {"type": {"key": {"type": "integer",
> +                                          "minInteger": 1,
> +                                          "maxInteger": 4294967295}}},
> +                "burst_size": {"type": {"key": {"type": "integer",
> +                                                "minInteger": 0,
> +                                                "maxInteger": 4294967295}}}},
> +            "isRoot": false},
>           "Datapath_Binding": {
>               "columns": {
>                   "tunnel_key": {
> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> index f9724d398ce6..57d8a9e042a5 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -1923,6 +1923,78 @@ tcp.flags = RST;
>       </column>
>     </table>
>   
> +  <table name="Meter" title="Meter table">
> +    <p>
> +      Each row in this table represents a meter that can be used for QoS or
> +      rate-limiting.
> +    </p>
> +
> +    <column name="name">
> +      <p>
> +        A name for this meter.
> +      </p>
> +
> +      <p>
> +        Names that begin with "__" are reserved for OVN internal use and should
> +        not be added manually.
> +      </p>
> +    </column>
> +
> +    <column name="unit">
> +      <p>
> +        The unit for <ref column="rate" table="Meter_Band"/> and
> +        <ref column="burst_rate" table="Meter_Band"/> parameters in
> +        the <ref column="bands"/> entry.  <code>kbps</code> specifies
> +        kilobits per second, and <code>pktps</code> specifies packets
> +        per second.
> +      </p>
> +    </column>
> +
> +    <column name="bands">
> +      <p>
> +        The bands associated with this meter.  Each band specifies a
> +        rate above which the band is to take the action
> +        <code>action</code>.  If multiple bands' rates are exceeded,
> +        then the band with the highest rate among the exceeded bands is
> +        selected.
> +      </p>
> +    </column>
> +  </table>
> +
> +  <table name="Meter_Band" title="Meter_Band table">
> +    <p>
> +      Each row in this table represents a meter band which specifies the
> +      rate above which the configured action should be applied.  These bands
> +      are referenced by the <ref column="bands" table="Meter"/> column in
> +      the <ref table="Meter"/> table.
> +    </p>
> +
> +    <column name="action">
> +      <p>
> +        The action to execute when this band matches.  The only supported
> +        action is <code>drop</code>.
> +      </p>
> +    </column>
> +
> +    <column name="rate">
> +      <p>
> +        The relative rate limit for this band, in kilobits per second or
> +        bits per second, depending on whether the parent <ref table="Meter"/>
> +        entry's <ref column="unit" table="Meter"/> column specified
> +        <code>kbps</code> or <code>pktps</code>.
> +      </p>
> +    </column>
> +
> +    <column name="burst_size">
> +      <p>
> +        The maximum burst allowed for the band in kilobits or packets,
> +        depending on whether <code>kbps</code> or <code>pktps</code> was
> +        selected in the parent <ref table="Meter"/> entry's
> +        <ref column="unit" table="Meter"/> column.
> +      </p>
> +    </column>
> +  </table>
> +
>     <table name="Datapath_Binding" title="Physical-Logical Datapath Bindings">
>       <p>
>         Each row in this table represents a logical datapath, which implements a
> diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
> index 2cd2fab304cd..a8ea7d8cb1e1 100644
> --- a/ovn/utilities/ovn-nbctl.8.xml
> +++ b/ovn/utilities/ovn-nbctl.8.xml
> @@ -172,6 +172,56 @@
>         </dd>
>       </dl>
>   
> +    <h1>Meter Commands</h1>
> +    <dl>
> +        <dt><code>meter-add</code> <var>name</var> <var>unit</var> <var>action</var> <var>rate</var> [<var>burst_size</var>]</dt>
> +      <dd>
> +        <p>
> +          Adds the specified meter.  <var>name</var> must be a unique
> +          name to identify this meter.  The <var>action</var> argument
> +          specifies what should happen when this meter is exceeded.
> +          The only supported action is <code>drop</code>.
> +        </p>
> +
> +        <p>
> +          The <var>unit</var> specifies the unit for the <var>rate</var>
> +          argument; valid values are <code>kbps</code> and
> +          <code>pktps</code> for kilobits per second and packets per
> +          second, respectively.  The <var>burst_rate</var> option
> +          configures the maximum burst allowed for the band in kilobits
> +          or packets depending on whether the <var>unit</var> chosen was
> +          <code>kbps</code> or <code>pktps</code>, respectively.
> +        </p>
> +
> +        <p>
> +          <code>ovn-nbctl</code> only supports adding a meter with a
> +          single band, but the other commands support meters with
> +          multiple bands.
> +        </p>
> +
> +        <p>
> +          Names that start with "__" are reserved for internal use by OVN,
> +          so <code>ovn-nbctl</code> does not allow adding them.
> +        </p>
> +      </dd>
> +
> +      <dt><code>meter-del</code> [<var>name</var>]</dt>
> +      <dd>
> +        <p>
> +          Deletes meters.  By default, all meters are deleted.  If
> +          <var>name</var> is supplied, only the meter with that name
> +          will be deleted.
> +      </p>
> +      </dd>
> +
> +      <dt><code>meter-list</code></dt>
> +      <dd>
> +        <p>
> +          Lists all meters.
> +        </p>
> +      </dd>
> +    </dl>
> +
>       <h1>Logical Switch Port Commands</h1>
>       <dl>
>         <dt>[<code>--may-exist</code>] <code>lsp-add</code> <var>switch</var> <var>port</var></dt>
> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
> index 3c3e582cb906..9f0e6347c104 100644
> --- a/ovn/utilities/ovn-nbctl.c
> +++ b/ovn/utilities/ovn-nbctl.c
> @@ -496,6 +496,12 @@ QoS commands:\n\
>                               remove QoS rules from SWITCH\n\
>     qos-list SWITCH           print QoS rules for SWITCH\n\
>   \n\
> +Meter commands:\n\
> +  meter-add NAME UNIT ACTION RATE [BURST_SIZE]\n\
> +                            add a meter\n\
> +  meter-del [NAME]          remove meters\n\
> +  meter-list                print meters\n\
> +\n\
>   Logical switch port commands:\n\
>     lsp-add SWITCH PORT       add logical port PORT on SWITCH\n\
>     lsp-add SWITCH PORT PARENT TAG\n\
> @@ -2290,6 +2296,137 @@ nbctl_qos_del(struct ctl_context *ctx)
>       }
>   }
>   
> +static int
> +meter_cmp(const void *meter1_, const void *meter2_)
> +{
> +    struct nbrec_meter *const *meter1p = meter1_;
> +    struct nbrec_meter *const *meter2p = meter2_;
> +    const struct nbrec_meter *meter1 = *meter1p;
> +    const struct nbrec_meter *meter2 = *meter2p;
> +
> +    return strcmp(meter1->name, meter2->name);
> +}
> +
> +static void
> +nbctl_meter_list(struct ctl_context *ctx)
> +{
> +    const struct nbrec_meter **meters = NULL;
> +    const struct nbrec_meter *meter;
> +    size_t n_capacity = 0;
> +    size_t n_meters = 0;
> +
> +    NBREC_METER_FOR_EACH (meter, ctx->idl) {
> +        if (n_meters == n_capacity) {
> +            meters = x2nrealloc(meters, &n_capacity, sizeof *meters);
> +        }
> +
> +        meters[n_meters] = meter;
> +        n_meters++;
> +    }
> +
> +    if (n_meters) {
> +        qsort(meters, n_meters, sizeof *meters, meter_cmp);
> +    }
> +
> +    for (size_t i = 0; i < n_meters; i++) {
> +        meter = meters[i];
> +        ds_put_format(&ctx->output, "%s: unit=%s bands:\n", meter->name,
> +                      meter->unit);
> +
> +        for (size_t j = 0; j < meter->n_bands; j++) {
> +            const struct nbrec_meter_band *band = meter->bands[j];
> +
> +            ds_put_format(&ctx->output, "  %s: rate=%"PRId64"",
> +                          band->action, band->rate);
> +            if (band->burst_size) {
> +                ds_put_format(&ctx->output, ", burst_size=%"PRId64"",
> +                              band->burst_size);
> +            }
> +        }
> +
> +        ds_put_cstr(&ctx->output, "\n");
> +    }
> +
> +    free(meters);
> +}
> +
> +static void
> +nbctl_meter_add(struct ctl_context *ctx)
> +{
> +    const struct nbrec_meter *meter;
> +
> +    const char *name = ctx->argv[1];
> +    NBREC_METER_FOR_EACH (meter, ctx->idl) {
> +        if (!strcmp(meter->name, name)) {
> +            ctl_fatal("meter with name \"%s\" already exists", name);
> +        }
> +    }
> +
> +    if (!strncmp(name, "__", 2)) {
> +        ctl_fatal("meter names that begin with \"__\" are reserved");
> +    }
> +
> +    const char *unit = ctx->argv[2];
> +    if (strcmp(unit, "kbps") && strcmp(unit, "pktps")) {
> +        ctl_fatal("unit must be \"kbps\" or \"pktps\"");
> +    }
> +
> +    const char *action = ctx->argv[3];
> +    if (strcmp(action, "drop")) {
> +        ctl_fatal("action must be \"drop\"");
> +    }
> +
> +    int64_t rate;
> +    if (!ovs_scan(ctx->argv[4], "%"SCNd64, &rate)
> +        || rate < 1 || rate > UINT32_MAX) {
> +        ctl_fatal("rate must be in the range 1...4294967295");
> +    }
> +
> +    int64_t burst_size = 0;
> +    if (ctx->argc > 5) {
> +        if (!ovs_scan(ctx->argv[5], "%"SCNd64, &burst_size)
> +            || burst_size < 0 || burst_size > UINT32_MAX) {
> +            ctl_fatal("burst_size must be in the range 0...4294967295");
> +        }
> +    }
> +
> +    /* Create the band.  We only support adding a single band. */
> +    struct nbrec_meter_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_size);
> +
> +    /* Create the 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);
> +}
> +
> +static void
> +nbctl_meter_del(struct ctl_context *ctx)
> +{
> +    const struct nbrec_meter *meter, *next;
> +
> +    /* If a name is not specified, delete all meters. */
> +    if (ctx->argc == 1) {
> +        NBREC_METER_FOR_EACH_SAFE (meter, next, ctx->idl) {
> +            nbrec_meter_delete(meter);
> +        }
> +        return;
> +    }
> +
> +    /* Remove the matching meter. */
> +    NBREC_METER_FOR_EACH (meter, ctx->idl) {
> +        if (strcmp(ctx->argv[1], meter->name)) {
> +            continue;
> +        }
> +
> +        nbrec_meter_delete(meter);
> +        return;
> +    }
> +}
> +
>   static void
>   nbctl_lb_add(struct ctl_context *ctx)
>   {
> @@ -4678,6 +4815,12 @@ static const struct ctl_command_syntax nbctl_commands[] = {
>         nbctl_qos_del, NULL, "", RW },
>       { "qos-list", 1, 1, "SWITCH", NULL, nbctl_qos_list, NULL, "", RO },
>   
> +    /* meter commands. */
> +    { "meter-add", 4, 5, "NAME UNIT ACTION RATE [BURST_SIZE]", NULL,
> +      nbctl_meter_add, NULL, "", RW },
> +    { "meter-del", 0, 1, "[NAME]", NULL, nbctl_meter_del, NULL, "", RW },
> +    { "meter-list", 0, 0, "", NULL, nbctl_meter_list, NULL, "", RO },
> +
>       /* logical switch port commands. */
>       { "lsp-add", 2, 4, "SWITCH PORT [PARENT] [TAG]", NULL, nbctl_lsp_add,
>         NULL, "--may-exist", RW },
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 64e217654c2f..7a1445e312ff 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -323,6 +323,64 @@ OVN_NBCTL_TEST_STOP
>   AT_CLEANUP
>   
>   dnl ---------------------------------------------------------------------
> +
> +AT_SETUP([ovn-nbctl - Meters])
> +OVN_NBCTL_TEST_START
> +
> +AT_CHECK([ovn-nbctl meter-add meter1 kbps drop 10])
> +AT_CHECK([ovn-nbctl meter-add meter2 kbps drop 3 2])
> +AT_CHECK([ovn-nbctl meter-add meter3 kbps drop 100 200])
> +
> +dnl Add duplicate meter name
> +AT_CHECK([ovn-nbctl meter-add meter1 kbps drop 10], [1], [], [stderr])
> +AT_CHECK([grep 'already exists' stderr], [0], [ignore])
> +
> +dnl Add reserved meter name
> +AT_CHECK([ovn-nbctl meter-add __meter1 kbps drop 10], [1], [], [stderr])
> +AT_CHECK([grep 'reserved' stderr], [0], [ignore])
> +
> +dnl Add meter with invalid rates
> +AT_CHECK([ovn-nbctl meter-add meter4 kbps drop 100010111111], [1], [],
> +[ovn-nbctl: rate must be in the range 1...4294967295
> +])
> +
> +AT_CHECK([ovn-nbctl meter-add meter4 kbps drop 0], [1], [],
> +[ovn-nbctl: rate must be in the range 1...4294967295
> +])
> +
> +dnl Add meter with invalid burst_size
> +AT_CHECK([ovn-nbctl meter-add meter4 kbps drop 10 100010111111], [1], [],
> +[ovn-nbctl: burst_size must be in the range 0...4294967295
> +])
> +
> +AT_CHECK([ovn-nbctl meter-list], [0], [dnl
> +meter1: unit=kbps bands:
> +  drop: rate=10
> +meter2: unit=kbps bands:
> +  drop: rate=3, burst_size=2
> +meter3: unit=kbps bands:
> +  drop: rate=100, burst_size=200
> +])
> +
> +dnl Delete a single meter.
> +AT_CHECK([ovn-nbctl meter-del meter2])
> +AT_CHECK([ovn-nbctl meter-list], [0], [dnl
> +meter1: unit=kbps bands:
> +  drop: rate=10
> +meter3: unit=kbps bands:
> +  drop: rate=100, burst_size=200
> +])
> +
> +dnl Delete all meters.
> +AT_CHECK([ovn-nbctl meter-del])
> +AT_CHECK([ovn-nbctl meter-list], [0], [dnl
> +])
> +
> +OVN_NBCTL_TEST_STOP
> +AT_CLEANUP
> +
> +dnl ---------------------------------------------------------------------
> +
>   AT_SETUP([ovn-nbctl - NATs])
>   OVN_NBCTL_TEST_START
>   AT_CHECK([ovn-nbctl lr-add lr0])
>

> On Jul 30, 2018, at 11:26 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Sun, Jul 29, 2018 at 11:46:35PM -0700, Justin Pettit wrote:
>> Add support for configuring meters through the Meter and Meter_Band
>> tables in the Northbound database.  This commit also has ovn-northd
>> sync those tables between the Northbound and Southbound databases.
>> 
>> Add support for configuring meters with ovn-nbctl.
>> 
>> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> 
> Thanks for the patches!
> 
> band_cmp() uses the form "a > b ? -1 : 1" a couple of times.  I believe
> that this will sort 'a' and 'b' into descending order.  Is that what you
> want?
> 
> (Oh, actually I see it doesn't matter, it just needs to be consistent.)

Right, it shouldn't matter, so I'll just leave it.

> In ovn-[ns]b.xml, for consistency with the rest of the tables,
> title="..." in <table> start tags should not include the word "table"
> and maybe should give better descriptions.

Yes, I copied that from QoS, which was right above it.  I went ahead and changed QoS, too.

> The Meter_Band table says the following.  I'm not sure what a "relative
> rate limit" is.  It kind of sounds like it would be an additive one,
> where if you have a first band with a rate of 500 Mbps then the second
> one with a rate of 250 Mbps would effectively be 500 + 250 = 750 Mbps.
> But I don't think you really mean that; I think it's actually an
> absolute rate.  Maybe by relative you just mean that the unit is
> specified by the Meter.
> 
>    <column name="rate">
>      <p>
>        The relative rate limit for this band, in kilobits per second or
>        bits per second, depending on whether the parent <ref table="Meter"/>
>        entry's <ref column="unit" table="Meter"/> column specified
>        <code>kbps</code> or <code>pktps</code>.
>      </p>
>    </column>

Honestly, I just took it from the ovs-ofctl man page.  I dropped relative, and I'll send out a patch to update the ovs-ofctl man page, too.

> The documentation for "meter-add" uses two different names burst_size
> and burst_rate for the same argument.  (Maybe just "burst" would be a
> better name.)

I just went ahead and changed it to "burst".

> The documentation writes "__" for two underscores, but it's a little
> hard to tell in most fonts whether there are one or two underscores, so
> you might add (two underscores) to make it clear.

Good point.  Done.

> The command
>        meter-add name unit action rate [burst]
> might be a little more natural if it were more like
>        meter-add name action rate [burst]
> where "rate" was to be supplied as, e.g. 1000kbps or 50000pktps.
> 
> Possibly the "list" output could be a little more human friendly in the
> same way, e.g.:
> 
>    meter1:
>      drop: 10 kbps
>    meter3:
>      drop: 100 kbps, 200 kb burst
> 
> but it's not a big deal either way.

Good suggestion.  I cleaned these up a bit.

> If the burst size is truly optional, it might be nice to make it
> optional in the schema, so that it could be omitted instead of set to
> 0.  The schema documentation for Meter_Band doesn't say that it's
> optional; it probably should explain what it means to set it to 0.

I originally had it that way, but it made dealing with the schema a bit more complicated without much benefit, since 0 is effectively the same.  I provided a description of a 0 value to ovn-nbctl, ovn-nb, and ovn-sb.

> Acked-by: Ben Pfaff <blp@ovn.org>

Thanks!

--Justin

> On Jul 30, 2018, at 11:40 AM, Mark Michelson <mmichels@redhat.com> wrote:
> 
> Hi Justin,
> 
> I took a look through the patch series, and this is the only one that I had some immediate feedback on.
> 
> First, it would be nice if we could refer to Meters by name when issuing DB commands. For instance `ovn-nbctl add Meter <meter_name> bands <band UUID>`.

Okay, I'll work on a follow-up patch to add that.  I didn't think it was a blocker for this series, though.

> Second, I noticed that the algorithm for computing southbound meter bands can result in a larger number of bands than is in the northbound table. For instance, you could issue the following:
> 
> ovn-nbctl --id=@id create Meter_Band action=drop rate=1000 -- \
> create Meter name=foo unit=kbps band=@id -- \
> create Meter name=bar unit=kbps band=@id
> 
> In the northbound database, you'll have one entry in the meter_band table. In the southbound database, you'll have two entries in the meter_band table. The algorithm does not take into account that multiple northbound meters may refer to the same meter_band. I'm not sure how big an issue this is (or really if it is an issue), but it surprised me a bit when I was playing around with it.

That's interesting, but I don't think it's a problem.  Let me know if you think of an issue with it, though.

Thanks for checking out the series, and please let me know if you have any other thoughts.

--Justin

On 07/30/2018 09:00 PM, Justin Pettit wrote:
> 
> 
>> On Jul 30, 2018, at 11:40 AM, Mark Michelson <mmichels@redhat.com> wrote:
>>
>> Hi Justin,
>>
>> I took a look through the patch series, and this is the only one that I had some immediate feedback on.
>>
>> First, it would be nice if we could refer to Meters by name when issuing DB commands. For instance `ovn-nbctl add Meter <meter_name> bands <band UUID>`.
> 
> Okay, I'll work on a follow-up patch to add that.  I didn't think it was a blocker for this series, though.
> 
>> Second, I noticed that the algorithm for computing southbound meter bands can result in a larger number of bands than is in the northbound table. For instance, you could issue the following:
>>
>> ovn-nbctl --id=@id create Meter_Band action=drop rate=1000 -- \
>> create Meter name=foo unit=kbps band=@id -- \
>> create Meter name=bar unit=kbps band=@id
>>
>> In the northbound database, you'll have one entry in the meter_band table. In the southbound database, you'll have two entries in the meter_band table. The algorithm does not take into account that multiple northbound meters may refer to the same meter_band. I'm not sure how big an issue this is (or really if it is an issue), but it surprised me a bit when I was playing around with it.
> 
> That's interesting, but I don't think it's a problem.  Let me know if you think of an issue with it, though.

I figured more than anything I should bring this to your attention in 
case you had not noticed it. Given that you provide an 'ovn-nbctl 
meter-add' command, it strikes me that most people will probably add 
meters this way. If they do it this way, then they won't run into any 
surprises.

> 
> Thanks for checking out the series, and please let me know if you have any other thoughts.
> 
> --Justin
> 
>

> On Jul 31, 2018, at 5:07 AM, Mark Michelson <mmichels@redhat.com> wrote:
> 
> I figured more than anything I should bring this to your attention in case you had not noticed it. Given that you provide an 'ovn-nbctl meter-add' command, it strikes me that most people will probably add meters this way. If they do it this way, then they won't run into any surprises.

That's my feeling, too, but it was an interesting angle I hadn't thought about.  :-)

--Justin

On Sun, Jul 29, 2018 at 11:46:35PM -0700, Justin Pettit wrote:
> Add support for configuring meters through the Meter and Meter_Band
> tables in the Northbound database.  This commit also has ovn-northd
> sync those tables between the Northbound and Southbound databases.
> 
> Add support for configuring meters with ovn-nbctl.
> 
> Signed-off-by: Justin Pettit <jpettit@ovn.org>

Hi Justin,

it seems that this patch broke building with older GCC:

	https://travis-ci.org/openvswitch/ovs/jobs/410404752:

Ben applied a fix for that to master.

	04a12e42e089 ("ofctrl: Placate GCC.")

I believe that change is also needed in branch-2.10.

On Wed, Aug 01, 2018 at 02:21:51PM +0200, Simon Horman wrote:
> On Sun, Jul 29, 2018 at 11:46:35PM -0700, Justin Pettit wrote:
> > Add support for configuring meters through the Meter and Meter_Band
> > tables in the Northbound database.  This commit also has ovn-northd
> > sync those tables between the Northbound and Southbound databases.
> > 
> > Add support for configuring meters with ovn-nbctl.
> > 
> > Signed-off-by: Justin Pettit <jpettit@ovn.org>
> 
> Hi Justin,
> 
> it seems that this patch broke building with older GCC:
> 
> 	https://travis-ci.org/openvswitch/ovs/jobs/410404752:
> 
> Ben applied a fix for that to master.
> 
> 	04a12e42e089 ("ofctrl: Placate GCC.")
> 
> I believe that change is also needed in branch-2.10.

Thanks for the report, I've now cherry-picked it.