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 <fla...@flaviof.com>
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2020-October/050769.html
> Signed-off-by: Flavio Fernandes <fla...@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.
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.
OK, all that is quibbling. You asked me to help with the ddlog part. I
did that, and let me explain it.
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 <fla...@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 <fla...@flaviof.com>
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2020-October/050769.html
Signed-off-by: Flavio Fernandes <fla...@flaviof.com>
Signed-off-by: Ben Pfaff <b...@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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
