| Message ID | 20240731090520.342643-4-dceara@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add ACL Sampling using per-flow IPFIX. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On Wed, Jul 31, 2024 at 11:05 AM Dumitru Ceara <dceara@redhat.com> 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 <mmichels@redhat.com> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > 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 > > diff --git a/northd/automake.mk b/northd/automake.mk > index d491973a8b..6566ad2999 100644 > --- a/northd/automake.mk > +++ b/northd/automake.mk > @@ -32,6 +32,8 @@ northd_ovn_northd_SOURCES = \ > northd/en-lr-stateful.h \ > northd/en-ls-stateful.c \ > northd/en-ls-stateful.h \ > + northd/en-sampling-app.c \ > + northd/en-sampling-app.h \ > northd/inc-proc-northd.c \ > northd/inc-proc-northd.h \ > northd/ipam.c \ > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > index c4b927fb8c..eb91f2a651 100644 > --- a/northd/en-lflow.c > +++ b/northd/en-lflow.c > @@ -25,6 +25,7 @@ > #include "en-ls-stateful.h" > #include "en-northd.h" > #include "en-meters.h" > +#include "en-sampling-app.h" > #include "lflow-mgr.h" > > #include "lib/inc-proc-eng.h" > @@ -86,6 +87,10 @@ lflow_get_input_data(struct engine_node *node, > lflow_input->ovn_internal_version_changed = > global_config->ovn_internal_version_changed; > lflow_input->svc_monitor_mac = global_config->svc_monitor_mac; > + > + struct ed_type_sampling_app_data *sampling_app_data = > + engine_get_input_data("sampling_app", node); > + lflow_input->sampling_apps = &sampling_app_data->apps; > } > > void en_lflow_run(struct engine_node *node, void *data) > diff --git a/northd/en-sampling-app.c b/northd/en-sampling-app.c > new file mode 100644 > index 0000000000..03706561e7 > --- /dev/null > +++ b/northd/en-sampling-app.c > @@ -0,0 +1,120 @@ > +/* > + * Copyright (c) 2024, Red Hat, Inc. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#include <config.h> > + > +#include "openvswitch/vlog.h" > + > +#include "en-sampling-app.h" > + > +VLOG_DEFINE_THIS_MODULE(en_sampling_app); > + > +/* Static function declarations. */ > +static void sampling_app_table_add(struct sampling_app_table *, > + const struct nbrec_sampling_app *); > +static uint8_t sampling_app_table_get_id(const struct sampling_app_table > *, > + enum sampling_app_type); > +static void sampling_app_table_reset(struct sampling_app_table *); > +static enum sampling_app_type sampling_app_get_by_name(const char > *app_name); > + > +void * > +en_sampling_app_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + struct ed_type_sampling_app_data *data = xzalloc(sizeof *data); > + sampling_app_table_reset(&data->apps); > + return data; > +} > + > +void > +en_sampling_app_cleanup(void *data OVS_UNUSED) > +{ > +} > + > +void > +en_sampling_app_run(struct engine_node *node, void *data_) > +{ > + const struct nbrec_sampling_app_table *nb_sampling_app_table = > + EN_OVSDB_GET(engine_get_input("NB_sampling_app", node)); > + struct ed_type_sampling_app_data *data = data_; > + > + sampling_app_table_reset(&data->apps); > + > + const struct nbrec_sampling_app *sa; > + NBREC_SAMPLING_APP_TABLE_FOR_EACH (sa, nb_sampling_app_table) { > + sampling_app_table_add(&data->apps, sa); > + } > + > + engine_set_node_state(node, EN_UPDATED); > +} > + > +uint8_t > +sampling_app_get_id(const struct sampling_app_table *app_table, > + enum sampling_app_type app_type) > +{ > + return sampling_app_table_get_id(app_table, app_type); > +} > + > +/* Static functions. */ > +static void > +sampling_app_table_add(struct sampling_app_table *table, > + const struct nbrec_sampling_app *sa) > +{ > + enum sampling_app_type app_type = sampling_app_get_by_name(sa->name); > + > + if (app_type == SAMPLING_APP_MAX) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_WARN_RL(&rl, "Unexpected Sampling_App name: %s", sa->name); > + return; > + } > + table->app_ids[app_type] = sa->id; > +} > + > +static uint8_t > +sampling_app_table_get_id(const struct sampling_app_table *table, > + enum sampling_app_type app_type) > +{ > + ovs_assert(app_type < SAMPLING_APP_MAX); > + return table->app_ids[app_type]; > +} > + > +static void > +sampling_app_table_reset(struct sampling_app_table *table) > +{ > + for (size_t i = 0; i < SAMPLING_APP_MAX; i++) { > + table->app_ids[i] = SAMPLING_APP_ID_NONE; > + } > +} > + > +static const char *app_names[] = { > + [SAMPLING_APP_DROP_DEBUG] = > + "drop-sampling", > + [SAMPLING_APP_ACL_NEW_TRAFFIC] = > + "acl-new-traffic-sampling", > + [SAMPLING_APP_ACL_EST_TRAFFIC] = > + "acl-est-traffic-sampling", > +}; > + > +static enum sampling_app_type > +sampling_app_get_by_name(const char *app_name) > +{ > + for (size_t app_type = 0; app_type < ARRAY_SIZE(app_names); > app_type++) { > + if (!strcmp(app_name, app_names[app_type])) { > + return app_type; > + } > + } > + return SAMPLING_APP_MAX; > +} > diff --git a/northd/en-sampling-app.h b/northd/en-sampling-app.h > new file mode 100644 > index 0000000000..d80bfe2921 > --- /dev/null > +++ b/northd/en-sampling-app.h > @@ -0,0 +1,51 @@ > +/* > + * Copyright (c) 2024, Red Hat, Inc. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > +#ifndef EN_SAMPLING_APP_H > +#define EN_SAMPLING_APP_H 1 > + > +/* OVS includes. */ > +#include "openvswitch/shash.h" > + > +/* OVN includes. */ > +#include "lib/inc-proc-eng.h" > +#include "lib/ovn-nb-idl.h" > + > +/* Valid sample IDs are in the 1..255 interval. */ > +#define SAMPLING_APP_ID_NONE 0 > + > +/* 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, > +}; > + > +struct sampling_app_table { > + uint8_t app_ids[SAMPLING_APP_MAX]; > +}; > + > +struct ed_type_sampling_app_data { > + struct sampling_app_table apps; > +}; > + > +void *en_sampling_app_init(struct engine_node *, struct engine_arg *); > +void en_sampling_app_cleanup(void *data); > +void en_sampling_app_run(struct engine_node *, void *data); > +uint8_t sampling_app_get_id(const struct sampling_app_table *, > + enum sampling_app_type); > + > +#endif /* EN_SAMPLING_APP_H */ > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 522236ad2a..5d89670c29 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -39,6 +39,7 @@ > #include "en-lflow.h" > #include "en-northd-output.h" > #include "en-meters.h" > +#include "en-sampling-app.h" > #include "en-sync-sb.h" > #include "en-sync-from-sb.h" > #include "unixctl.h" > @@ -61,7 +62,8 @@ static unixctl_cb_func chassis_features_list; > NB_NODE(meter, "meter") \ > NB_NODE(bfd, "bfd") \ > NB_NODE(static_mac_binding, "static_mac_binding") \ > - NB_NODE(chassis_template_var, "chassis_template_var") > + NB_NODE(chassis_template_var, "chassis_template_var") \ > + NB_NODE(sampling_app, "sampling_app") > > enum nb_engine_node { > #define NB_NODE(NAME, NAME_STR) NB_##NAME, > @@ -138,6 +140,7 @@ enum sb_engine_node { > * avoid sparse errors. */ > static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(northd, "northd"); > static ENGINE_NODE(sync_from_sb, "sync_from_sb"); > +static ENGINE_NODE(sampling_app, "sampling_app"); > static ENGINE_NODE(lflow, "lflow"); > static ENGINE_NODE(mac_binding_aging, "mac_binding_aging"); > static ENGINE_NODE(mac_binding_aging_waker, "mac_binding_aging_waker"); > @@ -170,6 +173,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_lb_data, &en_nb_logical_router, > lb_data_logical_router_handler); > > + engine_add_input(&en_sampling_app, &en_nb_sampling_app, NULL); > + > engine_add_input(&en_global_config, &en_nb_nb_global, > global_config_nb_global_handler); > engine_add_input(&en_global_config, &en_sb_sb_global, > @@ -251,6 +256,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_lflow, &en_sb_logical_dp_group, NULL); > engine_add_input(&en_lflow, &en_global_config, > node_global_config_handler); > + > + engine_add_input(&en_lflow, &en_sampling_app, NULL); > + > engine_add_input(&en_lflow, &en_northd, lflow_northd_handler); > engine_add_input(&en_lflow, &en_port_group, lflow_port_group_handler); > engine_add_input(&en_lflow, &en_lr_stateful, > lflow_lr_stateful_handler); > diff --git a/northd/northd.h b/northd/northd.h > index d4a8d75abc..e50aa6731a 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -190,6 +190,7 @@ struct lflow_input { > const struct hmap *svc_monitor_map; > bool ovn_internal_version_changed; > const char *svc_monitor_mac; > + const struct sampling_app_table *sampling_apps; > }; > > extern int parallelization_state; > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema > index e3c4aff9df..836d742390 100644 > --- a/ovn-nb.ovsschema > +++ b/ovn-nb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Northbound", > - "version": "7.4.0", > - "cksum": "1908497390 35615", > + "version": "7.5.0", > + "cksum": "2718852723 36355", > "tables": { > "NB_Global": { > "columns": { > @@ -691,6 +691,23 @@ > "type": {"key": "string", "value": "string", > "min": 0, "max": "unlimited"}}}, > "indexes": [["chassis"]], > - "isRoot": true} > + "isRoot": true}, > + "Sampling_App": { > + "columns": { > + "name": {"type": {"key": {"type": "string", > + "enum": ["set", > + ["drop-sampling", > + "acl-new-traffic-sampling", > + "acl-est-traffic-sampling"]]}}}, > + "id": {"type": {"key": {"type": "integer", > + "minInteger": 1, > + "maxInteger": 255}}}, > + "external_ids": { > + "type": {"key": "string", "value": "string", > + "min": 0, "max": "unlimited"}} > + }, > + "indexes": [["name"]], > + "isRoot": true > + } > } > } > 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"> > + The name of the application to be configured for sampling. > Currently > + supported options are: "drop-sampling", "acl-new-traffic-sampling", > + "acl-est-traffic-sampling". > + </column> > + <column name="id"> > + The identifier to be encoded in the (IPFIX) samples generated for > this > + 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 > -- > 2.45.2 > > Acked-by: Ales Musil <amusil@redhat.com>
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 <mmichels@redhat.com> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > 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. > + 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 of the sampling application after all. Also, 'traffic' may also not be needed as we're sampling traffic, there is nothing else to sample. > + </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
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 <mmichels@redhat.com> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> 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. > >> + 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. > >> + </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 >
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 <mmichels@redhat.com> >>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>> --- >>> 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
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 <mmichels@redhat.com> >>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>>> --- >>>> 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 >
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 <mmichels@redhat.com> >>>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>>>> --- >>>>> 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 >> >
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 <mmichels@redhat.com> >>>>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>>>>> --- >>>>>> 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 >>> >> >
On Wed, Jul 31, 2024 at 11:48 AM Ilya Maximets <i.maximets@ovn.org> 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 <mmichels@redhat.com> > >>>>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> > >>>>>> --- > >>>>>> 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. 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. Thanks Numan > > > > >>> > >>>>>> + 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Jul 31, 2024 at 4:20 PM Numan Siddique <numans@ovn.org> wrote: > > On Wed, Jul 31, 2024 at 11:48 AM Ilya Maximets <i.maximets@ovn.org> 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 <mmichels@redhat.com> > > >>>>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > >>>>>> --- > > >>>>>> 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. > > 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 <numans@ovn.org> Numan > > Thanks > Numan > > > > > > > > > > > > > > > >>> > > >>>>>> + 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 8/1/24 00:57, Numan Siddique wrote: > On Wed, Jul 31, 2024 at 4:20 PM Numan Siddique <numans@ovn.org> wrote: >> >> On Wed, Jul 31, 2024 at 11:48 AM Ilya Maximets <i.maximets@ovn.org> 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 <mmichels@redhat.com> >>>>>>>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>>>>>>>> --- >>>>>>>>> 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 <numans@ovn.org> > Thanks, Numan and Ilya! I'll make these changes and post v5. Regards, Dumitru
diff --git a/northd/automake.mk b/northd/automake.mk index d491973a8b..6566ad2999 100644 --- a/northd/automake.mk +++ b/northd/automake.mk @@ -32,6 +32,8 @@ northd_ovn_northd_SOURCES = \ northd/en-lr-stateful.h \ northd/en-ls-stateful.c \ northd/en-ls-stateful.h \ + northd/en-sampling-app.c \ + northd/en-sampling-app.h \ northd/inc-proc-northd.c \ northd/inc-proc-northd.h \ northd/ipam.c \ diff --git a/northd/en-lflow.c b/northd/en-lflow.c index c4b927fb8c..eb91f2a651 100644 --- a/northd/en-lflow.c +++ b/northd/en-lflow.c @@ -25,6 +25,7 @@ #include "en-ls-stateful.h" #include "en-northd.h" #include "en-meters.h" +#include "en-sampling-app.h" #include "lflow-mgr.h" #include "lib/inc-proc-eng.h" @@ -86,6 +87,10 @@ lflow_get_input_data(struct engine_node *node, lflow_input->ovn_internal_version_changed = global_config->ovn_internal_version_changed; lflow_input->svc_monitor_mac = global_config->svc_monitor_mac; + + struct ed_type_sampling_app_data *sampling_app_data = + engine_get_input_data("sampling_app", node); + lflow_input->sampling_apps = &sampling_app_data->apps; } void en_lflow_run(struct engine_node *node, void *data) diff --git a/northd/en-sampling-app.c b/northd/en-sampling-app.c new file mode 100644 index 0000000000..03706561e7 --- /dev/null +++ b/northd/en-sampling-app.c @@ -0,0 +1,120 @@ +/* + * Copyright (c) 2024, Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <config.h> + +#include "openvswitch/vlog.h" + +#include "en-sampling-app.h" + +VLOG_DEFINE_THIS_MODULE(en_sampling_app); + +/* Static function declarations. */ +static void sampling_app_table_add(struct sampling_app_table *, + const struct nbrec_sampling_app *); +static uint8_t sampling_app_table_get_id(const struct sampling_app_table *, + enum sampling_app_type); +static void sampling_app_table_reset(struct sampling_app_table *); +static enum sampling_app_type sampling_app_get_by_name(const char *app_name); + +void * +en_sampling_app_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED) +{ + struct ed_type_sampling_app_data *data = xzalloc(sizeof *data); + sampling_app_table_reset(&data->apps); + return data; +} + +void +en_sampling_app_cleanup(void *data OVS_UNUSED) +{ +} + +void +en_sampling_app_run(struct engine_node *node, void *data_) +{ + const struct nbrec_sampling_app_table *nb_sampling_app_table = + EN_OVSDB_GET(engine_get_input("NB_sampling_app", node)); + struct ed_type_sampling_app_data *data = data_; + + sampling_app_table_reset(&data->apps); + + const struct nbrec_sampling_app *sa; + NBREC_SAMPLING_APP_TABLE_FOR_EACH (sa, nb_sampling_app_table) { + sampling_app_table_add(&data->apps, sa); + } + + engine_set_node_state(node, EN_UPDATED); +} + +uint8_t +sampling_app_get_id(const struct sampling_app_table *app_table, + enum sampling_app_type app_type) +{ + return sampling_app_table_get_id(app_table, app_type); +} + +/* Static functions. */ +static void +sampling_app_table_add(struct sampling_app_table *table, + const struct nbrec_sampling_app *sa) +{ + enum sampling_app_type app_type = sampling_app_get_by_name(sa->name); + + if (app_type == SAMPLING_APP_MAX) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); + VLOG_WARN_RL(&rl, "Unexpected Sampling_App name: %s", sa->name); + return; + } + table->app_ids[app_type] = sa->id; +} + +static uint8_t +sampling_app_table_get_id(const struct sampling_app_table *table, + enum sampling_app_type app_type) +{ + ovs_assert(app_type < SAMPLING_APP_MAX); + return table->app_ids[app_type]; +} + +static void +sampling_app_table_reset(struct sampling_app_table *table) +{ + for (size_t i = 0; i < SAMPLING_APP_MAX; i++) { + table->app_ids[i] = SAMPLING_APP_ID_NONE; + } +} + +static const char *app_names[] = { + [SAMPLING_APP_DROP_DEBUG] = + "drop-sampling", + [SAMPLING_APP_ACL_NEW_TRAFFIC] = + "acl-new-traffic-sampling", + [SAMPLING_APP_ACL_EST_TRAFFIC] = + "acl-est-traffic-sampling", +}; + +static enum sampling_app_type +sampling_app_get_by_name(const char *app_name) +{ + for (size_t app_type = 0; app_type < ARRAY_SIZE(app_names); app_type++) { + if (!strcmp(app_name, app_names[app_type])) { + return app_type; + } + } + return SAMPLING_APP_MAX; +} diff --git a/northd/en-sampling-app.h b/northd/en-sampling-app.h new file mode 100644 index 0000000000..d80bfe2921 --- /dev/null +++ b/northd/en-sampling-app.h @@ -0,0 +1,51 @@ +/* + * Copyright (c) 2024, Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#ifndef EN_SAMPLING_APP_H +#define EN_SAMPLING_APP_H 1 + +/* OVS includes. */ +#include "openvswitch/shash.h" + +/* OVN includes. */ +#include "lib/inc-proc-eng.h" +#include "lib/ovn-nb-idl.h" + +/* Valid sample IDs are in the 1..255 interval. */ +#define SAMPLING_APP_ID_NONE 0 + +/* 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, +}; + +struct sampling_app_table { + uint8_t app_ids[SAMPLING_APP_MAX]; +}; + +struct ed_type_sampling_app_data { + struct sampling_app_table apps; +}; + +void *en_sampling_app_init(struct engine_node *, struct engine_arg *); +void en_sampling_app_cleanup(void *data); +void en_sampling_app_run(struct engine_node *, void *data); +uint8_t sampling_app_get_id(const struct sampling_app_table *, + enum sampling_app_type); + +#endif /* EN_SAMPLING_APP_H */ diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 522236ad2a..5d89670c29 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -39,6 +39,7 @@ #include "en-lflow.h" #include "en-northd-output.h" #include "en-meters.h" +#include "en-sampling-app.h" #include "en-sync-sb.h" #include "en-sync-from-sb.h" #include "unixctl.h" @@ -61,7 +62,8 @@ static unixctl_cb_func chassis_features_list; NB_NODE(meter, "meter") \ NB_NODE(bfd, "bfd") \ NB_NODE(static_mac_binding, "static_mac_binding") \ - NB_NODE(chassis_template_var, "chassis_template_var") + NB_NODE(chassis_template_var, "chassis_template_var") \ + NB_NODE(sampling_app, "sampling_app") enum nb_engine_node { #define NB_NODE(NAME, NAME_STR) NB_##NAME, @@ -138,6 +140,7 @@ enum sb_engine_node { * avoid sparse errors. */ static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(northd, "northd"); static ENGINE_NODE(sync_from_sb, "sync_from_sb"); +static ENGINE_NODE(sampling_app, "sampling_app"); static ENGINE_NODE(lflow, "lflow"); static ENGINE_NODE(mac_binding_aging, "mac_binding_aging"); static ENGINE_NODE(mac_binding_aging_waker, "mac_binding_aging_waker"); @@ -170,6 +173,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_lb_data, &en_nb_logical_router, lb_data_logical_router_handler); + engine_add_input(&en_sampling_app, &en_nb_sampling_app, NULL); + engine_add_input(&en_global_config, &en_nb_nb_global, global_config_nb_global_handler); engine_add_input(&en_global_config, &en_sb_sb_global, @@ -251,6 +256,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_lflow, &en_sb_logical_dp_group, NULL); engine_add_input(&en_lflow, &en_global_config, node_global_config_handler); + + engine_add_input(&en_lflow, &en_sampling_app, NULL); + engine_add_input(&en_lflow, &en_northd, lflow_northd_handler); engine_add_input(&en_lflow, &en_port_group, lflow_port_group_handler); engine_add_input(&en_lflow, &en_lr_stateful, lflow_lr_stateful_handler); diff --git a/northd/northd.h b/northd/northd.h index d4a8d75abc..e50aa6731a 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -190,6 +190,7 @@ struct lflow_input { const struct hmap *svc_monitor_map; bool ovn_internal_version_changed; const char *svc_monitor_mac; + const struct sampling_app_table *sampling_apps; }; extern int parallelization_state; diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema index e3c4aff9df..836d742390 100644 --- a/ovn-nb.ovsschema +++ b/ovn-nb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Northbound", - "version": "7.4.0", - "cksum": "1908497390 35615", + "version": "7.5.0", + "cksum": "2718852723 36355", "tables": { "NB_Global": { "columns": { @@ -691,6 +691,23 @@ "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}}, "indexes": [["chassis"]], - "isRoot": true} + "isRoot": true}, + "Sampling_App": { + "columns": { + "name": {"type": {"key": {"type": "string", + "enum": ["set", + ["drop-sampling", + "acl-new-traffic-sampling", + "acl-est-traffic-sampling"]]}}}, + "id": {"type": {"key": {"type": "integer", + "minInteger": 1, + "maxInteger": 255}}}, + "external_ids": { + "type": {"key": "string", "value": "string", + "min": 0, "max": "unlimited"}} + }, + "indexes": [["name"]], + "isRoot": true + } } } 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"> + The name of the application to be configured for sampling. Currently + supported options are: "drop-sampling", "acl-new-traffic-sampling", + "acl-est-traffic-sampling". + </column> + <column name="id"> + The identifier to be encoded in the (IPFIX) samples generated for this + 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