[ovs-dev] ddlog for ACL log meters (was: [PATCH v3 ovn 1/1] northd: Enhance the implementation of ACL log meters.)

Flavio Fernandes flavio at flaviof.com
Mon Nov 9 14:53:16 UTC 2020


[inline]

> On Nov 7, 2020, at 4:39 PM, Ben Pfaff <blp at ovn.org> wrote:
> 
> On Tue, Nov 03, 2020 at 10:18:34PM +0000, Flavio Fernandes 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 an option in NB_Global that allows for a
>> Meter to rate-limit each ACL log separately.
>> 
>> The change is backward compatible. Based on a well known option, 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 this option:
>> 
>>  ovn-nbctl set nb_global . options:acl_shared_log_meters="${meter_name}"
>> 
>> If more than one Meter is wanted, simply use a comma to separate each name.
>> 
>> Reported-by: Flavio Fernandes <flavio at flaviof.com>
>> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-October/050769.html
>> Signed-off-by: Flavio Fernandes <flavio at flaviof.com>
> 
> I'm not sure I like the overall design here.  This option isn't going to
> scale very well to many of these meters, since they'd all need to be
> shoved into a single comma-separated list.  Another way might be to add
> a column to the ACL table to mark a meter as shared or private.  Or
> there could be a name convention, e.g. prefix with "shared_" to make it
> shared.

Understood. I think I tried "too hard" to avoid making changes to the northbound schema, but
you bring a valid concern about scalability. Adding a "bool" to the ACL row, or to the Meter row
would definitely make this more scalable. I would like to ask for opinion from you on the following
choices (not listed in any order). You == Ben and the rest of the ovn core developers and users.

A) add a bool column to ACL row called "fair_log_meter" (please help me with the name);
B) add a bool column to Meter row called ^same as above^;
C) change parsing of the value of the global to allow for up to one Meter name
D) change parsing of the value of the global to allow for up to a constant Meter names
E) have an implict behavior based on the name of the Meter "shared_", so that multiple meters are created in the SB as needed.
F) same as 'E', but use a different prefix str
G) any other good approach?

> 
> I don't really understand the vocabulary here, either.  I would have
> guessed that "shared" would mean that all the ACLs with a given meter
> name share a single southbound Meter, but it's the opposite: a "shared"
> meter has a different southbound Meter for each ACL.

Yes, good point. I was seeing this change mostly from the Northbound perspective,
but you are right that this is a confusing name looking at what happens at the SB.
How about we use the word "fair" instead? So, something like "acl_fair_log_meters".
Any suggestions on a better name are very welcome.

> 
> OK, all that is quibbling.  You asked me to help with the ddlog part.  I
> did that, and let me explain it.


I am super grateful for the help below. I will defer going over that in this
message to focus on the topics above. Regardless, this is super useful for
folks [like me] to ramp up on ddlog-northd.

Best,

-- flaviof

