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

[Justin Pettit]

> On Jul 30, 2018, at 11:26 AM, Ben Pfaff <blp at 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 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.)

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

Thanks!

--Justin