On 7/15/24 11:18, Ales Musil wrote:
> On Fri, Jul 12, 2024 at 5:15 PM Dumitru Ceara <dce...@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).
>>
>> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
>> ---
>>
>
> Hi Dumitru,
> I have a couple of small comments down below.
>
> 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 | 21 ++++++-
>> ovn-nb.xml | 17 ++++++
>> tests/ovn-northd.at | 17 ++++++
>> 9 files changed, 241 insertions(+), 3 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..8d2d45a90f
>> --- /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)
>>
>
> nit: Wrong format of the function return type, that seems to be the case
> for the whole file.
>
Oops, I was too much in a hurry, will fix them up.
>
>> +{
>> + 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..e131a4c083 100644
>> --- a/ovn-nb.ovsschema
>> +++ b/ovn-nb.ovsschema
>> @@ -1,7 +1,7 @@
>> {
>> "name": "OVN_Northbound",
>> "version": "7.4.0",
>>
>
> Missing version bump.
>
It was on purpose as I don't expect this patch to ever be committed on
its own but instead together with the one that adds Sample to the
schema. But I can bump the version twice, no problem.
> - "cksum": "1908497390 35615",
>> + "cksum": "1498303893 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 9552534f6d..f2a8b5c076 100644
>> --- a/ovn-nb.xml
>> +++ b/ovn-nb.xml
>> @@ -5021,4 +5021,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 5acb13c519..f2f11c005c 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -12449,6 +12449,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.44.0
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
> Thanks,
> Ales
Thanks,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