> 
> I'll start with a way that the ddlog implementation I built differs from
> the C implementation.  That's in the southbound Meter_Band table.  The C
> implementation creates Meter_Band rows per meter instance; if a "shared"
> meter has 3 instances, it'll create 3 copies of the Meter_Band rows.
> There's nothing valuable about the extra instances, since they are all
> the same, and ovn-controller only looks at the values in the rows and
> doesn't care that they're different rows.  It's slightly easier not to
> create the extra instances in the ddlog implementation, so I just made
> it create one.  So this existing code in ovn_northd.dl can stay:
> 
>    /* Meter_Band table */
>    for (mb in nb::Meter_Band) {
>        sb::Out_Meter_Band(._uuid = mb._uuid,
>                          .action = mb.action,
>                          .rate = mb.rate,
>                          .burst_size = mb.burst_size)
>    }
> 
> The new test does check that there are three (identical) Meter_Band
> rows.  I updated the test to be more flexible, so that it expects 1 or 3
> rows all with the correct rate:
> 
>    # 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
> 
> OK, now for what does need to change in the DDlog code.  First, we need
> to extract the set of shared meters from NB_Global.  There's a few ways
> we could do that.  One way would be to put them into a relation, one
> shared meter name per row.  Then later on we could do joins against that
> relation to figure out whether a meter was shared.  That would be the
> best way, probably, if there might be hundreds or thousands of these
> things.  I guess there won't be, though, since they're all going to be
> glommed into a single comma-separated value.  Instead, I decided that it
> would be easier to have a relation with just a single row with a single
> column, typed as "set of string".  It's easy to do that:
> 
>    relation AclSharedLogMeters0[Set<string>]
>    AclSharedLogMeters0[meters] :-
>        nb in nb::NB_Global(),
>        Some{var s} = nb.options.get("acl_shared_log_meters"),
>        var meters = s.split(",").to_set().
> 
> Note the use of [] instead of () for this relation.  () would be if I
> wanted the relation to contain rows whose values are structs with named
> columns, which is kind of the most common case, but I just want it to
> have a single unnamed value so this is slightly easier (I don't have to
> invent a name for the column).
> 
> We're almost there for the set of shared meters.  There is, however, the
> possibility that NB_Global.options doesn't have an
> "acl_shared_log_meters" key in it.  In that case, the relation I just
> invented would be empty, so the joins I want to do on it later would
> fail to work the way I want.  What I want in that case is for the
> relation to have a single row whose value is the empty set.  The easiest
> way to get that is to bootstrap another relation off of the one I just
> made, like this:
> 
>    relation AclSharedLogMeters[Set<string>]
>    AclSharedLogMeters[meters] :- AclSharedLogMeters0[meters].
>    AclSharedLogMeters[set_empty()] :-
>        Unit(),
>        not AclSharedLogMeters0[_].
> 
> This says that, if AclSharedLogMeters0 has a value, then so does
> AclSharedLogMeters.  And if AclSharedLogMeters0 has no values, then
> AclSharedLogMeters has row with the empty set in it.  (Unit() is here
> because DDlog doesn't allow a negative ("not") clause as the first
> clause in a production.  We're talking about getting rid of the need for
> that sometime, since this kind of construction is a common idiom in
> DDlog.)
> 
> Now we can use the set of shared meters.  We'll need to pass it into the
> DDlog build_acl_log() function.  We'll add it as a parameter to that
> function:
> 
>    -function build_acl_log(acl: nb::ACL): string =
>    +function build_acl_log(acl: nb::ACL, shared_meters: Set<string>): string =
> 
> and then use it inside the function:
> 
>             match (acl.meter) {
>                 None -> (),
>    -            Some{meter} -> vec_push(strs, "meter=${json_string_escape(meter)}")
>    +            Some{meter} -> {
>    +                var s = if (shared_meters.contains(meter)) {
>    +                    "${meter}__${uuid2str(acl._uuid)}"
>    +                } else {
>    +                    meter
>    +                };
>    +                vec_push(strs, "meter=${json_string_escape(s)}")
>    +            }
>             };
> 
> and then pass in the value from each caller by adding a join against
> AclSharedLogMeters.  Here's one example (ignoring indentation changes):
> 
>    +for (AclSharedLogMeters[shared_meters]) {
>         for (Reject(lsuuid, pipeline, stage, acl, extra_match_, extra_actions_)) {
>    ...
>    -        var acl_log = build_acl_log(acl) in {
>    +        var acl_log = build_acl_log(acl, shared_meters) in {
>    ...
>             }
>         }
>    +}
> 
> Finally, we need to create new southbound Meter records for the "shared"
> meters.  I'm actually a bit confused about this.  I would have thought
> that "shared" meters wouldn't need the Meter records whose names are
> un-suffixed by __<uuid>, but the C implementation appears to always
> create them.  So I left in place the existing DDlog code that just
> copies from nb::Meter to sb::Out_Meter:
> 
>    /* Meter table */
>    for (meter in nb::Meter) {
>        sb::Out_Meter(._uuid = meter._uuid,
>                     .name = meter.name,
>                     .unit = meter.unit,
>                     .bands = meter.bands)
>    }
> 
> and just added another production that generates the suffixed ones:
> 
>    sb::Out_Meter(._uuid = hash128(meter_uuid ++ acl_uuid),
>                  .name = "${name}__${uuid2str(acl_uuid)}",
>                  .unit = unit,
>                  .bands = bands) :-
>        AclSharedLogMeters[names],
>        var name = FlatMap(names),
>        nb::Meter(._uuid = meter_uuid, .name = name, .unit = unit, .bands = bands),
>        nb::ACL(._uuid = acl_uuid, .meter = Some{name}).
> 
> The above might need some explanation for those relatively new to
> Datalog or DDlog.  First, the style.  This uses Datalog-style syntax
> (output :- input) instead of the FLWOR-style syntax (for (input) {
> output }).  One reason for that is because the FLWOR-style syntax
> currently doesn't support FlatMap, which we need in that case.  Maybe
> DDlog will add support for it later (we've talked about it).  Another
> reason is that, after working with DDlog for some months, I've come to
> find the Datalog-style syntax better for most purposes than the FLWOR
> syntax (it doesn't result in cascading indentations, for instance).
> 
> Anyway, how do we read this?  I usually start by skipping to the clauses
> after the :-.  Each of these is sort of equivalent to a for loop.  You
> can read them like this:
> 
>  AclSharedLogMeters[names],
> 
>    "For every row in AclSharedLogMeters, and call its value 'names'".
>    By design, there is exactly one row in AclSharedLogMeters, so this
>    just grabs its value as 'names'.  Its type is "set of string".
> 
>  var name = FlatMap(names),
> 
>    "For every value in 'names', and call the value 'name'".  This means
>    that everything after this is repeated for every string in 'names'.
>    (It also unbinds 'names'; you can't use that again after this
>    point.)  If 'names' was empty (i.e. if there were no shared ACL
>    meters), then it's an iteration over 0 elements, and we're done.
> 
>  nb::Meter(._uuid = meter_uuid, .name = name, .unit = unit, .bands = bands),
> 
>    "For every row in the northbound Meter table whose 'name' column
>    equals 'name'; also, call the meter's _uuid column 'meter_uuid' (and
>    so on)".  This is a join against the northbound Meter table on
>    column 'name'.
> 
>    This syntax can be confusing at first because the .<x> = <y> syntax
>    has two different meanings.  For _uuid, unit, and bands, it names
>    the value in the column.  For name, it performs a join against the
>    existing value of name.  If that's confusing, consider a different
>    situation where we wanted to join against, not 'name' itself, but
>    "shared_<name>".  We could write this as:
> 
>       nb::Meter(._uuid = meter_uuid,
>                 .name = "shared_" ++ name,
>                 .unit = unit,
>                 .bands = bands),
> 
>    On the other hand, we could not write expressions here in terms of
>    meter_uuid or unit or bands, because none of them has been bound to
>    a value yet; this join is what binds them.
> 
>    (Yes, this is confusing.  It's inherited from Datalog.  Maybe we
>    will invent better syntax someday.)
> 
>    Here's another way we could have written this, using an expression
>    to make the join really explicit:
> 
>      nb::Meter(._uuid = meter_uuid,
>                .name = meter_name,
>                .unit = unit,
>                .bands = bands),
>      meter_name == name,
> 
>  nb::ACL(._uuid = acl_uuid, .meter = Some{name}).
> 
>    "For every row in the northbound ACL table, where the 'meter' column
>    has 'name' in it, call the _uuid column's value 'acl_uuid'".  This
>    is a join against the northbound ACL table on the 'meter' column.
>    The 'meter' column has type Option<string>, where Option is defined
>    in differential-datalog/lib/ddlog_std.dl as:
> 
>      typedef Option<'A> = None
>                         | Some{x: 'A}
> 
> OK, so the above describes the join we're doing, which amounts to
> finding every ACL that names a shared meter.  Then we skip back up to
> the part of the production before the :-.  It says:
> 
>  sb::Out_Meter(._uuid = hash128(meter_uuid ++ acl_uuid),
>                .name = "${name}__${uuid2str(acl_uuid)}",
>                .unit = unit,
>                .bands = bands) :-
> 
> This produces a row in the southbound output ("Out_") Meter table.  We
> have to construct the _uuid ourselves in some unique way; a hash of the
> meter and the ACL seems reasonable.  We also construct the name
> according to the same algorithm used in the C version.  (My first
> attempt at this omitted the call to uuid2str(), which was wrong because,
> in the DDlog version, UUIDs are just 128-bit integers, which format as
> decimal integers by default.  There's nothing really wrong with that
> except that the test expected them to be formatted in the canonical way
> for UUIDs.)
> 
> Here's the incremental patch to add ddlog support.  It is bigger than
> one might expect because of the increased indentation in a couple of
> blocks in ovn_northd.dl.  After that, the revised patch overall.
> 
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index 23e140148c35..135ff559520b 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -27,6 +27,19 @@ output relation Warning[string]
> 
> index Logical_Flow_Index() on sb::Out_Logical_Flow()
> 
> +/* Single-row relation that contains the set of shared meters. */
> +relation AclSharedLogMeters[Set<string>]
> +AclSharedLogMeters[meters] :- AclSharedLogMeters0[meters].
> +AclSharedLogMeters[set_empty()] :-
> +    Unit(),
> +    not AclSharedLogMeters0[_].
> +
> +relation AclSharedLogMeters0[Set<string>]
> +AclSharedLogMeters0[meters] :-
> +    nb in nb::NB_Global(),
> +    Some{var s} = nb.options.get("acl_shared_log_meters"),
> +    var meters = s.split(",").to_set().
> +
> /* Meter_Band table */
> for (mb in nb::Meter_Band) {
>     sb::Out_Meter_Band(._uuid = mb._uuid,
> @@ -42,6 +55,14 @@ for (meter in nb::Meter) {
>                  .unit = meter.unit,
>                  .bands = meter.bands)
> }
> +sb::Out_Meter(._uuid = hash128(meter_uuid ++ acl_uuid),
> +              .name = "${name}__${uuid2str(acl_uuid)}",
> +              .unit = unit,
> +              .bands = bands) :-
> +    AclSharedLogMeters[names],
> +    var name = FlatMap(names),
> +    nb::Meter(._uuid = meter_uuid, .name = name, .unit = unit, .bands = bands),
> +    nb::ACL(._uuid = acl_uuid, .meter = Some{name}).
> 
> /* Proxy table for Out_Datapath_Binding: contains all Datapath_Binding fields,
>  * except tunnel id, which is allocated separately (see TunKeyAllocation). */
> @@ -2008,7 +2029,7 @@ for (&Switch(.ls = ls)) {
>          .external_ids     = map_empty())
> }
> 
> -function build_acl_log(acl: nb::ACL): string =
> +function build_acl_log(acl: nb::ACL, shared_meters: Set<string>): string =
> {
>     if (not acl.log) {
>         ""
> @@ -2040,7 +2061,14 @@ function build_acl_log(acl: nb::ACL): string =
>         };
>         match (acl.meter) {
>             None -> (),
> -            Some{meter} -> vec_push(strs, "meter=${json_string_escape(meter)}")
> +            Some{meter} -> {
> +                var s = if (shared_meters.contains(meter)) {
> +                    "${meter}__${uuid2str(acl._uuid)}"
> +                } else {
> +                    meter
> +                };
> +                vec_push(strs, "meter=${json_string_escape(s)}")
> +            }
>         };
>         "log(${string_join(strs, \", \")}); "
>     }
> @@ -2058,31 +2086,33 @@ function oVN_ACL_PRI_OFFSET(): integer = 1000
> relation Reject(lsuuid: uuid, pipeline: string, stage: Stage, acl: nb::ACL, extra_match: string, extra_actions: string)
> 
> /* build_reject_acl_rules() */
> -for (Reject(lsuuid, pipeline, stage, acl, extra_match_, extra_actions_)) {
> -    var extra_match = match (extra_match_) {
> -        "" -> "",
> -        s -> "(${s}) && "
> -    } in
> -    var extra_actions = match (extra_actions_) {
> -        "" -> "",
> -        s -> "${s} "
> -    } in
> -    var next = match (pipeline == "ingress") {
> -        true  -> "next(pipeline=egress,table=${stage_id(switch_stage(OUT, QOS_MARK)).0})",
> -        false -> "next(pipeline=ingress,table=${stage_id(switch_stage(IN, L2_LKUP)).0})"
> -    } in
> -    var acl_log = build_acl_log(acl) in {
> -        var __match = extra_match ++ acl.__match in
> -        var actions = acl_log ++ extra_actions ++ "reg0 = 0; "
> -                      "reject { "
> -                      "/* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ "
> -                      "outport <-> inport; ${next}; };" in
> -        Flow(.logical_datapath = lsuuid,
> -             .stage            = stage,
> -             .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -             .__match          = __match,
> -             .actions          = actions,
> -             .external_ids     = stage_hint(acl._uuid))
> +for (AclSharedLogMeters[shared_meters]) {
> +    for (Reject(lsuuid, pipeline, stage, acl, extra_match_, extra_actions_)) {
> +        var extra_match = match (extra_match_) {
> +            "" -> "",
> +            s -> "(${s}) && "
> +        } in
> +        var extra_actions = match (extra_actions_) {
> +            "" -> "",
> +            s -> "${s} "
> +        } in
> +        var next = match (pipeline == "ingress") {
> +            true  -> "next(pipeline=egress,table=${stage_id(switch_stage(OUT, QOS_MARK)).0})",
> +            false -> "next(pipeline=ingress,table=${stage_id(switch_stage(IN, L2_LKUP)).0})"
> +        } in
> +        var acl_log = build_acl_log(acl, shared_meters) in {
> +            var __match = extra_match ++ acl.__match in
> +            var actions = acl_log ++ extra_actions ++ "reg0 = 0; "
> +                          "reject { "
> +                          "/* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ "
> +                          "outport <-> inport; ${next}; };" in
> +            Flow(.logical_datapath = lsuuid,
> +                 .stage            = stage,
> +                 .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> +                 .__match          = __match,
> +                 .actions          = actions,
> +                 .external_ids     = stage_hint(acl._uuid))
> +        }
>     }
> }
> 
> @@ -2338,114 +2368,117 @@ for (&Switch(.ls = ls)) {
> }
> 
> /* Ingress or Egress ACL Table (Various priorities). */
> -for (&SwitchACL(.sw = &Switch{.ls = ls, .has_stateful_acl = has_stateful}, .acl = &acl)) {
> -    /* consider_acl */
> -    var ingress = acl.direction == "from-lport" in
> -    var stage = if (ingress) { switch_stage(IN, ACL) } else { switch_stage(OUT, ACL) } in
> -    var pipeline = if ingress "ingress" else "egress" in
> -    var stage_hint = stage_hint(acl._uuid) in
> -    if (acl.action == "allow" or acl.action == "allow-related") {
> -        /* If there are any stateful flows, we must even commit "allow"
> -         * actions.  This is because, while the initiater's
> -         * direction may not have any stateful rules, the server's
> -         * may and then its return traffic would not have an
> -         * associated conntrack entry and would return "+invalid". */
> -        if (not has_stateful) {
> -            Flow(.logical_datapath = ls._uuid,
> -                 .stage            = stage,
> -                 .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -                 .__match          = acl.__match,
> -                 .actions          = "${build_acl_log(acl)}next;",
> -                 .external_ids     = stage_hint)
> -        } else {
> -            /* Commit the connection tracking entry if it's a new
> -             * connection that matches this ACL.  After this commit,
> -             * the reply traffic is allowed by a flow we create at
> -             * priority 65535, defined earlier.
> -             *
> -             * It's also possible that a known connection was marked for
> -             * deletion after a policy was deleted, but the policy was
> -             * re-added while that connection is still known.  We catch
> -             * that case here and un-set ct_label.blocked (which will be done
> -             * by ct_commit in the "stateful" stage) to indicate that the
> -             * connection should be allowed to resume.
> -             */
> -            Flow(.logical_datapath = ls._uuid,
> -                 .stage            = stage,
> -                 .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -                 .__match          = "${rEGBIT_ACL_HINT_ALLOW_NEW()} == 1 && (${acl.__match})",
> -                 .actions          = "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${build_acl_log(acl)}next;",
> -                 .external_ids     = stage_hint);
> -
> -            /* Match on traffic in the request direction for an established
> -             * connection tracking entry that has not been marked for
> -             * deletion.  There is no need to commit here, so we can just
> -             * proceed to the next table. We use this to ensure that this
> -             * connection is still allowed by the currently defined
> -             * policy. Match untracked packets too. */
> -            Flow(.logical_datapath = ls._uuid,
> -                 .stage            = stage,
> -                 .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -                 .__match          = "${rEGBIT_ACL_HINT_ALLOW()} == 1 && (${acl.__match})",
> -                 .actions          = "${build_acl_log(acl)}next;",
> -                 .external_ids     = stage_hint)
> -        }
> -    } else if (acl.action == "drop" or acl.action == "reject") {
> -        /* The implementation of "drop" differs if stateful ACLs are in
> -         * use for this datapath.  In that case, the actions differ
> -         * depending on whether the connection was previously committed
> -         * to the connection tracker with ct_commit. */
> -        if (has_stateful) {
> -            /* If the packet is not tracked or not part of an established
> -             * connection, then we can simply reject/drop it. */
> -            var __match = "${rEGBIT_ACL_HINT_DROP()} == 1" in
> -            if (acl.action == "reject") {
> -                Reject(ls._uuid, pipeline, stage, acl, __match, "")
> -            } else {
> +for (AclSharedLogMeters[shared_meters]) {
> +    for (&SwitchACL(.sw = &Switch{.ls = ls, .has_stateful_acl = has_stateful}, .acl = &acl)) {
> +        /* consider_acl */
> +        var ingress = acl.direction == "from-lport" in
> +        var stage = if (ingress) { switch_stage(IN, ACL) } else { switch_stage(OUT, ACL) } in
> +        var pipeline = if ingress "ingress" else "egress" in
> +        var stage_hint = stage_hint(acl._uuid) in
> +        var log_acl = build_acl_log(acl, shared_meters) in
> +        if (acl.action == "allow" or acl.action == "allow-related") {
> +            /* If there are any stateful flows, we must even commit "allow"
> +             * actions.  This is because, while the initiater's
> +             * direction may not have any stateful rules, the server's
> +             * may and then its return traffic would not have an
> +             * associated conntrack entry and would return "+invalid". */
> +            if (not has_stateful) {
>                 Flow(.logical_datapath = ls._uuid,
>                      .stage            = stage,
>                      .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -                     .__match          = __match ++ " && (${acl.__match})",
> -                     .actions          = "${build_acl_log(acl)}/* drop */",
> +                     .__match          = acl.__match,
> +                     .actions          = "${log_acl}next;",
>                      .external_ids     = stage_hint)
> -            };
> -            /* For an existing connection without ct_label set, we've
> -             * encountered a policy change. ACLs previously allowed
> -             * this connection and we committed the connection tracking
> -             * entry.  Current policy says that we should drop this
> -             * connection.  First, we set bit 0 of ct_label to indicate
> -             * that this connection is set for deletion.  By not
> -             * specifying "next;", we implicitly drop the packet after
> -             * updating conntrack state.  We would normally defer
> -             * ct_commit() to the "stateful" stage, but since we're
> -             * rejecting/dropping the packet, we go ahead and do it here.
> -             */
> -            var __match = "${rEGBIT_ACL_HINT_BLOCK()} == 1" in
> -            var actions = "ct_commit { ct_label.blocked = 1; }; " in
> -            if (acl.action == "reject") {
> -                Reject(ls._uuid, pipeline, stage, acl, __match, actions)
>             } else {
> +                /* Commit the connection tracking entry if it's a new
> +                 * connection that matches this ACL.  After this commit,
> +                 * the reply traffic is allowed by a flow we create at
> +                 * priority 65535, defined earlier.
> +                 *
> +                 * It's also possible that a known connection was marked for
> +                 * deletion after a policy was deleted, but the policy was
> +                 * re-added while that connection is still known.  We catch
> +                 * that case here and un-set ct_label.blocked (which will be done
> +                 * by ct_commit in the "stateful" stage) to indicate that the
> +                 * connection should be allowed to resume.
> +                 */
>                 Flow(.logical_datapath = ls._uuid,
>                      .stage            = stage,
>                      .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -                     .__match          = __match ++ " && (${acl.__match})",
> -                     .actions          = "${actions}${build_acl_log(acl)}/* drop */",
> -                     .external_ids     = stage_hint)
> -            }
> -        } else {
> -            /* There are no stateful ACLs in use on this datapath,
> -             * so a "reject/drop" ACL is simply the "reject/drop"
> -             * logical flow action in all cases. */
> -            if (acl.action == "reject") {
> -                Reject(ls._uuid, pipeline, stage, acl, "", "")
> -            } else {
> +                     .__match          = "${rEGBIT_ACL_HINT_ALLOW_NEW()} == 1 && (${acl.__match})",
> +                     .actions          = "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${log_acl}next;",
> +                     .external_ids     = stage_hint);
> +
> +                /* Match on traffic in the request direction for an established
> +                 * connection tracking entry that has not been marked for
> +                 * deletion.  There is no need to commit here, so we can just
> +                 * proceed to the next table. We use this to ensure that this
> +                 * connection is still allowed by the currently defined
> +                 * policy. Match untracked packets too. */
>                 Flow(.logical_datapath = ls._uuid,
>                      .stage            = stage,
>                      .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -                     .__match          = acl.__match,
> -                     .actions          = "${build_acl_log(acl)}/* drop */",
> +                     .__match          = "${rEGBIT_ACL_HINT_ALLOW()} == 1 && (${acl.__match})",
> +                     .actions          = "${log_acl}next;",
>                      .external_ids     = stage_hint)
>             }
> +        } else if (acl.action == "drop" or acl.action == "reject") {
> +            /* The implementation of "drop" differs if stateful ACLs are in
> +             * use for this datapath.  In that case, the actions differ
> +             * depending on whether the connection was previously committed
> +             * to the connection tracker with ct_commit. */
> +            if (has_stateful) {
> +                /* If the packet is not tracked or not part of an established
> +                 * connection, then we can simply reject/drop it. */
> +                var __match = "${rEGBIT_ACL_HINT_DROP()} == 1" in
> +                if (acl.action == "reject") {
> +                    Reject(ls._uuid, pipeline, stage, acl, __match, "")
> +                } else {
> +                    Flow(.logical_datapath = ls._uuid,
> +                         .stage            = stage,
> +                         .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> +                         .__match          = __match ++ " && (${acl.__match})",
> +                         .actions          = "${log_acl}/* drop */",
> +                         .external_ids     = stage_hint)
> +                };
> +                /* For an existing connection without ct_label set, we've
> +                 * encountered a policy change. ACLs previously allowed
> +                 * this connection and we committed the connection tracking
> +                 * entry.  Current policy says that we should drop this
> +                 * connection.  First, we set bit 0 of ct_label to indicate
> +                 * that this connection is set for deletion.  By not
> +                 * specifying "next;", we implicitly drop the packet after
> +                 * updating conntrack state.  We would normally defer
> +                 * ct_commit() to the "stateful" stage, but since we're
> +                 * rejecting/dropping the packet, we go ahead and do it here.
> +                 */
> +                var __match = "${rEGBIT_ACL_HINT_BLOCK()} == 1" in
> +                var actions = "ct_commit { ct_label.blocked = 1; }; " in
> +                if (acl.action == "reject") {
> +                    Reject(ls._uuid, pipeline, stage, acl, __match, actions)
> +                } else {
> +                    Flow(.logical_datapath = ls._uuid,
> +                         .stage            = stage,
> +                         .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> +                         .__match          = __match ++ " && (${acl.__match})",
> +                         .actions          = "${actions}${log_acl}/* drop */",
> +                         .external_ids     = stage_hint)
> +                }
> +            } else {
> +                /* There are no stateful ACLs in use on this datapath,
> +                 * so a "reject/drop" ACL is simply the "reject/drop"
> +                 * logical flow action in all cases. */
> +                if (acl.action == "reject") {
> +                    Reject(ls._uuid, pipeline, stage, acl, "", "")
> +                } else {
> +                    Flow(.logical_datapath = ls._uuid,
> +                         .stage            = stage,
> +                         .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> +                         .__match          = acl.__match,
> +                         .actions          = "${log_acl}/* drop */",
> +                         .external_ids     = stage_hint)
> +                }
> +            }
>         }
>     }
> }
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 590d00c105e9..778482741b10 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1981,7 +1981,12 @@ done
> # Change template meter and make sure that is reflected on acl meters as well
> template_band=$(ovn-nbctl --bare --column=bands find meter name=meter_me)
> ovn-nbctl --wait=sb set meter_band $template_band rate=123
> -check_row_count meter_band 3 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}
> 
> -8<--------------------------cut here-------------------------->8--
> 
> From: Flavio Fernandes <flavio at flaviof.com>
> Date: Tue, 3 Nov 2020 22:18:34 +0000
> Subject: [PATCH ovn] northd: Enhance the implementation of ACL log meters.
> 
> 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 an option in NB_Global that allows for a
> Meter to rate-limit each ACL log separately.
> 
> The change is backward compatible. Based on a well known option, 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 this option:
> 
>  ovn-nbctl set nb_global . options:acl_shared_log_meters="${meter_name}"
> 
> If more than one Meter is wanted, simply use a comma to separate each name.
> 
> Reported-by: Flavio Fernandes <flavio at flaviof.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-October/050769.html
> Signed-off-by: Flavio Fernandes <flavio at flaviof.com>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
> northd/ovn-northd.c  | 201 +++++++++++++++++++++++--------
> northd/ovn_northd.dl | 277 ++++++++++++++++++++++++-------------------
> ovn-nb.xml           |  14 +++
> tests/ovn-northd.at  |  99 ++++++++++++++++
> 4 files changed, 417 insertions(+), 174 deletions(-)
> 
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 684c2bd478e5..2886d2f2d292 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -5356,8 +5356,53 @@ build_acl_hints(struct ovn_datapath *od, struct hmap *lflows)
>     }
> }
> 
> +static bool
> +is_a_shared_meter(const struct smap *nb_options, const char *meter_name)
> +{
> +    bool is_shared_meter = false;
> +    const char *meters = smap_get(nb_options, "acl_shared_log_meters");
> +    if (meters && meters[0]) {
> +        char *cur, *next, *start;
> +        next = start = xstrdup(meters);
> +        while ((cur = strsep(&next, ",")) && *cur) {
> +            if (!strcmp(meter_name, cur)) {
> +                is_shared_meter = true;
> +                break;
> +            }
> +        }
> +        free(start);
> +    }
> +    return is_shared_meter;
> +}
> +
> +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_meter(struct ds *actions, const struct nbrec_acl *acl,
> +                    const struct smap *nb_options)
> +{
> +    if (!acl->meter) {
> +        return;
> +    }
> +
> +    /* If ACL log meter uses a shared meter, use unique Meter name. */
> +    if (is_a_shared_meter(nb_options, 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)
> +build_acl_log(struct ds *actions, const struct nbrec_acl *acl,
> +              const struct smap *nb_options)
> {
>     if (!acl->log) {
>         return;
> @@ -5385,9 +5430,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, nb_options);
> 
>     ds_chomp(actions, ' ');
>     ds_chomp(actions, ',');
> @@ -5398,7 +5441,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 smap *nb_options)
> {
>     struct ds match = DS_EMPTY_INITIALIZER;
>     struct ds actions = DS_EMPTY_INITIALIZER;
> @@ -5410,7 +5454,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, nb_options);
>     if (extra_match->length > 0) {
>         ds_put_format(&match, "(%s) && ", extra_match->string);
>     }
> @@ -5435,7 +5479,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 smap *nb_options)
> {
>     bool ingress = !strcmp(acl->direction, "from-lport") ? true :false;
>     enum ovn_stage stage = ingress ? S_SWITCH_IN_ACL : S_SWITCH_OUT_ACL;
> @@ -5449,7 +5494,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, nb_options);
>             ds_put_cstr(&actions, "next;");
>             ovn_lflow_add_with_hint(lflows, od, stage,
>                                     acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -5475,7 +5520,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, nb_options);
>             ds_put_cstr(&actions, "next;");
>             ovn_lflow_add_with_hint(lflows, od, stage,
>                                     acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -5494,7 +5539,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, nb_options);
>             ds_put_cstr(&actions, "next;");
>             ovn_lflow_add_with_hint(lflows, od, stage,
>                                     acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -5519,10 +5564,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_, nb_options);
>             } else {
>                 ds_put_format(&match, " && (%s)", acl->match);
> -                build_acl_log(&actions, acl);
> +                build_acl_log(&actions, acl, nb_options);
>                 ds_put_cstr(&actions, "/* drop */");
>                 ovn_lflow_add_with_hint(lflows, od, stage,
>                                         acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -5546,10 +5591,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_, nb_options);
>             } else {
>                 ds_put_format(&match, " && (%s)", acl->match);
> -                build_acl_log(&actions, acl);
> +                build_acl_log(&actions, acl, nb_options);
>                 ds_put_cstr(&actions, "/* drop */");
>                 ovn_lflow_add_with_hint(lflows, od, stage,
>                                         acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -5562,9 +5607,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_, nb_options);
>             } else {
> -                build_acl_log(&actions, acl);
> +                build_acl_log(&actions, acl, nb_options);
>                 ds_put_cstr(&actions, "/* drop */");
>                 ovn_lflow_add_with_hint(lflows, od, stage,
>                                         acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -5644,7 +5689,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 smap *nb_options)
> {
>     bool has_stateful = (has_stateful_acl(od) || has_lb_vip(od));
> 
> @@ -5747,13 +5792,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, nb_options);
>     }
>     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,
> +                             nb_options);
>             }
>         }
>     }
> @@ -6776,7 +6822,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
>                     struct hmap *port_groups, struct hmap *lflows,
>                     struct hmap *mcgroups, struct hmap *igmp_groups,
>                     struct shash *meter_groups,
> -                    struct hmap *lbs)
> +                    struct hmap *lbs, const struct smap *nb_options)
> {
>     /* This flow table structure is documented in ovn-northd(8), so please
>      * update ovn-northd.8.xml if you change anything. */
> @@ -6796,7 +6842,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
>         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, nb_options);
>         build_qos(od, lflows);
>         build_lb(od, lflows);
>         build_stateful(od, lflows, lbs);
> @@ -11379,8 +11425,12 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths,
> {
>     struct hmap lflows = HMAP_INITIALIZER(&lflows);
> 
> +    const struct nbrec_nb_global *nb_global =
> +        nbrec_nb_global_first(ctx->ovnnb_idl);
> +    ovs_assert(nb_global);
> +
>     build_lswitch_flows(datapaths, ports, port_groups, &lflows, mcgroups,
> -                        igmp_groups, meter_groups, lbs);
> +                        igmp_groups, meter_groups, lbs, &nb_global->options);
>     build_lrouter_flows(datapaths, ports, &lflows, meter_groups, lbs);
> 
>     /* Push changes to the Logical_Flow table to database. */
> @@ -11706,15 +11756,85 @@ 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);
> +}
> +
> +static void
> +sync_acl_shared_meters(struct northd_context *ctx,
> +                       struct hmap *datapaths,
> +                       const struct smap *nb_options,
> +                       const struct nbrec_meter *nb_meter,
> +                       struct shash *sb_meters)
> +{
> +    if (!is_a_shared_meter(nb_options, nb_meter->name)) {
> +        return;
> +    }
> +
> +    struct ovn_datapath *od;
> +    HMAP_FOR_EACH (od, key_node, datapaths) {
> +        if (!od->nbs) {
> +            continue;
> +        }
> +        for (size_t i = 0; i < od->nbs->n_acls; i++) {
> +            struct nbrec_acl *acl = od->nbs->acls[i];
> +            if (!acl->meter || strcmp(nb_meter->name, acl->meter)) {
> +                continue;
> +            }
> +
> +            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);
> +        }
> +    }
> +}
> +
> /* 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 shared 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 sb_meters = SHASH_INITIALIZER(&sb_meters);
> 
> +    const struct nbrec_nb_global *nb_global =
> +        nbrec_nb_global_first(ctx->ovnnb_idl);
> +    ovs_assert(nb_global);
> +
>     const struct sbrec_meter *sb_meter;
>     SBREC_METER_FOR_EACH (sb_meter, ctx->ovnsb_idl) {
>         shash_add(&sb_meters, sb_meter->name, sb_meter);
> @@ -11722,33 +11842,10 @@ 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);
> +        sync_acl_shared_meters(ctx, datapaths, &nb_global->options, nb_meter,
> +                               &sb_meters);
>     }
> 
>     struct shash_node *node, *next;
> @@ -12241,7 +12338,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);
>     sync_dns_entries(ctx, datapaths);
>     destroy_ovn_lbs(&lbs);
>     hmap_destroy(&lbs);
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index 23e140148c35..135ff559520b 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -27,6 +27,19 @@ output relation Warning[string]
> 
> index Logical_Flow_Index() on sb::Out_Logical_Flow()
> 
> +/* Single-row relation that contains the set of shared meters. */
> +relation AclSharedLogMeters[Set<string>]
> +AclSharedLogMeters[meters] :- AclSharedLogMeters0[meters].
> +AclSharedLogMeters[set_empty()] :-
> +    Unit(),
> +    not AclSharedLogMeters0[_].
> +
> +relation AclSharedLogMeters0[Set<string>]
> +AclSharedLogMeters0[meters] :-
> +    nb in nb::NB_Global(),
> +    Some{var s} = nb.options.get("acl_shared_log_meters"),
> +    var meters = s.split(",").to_set().
> +
> /* Meter_Band table */
> for (mb in nb::Meter_Band) {
>     sb::Out_Meter_Band(._uuid = mb._uuid,
> @@ -42,6 +55,14 @@ for (meter in nb::Meter) {
>                  .unit = meter.unit,
>                  .bands = meter.bands)
> }
> +sb::Out_Meter(._uuid = hash128(meter_uuid ++ acl_uuid),
> +              .name = "${name}__${uuid2str(acl_uuid)}",
> +              .unit = unit,
> +              .bands = bands) :-
> +    AclSharedLogMeters[names],
> +    var name = FlatMap(names),
> +    nb::Meter(._uuid = meter_uuid, .name = name, .unit = unit, .bands = bands),
> +    nb::ACL(._uuid = acl_uuid, .meter = Some{name}).
> 
> /* Proxy table for Out_Datapath_Binding: contains all Datapath_Binding fields,
>  * except tunnel id, which is allocated separately (see TunKeyAllocation). */
> @@ -2008,7 +2029,7 @@ for (&Switch(.ls = ls)) {
>          .external_ids     = map_empty())
> }
> 
> -function build_acl_log(acl: nb::ACL): string =
> +function build_acl_log(acl: nb::ACL, shared_meters: Set<string>): string =
> {
>     if (not acl.log) {
>         ""
> @@ -2040,7 +2061,14 @@ function build_acl_log(acl: nb::ACL): string =
>         };
>         match (acl.meter) {
>             None -> (),
> -            Some{meter} -> vec_push(strs, "meter=${json_string_escape(meter)}")
> +            Some{meter} -> {
> +                var s = if (shared_meters.contains(meter)) {
> +                    "${meter}__${uuid2str(acl._uuid)}"
> +                } else {
> +                    meter
> +                };
> +                vec_push(strs, "meter=${json_string_escape(s)}")
> +            }
>         };
>         "log(${string_join(strs, \", \")}); "
>     }
> @@ -2058,31 +2086,33 @@ function oVN_ACL_PRI_OFFSET(): integer = 1000
> relation Reject(lsuuid: uuid, pipeline: string, stage: Stage, acl: nb::ACL, extra_match: string, extra_actions: string)
> 
> /* build_reject_acl_rules() */
> -for (Reject(lsuuid, pipeline, stage, acl, extra_match_, extra_actions_)) {
> -    var extra_match = match (extra_match_) {
> -        "" -> "",
> -        s -> "(${s}) && "
> -    } in
> -    var extra_actions = match (extra_actions_) {
> -        "" -> "",
> -        s -> "${s} "
> -    } in
> -    var next = match (pipeline == "ingress") {
> -        true  -> "next(pipeline=egress,table=${stage_id(switch_stage(OUT, QOS_MARK)).0})",
> -        false -> "next(pipeline=ingress,table=${stage_id(switch_stage(IN, L2_LKUP)).0})"
> -    } in
> -    var acl_log = build_acl_log(acl) in {
> -        var __match = extra_match ++ acl.__match in
> -        var actions = acl_log ++ extra_actions ++ "reg0 = 0; "
> -                      "reject { "
> -                      "/* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ "
> -                      "outport <-> inport; ${next}; };" in
> -        Flow(.logical_datapath = lsuuid,
> -             .stage            = stage,
> -             .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -             .__match          = __match,
> -             .actions          = actions,
> -             .external_ids     = stage_hint(acl._uuid))
> +for (AclSharedLogMeters[shared_meters]) {
> +    for (Reject(lsuuid, pipeline, stage, acl, extra_match_, extra_actions_)) {
> +        var extra_match = match (extra_match_) {
> +            "" -> "",
> +            s -> "(${s}) && "
> +        } in
> +        var extra_actions = match (extra_actions_) {
> +            "" -> "",
> +            s -> "${s} "
> +        } in
> +        var next = match (pipeline == "ingress") {
> +            true  -> "next(pipeline=egress,table=${stage_id(switch_stage(OUT, QOS_MARK)).0})",
> +            false -> "next(pipeline=ingress,table=${stage_id(switch_stage(IN, L2_LKUP)).0})"
> +        } in
> +        var acl_log = build_acl_log(acl, shared_meters) in {
> +            var __match = extra_match ++ acl.__match in
> +            var actions = acl_log ++ extra_actions ++ "reg0 = 0; "
> +                          "reject { "
> +                          "/* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ "
> +                          "outport <-> inport; ${next}; };" in
> +            Flow(.logical_datapath = lsuuid,
> +                 .stage            = stage,
> +                 .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> +                 .__match          = __match,
> +                 .actions          = actions,
> +                 .external_ids     = stage_hint(acl._uuid))
> +        }
>     }
> }
> 
> @@ -2338,114 +2368,117 @@ for (&Switch(.ls = ls)) {
> }
> 
> /* Ingress or Egress ACL Table (Various priorities). */
> -for (&SwitchACL(.sw = &Switch{.ls = ls, .has_stateful_acl = has_stateful}, .acl = &acl)) {
> -    /* consider_acl */
> -    var ingress = acl.direction == "from-lport" in
> -    var stage = if (ingress) { switch_stage(IN, ACL) } else { switch_stage(OUT, ACL) } in
> -    var pipeline = if ingress "ingress" else "egress" in
> -    var stage_hint = stage_hint(acl._uuid) in
> -    if (acl.action == "allow" or acl.action == "allow-related") {
> -        /* If there are any stateful flows, we must even commit "allow"
> -         * actions.  This is because, while the initiater's
> -         * direction may not have any stateful rules, the server's
> -         * may and then its return traffic would not have an
> -         * associated conntrack entry and would return "+invalid". */
> -        if (not has_stateful) {
> -            Flow(.logical_datapath = ls._uuid,
> -                 .stage            = stage,
> -                 .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -                 .__match          = acl.__match,
> -                 .actions          = "${build_acl_log(acl)}next;",
> -                 .external_ids     = stage_hint)
> -        } else {
> -            /* Commit the connection tracking entry if it's a new
> -             * connection that matches this ACL.  After this commit,
> -             * the reply traffic is allowed by a flow we create at
> -             * priority 65535, defined earlier.
> -             *
> -             * It's also possible that a known connection was marked for
> -             * deletion after a policy was deleted, but the policy was
> -             * re-added while that connection is still known.  We catch
> -             * that case here and un-set ct_label.blocked (which will be done
> -             * by ct_commit in the "stateful" stage) to indicate that the
> -             * connection should be allowed to resume.
> -             */
> -            Flow(.logical_datapath = ls._uuid,
> -                 .stage            = stage,
> -                 .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -                 .__match          = "${rEGBIT_ACL_HINT_ALLOW_NEW()} == 1 && (${acl.__match})",
> -                 .actions          = "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${build_acl_log(acl)}next;",
> -                 .external_ids     = stage_hint);
> -
> -            /* Match on traffic in the request direction for an established
> -             * connection tracking entry that has not been marked for
> -             * deletion.  There is no need to commit here, so we can just
> -             * proceed to the next table. We use this to ensure that this
> -             * connection is still allowed by the currently defined
> -             * policy. Match untracked packets too. */
> -            Flow(.logical_datapath = ls._uuid,
> -                 .stage            = stage,
> -                 .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -                 .__match          = "${rEGBIT_ACL_HINT_ALLOW()} == 1 && (${acl.__match})",
> -                 .actions          = "${build_acl_log(acl)}next;",
> -                 .external_ids     = stage_hint)
> -        }
> -    } else if (acl.action == "drop" or acl.action == "reject") {
> -        /* The implementation of "drop" differs if stateful ACLs are in
> -         * use for this datapath.  In that case, the actions differ
> -         * depending on whether the connection was previously committed
> -         * to the connection tracker with ct_commit. */
> -        if (has_stateful) {
> -            /* If the packet is not tracked or not part of an established
> -             * connection, then we can simply reject/drop it. */
> -            var __match = "${rEGBIT_ACL_HINT_DROP()} == 1" in
> -            if (acl.action == "reject") {
> -                Reject(ls._uuid, pipeline, stage, acl, __match, "")
> -            } else {
> +for (AclSharedLogMeters[shared_meters]) {
> +    for (&SwitchACL(.sw = &Switch{.ls = ls, .has_stateful_acl = has_stateful}, .acl = &acl)) {
> +        /* consider_acl */
> +        var ingress = acl.direction == "from-lport" in
> +        var stage = if (ingress) { switch_stage(IN, ACL) } else { switch_stage(OUT, ACL) } in
> +        var pipeline = if ingress "ingress" else "egress" in
> +        var stage_hint = stage_hint(acl._uuid) in
> +        var log_acl = build_acl_log(acl, shared_meters) in
> +        if (acl.action == "allow" or acl.action == "allow-related") {
> +            /* If there are any stateful flows, we must even commit "allow"
> +             * actions.  This is because, while the initiater's
> +             * direction may not have any stateful rules, the server's
> +             * may and then its return traffic would not have an
> +             * associated conntrack entry and would return "+invalid". */
> +            if (not has_stateful) {
>                 Flow(.logical_datapath = ls._uuid,
>                      .stage            = stage,
>                      .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -                     .__match          = __match ++ " && (${acl.__match})",
> -                     .actions          = "${build_acl_log(acl)}/* drop */",
> +                     .__match          = acl.__match,
> +                     .actions          = "${log_acl}next;",
>                      .external_ids     = stage_hint)
> -            };
> -            /* For an existing connection without ct_label set, we've
> -             * encountered a policy change. ACLs previously allowed
> -             * this connection and we committed the connection tracking
> -             * entry.  Current policy says that we should drop this
> -             * connection.  First, we set bit 0 of ct_label to indicate
> -             * that this connection is set for deletion.  By not
> -             * specifying "next;", we implicitly drop the packet after
> -             * updating conntrack state.  We would normally defer
> -             * ct_commit() to the "stateful" stage, but since we're
> -             * rejecting/dropping the packet, we go ahead and do it here.
> -             */
> -            var __match = "${rEGBIT_ACL_HINT_BLOCK()} == 1" in
> -            var actions = "ct_commit { ct_label.blocked = 1; }; " in
> -            if (acl.action == "reject") {
> -                Reject(ls._uuid, pipeline, stage, acl, __match, actions)
>             } else {
> +                /* Commit the connection tracking entry if it's a new
> +                 * connection that matches this ACL.  After this commit,
> +                 * the reply traffic is allowed by a flow we create at
> +                 * priority 65535, defined earlier.
> +                 *
> +                 * It's also possible that a known connection was marked for
> +                 * deletion after a policy was deleted, but the policy was
> +                 * re-added while that connection is still known.  We catch
> +                 * that case here and un-set ct_label.blocked (which will be done
> +                 * by ct_commit in the "stateful" stage) to indicate that the
> +                 * connection should be allowed to resume.
> +                 */
>                 Flow(.logical_datapath = ls._uuid,
>                      .stage            = stage,
>                      .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -                     .__match          = __match ++ " && (${acl.__match})",
> -                     .actions          = "${actions}${build_acl_log(acl)}/* drop */",
> -                     .external_ids     = stage_hint)
> -            }
> -        } else {
> -            /* There are no stateful ACLs in use on this datapath,
> -             * so a "reject/drop" ACL is simply the "reject/drop"
> -             * logical flow action in all cases. */
> -            if (acl.action == "reject") {
> -                Reject(ls._uuid, pipeline, stage, acl, "", "")
> -            } else {
> +                     .__match          = "${rEGBIT_ACL_HINT_ALLOW_NEW()} == 1 && (${acl.__match})",
> +                     .actions          = "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${log_acl}next;",
> +                     .external_ids     = stage_hint);
> +
> +                /* Match on traffic in the request direction for an established
> +                 * connection tracking entry that has not been marked for
> +                 * deletion.  There is no need to commit here, so we can just
> +                 * proceed to the next table. We use this to ensure that this
> +                 * connection is still allowed by the currently defined
> +                 * policy. Match untracked packets too. */
>                 Flow(.logical_datapath = ls._uuid,
>                      .stage            = stage,
>                      .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -                     .__match          = acl.__match,
> -                     .actions          = "${build_acl_log(acl)}/* drop */",
> +                     .__match          = "${rEGBIT_ACL_HINT_ALLOW()} == 1 && (${acl.__match})",
> +                     .actions          = "${log_acl}next;",
>                      .external_ids     = stage_hint)
>             }
> +        } else if (acl.action == "drop" or acl.action == "reject") {
> +            /* The implementation of "drop" differs if stateful ACLs are in
> +             * use for this datapath.  In that case, the actions differ
> +             * depending on whether the connection was previously committed
> +             * to the connection tracker with ct_commit. */
> +            if (has_stateful) {
> +                /* If the packet is not tracked or not part of an established
> +                 * connection, then we can simply reject/drop it. */
> +                var __match = "${rEGBIT_ACL_HINT_DROP()} == 1" in
> +                if (acl.action == "reject") {
> +                    Reject(ls._uuid, pipeline, stage, acl, __match, "")
> +                } else {
> +                    Flow(.logical_datapath = ls._uuid,
> +                         .stage            = stage,
> +                         .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> +                         .__match          = __match ++ " && (${acl.__match})",
> +                         .actions          = "${log_acl}/* drop */",
> +                         .external_ids     = stage_hint)
> +                };
> +                /* For an existing connection without ct_label set, we've
> +                 * encountered a policy change. ACLs previously allowed
> +                 * this connection and we committed the connection tracking
> +                 * entry.  Current policy says that we should drop this
> +                 * connection.  First, we set bit 0 of ct_label to indicate
> +                 * that this connection is set for deletion.  By not
> +                 * specifying "next;", we implicitly drop the packet after
> +                 * updating conntrack state.  We would normally defer
> +                 * ct_commit() to the "stateful" stage, but since we're
> +                 * rejecting/dropping the packet, we go ahead and do it here.
> +                 */
> +                var __match = "${rEGBIT_ACL_HINT_BLOCK()} == 1" in
> +                var actions = "ct_commit { ct_label.blocked = 1; }; " in
> +                if (acl.action == "reject") {
> +                    Reject(ls._uuid, pipeline, stage, acl, __match, actions)
> +                } else {
> +                    Flow(.logical_datapath = ls._uuid,
> +                         .stage            = stage,
> +                         .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> +                         .__match          = __match ++ " && (${acl.__match})",
> +                         .actions          = "${actions}${log_acl}/* drop */",
> +                         .external_ids     = stage_hint)
> +                }
> +            } else {
> +                /* There are no stateful ACLs in use on this datapath,
> +                 * so a "reject/drop" ACL is simply the "reject/drop"
> +                 * logical flow action in all cases. */
> +                if (acl.action == "reject") {
> +                    Reject(ls._uuid, pipeline, stage, acl, "", "")
> +                } else {
> +                    Flow(.logical_datapath = ls._uuid,
> +                         .stage            = stage,
> +                         .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> +                         .__match          = acl.__match,
> +                         .actions          = "${log_acl}/* drop */",
> +                         .external_ids     = stage_hint)
> +                }
> +            }
>         }
>     }
> }
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 5e8635992e49..51b186b84419 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -193,6 +193,20 @@
>         </p>
>       </column>
> 
> +      <column name="options" key="acl_shared_log_meters">
> +        <p>
> +          A string value that contains a list of meter names delimited by ",".
> +          The <ref column="meter" table="ACL"/> column is used by ACL to rate
> +          limit log events. As multiple ACL rows may opt to use the same meter
> +          name, a potentially adverse effect is that a "noisy" ACL log could
> +          consume all the allowable events for the shared
> +          <ref column="name" table="meter"/>. This global option can be used to
> +          eliminate that behavior, by listing the meters that are meant to
> +          provide its "complete" rate to each one of the <ref table="ACL"/>
> +          logs that use it.
> +        </p>
> +      </column>
> +
>       <group title="Options for configuring interconnection route advertisement">
>         <p>
>           These options control how routes are advertised between OVN
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index d2aefa8d840a..778482741b10 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1920,6 +1920,105 @@ sw1flows3:  table=5 (ls_out_acl         ), priority=2003 , match=((reg0[[9]] ==
> AT_CLEANUP
> ])
> 
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-northd -- ACL shared Meter])
> +AT_KEYWORDS([acl meter])
> +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 remove nb_global . options acl_shared_log_meters
> +ovn-nbctl meter-add meter_me drop 1 pktps
> +
> +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
> +
> +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)
> +ovn-nbctl set acl $acl1 log=true severity=alert meter=meter_me name=acl_one
> +ovn-nbctl set acl $acl2 log=true severity=info  meter=meter_me name=acl_two
> +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 in $* ; do
> +        # echo checking for $expected_count $meter in sb meter table
> +        check_row_count meter $expected_count name=$meter
> +    done
> +}
> +
> +# With acl_shared_log_meters unset, make sure no unique meters are in SB
> +check_meter_by_name meter_me
> +check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2}
> +for other_meters in 'm1' 'm1,m2,m3' 'meter_Xe'; do
> +    ovn-nbctl --wait=sb set nb_global . options:acl_shared_log_meters="$other_meters"
> +    check_meter_by_name meter_me
> +    check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2}
> +done
> +
> +# Check variations where unique meters are in SB
> +for other_meters in 'm1,meter_me' 'm1,m2,meter_me,m3' 'meter_me,m1' 'meter_me'; do
> +    ovn-nbctl --wait=sb set nb_global . options:acl_shared_log_meters="$other_meters"
> +    check_meter_by_name meter_me meter_me__${acl1} meter_me__${acl2}
> +done
> +
> +# Change template meter and make sure that is reflected on acl meters as well
> +template_band=$(ovn-nbctl --bare --column=bands find meter name=meter_me)
> +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
> +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
> +ovn-nbctl --wait=sb meter-del meter_me
> +check_row_count meter 0
> +# Check that logical flow remains. It refers to a named row that does not exist.
> +check_acl_lflow acl_two meter_me__${acl2}
> +
> +# Re-add template meter and make sure acl2's meter is back in sb
> +ovn-nbctl --wait=sb 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=$(ovn-nbctl --bare --column=_uuid find logical_switch name=sw0)
> +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
> +])
> +
> OVN_FOR_EACH_NORTHD([
> AT_SETUP([datapath requested-tnl-key])
> AT_KEYWORDS([requested tnl tunnel key keys])
> --
> 2.26.2
> 



More information about the dev mailing list