Re: [ovs-dev] [PATCH ovn v3 7/8] northd: Override NB_Global drop sampling id with Sampling_App config.

Tue, 23 Jul 2024 12:56:45 -0700 

On 7/23/24 15:20, Mark Michelson wrote:

On 7/23/24 15:18, Mark Michelson wrote:

I have no additions other than Ales's, so with those fixed,

Acked-by: Mark Michelson <mmich...@redhat.com>
Whoops, this was meant to be my reply on patch 6, not patch 7. I'll move the reply to patch 6, and you can ignore this reply for now. It's possible that once I actually review patch 7 this will also be my opinion. Please hold...
As it turns out, I still have the same opinion as I originally replied when I thought I was replying to patch 6. 

Acked-by: Mark Michelson <mmich...@redhat.com>

(after addressing Ales's comments)


On 7/15/24 05:22, Ales Musil wrote:

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







_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to