| Message ID | 20201120182211.2646-2-flavio@flaviof.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | northd: Enhance the implementation of ACL log meters (pre-ddlog merge). | expand |
On Fri, Nov 20, 2020 at 11:53 PM Flavio Fernandes <flavio@flaviof.com> wrote: > > When multiple ACL rows use the same Meter for log rate-limiting, the > 'noisiest' ACL matches may completely consume the Meter and shadow other > ACL logs. This patch introduces a column in northbound's Meter table > called fair that allows for a Meter to rate-limit each ACL log separately. > > The change is backward compatible. Based on the fair column of a Meter row, > northd will turn a single Meter in the NB into multiple Meters in the SB > by appending the ACL uuid to its name. It will also make action in logical > flow use the unique Meter name for each affected ACL log. In order to > make the Meter behave as described, add Meter with this option: > > ovn-nbctl --fair meter-add ${meter_name} drop ${rate} ${unit} > > Example: > > ovn-nbctl --fair meter-add meter_me drop 1 pktps > > Reported-by: Flavio Fernandes <flavio@flaviof.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-October/050769.html > Suggested-by: Ben Pfaff <blp@ovn.org> > Suggested-by: Dumitru Ceara <dceara@redhat.com> > Suggested-by: Numan Siddique <numans@ovn.org> > Signed-off-by: Flavio Fernandes <flavio@flaviof.com> Thanks for addressing the comments. I applied this patch to master. Since I only provide review comments and didn't suggest any solution, I removed the suggested-by tag with my name :). Numan > --- > This change can be tracked in the following github clone/branch: > > https://github.com/flavio-fernandes/ovn/commits/acl-meters.v6 > > NEWS | 2 + > northd/ovn-northd.c | 168 ++++++++++++++++++++++++++++++------------ > ovn-nb.ovsschema | 5 +- > ovn-nb.xml | 16 +++- > tests/ovn-nbctl.at | 6 +- > tests/ovn-northd.at | 97 ++++++++++++++++++++++++ > utilities/ovn-nbctl.c | 16 +++- > 7 files changed, 256 insertions(+), 54 deletions(-) > > diff --git a/NEWS b/NEWS > index a681573aa..18da14b18 100644 > --- a/NEWS > +++ b/NEWS > @@ -10,6 +10,8 @@ Post-v20.09.0 > traffic. > - Propagate currently processed SB_Global.nb_cfg in ovn-controller to the > local OVS DB integration bridge external_ids:ovn-nb-cfg. > + - Add "fair" column in Meter table to allow multiple ACL logs to use the > + same Meter while being rate-limited independently. > > OVN v20.09.0 - 28 Sep 2020 > -------------------------- > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index d08a8a643..c2176f105 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -5265,8 +5265,46 @@ build_acl_hints(struct ovn_datapath *od, struct hmap *lflows) > } > } > > +static const struct nbrec_meter* > +fair_meter_lookup_by_name(const struct shash *meter_groups, > + const char *meter_name) > +{ > + const struct nbrec_meter *nb_meter = > + meter_name ? shash_find_data(meter_groups, meter_name) : NULL; > + if (nb_meter) { > + return (nb_meter->fair && *nb_meter->fair) ? nb_meter : NULL; > + } > + return NULL; > +} > + > +static char* > +alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl) > +{ > + return xasprintf("%s__" UUID_FMT, > + acl->meter, UUID_ARGS(&acl->header_.uuid)); > +} > + > static void > -build_acl_log(struct ds *actions, const struct nbrec_acl *acl) > +build_acl_log_meter(struct ds *actions, const struct nbrec_acl *acl, > + const struct shash *meter_groups) > +{ > + if (!acl->meter) { > + return; > + } > + > + /* If ACL log meter uses a fair meter, use unique Meter name. */ > + if (fair_meter_lookup_by_name(meter_groups, acl->meter)) { > + char *meter_name = alloc_acl_log_unique_meter_name(acl); > + ds_put_format(actions, "meter=\"%s\", ", meter_name); > + free(meter_name); > + } else { > + ds_put_format(actions, "meter=\"%s\", ", acl->meter); > + } > +} > + > +static void > +build_acl_log(struct ds *actions, const struct nbrec_acl *acl, > + const struct shash *meter_groups) > { > if (!acl->log) { > return; > @@ -5294,9 +5332,7 @@ build_acl_log(struct ds *actions, const struct nbrec_acl *acl) > ds_put_cstr(actions, "verdict=allow, "); > } > > - if (acl->meter) { > - ds_put_format(actions, "meter=\"%s\", ", acl->meter); > - } > + build_acl_log_meter(actions, acl, meter_groups); > > ds_chomp(actions, ' '); > ds_chomp(actions, ','); > @@ -5307,7 +5343,8 @@ static void > build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, > enum ovn_stage stage, struct nbrec_acl *acl, > struct ds *extra_match, struct ds *extra_actions, > - const struct ovsdb_idl_row *stage_hint) > + const struct ovsdb_idl_row *stage_hint, > + const struct shash *meter_groups) > { > struct ds match = DS_EMPTY_INITIALIZER; > struct ds actions = DS_EMPTY_INITIALIZER; > @@ -5319,7 +5356,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, > ingress ? ovn_stage_get_table(S_SWITCH_OUT_QOS_MARK) > : ovn_stage_get_table(S_SWITCH_IN_L2_LKUP)); > > - build_acl_log(&actions, acl); > + build_acl_log(&actions, acl, meter_groups); > if (extra_match->length > 0) { > ds_put_format(&match, "(%s) && ", extra_match->string); > } > @@ -5344,7 +5381,8 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, > > static void > consider_acl(struct hmap *lflows, struct ovn_datapath *od, > - struct nbrec_acl *acl, bool has_stateful) > + struct nbrec_acl *acl, bool has_stateful, > + const struct shash *meter_groups) > { > bool ingress = !strcmp(acl->direction, "from-lport") ? true :false; > enum ovn_stage stage = ingress ? S_SWITCH_IN_ACL : S_SWITCH_OUT_ACL; > @@ -5358,7 +5396,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > * associated conntrack entry and would return "+invalid". */ > if (!has_stateful) { > struct ds actions = DS_EMPTY_INITIALIZER; > - build_acl_log(&actions, acl); > + build_acl_log(&actions, acl, meter_groups); > ds_put_cstr(&actions, "next;"); > ovn_lflow_add_with_hint(lflows, od, stage, > acl->priority + OVN_ACL_PRI_OFFSET, > @@ -5384,7 +5422,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > ds_put_format(&match, REGBIT_ACL_HINT_ALLOW_NEW " == 1 && (%s)", > acl->match); > ds_put_cstr(&actions, REGBIT_CONNTRACK_COMMIT" = 1; "); > - build_acl_log(&actions, acl); > + build_acl_log(&actions, acl, meter_groups); > ds_put_cstr(&actions, "next;"); > ovn_lflow_add_with_hint(lflows, od, stage, > acl->priority + OVN_ACL_PRI_OFFSET, > @@ -5403,7 +5441,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > ds_put_format(&match, REGBIT_ACL_HINT_ALLOW " == 1 && (%s)", > acl->match); > > - build_acl_log(&actions, acl); > + build_acl_log(&actions, acl, meter_groups); > ds_put_cstr(&actions, "next;"); > ovn_lflow_add_with_hint(lflows, od, stage, > acl->priority + OVN_ACL_PRI_OFFSET, > @@ -5428,10 +5466,10 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > ds_put_cstr(&match, REGBIT_ACL_HINT_DROP " == 1"); > if (!strcmp(acl->action, "reject")) { > build_reject_acl_rules(od, lflows, stage, acl, &match, > - &actions, &acl->header_); > + &actions, &acl->header_, meter_groups); > } else { > ds_put_format(&match, " && (%s)", acl->match); > - build_acl_log(&actions, acl); > + build_acl_log(&actions, acl, meter_groups); > ds_put_cstr(&actions, "/* drop */"); > ovn_lflow_add_with_hint(lflows, od, stage, > acl->priority + OVN_ACL_PRI_OFFSET, > @@ -5455,10 +5493,10 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > ds_put_cstr(&actions, "ct_commit { ct_label.blocked = 1; }; "); > if (!strcmp(acl->action, "reject")) { > build_reject_acl_rules(od, lflows, stage, acl, &match, > - &actions, &acl->header_); > + &actions, &acl->header_, meter_groups); > } else { > ds_put_format(&match, " && (%s)", acl->match); > - build_acl_log(&actions, acl); > + build_acl_log(&actions, acl, meter_groups); > ds_put_cstr(&actions, "/* drop */"); > ovn_lflow_add_with_hint(lflows, od, stage, > acl->priority + OVN_ACL_PRI_OFFSET, > @@ -5471,9 +5509,9 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > * logical flow action in all cases. */ > if (!strcmp(acl->action, "reject")) { > build_reject_acl_rules(od, lflows, stage, acl, &match, > - &actions, &acl->header_); > + &actions, &acl->header_, meter_groups); > } else { > - build_acl_log(&actions, acl); > + build_acl_log(&actions, acl, meter_groups); > ds_put_cstr(&actions, "/* drop */"); > ovn_lflow_add_with_hint(lflows, od, stage, > acl->priority + OVN_ACL_PRI_OFFSET, > @@ -5553,7 +5591,7 @@ build_port_group_lswitches(struct northd_context *ctx, struct hmap *pgs, > > static void > build_acls(struct ovn_datapath *od, struct hmap *lflows, > - struct hmap *port_groups) > + struct hmap *port_groups, const struct shash *meter_groups) > { > bool has_stateful = (has_stateful_acl(od) || has_lb_vip(od)); > > @@ -5656,13 +5694,14 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, > /* Ingress or Egress ACL Table (Various priorities). */ > for (size_t i = 0; i < od->nbs->n_acls; i++) { > struct nbrec_acl *acl = od->nbs->acls[i]; > - consider_acl(lflows, od, acl, has_stateful); > + consider_acl(lflows, od, acl, has_stateful, meter_groups); > } > struct ovn_port_group *pg; > HMAP_FOR_EACH (pg, key_node, port_groups) { > if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) { > for (size_t i = 0; i < pg->nb_pg->n_acls; i++) { > - consider_acl(lflows, od, pg->nb_pg->acls[i], has_stateful); > + consider_acl(lflows, od, pg->nb_pg->acls[i], has_stateful, > + meter_groups); > } > } > } > @@ -7311,7 +7350,7 @@ build_lswitch_lflows_pre_acl_and_acl(struct ovn_datapath *od, > build_pre_lb(od, lflows, meter_groups, lbs); > build_pre_stateful(od, lflows); > build_acl_hints(od, lflows); > - build_acls(od, lflows, port_groups); > + build_acls(od, lflows, port_groups, meter_groups); > build_qos(od, lflows); > build_lb(od, lflows); > build_stateful(od, lflows, lbs); > @@ -11502,12 +11541,50 @@ done: > return need_update; > } > > +static void > +sync_meters_iterate_nb_meter(struct northd_context *ctx, > + const char *meter_name, > + const struct nbrec_meter *nb_meter, > + struct shash *sb_meters) > +{ > + bool new_sb_meter = false; > + > + const struct sbrec_meter *sb_meter = shash_find_and_delete(sb_meters, > + meter_name); > + if (!sb_meter) { > + sb_meter = sbrec_meter_insert(ctx->ovnsb_txn); > + sbrec_meter_set_name(sb_meter, 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); > +} > + > /* 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. > + * OVN_Southbound. Additionally, ACL logs that use fair meters have > + * a private copy of its meter in the SB table. > */ > static void > -sync_meters(struct northd_context *ctx) > +sync_meters(struct northd_context *ctx, struct hmap *datapaths, > + struct shash *meter_groups) > { > struct shash sb_meters = SHASH_INITIALIZER(&sb_meters); > > @@ -11518,33 +11595,32 @@ sync_meters(struct northd_context *ctx) > > const struct nbrec_meter *nb_meter; > NBREC_METER_FOR_EACH (nb_meter, ctx->ovnnb_idl) { > - bool new_sb_meter = false; > + sync_meters_iterate_nb_meter(ctx, nb_meter->name, nb_meter, > + &sb_meters); > + } > > - 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; > + /* > + * In addition to creating Meters in the SB from the block above, check > + * and see if additional rows are needed to get ACLs logs individually > + * rate-limited. > + */ > + struct ovn_datapath *od; > + HMAP_FOR_EACH (od, key_node, datapaths) { > + if (!od->nbs) { > + continue; > } > - > - 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); > + for (size_t i = 0; i < od->nbs->n_acls; i++) { > + struct nbrec_acl *acl = od->nbs->acls[i]; > + nb_meter = fair_meter_lookup_by_name(meter_groups, acl->meter); > + if (!nb_meter) { > + continue; > } > - sbrec_meter_set_bands(sb_meter, sb_bands, nb_meter->n_bands); > - free(sb_bands); > - } > > - sbrec_meter_set_unit(sb_meter, nb_meter->unit); > + char *meter_name = alloc_acl_log_unique_meter_name(acl); > + sync_meters_iterate_nb_meter(ctx, meter_name, nb_meter, > + &sb_meters); > + free(meter_name); > + } > } > > struct shash_node *node, *next; > @@ -12037,7 +12113,7 @@ ovnnb_db_run(struct northd_context *ctx, > > sync_address_sets(ctx); > sync_port_groups(ctx, &port_groups); > - sync_meters(ctx); > + sync_meters(ctx, datapaths, &meter_groups); > sync_dns_entries(ctx, datapaths); > > struct ovn_northd_lb *lb; > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema > index 092322ab2..269e3a888 100644 > --- a/ovn-nb.ovsschema > +++ b/ovn-nb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Northbound", > - "version": "5.27.0", > - "cksum": "3507518247 26773", > + "version": "5.28.0", > + "cksum": "610359755 26847", > "tables": { > "NB_Global": { > "columns": { > @@ -264,6 +264,7 @@ > "refType": "strong"}, > "min": 1, > "max": "unlimited"}}, > + "fair": {"type": {"key": "boolean", "min": 0, "max": 1}}, > "external_ids": { > "type": {"key": "string", "value": "string", > "min": 0, "max": "unlimited"}}}, > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 619c0e299..a457b9cee 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -1785,7 +1785,9 @@ > The name of a meter to rate-limit log messages for the ACL. > The string must match the <ref column="name" table="meter"/> > column of a row in the <ref table="Meter"/> table. By > - default, log messages are not rate-limited. > + default, log messages are not rate-limited. In order to ensure > + that the same <ref table="Meter"/> rate limits multiple ACL logs > + separately, set the <ref column="fair" table="meter"/> column. > </p> > </column> > </group> > @@ -2089,6 +2091,18 @@ > </p> > </column> > > + <column name="fair"> > + <p> > + This column is used to further describe the desired behavior > + of the meter when there are multiple references to it. If this > + column is empty or is set to <code>false</code>, the rate will > + be shared across all rows that refer to the same Meter > + <ref column="name" table="meter"/>. Conversely, when this column > + is set to <code>true</code>, each user of the same Meter will be > + rate-limited on its own. > + </p> > + </column> > + > <column name="external_ids"> > See <em>External IDs</em> at the beginning of this document. > </column> > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > index 79d580d3f..11091d8d0 100644 > --- a/tests/ovn-nbctl.at > +++ b/tests/ovn-nbctl.at > @@ -346,7 +346,7 @@ OVN_NBCTL_TEST([ovn_nbctl_meters], [meters], [ > AT_CHECK([ovn-nbctl meter-add meter1 drop 10 kbps]) > AT_CHECK([ovn-nbctl meter-add meter2 drop 3 kbps 2]) > AT_CHECK([ovn-nbctl meter-add meter3 drop 100 kbps 200]) > -AT_CHECK([ovn-nbctl meter-add meter4 drop 10 pktps 30]) > +AT_CHECK([ovn-nbctl --fair meter-add meter4 drop 10 pktps 30]) > > dnl Add duplicate meter name > AT_CHECK([ovn-nbctl meter-add meter1 drop 10 kbps], [1], [], [stderr]) > @@ -382,7 +382,7 @@ meter2: bands: > drop: 3 kbps, 2 kb burst > meter3: bands: > drop: 100 kbps, 200 kb burst > -meter4: bands: > +meter4: (fair) bands: > drop: 10 pktps, 30 packet burst > ]) > > @@ -393,7 +393,7 @@ meter1: bands: > drop: 10 kbps > meter3: bands: > drop: 100 kbps, 200 kb burst > -meter4: bands: > +meter4: (fair) bands: > drop: 10 pktps, 30 packet burst > ]) > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 85f61f357..ef77c5318 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -1813,6 +1813,103 @@ action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implici > > AT_CLEANUP > > +AT_SETUP([ovn -- ACL fair Meters]) > +AT_KEYWORDS([acl log meter fair]) > +ovn_start > + > +check ovn-nbctl ls-add sw0 > +check ovn-nbctl lsp-add sw0 sw0-p1 -- lsp-set-addresses sw0-p1 "50:54:00:00:00:01 10.0.0.11" > +check ovn-nbctl lsp-add sw0 sw0-p2 -- lsp-set-addresses sw0-p2 "50:54:00:00:00:02 10.0.0.12" > +check ovn-nbctl lsp-add sw0 sw0-p3 -- lsp-set-addresses sw0-p3 "50:54:00:00:00:03 10.0.0.13" > + > +check ovn-nbctl meter-add meter_me drop 1 pktps > +nb_meter_uuid=$(fetch_column nb:Meter _uuid name=meter_me) > + > +check ovn-nbctl acl-add sw0 to-lport 1002 'outport == "sw0-p1" && ip4.src == 10.0.0.12' allow > +check ovn-nbctl acl-add sw0 to-lport 1002 'outport == "sw0-p1" && ip4.src == 10.0.0.13' allow > + > +acl1=$(ovn-nbctl --bare --column _uuid,match find acl | grep -B1 '10.0.0.12' | head -1) > +acl2=$(ovn-nbctl --bare --column _uuid,match find acl | grep -B1 '10.0.0.13' | head -1) > +check ovn-nbctl set acl $acl1 log=true severity=alert meter=meter_me name=acl_one > +check ovn-nbctl set acl $acl2 log=true severity=info meter=meter_me name=acl_two > +check ovn-nbctl --wait=sb sync > + > +check_row_count nb:meter 1 > +check_column meter_me nb:meter name > + > +check_acl_lflow() { > + acl_log_name=$1 > + meter_name=$2 > + # echo checking that logical flow for acl log $acl_log_name has $meter_name > + AT_CHECK([ovn-sbctl lflow-list | grep ls_out_acl | \ > + grep "\"${acl_log_name}\"" | \ > + grep -c "meter=\"${meter_name}\""], [0], [1 > +]) > +} > + > +check_meter_by_name() { > + [test "$1" = "NOT"] && { expected_count=0; shift; } || expected_count=1 > + for meter_name in $* ; do > + # echo checking for $expected_count $meter_name in sb meter table > + check_row_count meter $expected_count name=$meter_name > + done > +} > + > +# Make sure 'fair' value properly affects the Meters in SB > +check_meter_by_name meter_me > +check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2} > + > +check ovn-nbctl --wait=sb set Meter $nb_meter_uuid fair=true > +check_meter_by_name meter_me meter_me__${acl1} meter_me__${acl2} > + > +check ovn-nbctl --wait=sb set Meter $nb_meter_uuid fair=false > +check_meter_by_name meter_me > +check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2} > + > +check ovn-nbctl --wait=sb set Meter $nb_meter_uuid fair=true > +check_meter_by_name meter_me meter_me__${acl1} meter_me__${acl2} > + > +# Change template meter and make sure that is reflected on acl meters as well > +template_band=$(fetch_column nb:meter bands name=meter_me) > +check ovn-nbctl --wait=sb set meter_band $template_band rate=123 > +# Make sure that every Meter_Band has the right rate. (ovn-northd > +# creates 3 identical Meter_Band rows, all identical; ovn-northd-ddlog > +# creates just 1. It doesn't matter, they work just as well.) > +n_meter_bands=$(count_rows meter_band) > +AT_FAIL_IF([test "$n_meter_bands" != 1 && test "$n_meter_bands" != 3]) > +check_row_count meter_band $n_meter_bands rate=123 > + > +# Check meter in logical flows for acl logs > +check_acl_lflow acl_one meter_me__${acl1} > +check_acl_lflow acl_two meter_me__${acl2} > + > +# Stop using meter for acl1 > +check ovn-nbctl --wait=sb clear acl $acl1 meter > +check_meter_by_name meter_me meter_me__${acl2} > +check_meter_by_name NOT meter_me__${acl1} > +check_acl_lflow acl_two meter_me__${acl2} > + > +# Remove template Meter should remove all others as well > +check ovn-nbctl --wait=sb meter-del meter_me > +check_row_count meter 0 > +# Check that logical flow remains but uses non-unique meter since fair > +# attribute is lost by the removal of the Meter row. > +check_acl_lflow acl_two meter_me > + > +# Re-add template meter and make sure acl2's meter is back in sb > +check ovn-nbctl --wait=sb --fair meter-add meter_me drop 1 pktps > +check_meter_by_name meter_me meter_me__${acl2} > +check_meter_by_name NOT meter_me__${acl1} > +check_acl_lflow acl_two meter_me__${acl2} > + > +# Remove acl2 > +sw0=$(fetch_column nb:logical_switch _uuid name=sw0) > +check ovn-nbctl --wait=sb remove logical_switch $sw0 acls $acl2 > +check_meter_by_name meter_me > +check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2} > + > +AT_CLEANUP > + > AT_SETUP([datapath requested-tnl-key]) > AT_KEYWORDS([requested tnl tunnel key keys]) > ovn_start > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index b5633c725..11c6b5576 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -617,6 +617,7 @@ QoS commands:\n\ > qos-list SWITCH print QoS rules for SWITCH\n\ > \n\ > Meter commands:\n\ > + [--fair]\n\ > meter-add NAME ACTION RATE UNIT [BURST]\n\ > add a meter\n\ > meter-del [NAME] remove meters\n\ > @@ -2693,7 +2694,12 @@ nbctl_meter_list(struct ctl_context *ctx) > > for (size_t i = 0; i < n_meters; i++) { > meter = meters[i]; > - ds_put_format(&ctx->output, "%s: bands:\n", meter->name); > + ds_put_format(&ctx->output, "%s:", meter->name); > + if (meter->fair) { > + ds_put_format(&ctx->output, " (%s)", > + *meter->fair ? "fair" : "shared"); > + } > + ds_put_format(&ctx->output, " bands:\n"); > > for (size_t j = 0; j < meter->n_bands; j++) { > const struct nbrec_meter_band *band = meter->bands[j]; > @@ -2770,6 +2776,12 @@ nbctl_meter_add(struct ctl_context *ctx) > nbrec_meter_set_name(meter, name); > nbrec_meter_set_unit(meter, unit); > nbrec_meter_set_bands(meter, &band, 1); > + > + /* Fair option */ > + bool fair = shash_find(&ctx->options, "--fair") != NULL; > + if (fair) { > + nbrec_meter_set_fair(meter, &fair, 1); > + } > } > > static void > @@ -6464,7 +6476,7 @@ static const struct ctl_command_syntax nbctl_commands[] = { > > /* meter commands. */ > { "meter-add", 4, 5, "NAME ACTION RATE UNIT [BURST]", NULL, > - nbctl_meter_add, NULL, "", RW }, > + nbctl_meter_add, NULL, "--fair", RW }, > { "meter-del", 0, 1, "[NAME]", NULL, nbctl_meter_del, NULL, "", RW }, > { "meter-list", 0, 0, "", NULL, nbctl_meter_list, NULL, "", RO }, > > -- > 2.18.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/NEWS b/NEWS index a681573aa..18da14b18 100644 --- a/NEWS +++ b/NEWS @@ -10,6 +10,8 @@ Post-v20.09.0 traffic. - Propagate currently processed SB_Global.nb_cfg in ovn-controller to the local OVS DB integration bridge external_ids:ovn-nb-cfg. + - Add "fair" column in Meter table to allow multiple ACL logs to use the + same Meter while being rate-limited independently. OVN v20.09.0 - 28 Sep 2020 -------------------------- diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index d08a8a643..c2176f105 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -5265,8 +5265,46 @@ build_acl_hints(struct ovn_datapath *od, struct hmap *lflows) } } +static const struct nbrec_meter* +fair_meter_lookup_by_name(const struct shash *meter_groups, + const char *meter_name) +{ + const struct nbrec_meter *nb_meter = + meter_name ? shash_find_data(meter_groups, meter_name) : NULL; + if (nb_meter) { + return (nb_meter->fair && *nb_meter->fair) ? nb_meter : NULL; + } + return NULL; +} + +static char* +alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl) +{ + return xasprintf("%s__" UUID_FMT, + acl->meter, UUID_ARGS(&acl->header_.uuid)); +} + static void -build_acl_log(struct ds *actions, const struct nbrec_acl *acl) +build_acl_log_meter(struct ds *actions, const struct nbrec_acl *acl, + const struct shash *meter_groups) +{ + if (!acl->meter) { + return; + } + + /* If ACL log meter uses a fair meter, use unique Meter name. */ + if (fair_meter_lookup_by_name(meter_groups, acl->meter)) { + char *meter_name = alloc_acl_log_unique_meter_name(acl); + ds_put_format(actions, "meter=\"%s\", ", meter_name); + free(meter_name); + } else { + ds_put_format(actions, "meter=\"%s\", ", acl->meter); + } +} + +static void +build_acl_log(struct ds *actions, const struct nbrec_acl *acl, + const struct shash *meter_groups) { if (!acl->log) { return; @@ -5294,9 +5332,7 @@ build_acl_log(struct ds *actions, const struct nbrec_acl *acl) ds_put_cstr(actions, "verdict=allow, "); } - if (acl->meter) { - ds_put_format(actions, "meter=\"%s\", ", acl->meter); - } + build_acl_log_meter(actions, acl, meter_groups); ds_chomp(actions, ' '); ds_chomp(actions, ','); @@ -5307,7 +5343,8 @@ static void build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, enum ovn_stage stage, struct nbrec_acl *acl, struct ds *extra_match, struct ds *extra_actions, - const struct ovsdb_idl_row *stage_hint) + const struct ovsdb_idl_row *stage_hint, + const struct shash *meter_groups) { struct ds match = DS_EMPTY_INITIALIZER; struct ds actions = DS_EMPTY_INITIALIZER; @@ -5319,7 +5356,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, ingress ? ovn_stage_get_table(S_SWITCH_OUT_QOS_MARK) : ovn_stage_get_table(S_SWITCH_IN_L2_LKUP)); - build_acl_log(&actions, acl); + build_acl_log(&actions, acl, meter_groups); if (extra_match->length > 0) { ds_put_format(&match, "(%s) && ", extra_match->string); } @@ -5344,7 +5381,8 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, static void consider_acl(struct hmap *lflows, struct ovn_datapath *od, - struct nbrec_acl *acl, bool has_stateful) + struct nbrec_acl *acl, bool has_stateful, + const struct shash *meter_groups) { bool ingress = !strcmp(acl->direction, "from-lport") ? true :false; enum ovn_stage stage = ingress ? S_SWITCH_IN_ACL : S_SWITCH_OUT_ACL; @@ -5358,7 +5396,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, * associated conntrack entry and would return "+invalid". */ if (!has_stateful) { struct ds actions = DS_EMPTY_INITIALIZER; - build_acl_log(&actions, acl); + build_acl_log(&actions, acl, meter_groups); ds_put_cstr(&actions, "next;"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -5384,7 +5422,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_put_format(&match, REGBIT_ACL_HINT_ALLOW_NEW " == 1 && (%s)", acl->match); ds_put_cstr(&actions, REGBIT_CONNTRACK_COMMIT" = 1; "); - build_acl_log(&actions, acl); + build_acl_log(&actions, acl, meter_groups); ds_put_cstr(&actions, "next;"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -5403,7 +5441,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_put_format(&match, REGBIT_ACL_HINT_ALLOW " == 1 && (%s)", acl->match); - build_acl_log(&actions, acl); + build_acl_log(&actions, acl, meter_groups); ds_put_cstr(&actions, "next;"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -5428,10 +5466,10 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_put_cstr(&match, REGBIT_ACL_HINT_DROP " == 1"); if (!strcmp(acl->action, "reject")) { build_reject_acl_rules(od, lflows, stage, acl, &match, - &actions, &acl->header_); + &actions, &acl->header_, meter_groups); } else { ds_put_format(&match, " && (%s)", acl->match); - build_acl_log(&actions, acl); + build_acl_log(&actions, acl, meter_groups); ds_put_cstr(&actions, "/* drop */"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -5455,10 +5493,10 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_put_cstr(&actions, "ct_commit { ct_label.blocked = 1; }; "); if (!strcmp(acl->action, "reject")) { build_reject_acl_rules(od, lflows, stage, acl, &match, - &actions, &acl->header_); + &actions, &acl->header_, meter_groups); } else { ds_put_format(&match, " && (%s)", acl->match); - build_acl_log(&actions, acl); + build_acl_log(&actions, acl, meter_groups); ds_put_cstr(&actions, "/* drop */"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -5471,9 +5509,9 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, * logical flow action in all cases. */ if (!strcmp(acl->action, "reject")) { build_reject_acl_rules(od, lflows, stage, acl, &match, - &actions, &acl->header_); + &actions, &acl->header_, meter_groups); } else { - build_acl_log(&actions, acl); + build_acl_log(&actions, acl, meter_groups); ds_put_cstr(&actions, "/* drop */"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -5553,7 +5591,7 @@ build_port_group_lswitches(struct northd_context *ctx, struct hmap *pgs, static void build_acls(struct ovn_datapath *od, struct hmap *lflows, - struct hmap *port_groups) + struct hmap *port_groups, const struct shash *meter_groups) { bool has_stateful = (has_stateful_acl(od) || has_lb_vip(od)); @@ -5656,13 +5694,14 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, /* Ingress or Egress ACL Table (Various priorities). */ for (size_t i = 0; i < od->nbs->n_acls; i++) { struct nbrec_acl *acl = od->nbs->acls[i]; - consider_acl(lflows, od, acl, has_stateful); + consider_acl(lflows, od, acl, has_stateful, meter_groups); } struct ovn_port_group *pg; HMAP_FOR_EACH (pg, key_node, port_groups) { if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) { for (size_t i = 0; i < pg->nb_pg->n_acls; i++) { - consider_acl(lflows, od, pg->nb_pg->acls[i], has_stateful); + consider_acl(lflows, od, pg->nb_pg->acls[i], has_stateful, + meter_groups); } } } @@ -7311,7 +7350,7 @@ build_lswitch_lflows_pre_acl_and_acl(struct ovn_datapath *od, build_pre_lb(od, lflows, meter_groups, lbs); build_pre_stateful(od, lflows); build_acl_hints(od, lflows); - build_acls(od, lflows, port_groups); + build_acls(od, lflows, port_groups, meter_groups); build_qos(od, lflows); build_lb(od, lflows); build_stateful(od, lflows, lbs); @@ -11502,12 +11541,50 @@ done: return need_update; } +static void +sync_meters_iterate_nb_meter(struct northd_context *ctx, + const char *meter_name, + const struct nbrec_meter *nb_meter, + struct shash *sb_meters) +{ + bool new_sb_meter = false; + + const struct sbrec_meter *sb_meter = shash_find_and_delete(sb_meters, + meter_name); + if (!sb_meter) { + sb_meter = sbrec_meter_insert(ctx->ovnsb_txn); + sbrec_meter_set_name(sb_meter, 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); +} + /* 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. + * OVN_Southbound. Additionally, ACL logs that use fair meters have + * a private copy of its meter in the SB table. */ static void -sync_meters(struct northd_context *ctx) +sync_meters(struct northd_context *ctx, struct hmap *datapaths, + struct shash *meter_groups) { struct shash sb_meters = SHASH_INITIALIZER(&sb_meters); @@ -11518,33 +11595,32 @@ sync_meters(struct northd_context *ctx) const struct nbrec_meter *nb_meter; NBREC_METER_FOR_EACH (nb_meter, ctx->ovnnb_idl) { - bool new_sb_meter = false; + sync_meters_iterate_nb_meter(ctx, nb_meter->name, nb_meter, + &sb_meters); + } - 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; + /* + * In addition to creating Meters in the SB from the block above, check + * and see if additional rows are needed to get ACLs logs individually + * rate-limited. + */ + struct ovn_datapath *od; + HMAP_FOR_EACH (od, key_node, datapaths) { + if (!od->nbs) { + continue; } - - 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); + for (size_t i = 0; i < od->nbs->n_acls; i++) { + struct nbrec_acl *acl = od->nbs->acls[i]; + nb_meter = fair_meter_lookup_by_name(meter_groups, acl->meter); + if (!nb_meter) { + continue; } - sbrec_meter_set_bands(sb_meter, sb_bands, nb_meter->n_bands); - free(sb_bands); - } - sbrec_meter_set_unit(sb_meter, nb_meter->unit); + char *meter_name = alloc_acl_log_unique_meter_name(acl); + sync_meters_iterate_nb_meter(ctx, meter_name, nb_meter, + &sb_meters); + free(meter_name); + } } struct shash_node *node, *next; @@ -12037,7 +12113,7 @@ ovnnb_db_run(struct northd_context *ctx, sync_address_sets(ctx); sync_port_groups(ctx, &port_groups); - sync_meters(ctx); + sync_meters(ctx, datapaths, &meter_groups); sync_dns_entries(ctx, datapaths); struct ovn_northd_lb *lb; diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema index 092322ab2..269e3a888 100644 --- a/ovn-nb.ovsschema +++ b/ovn-nb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Northbound", - "version": "5.27.0", - "cksum": "3507518247 26773", + "version": "5.28.0", + "cksum": "610359755 26847", "tables": { "NB_Global": { "columns": { @@ -264,6 +264,7 @@ "refType": "strong"}, "min": 1, "max": "unlimited"}}, + "fair": {"type": {"key": "boolean", "min": 0, "max": 1}}, "external_ids": { "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}}, diff --git a/ovn-nb.xml b/ovn-nb.xml index 619c0e299..a457b9cee 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -1785,7 +1785,9 @@ The name of a meter to rate-limit log messages for the ACL. The string must match the <ref column="name" table="meter"/> column of a row in the <ref table="Meter"/> table. By - default, log messages are not rate-limited. + default, log messages are not rate-limited. In order to ensure + that the same <ref table="Meter"/> rate limits multiple ACL logs + separately, set the <ref column="fair" table="meter"/> column. </p> </column> </group> @@ -2089,6 +2091,18 @@ </p> </column> + <column name="fair"> + <p> + This column is used to further describe the desired behavior + of the meter when there are multiple references to it. If this + column is empty or is set to <code>false</code>, the rate will + be shared across all rows that refer to the same Meter + <ref column="name" table="meter"/>. Conversely, when this column + is set to <code>true</code>, each user of the same Meter will be + rate-limited on its own. + </p> + </column> + <column name="external_ids"> See <em>External IDs</em> at the beginning of this document. </column> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at index 79d580d3f..11091d8d0 100644 --- a/tests/ovn-nbctl.at +++ b/tests/ovn-nbctl.at @@ -346,7 +346,7 @@ OVN_NBCTL_TEST([ovn_nbctl_meters], [meters], [ AT_CHECK([ovn-nbctl meter-add meter1 drop 10 kbps]) AT_CHECK([ovn-nbctl meter-add meter2 drop 3 kbps 2]) AT_CHECK([ovn-nbctl meter-add meter3 drop 100 kbps 200]) -AT_CHECK([ovn-nbctl meter-add meter4 drop 10 pktps 30]) +AT_CHECK([ovn-nbctl --fair meter-add meter4 drop 10 pktps 30]) dnl Add duplicate meter name AT_CHECK([ovn-nbctl meter-add meter1 drop 10 kbps], [1], [], [stderr]) @@ -382,7 +382,7 @@ meter2: bands: drop: 3 kbps, 2 kb burst meter3: bands: drop: 100 kbps, 200 kb burst -meter4: bands: +meter4: (fair) bands: drop: 10 pktps, 30 packet burst ]) @@ -393,7 +393,7 @@ meter1: bands: drop: 10 kbps meter3: bands: drop: 100 kbps, 200 kb burst -meter4: bands: +meter4: (fair) bands: drop: 10 pktps, 30 packet burst ]) diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 85f61f357..ef77c5318 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -1813,6 +1813,103 @@ action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implici AT_CLEANUP +AT_SETUP([ovn -- ACL fair Meters]) +AT_KEYWORDS([acl log meter fair]) +ovn_start + +check ovn-nbctl ls-add sw0 +check ovn-nbctl lsp-add sw0 sw0-p1 -- lsp-set-addresses sw0-p1 "50:54:00:00:00:01 10.0.0.11" +check ovn-nbctl lsp-add sw0 sw0-p2 -- lsp-set-addresses sw0-p2 "50:54:00:00:00:02 10.0.0.12" +check ovn-nbctl lsp-add sw0 sw0-p3 -- lsp-set-addresses sw0-p3 "50:54:00:00:00:03 10.0.0.13" + +check ovn-nbctl meter-add meter_me drop 1 pktps +nb_meter_uuid=$(fetch_column nb:Meter _uuid name=meter_me) + +check ovn-nbctl acl-add sw0 to-lport 1002 'outport == "sw0-p1" && ip4.src == 10.0.0.12' allow +check ovn-nbctl acl-add sw0 to-lport 1002 'outport == "sw0-p1" && ip4.src == 10.0.0.13' allow + +acl1=$(ovn-nbctl --bare --column _uuid,match find acl | grep -B1 '10.0.0.12' | head -1) +acl2=$(ovn-nbctl --bare --column _uuid,match find acl | grep -B1 '10.0.0.13' | head -1) +check ovn-nbctl set acl $acl1 log=true severity=alert meter=meter_me name=acl_one +check ovn-nbctl set acl $acl2 log=true severity=info meter=meter_me name=acl_two +check ovn-nbctl --wait=sb sync + +check_row_count nb:meter 1 +check_column meter_me nb:meter name + +check_acl_lflow() { + acl_log_name=$1 + meter_name=$2 + # echo checking that logical flow for acl log $acl_log_name has $meter_name + AT_CHECK([ovn-sbctl lflow-list | grep ls_out_acl | \ + grep "\"${acl_log_name}\"" | \ + grep -c "meter=\"${meter_name}\""], [0], [1 +]) +} + +check_meter_by_name() { + [test "$1" = "NOT"] && { expected_count=0; shift; } || expected_count=1 + for meter_name in $* ; do + # echo checking for $expected_count $meter_name in sb meter table + check_row_count meter $expected_count name=$meter_name + done +} + +# Make sure 'fair' value properly affects the Meters in SB +check_meter_by_name meter_me +check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2} + +check ovn-nbctl --wait=sb set Meter $nb_meter_uuid fair=true +check_meter_by_name meter_me meter_me__${acl1} meter_me__${acl2} + +check ovn-nbctl --wait=sb set Meter $nb_meter_uuid fair=false +check_meter_by_name meter_me +check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2} + +check ovn-nbctl --wait=sb set Meter $nb_meter_uuid fair=true +check_meter_by_name meter_me meter_me__${acl1} meter_me__${acl2} + +# Change template meter and make sure that is reflected on acl meters as well +template_band=$(fetch_column nb:meter bands name=meter_me) +check ovn-nbctl --wait=sb set meter_band $template_band rate=123 +# Make sure that every Meter_Band has the right rate. (ovn-northd +# creates 3 identical Meter_Band rows, all identical; ovn-northd-ddlog +# creates just 1. It doesn't matter, they work just as well.) +n_meter_bands=$(count_rows meter_band) +AT_FAIL_IF([test "$n_meter_bands" != 1 && test "$n_meter_bands" != 3]) +check_row_count meter_band $n_meter_bands rate=123 + +# Check meter in logical flows for acl logs +check_acl_lflow acl_one meter_me__${acl1} +check_acl_lflow acl_two meter_me__${acl2} + +# Stop using meter for acl1 +check ovn-nbctl --wait=sb clear acl $acl1 meter +check_meter_by_name meter_me meter_me__${acl2} +check_meter_by_name NOT meter_me__${acl1} +check_acl_lflow acl_two meter_me__${acl2} + +# Remove template Meter should remove all others as well +check ovn-nbctl --wait=sb meter-del meter_me +check_row_count meter 0 +# Check that logical flow remains but uses non-unique meter since fair +# attribute is lost by the removal of the Meter row. +check_acl_lflow acl_two meter_me + +# Re-add template meter and make sure acl2's meter is back in sb +check ovn-nbctl --wait=sb --fair meter-add meter_me drop 1 pktps +check_meter_by_name meter_me meter_me__${acl2} +check_meter_by_name NOT meter_me__${acl1} +check_acl_lflow acl_two meter_me__${acl2} + +# Remove acl2 +sw0=$(fetch_column nb:logical_switch _uuid name=sw0) +check ovn-nbctl --wait=sb remove logical_switch $sw0 acls $acl2 +check_meter_by_name meter_me +check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2} + +AT_CLEANUP + AT_SETUP([datapath requested-tnl-key]) AT_KEYWORDS([requested tnl tunnel key keys]) ovn_start diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c index b5633c725..11c6b5576 100644 --- a/utilities/ovn-nbctl.c +++ b/utilities/ovn-nbctl.c @@ -617,6 +617,7 @@ QoS commands:\n\ qos-list SWITCH print QoS rules for SWITCH\n\ \n\ Meter commands:\n\ + [--fair]\n\ meter-add NAME ACTION RATE UNIT [BURST]\n\ add a meter\n\ meter-del [NAME] remove meters\n\ @@ -2693,7 +2694,12 @@ nbctl_meter_list(struct ctl_context *ctx) for (size_t i = 0; i < n_meters; i++) { meter = meters[i]; - ds_put_format(&ctx->output, "%s: bands:\n", meter->name); + ds_put_format(&ctx->output, "%s:", meter->name); + if (meter->fair) { + ds_put_format(&ctx->output, " (%s)", + *meter->fair ? "fair" : "shared"); + } + ds_put_format(&ctx->output, " bands:\n"); for (size_t j = 0; j < meter->n_bands; j++) { const struct nbrec_meter_band *band = meter->bands[j]; @@ -2770,6 +2776,12 @@ nbctl_meter_add(struct ctl_context *ctx) nbrec_meter_set_name(meter, name); nbrec_meter_set_unit(meter, unit); nbrec_meter_set_bands(meter, &band, 1); + + /* Fair option */ + bool fair = shash_find(&ctx->options, "--fair") != NULL; + if (fair) { + nbrec_meter_set_fair(meter, &fair, 1); + } } static void @@ -6464,7 +6476,7 @@ static const struct ctl_command_syntax nbctl_commands[] = { /* meter commands. */ { "meter-add", 4, 5, "NAME ACTION RATE UNIT [BURST]", NULL, - nbctl_meter_add, NULL, "", RW }, + nbctl_meter_add, NULL, "--fair", RW }, { "meter-del", 0, 1, "[NAME]", NULL, nbctl_meter_del, NULL, "", RW }, { "meter-list", 0, 0, "", NULL, nbctl_meter_list, NULL, "", RO },
When multiple ACL rows use the same Meter for log rate-limiting, the 'noisiest' ACL matches may completely consume the Meter and shadow other ACL logs. This patch introduces a column in northbound's Meter table called fair that allows for a Meter to rate-limit each ACL log separately. The change is backward compatible. Based on the fair column of a Meter row, northd will turn a single Meter in the NB into multiple Meters in the SB by appending the ACL uuid to its name. It will also make action in logical flow use the unique Meter name for each affected ACL log. In order to make the Meter behave as described, add Meter with this option: ovn-nbctl --fair meter-add ${meter_name} drop ${rate} ${unit} Example: ovn-nbctl --fair meter-add meter_me drop 1 pktps Reported-by: Flavio Fernandes <flavio@flaviof.com> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-October/050769.html Suggested-by: Ben Pfaff <blp@ovn.org> Suggested-by: Dumitru Ceara <dceara@redhat.com> Suggested-by: Numan Siddique <numans@ovn.org> Signed-off-by: Flavio Fernandes <flavio@flaviof.com> --- This change can be tracked in the following github clone/branch: https://github.com/flavio-fernandes/ovn/commits/acl-meters.v6 NEWS | 2 + northd/ovn-northd.c | 168 ++++++++++++++++++++++++++++++------------ ovn-nb.ovsschema | 5 +- ovn-nb.xml | 16 +++- tests/ovn-nbctl.at | 6 +- tests/ovn-northd.at | 97 ++++++++++++++++++++++++ utilities/ovn-nbctl.c | 16 +++- 7 files changed, 256 insertions(+), 54 deletions(-)