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

Flavio Fernandes flavio at flaviof.com
Tue Nov 10 20:43:17 UTC 2020


[cc Dimitru, Numan, MarkM]


> On Nov 10, 2020, at 2:15 PM, Ben Pfaff <blp at ovn.org> wrote:
> 
> On Mon, Nov 09, 2020 at 09:53:16AM -0500, Flavio Fernandes wrote:
>>> On Nov 7, 2020, at 4:39 PM, Ben Pfaff <blp at ovn.org> wrote:
>>> On Tue, Nov 03, 2020 at 10:18:34PM +0000, Flavio Fernandes wrote:
>>>> When multiple ACL rows use the same Meter for log rate-limiting, the
>>>> 'noisiest' ACL matches may completely consume the Meter and shadow other
>>>> ACL logs. This patch introduces an option in NB_Global that allows for a
>>>> Meter to rate-limit each ACL log separately.
>>> 
>>> 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 think that A and B are reasonable approaches.  I don't understand the
> issue well enough to know whether this property has more to do with the
> ACL or with the Meter, so I'm not sure where it better to put it.

Let's pursue the schema change, then! Funny thing is that my very initial idea [1] was 'A'.
I can see how 'B' could benefit non-ACL-log related functionality in the NB that refers to
Meters, but I think that is overkill. I ended up hesitant in changing the schema after talking
with my OVN guru Dumitru. I will follow up with him on this idea to see how strongly he feels
about it.

> I don't think C or D makes sense, because I don't know a reason why
> there would be only one of these meter or only a small number of these
> meters.

Option C (not the language) is likely too restrictive and D would still feel like a leash.
A small number of 'fair' Meters would be all that ACL logs needed. If many ACL logs end up having
different Meter needs, then having a 1:1 mapping between ACLs and Meter rows would be
the answer. And that is what we already have the ability to do today. So, I cannot imagine a usage
case where we would have more than a couple of values for the NB_global. The potential for scaling
troubles does not give me a good felling, so I'm back to 'A'. ;)

> 
> I think that E or F would only make sense if there was some reason A or
> B would not work.

++

> 
> OK, let's step back here and consider G.  Why do we need a new
> southbound Meter row for each instance?  That's a bit of a waste, isn't
> it?  Why can't we just add an indicator to the southbound Meter that
> says "each reference to me is unique"?  Or a new argument to the
> southbound log() action that distinguishes references to a given meter,
> so that identical values get the same one and different values get
> distinct ones?

Hmm.... That sounds really good, actually. We would still need 'A' as a way
to propagate that desire from the CMS' perspective, agree?

I must confess that the wasteful approach of rows in the SB comes from my
limited knowledge on how to efficiently use the log action to do what you
described. Also, because I was hoping to solve the whole problem within
northd.

If I understand you correctly, option 'G' you mention here would require
changes in the SB schema as well as in ovn-controller? I will definitely
need some pointers to make that happen. Wanna be my partner in
crime? :) No pressure.

> 
>>> 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.
> 
> "fair" seems OK to me.
> 

++

Let me take the opportunity to answer to something you mentioned in the
previous message, even though it may not be all that relevant anymore.

> 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:

Make sense. I was leveraging the existing code that creates SB meters
from the NB into a function and decided not to break it up any further. I
was also unsure that if I used the same Meter_Band, I would cause the
adverse effect of the 'fair' meters end up draining from the same rate
pool and moot the whole effort of making separate meters to begin with.
But I now see that sharing the same meter_band as you did on
ovn-northd-ddlog works just fine [2].

> 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:


I did not want to alienate any other users of the Meter name. So yes,
I intentionally kept the creation of the unmodified meter in place to
cover potential cases where something unrelated to acl log decided
to use the same meter name. Maybe a bit too paranoid?!? ;)

> 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.

It will be a while longer until I manage to acclimate to the syntax in
ovn_northd.dl. But I can see how powerful it can be. Reminds me of
the few months it took me to learn Erlang, which remains as one of
my favorite languages.

Lastly, it goes w/out saying but just to be clear: Given how fast you move
and how far along ddlog patches are, my intentions are for not getting any
of the 'fair' meters code merged before ddlog's.

Best,

-- flaviof

[1]: https://github.com/flavio-fernandes/ovn/commit/f265b01fcb7866bdc697eccc151b7dedebd0085b <https://github.com/flavio-fernandes/ovn/commit/f265b01fcb7866bdc697eccc151b7dedebd0085b>
[2]: https://github.com/ovn-org/ovn/blob/c108f23e1c10910031f9409b79001d001aae0c8f/controller/ofctrl.c#L1715 <https://github.com/ovn-org/ovn/blob/c108f23e1c10910031f9409b79001d001aae0c8f/controller/ofctrl.c#L1715>










More information about the dev mailing list