On Wed, Jul 31, 2024 at 11:48 AM Ilya Maximets <[email protected]> wrote:
>
> On 7/31/24 17:05, Dumitru Ceara wrote:
> > On 7/31/24 16:40, Ilya Maximets wrote:
> >> On 7/31/24 16:17, Dumitru Ceara wrote:
> >>> On 7/31/24 16:07, Ilya Maximets wrote:
> >>>> On 7/31/24 16:04, Ilya Maximets wrote:
> >>>>> On 7/31/24 11:05, Dumitru Ceara wrote:
> >>>>>> This will represent a unified place to store IPFIX observation domain
> >>>>>> ID
> >>>>>> configurations for sampling applications (currently only drop sampling
> >>>>>> is supported as application but following commits will add more).
> >>>>>>
> >>>>>> Acked-by: Mark Michelson <[email protected]>
> >>>>>> Signed-off-by: Dumitru Ceara <[email protected]>
> >>>>>> ---
> >>>>>> V4:
> >>>>>> - Addressed Ales' comments:
> >>>>>> - fix up indentation
> >>>>>> - bump NB schema version
> >>>>>> - Added Mark's ack.
> >>>>>> ---
> >>>>>> northd/automake.mk | 2 +
> >>>>>> northd/en-lflow.c | 5 ++
> >>>>>> northd/en-sampling-app.c | 120 +++++++++++++++++++++++++++++++++++++++
> >>>>>> northd/en-sampling-app.h | 51 +++++++++++++++++
> >>>>>> northd/inc-proc-northd.c | 10 +++-
> >>>>>> northd/northd.h | 1 +
> >>>>>> ovn-nb.ovsschema | 23 +++++++-
> >>>>>> ovn-nb.xml | 17 ++++++
> >>>>>> tests/ovn-northd.at | 17 ++++++
> >>>>>> 9 files changed, 242 insertions(+), 4 deletions(-)
> >>>>>> create mode 100644 northd/en-sampling-app.c
> >>>>>> create mode 100644 northd/en-sampling-app.h
> >>>>>
> >>>>> <snip>
> >>>>>
> >>>>>> diff --git a/ovn-nb.xml b/ovn-nb.xml
> >>>>>> index 6376320d31..b96b0b34ed 100644
> >>>>>> --- a/ovn-nb.xml
> >>>>>> +++ b/ovn-nb.xml
> >>>>>> @@ -5093,4 +5093,21 @@ or
> >>>>>> </column>
> >>>>>> </group>
> >>>>>> </table>
> >>>>>> + <table name="Sampling_App">
> >>>>>> + <column name="name">
> >>>>>
> >>>>> Maybe this should be 'type' instead of a 'name'?
> >>>>> 'name' makes me think that I can create multiple of them
> >>>>> with different parameters, but that's not the case.
> >>>>>
> >>>
> >>> I guess it depends a bit how you look at it. It's just a 1:1 mapping
> >>> between an "OVN application" (ACL, default drops) and an integer ID.
> >>> I'm not sure why you get the impression that you can create multiple of
> >>> them, there's an explicit index on 'name' that doesn't allow that.
> >>
> >> Index is rather indirect way of saying that there should be only one
> >> instance
> >> of each application.
> >>
> >> The word 'name' says to me that I can choose an arbitrary one, but there
> >> is a
> >> predefined list here. So, it is actually a type and not a name.
> >>
> >
> > Hmm, OK, I guess I can live with 'type'.
> >
> >>>
> >>> I could change it to 'type' but for me 'name' made more sense.
> >>
> >> Another way to represent the same thing is to have a table with a single
> >> row
> >> with a map from enum to an integer. But also, why is it a separate table
> >> at all?
> >> Why not a column in the NB_Global table?
> >>
> >> Something like:
> >>
> >> "NB_Global": {
> >> ...
> >>
> >> "sample_domain_ids": {
> >> "type": {"key": {"type": "string",
> >> "enum": ["set", ["drop", "acl-new", "acl-est"]]},
> >> "value": {"type": "integer",
> >> "minInteger": 1,
> >> "maxInteger": 255}}}
> >>
> >> ?
> >>
> >
> > We could.
> >
> >> This way we don't need a table, don't need an index or strange column
> >> names,
> >
> > I think this is a matter of personal preference. To me it doesn't seem
> > "strange", it makes sense. But I wrote this code so I'm of course biased.
> >
> >> likely don't need a separate I-P node. We're just turning
> >> debug_drop_domain_id
> >> into a new column with a set.
> >>
> >
> > But we'd still need most of the logic that's currently in
> > en-sampling-app.c. That would have to move to en-global-config.c
> > because the "en_lflow" incremental processing node depends on
> > "en_global_config" whose data is built based on the "en_nb_nb_global"
> > database node.
>
> Sure, I just meant that we'd not need a separate node. We'll need
> the logic for the new column.
>
> >
> > So, with that in mind, in my opinion it seems cleaner if we have a
> > sampling_app table to define this kind of mapping instead of stuffing
> > more random feature-specific configuration fields in NB_Global. But I'm
> > open to discussion.
>
> NB_Global carries all sorts of global configuration including specific
> configuration for various features. Observation IDs for different
> sampling types is a global configuration, it doesn't depend on anything
> else. It would make some sense to have a separate table if there were
> more configuration options for each type, not only ID, but I'm not sure
> what else we would need there. Even then it would probably make more
> sense as a map in nb-global from type to a reference to a configuration
> table.
>
> >
> > Maybe others could share their opinion on this too. I'll wait a bit
> > with sending v5 until we clear this up.
>
> Sure.
My 2 cents.
1. I'd prefer "type" instead of "name" as the column name in the
Sample_App table.
Also I see in a few places, "name" and "type" are used interchangeably.
For example, the below enum in this patch (defined in en-sampling-app.h)
/* Supported sampling application types. */
enum sampling_app_type {
SAMPLING_APP_DROP_DEBUG,
SAMPLING_APP_ACL_NEW_TRAFFIC,
SAMPLING_APP_ACL_EST_TRAFFIC,
SAMPLING_APP_MAX,
};
Also in patch 5, the documentation in the "metadata" column of the
"Sample" table
references this as type.
2. Regarding the discussion on separate Sample_App table vs a column
in NB_Global,
my preference would be to have a separate table which this patch proposes.
I think it's easier to manage the I-P handling this way. NB Global
engine node
is an input to northd, lflow, mac binding aging and fdb aging engine nodes.
Any change to nb_global due to Sample (although I expect very few
and less frequent
changes to Sample_App table) would also trigger handlers of these nodes
(which is not an issue and should not be a big deal to handle),
Whereas only lflow engine depends on Sample App engine node.
Thanks
Numan
>
> >
> >>>
> >>>>>> + The name of the application to be configured for sampling.
> >>>>>> Currently
> >>>>>> + supported options are: "drop-sampling",
> >>>>>> "acl-new-traffic-sampling",
> >>>>>> + "acl-est-traffic-sampling".
> >>>>>
> >>>>> Do we really need the '-sampling' part in here? There are types
> >>>>
> >>>> s/There/These/
> >>>>
> >>>>> of the sampling application after all. Also, 'traffic' may also
> >>>>
> >>>> s/also//
> >>>>
> >>>>> not be needed as we're sampling traffic, there is nothing else
> >>>>> to sample.
> >>>>>
> >>>
> >>> OK, I'll change this to "drop"/ "acl-new" / "acl-est".
> >>>
> >>>>>> + </column>
> >>>>>> + <column name="id">
> >>>>>> + The identifier to be encoded in the (IPFIX) samples generated
> >>>>>> for this
> >>>>>
> >>>>> IPFIX here may be confusing, because collector set may not use IPFIX.
> >>>>>
> >
> > When I initially wrote this patch there was only IPFIX, I'll update it.
> >
> >>>>>> + type of application. This identifier is used as part of the
> >>>>>> sample's
> >>>>>> + observation domain ID.
> >>>>>> + </column>
> >>>>>> + <group title="Common Columns">
> >>>>>> + <column name="external_ids">
> >>>>>> + See <em>External IDs</em> at the beginning of this document.
> >>>>>> + </column>
> >>>>>> + </group>
> >>>>>> + </table>
> >>>>>> </database>
> >>>>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> >>>>>> index 27e8ec3388..b31da0063f 100644
> >>>>>> --- a/tests/ovn-northd.at
> >>>>>> +++ b/tests/ovn-northd.at
> >>>>>> @@ -12479,6 +12479,23 @@ check_engine_stats lflow recompute nocompute
> >>>>>>
> >>>>>> AT_CLEANUP
> >>>>>>
> >>>>>> +OVN_FOR_EACH_NORTHD_NO_HV([
> >>>>>> +AT_SETUP([Sampling_App incremental processing])
> >>>>>> +
> >>>>>> +ovn_start
> >>>>>> +
> >>>>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >>>>>> +
> >>>>>> +ovn-nbctl create Sampling_App name="acl-new-traffic-sampling" id="42"
> >>>>>> +check_row_count nb:Sampling_App 1
> >>>>>> +check_engine_stats sampling_app recompute nocompute
> >>>>>> +check_engine_stats northd norecompute nocompute
> >>>>>> +check_engine_stats lflow recompute nocompute
> >>>>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>>>>> +
> >>>>>> +AT_CLEANUP
> >>>>>> +])
> >>>>>> +
> >>>>>> OVN_FOR_EACH_NORTHD_NO_HV([
> >>>>>> AT_SETUP([NAT with match])
> >>>>>> ovn_start
> >>>>>
> >>>>
> >>>
> >>> Thanks,
> >>> Dumitru
> >>>
> >>
> >
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
