> On Jul 30, 2018, at 11:26 AM, Ben Pfaff <b...@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 <jpet...@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 <b...@ovn.org> Thanks! --Justin _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
