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.
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.
Maybe others could share their opinion on this too. I'll wait a bit
with sending v5 until we clear this up.
>>
>>>>> + 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
