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.
>
> 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}}}
?
This way we don't need a table, don't need an index or strange column names,
likely don't need a separate I-P node. We're just turning debug_drop_domain_id
into a new column with a set.
>
>>>> + 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.
>>>
>>>> + 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
