[ovs-dev] [PATCH v5 ovn 1/2] northd: Fair ACL log meters.
Flavio Fernandes
flavio at flaviof.com
Fri Nov 20 18:41:55 UTC 2020
> On Nov 19, 2020, at 4:01 AM, Numan Siddique <numans at ovn.org> wrote:
>
> On Mon, Nov 16, 2020 at 9:20 PM Flavio Fernandes <flavio at flaviof.com <mailto:flavio at 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 at flaviof.com>
>> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-October/050769.html
>> Suggested-by: Dumitru Ceara <dceara at redhat.com>
>> Suggested-by: Ben Pfaff <blp at ovn.org>
>> Signed-off-by: Flavio Fernandes <flavio at flaviof.com>
>> ---
>>
>> This change can be tracked in the following github clone/branch:
>>
>> https://github.com/flavio-fernandes/ovn/commits/acl-meters.v5.ddlog
>>
>> v4 -> v5:
>> * Rebase https://github.com/blp/ovs-reviews/ddlog10 (f56dafa83).
>
> Hi Flavio,
>
> Thanks for v5. This patch doesn't apply cleanly on the latest master.
>
> I'd suggest to post the patch 1 was an independent patch rebased on on
> top of latest master
>
> I think you can submit the patch 2 once the patch 1 is accepted ,
> either as part of ddlog patch series
> or after the ddlog patches are merged.
Okay! ;)
I submitted v6 as a standalone, based on master with changes from your
comments below:
https://patchwork.ozlabs.org/project/ovn/patch/20201120182211.2646-2-flavio@flaviof.com/
>
> Few comments below.
>
> Thanks
> Numan
>
>
>> v3 -> v4:
>> * Rename 'shared meters' to 'fair meters' to keep it less confusing.
>> * NB schema change: Add column in Meter to track which meters are 'fair'.
>> * Add optional '--fair' flag to ovn-nbctl meter-add command.
>> v2 -> v3:
>> * Use recently introduced testing helpers (4afe409e9).
>> v1 -> v2:
>> * Rebase master b38e10f4b.
>>
>> NEWS | 2 +
>> northd/ovn-northd.c | 184 ++++++++++++++++++++++++++++++------------
>> 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, 268 insertions(+), 58 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index 04b75e68c..0965dff4f 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -14,6 +14,8 @@ Post-v20.09.0
>> server.
>> - Support other_config:vlan-passthru=true to allow VLAN tagged incoming
>> traffic.
>> + - 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 4d4190cb9..cee8abce9 100644
>> --- a/northd/ovn-northd.c
>> +++ b/northd/ovn-northd.c
>> @@ -5353,8 +5353,45 @@ build_acl_hints(struct ovn_datapath *od, struct hmap *lflows)
>> }
>> }
>>
>> +static bool
>> +is_a_fair_meter(const struct shash *meter_groups, const char *meter_name)
>> +{
>> + const struct nbrec_meter *nb_meter =
>> + shash_find_data(meter_groups, meter_name);
>> + if (nb_meter && nb_meter->fair) {
>> + return *nb_meter->fair;
>> + }
>
> I would suggest to change the above 'if' as
>
> if (nb_meter) {
> return nb_meter->n_fair && *nb_meter->fair;
> }
Ack. Turns out I needed some further mods to this function to return the nb_meter,
so I renamed it to "fair_meter_lookup_by_name" and made the statement change
there.
<snip>
>> static void
>> -sync_meters(struct northd_context *ctx)
>> +sync_meters(struct northd_context *ctx, struct hmap *datapaths)
>> {
>> struct shash sb_meters = SHASH_INITIALIZER(&sb_meters);
>>
>> @@ -11649,33 +11752,14 @@ 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;
>> -
>> - 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;
>> - }
>> -
>> - 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);
>> + sync_meters_iterate_nb_meter(ctx, nb_meter->name, nb_meter,
>> + &sb_meters);
>> + /*
>> + * In addition to creating a 'non-fair' Meter in the SB from
>> + * the line above, check and see if additional rows are needed
>> + * to get ACLs logs individually rate-limited.
>> + */
>> + sync_acl_fair_meters(ctx, datapaths, nb_meter, &sb_meters);
>> }
>
> In the above NBREC_METER_FOR_EACH loop, for each nbrec meter we will
> run the od loop in
> sync_acl_fair_meters. I think we can optimize this a bit.
>
> I'd suggest
> - not to call sync_acl_fair_meters() in the above NBREC_METER_FOR_EACH loop
> - Run a datapaths loop and for each acl which references a meter,
> get the nbrec_meter from
> the shash - meter_groups (built by build_meter_groups()) and call
> sync_acl_fair_meters()
>
> What do you think ?
I like it. I made the changes in v6. Also, with the changes you proposed
it made no sense to have a separate function. So I simply moved it "here".
<snip>
>>
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index 7d73b0b83..21ce20c87 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -1920,6 +1920,103 @@ sw1flows3: table=5 (ls_out_acl ), priority=2003 , match=((reg0[[9]] ==
>> AT_CLEANUP
>> ])
>>
>> +AT_SETUP([ovn-northd -- ACL fair Meters])
>> +AT_KEYWORDS([acl log meter fair])
>> +ovn_start
>> +
>> +ovn-nbctl ls-add sw0
>> +ovn-nbctl lsp-add sw0 sw0-p1 -- lsp-set-addresses sw0-p1 "50:54:00:00:00:01 10.0.0.11"
>> +ovn-nbctl lsp-add sw0 sw0-p2 -- lsp-set-addresses sw0-p2 "50:54:00:00:00:02 10.0.0.12"
>> +ovn-nbctl lsp-add sw0 sw0-p3 -- lsp-set-addresses sw0-p3 "50:54:00:00:00:03 10.0.0.13"
>> +
>> +ovn-nbctl meter-add meter_me drop 1 pktps
>> +nb_meter_uuid=$(fetch_column nb:Meter _uuid name=meter_me)
>> +
>> +ovn-nbctl acl-add sw0 to-lport 1002 'outport == "sw0-p1" && ip4.src == 10.0.0.12' allow
>> +ovn-nbctl acl-add sw0 to-lport 1002 'outport == "sw0-p1" && ip4.src == 10.0.0.13' allow
>
> For all the ovn-nbctl commands in this test case, I'd suggest to wrap
> it in the helper test function - check.
Done.
Thank you, Numan!
-- flaviof
More information about the dev
mailing list