On 8/1/24 00:57, Numan Siddique wrote:
> On Wed, Jul 31, 2024 at 4:20 PM Numan Siddique <[email protected]> wrote:
>>
>> 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.
>>
Ack.
>> 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 <[email protected]>
>
Thanks, Numan and Ilya! I'll make these changes and post v5.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
