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

[Ben Pfaff]

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.

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.

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?

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