[inline] > On Nov 7, 2020, at 4:39 PM, Ben Pfaff <b...@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 <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.
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 <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