On Wed, Jul 31, 2024 at 4:20 PM Numan Siddique <num...@ovn.org> wrote:
>
> On Wed, Jul 31, 2024 at 11:48 AM Ilya Maximets <i.maxim...@ovn.org> 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 <mmich...@redhat.com>
> > >>>>>> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> > >>>>>> ---
> > >>>>>> 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.
>
With the column name change from "name" to "type" addressed:
Acked-by: Numan Siddique <num...@ovn.org>
Numan
>
> 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
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
