Re: [ovs-dev] [ACL Meters 4/7] ovn: Add Meter and Meter_Band tables to the NB and SB databases.
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
>
> 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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ACL Meters 4/7] ovn: Add Meter and Meter_Band tables to the NB and SB databases.
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
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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ACL Meters 4/7] ovn: Add Meter and Meter_Band tables to the NB and SB databases.
> On Jul 31, 2018, at 5:07 AM, Mark Michelson 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 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ACL Meters 4/7] ovn: Add Meter and Meter_Band tables to the NB and SB databases.
On 07/30/2018 09:00 PM, Justin Pettit wrote: On Jul 30, 2018, at 11:40 AM, Mark Michelson 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 bands `. 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 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ACL Meters 4/7] ovn: Add Meter and Meter_Band tables to the NB and SB databases.
> On Jul 30, 2018, at 11:40 AM, Mark Michelson 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 bands `. 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 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ACL Meters 4/7] ovn: Add Meter and Meter_Band tables to the NB and SB databases.
> On Jul 30, 2018, at 11:26 AM, Ben Pfaff 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 > > 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 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. > > > >The relative rate limit for this band, in kilobits per second or >bits per second, depending on whether the parent >entry's column specified >kbps or pktps. > > 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 5pktps. > > 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 Thanks! --Justin ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ACL Meters 4/7] ovn: Add Meter and Meter_Band tables to the NB and SB databases.
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 bands 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
---
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(_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 =
Re: [ovs-dev] [ACL Meters 4/7] ovn: Add Meter and Meter_Band tables to the NB and SB databases.
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 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 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. The relative rate limit for this band, in kilobits per second or bits per second, depending on whether the parent entry's column specified kbps or pktps. 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 5pktps. 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 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[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
---
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(_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(_meters);
+
+const struct sbrec_meter *sb_meter;
+SBREC_METER_FOR_EACH (sb_meter, ctx->ovnsb_idl) {
+shash_add(_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(_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],
