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. I could change it to 'type' but for me 'name' made more sense. >>> + 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
