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