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

[Ben Pfaff]

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 at 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 at ovn.org>