On Fri, Jul 12, 2024 at 5:15 PM Dumitru Ceara <dce...@redhat.com> wrote:
> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> ---
>
Hi Dumitru,
I have a couple of small comments down below.
NEWS | 3 +++
> northd/debug.c | 12 +++++++-----
> northd/debug.h | 3 ++-
> northd/en-global-config.c | 31 +++++++++++++++++++++++--------
> northd/inc-proc-northd.c | 1 +
> tests/ovn-northd.at | 27 ++++++++++++++++++++++++++-
> 6 files changed, 62 insertions(+), 15 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 3e392ff08b..fcf182bc02 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -38,6 +38,9 @@ Post v24.03.0
> ability to disable "VXLAN mode" to extend available tunnel IDs space
> for
> datapaths from 4095 to 16711680. For more details see man ovn-nb(5)
> for
> mentioned option.
> + - The NB_Global.debug_drop_domain_id configured value is now overridden
> by
> + the ID associated with the Sampling_App record created for drop
> sampling
> + (Sampling_App.name configured to be "drop-sampling").
>
Should we also deprecate the old global option once this drops? I think it
would make sense.
> OVN v24.03.0 - 01 Mar 2024
> --------------------------
> diff --git a/northd/debug.c b/northd/debug.c
> index 59da5d4f66..457993b7cf 100644
> --- a/northd/debug.c
> +++ b/northd/debug.c
> @@ -3,6 +3,7 @@
> #include <string.h>
>
> #include "debug.h"
> +#include "en-sampling-app.h"
>
> #include "openvswitch/dynamic-string.h"
> #include "openvswitch/vlog.h"
> @@ -26,16 +27,17 @@ debug_enabled(void)
> }
>
> void
> -init_debug_config(const struct nbrec_nb_global *nb)
> +init_debug_config(const struct nbrec_nb_global *nb,
> + uint8_t drop_domain_id_override)
> {
> -
> const struct smap *options = &nb->options;
> uint32_t collector_set_id = smap_get_uint(options,
> "debug_drop_collector_set",
> 0);
> - uint32_t observation_domain_id = smap_get_uint(options,
> - "debug_drop_domain_id",
> - 0);
> + uint32_t observation_domain_id =
> + drop_domain_id_override != SAMPLING_APP_ID_NONE
> + ? drop_domain_id_override
> + : smap_get_uint(options, "debug_drop_domain_id", 0);
>
> if (collector_set_id != config.collector_set_id ||
> observation_domain_id != config.observation_domain_id ||
> diff --git a/northd/debug.h b/northd/debug.h
> index c1a5e5aadb..a0b535253a 100644
> --- a/northd/debug.h
> +++ b/northd/debug.h
> @@ -21,7 +21,8 @@
> #include "lib/ovn-nb-idl.h"
> #include "openvswitch/dynamic-string.h"
>
> -void init_debug_config(const struct nbrec_nb_global *nb);
> +void init_debug_config(const struct nbrec_nb_global *nb,
> + uint8_t drop_domain_id_override);
> void destroy_debug_config(void);
>
> const char *debug_drop_action(void);
> diff --git a/northd/en-global-config.c b/northd/en-global-config.c
> index 5b71ede1f2..c683c8fae5 100644
> --- a/northd/en-global-config.c
> +++ b/northd/en-global-config.c
> @@ -24,6 +24,7 @@
> /* OVN includes */
> #include "debug.h"
> #include "en-global-config.h"
> +#include "en-sampling-app.h"
> #include "include/ovn/features.h"
> #include "ipam.h"
> #include "lib/ovn-nb-idl.h"
> @@ -42,8 +43,10 @@ static bool chassis_features_changed(const struct
> chassis_features *,
> static bool config_out_of_sync(const struct smap *config,
> const struct smap *saved_config,
> const char *key, bool must_be_present);
> -static bool check_nb_options_out_of_sync(const struct nbrec_nb_global *,
> - struct ed_type_global_config *);
> +static bool check_nb_options_out_of_sync(
> + const struct nbrec_nb_global *,
> + struct ed_type_global_config *,
> + const struct sampling_app_table *);
> static void update_sb_config_options_to_sbrec(struct
> ed_type_global_config *,
> const struct
> sbrec_sb_global *);
>
> @@ -72,6 +75,9 @@ en_global_config_run(struct engine_node *node , void
> *data)
> EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
> const struct sbrec_chassis_table *sbrec_chassis_table =
> EN_OVSDB_GET(engine_get_input("SB_chassis", node));
> + const struct ed_type_sampling_app_data *sampling_app_data =
> + engine_get_input_data("sampling_app", node);
> + const struct sampling_app_table *sampling_apps =
> &sampling_app_data->apps;
>
> struct ed_type_global_config *config_data = data;
>
> @@ -145,7 +151,8 @@ en_global_config_run(struct engine_node *node , void
> *data)
> build_chassis_features(sbrec_chassis_table,
> &config_data->features);
> }
>
> - init_debug_config(nb);
> + init_debug_config(nb, sampling_app_get_id(sampling_apps,
> + SAMPLING_APP_DROP_DEBUG));
>
> const struct sbrec_sb_global *sb =
> sbrec_sb_global_table_first(sb_global_table);
> @@ -186,6 +193,9 @@ global_config_nb_global_handler(struct engine_node
> *node, void *data)
> EN_OVSDB_GET(engine_get_input("NB_nb_global", node));
> const struct sbrec_sb_global_table *sb_global_table =
> EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
> + const struct ed_type_sampling_app_data *sampling_app_data =
> + engine_get_input_data("sampling_app", node);
> + const struct sampling_app_table *sampling_apps =
> &sampling_app_data->apps;
>
> const struct nbrec_nb_global *nb =
> nbrec_nb_global_table_first(nb_global_table);
> @@ -248,7 +258,7 @@ global_config_nb_global_handler(struct engine_node
> *node, void *data)
> return false;
> }
>
> - if (check_nb_options_out_of_sync(nb, config_data)) {
> + if (check_nb_options_out_of_sync(nb, config_data, sampling_apps)) {
> config_data->tracked_data.nb_options_changed = true;
> }
>
> @@ -461,8 +471,10 @@ config_out_of_sync(const struct smap *config, const
> struct smap *saved_config,
> }
>
> static bool
> -check_nb_options_out_of_sync(const struct nbrec_nb_global *nb,
> - struct ed_type_global_config *config_data)
> +check_nb_options_out_of_sync(
> + const struct nbrec_nb_global *nb,
> + struct ed_type_global_config *config_data,
> + const struct sampling_app_table *sampling_apps)
> {
> if (config_out_of_sync(&nb->options, &config_data->nb_options,
> "mac_binding_removal_limit", false)) {
> @@ -496,13 +508,16 @@ check_nb_options_out_of_sync(const struct
> nbrec_nb_global *nb,
>
> if (config_out_of_sync(&nb->options, &config_data->nb_options,
> "debug_drop_domain_id", false)) {
> - init_debug_config(nb);
> + init_debug_config(nb, sampling_app_get_id(sampling_apps,
> +
> SAMPLING_APP_DROP_DEBUG));
> +
> return true;
> }
>
> if (config_out_of_sync(&nb->options, &config_data->nb_options,
> "debug_drop_collector_set", false)) {
> - init_debug_config(nb);
> + init_debug_config(nb, sampling_app_get_id(sampling_apps,
> +
> SAMPLING_APP_DROP_DEBUG));
> return true;
> }
>
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index 5d89670c29..95bedc5cd0 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -181,6 +181,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> global_config_sb_global_handler);
> engine_add_input(&en_global_config, &en_sb_chassis,
> global_config_sb_chassis_handler);
> + engine_add_input(&en_global_config, &en_sampling_app, NULL);
>
> engine_add_input(&en_northd, &en_nb_mirror, NULL);
> engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL);
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index f2f11c005c..3fb883225a 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -12459,13 +12459,38 @@ 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 northd recomput nocompute
>
nit: s/recomput/recompute/
> check_engine_stats lflow recompute nocompute
> +check_engine_stats global_config recompute nocompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([Sampling_App override debug_drop_domain_id])
> +
> +ovn_start
> +
> +check ovn-nbctl -- set NB_Global . options:debug_drop_collector_set="123"
> \
> + -- set NB_Global . options:debug_drop_domain_id="1" \
> + -- ls-add ls
> +check ovn-nbctl --wait=sb sync
> +
> +AT_CHECK([ovn-sbctl lflow-list | grep ls_in_l2_unknown.*sample |
> ovn_strip_lflows], [0], [dnl
> + table=??(ls_in_l2_unknown ), priority=50 , match=(outport ==
> "none"),
> action=(sample(probability=65535,collector_set=123,obs_domain=1,obs_point=$cookie);
> /* drop */)
> +])
> +
> +ovn-nbctl --wait=sb create Sampling_App name="drop-sampling" id="42"
> +check_row_count nb:Sampling_App 1
> +
> +AT_CHECK([ovn-sbctl lflow-list | grep ls_in_l2_unknown.*sample |
> ovn_strip_lflows], [0], [dnl
> + table=??(ls_in_l2_unknown ), priority=50 , match=(outport ==
> "none"),
> action=(sample(probability=65535,collector_set=123,obs_domain=42,obs_point=$cookie);
> /* drop */)
> +])
> +
> +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
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
