On Mon, Dec 12, 2022 at 2:17 PM Ihar Hrachyshka <[email protected]> wrote:
>
> I appreciate the split of the code into pieces, since it became hard
> to track the changes and focus discussion on parts that are indeed
> controversial / require additional discussion, compared to rather
> static pieces.
>
> I'm not COMPLETELY thrilled by how the split is done (e.g. this patch
> includes southbound database changes that are not tested in any way in
> the newly added test cases), but I am personally not as concerned
> about it (others may probably have different ideas though).
>
> I think this patch is good and could be merged to get this piece out
> of the way (assuming code style errors are fixed). Granted, we may
> still have to line up all pieces and prove they work before any of the
> pieces actually merge (that's up to core maintainers). Anyway...
>

I think it was my suggestion to include the SB DB schema changes in this patch.

But I totally agree with Ihar.  Those changes should be part of patch 2.

This patch misses a few things like documentation in ovn-nb.xml.

I initially thought of applying this patch series and modified myself.
But I agree with Ihar to enhance the test case to cover the filter
type "both" scenario.

Can you please enhance the test case and submit v19 from here -
https://github.com/numansiddique/ovn/commits/port_mirror_v18_p3

I did the following changes in the patch 1

-------------------------------------------------------------------------------------------
diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
index bb747c187..eacbd8ae9 100644
--- a/ovn-nb.ovsschema
+++ b/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
     "name": "OVN_Northbound",
-    "version": "6.5.0",
-    "cksum": "3702451195 33843",
+    "version": "7.0.0",
+    "cksum": "1654169739 33478",
     "tables": {
         "NB_Global": {
             "columns": {
@@ -135,8 +135,8 @@
                 "mirror_rules": {"type": {"key": {"type": "uuid",
                                           "refTable": "Mirror",
                                           "refType": "weak"},
-                                  "min": 0,
-                                  "max": "unlimited"}},
+                                 "min": 0,
+                                 "max": "unlimited"}},
                 "ha_chassis_group": {
                     "type": {"key": {"type": "uuid",
                                      "refTable": "HA_Chassis_Group",
@@ -315,14 +315,8 @@
                                                              "both"]]}}},
                 "sink":{"type": "string"},
                 "type": {"type": {"key": {"type": "string",
-                                            "enum": ["set", ["gre",
-                                                             "erspan"]]}}},
+                                          "enum": ["set", ["gre",
"erspan"]]}}},
                 "index": {"type": "integer"},
-                "src": {"type": {"key": {"type": "uuid",
-                                           "refTable": "Logical_Switch_Port",
-                                           "refType": "weak"},
-                                   "min": 0,
-                                   "max": "unlimited"}},
                 "external_ids": {
                     "type": {"key": "string", "value": "string",
                              "min": 0, "max": "unlimited"}}},
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 4e0013cc1..ff1f6df89 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -2638,13 +2638,6 @@ or
       </p>
     </column>

-    <column name="src">
-      <p>
-        The value of this field represents a list of source ports for the
-        mirror. Please see the <ref table="Logical_Switch_Port"/> table.
-      </p>
-    </column>
-
     <column name="external_ids">
       See <em>External IDs</em> at the beginning of this document.
     </column>
diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
index 1986066ee..95c7c2d7e 100644
--- a/ovn-sb.ovsschema
+++ b/ovn-sb.ovsschema
@@ -1,7 +1,7 @@
 {
     "name": "OVN_Southbound",
-    "version": "20.27.0",
-    "cksum": "2610590441 30335",
+    "version": "20.26.0",
+    "cksum": "3311869408 29176",
     "tables": {
         "SB_Global": {
             "columns": {
@@ -142,23 +142,6 @@
             "indexes": [["datapath", "tunnel_key"],
                         ["datapath", "name"]],
             "isRoot": true},
-        "Mirror": {
-            "columns": {
-                "name": {"type": "string"},
-                "filter": {"type": {"key": {"type": "string",
-                                            "enum": ["set",
-                                                     ["from-lport",
-                                                      "to-lport","both"]]}}},
-                "sink":{"type": "string"},
-                "type": {"type": {"key": {"type": "string",
-                                            "enum": ["set",
-                                                     ["gre", "erspan"]]}}},
-                "index": {"type": "integer"},
-                "external_ids": {
-                    "type": {"key": "string", "value": "string",
-                             "min": 0, "max": "unlimited"}}},
-            "indexes": [["name"]],
-            "isRoot": true},
         "Meter": {
             "columns": {
                 "name": {"type": "string"},
@@ -247,11 +230,6 @@
                                                       "refTable": "Encap",
                                                       "refType": "weak"},
                                     "min": 0, "max": "unlimited"}},
-                "mirror_rules": {"type": {"key": {"type": "uuid",
-                                          "refTable": "Mirror",
-                                          "refType": "weak"},
-                                  "min": 0,
-                                  "max": "unlimited"}},
                 "mac": {"type": {"key": "string",
                                  "min": 0,
                                  "max": "unlimited"}},
diff --git a/ovn-sb.xml b/ovn-sb.xml
index ee0c5f77f..4f485b860 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -2881,51 +2881,6 @@ tcp.flags = RST;
     </column>
   </table>

-  <table name="Mirror" title="Mirror Entry">
-    <p>
-      Each row in this table represents one Mirror that can be used for
-      port mirroring. These Mirrors are referenced by the
-      <ref column="mirror_rules" table="Port_Binding"/> column in
-      the <ref table="Port_Binding"/> table.
-    </p>
-
-    <column name="name">
-      <p>
-        Represents the name of the mirror.
-      </p>
-    </column>
-
-    <column name="filter">
-      <p>
-        The value of this field represents selection criteria of the mirror.
-      </p>
-    </column>
-
-    <column name="sink">
-      <p>
-        The value of this field represents the destination/sink of the mirror.
-      </p>
-    </column>
-
-    <column name="type">
-      <p>
-        The value of this field represents the type of the tunnel used for
-        sending the mirrored packets
-      </p>
-    </column>
-
-    <column name="index">
-      <p>
-        The value of this field represents the key/idx depending on the
-        tunnel type configured
-      </p>
-    </column>
-
-    <column name="external_ids">
-      See <em>External IDs</em> at the beginning of this document.
-    </column>
-  </table>
-
   <table name="Meter" title="Meter entry">
     <p>
       Each row in this table represents a meter that can be used for QoS or
@@ -3428,11 +3383,6 @@ tcp.flags = RST;
       </column>
     </group>

-    <column name="mirror_rules">
-        Mirror rules that apply to the port binding.
-        Please see the <ref table="Mirror"/> table.
-    </column>
-
     <group title="Patch Options">
       <p>
         These options apply to logical ports with <ref column="type"/> of
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 8e0a110b5..730345264 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -472,19 +472,19 @@ mirror1:
   Sink     :  10.10.10.1
   Filter   :  from-lport
   Index/Key:  0
-  Sources  :  None attached
+
 mirror2:
   Type     :  erspan
   Sink     :  10.10.10.2
   Filter   :  both
   Index/Key:  1
-  Sources  :  None attached
+
 mirror3:
   Type     :  gre
   Sink     :  10.10.10.3
   Filter   :  to-lport
   Index/Key:  2
-  Sources  :  sw0-port1  sw0-port3
+
 ])

 dnl Detach one source port from mirror
@@ -493,28 +493,6 @@ check ovn-nbctl lsp-detach-mirror sw0-port3 mirror3
 dnl Check if the detach happened from source properly
 check_column "" nb:Logical_Switch_Port mirror_rules name=sw0-port3

-dnl Verify if detach source port from mirror happens properly
-AT_CHECK([ovn-nbctl mirror-list], [0], [dnl
-mirror1:
-  Type     :  gre
-  Sink     :  10.10.10.1
-  Filter   :  from-lport
-  Index/Key:  0
-  Sources  :  None attached
-mirror2:
-  Type     :  erspan
-  Sink     :  10.10.10.2
-  Filter   :  both
-  Index/Key:  1
-  Sources  :  None attached
-mirror3:
-  Type     :  gre
-  Sink     :  10.10.10.3
-  Filter   :  to-lport
-  Index/Key:  2
-  Sources  :  sw0-port1
-])
-
 dnl Delete a single mirror which has source attached.
 check ovn-nbctl mirror-del mirror3

@@ -528,13 +506,13 @@ mirror1:
   Sink     :  10.10.10.1
   Filter   :  from-lport
   Index/Key:  0
-  Sources  :  None attached
+
 mirror2:
   Type     :  erspan
   Sink     :  10.10.10.2
   Filter   :  both
   Index/Key:  1
-  Sources  :  None attached
+
 ])

 dnl Delete another mirror
@@ -549,7 +527,7 @@ mirror1:
   Sink     :  192.168.1.13
   Filter   :  from-lport
   Index/Key:  0
-  Sources  :  None attached
+
 ])

 dnl Delete all mirrors
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index 4bab22252..2a8e70bf7 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -1708,7 +1708,6 @@ nbctl_pre_lsp_mirror(struct ctl_context *ctx)
     ovsdb_idl_add_column(ctx->idl,
                          &nbrec_logical_switch_port_col_mirror_rules);
     ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_name);
-    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_src);
 }

 static int
@@ -1720,7 +1719,7 @@ mirror_cmp(const void *mirror1_, const void *mirror2_)
     const struct nbrec_mirror *mirror1 = *mirror_1;
     const struct nbrec_mirror *mirror2 = *mirror_2;

-    return strcmp(mirror1->name,mirror2->name);
+    return strcmp(mirror1->name, mirror2->name);
 }

 static char * OVS_WARN_UNUSED_RESULT
@@ -1770,7 +1769,6 @@ nbctl_lsp_attach_mirror(struct ctl_context *ctx)
         return;
     }

-
     /*check if a mirror rule actually exists on that name or not*/
     error = mirror_by_name_or_uuid(ctx, mirror_name, true, &mirror);
     if (error) {
@@ -1780,11 +1778,13 @@ nbctl_lsp_attach_mirror(struct ctl_context *ctx)

     /* Check if same mirror rule already exists for the lsp */
     for (size_t i = 0; i < lsp->n_mirror_rules; i++) {
-        if (!mirror_cmp(&lsp->mirror_rules[i], &mirror)) {
+        if (uuid_equals(&lsp->mirror_rules[i]->header_.uuid,
+                        &mirror->header_.uuid)) {
             bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
             if (!may_exist) {
-                ctl_error(ctx, "Same mirror already existed on the lsp %s.",
-                          ctx->argv[1]);
+                ctl_error(ctx, "mirror %s is already attached to the "
+                          "logical port %s.",
+                          lsp->mirror_rules[i]->name, ctx->argv[1]);
                 return;
             }
             return;
@@ -1792,8 +1792,6 @@ nbctl_lsp_attach_mirror(struct ctl_context *ctx)
     }

     nbrec_logical_switch_port_update_mirror_rules_addvalue(lsp, mirror);
-    nbrec_mirror_update_src_addvalue(mirror,lsp);
-
 }

 static void
@@ -1821,8 +1819,6 @@ nbctl_lsp_detach_mirror(struct ctl_context *ctx)
     }

     nbrec_logical_switch_port_update_mirror_rules_delvalue(lsp, mirror);
-    nbrec_mirror_update_src_delvalue(mirror,lsp);
-
 }

 static void
@@ -7390,7 +7386,7 @@ cmd_ha_ch_grp_set_chassis_prio(struct ctl_context *ctx)
 }

 static char * OVS_WARN_UNUSED_RESULT
-parse_filter(const char *arg, const char **selection_p)
+parse_mirror_filter(const char *arg, const char **selection_p)
 {
     /* Validate selection.  Only require the first letter. */
     if (arg[0] == 't') {
@@ -7408,7 +7404,7 @@ parse_filter(const char *arg, const char **selection_p)
 }

 static char * OVS_WARN_UNUSED_RESULT
-parse_type(const char *arg, const char **type_p)
+parse_mirror_tunnel_type(const char *arg, const char **type_p)
 {
     /* Validate type.  Only require the first letter. */
     if (arg[0] == 'g') {
@@ -7456,7 +7452,7 @@ nbctl_mirror_add(struct ctl_context *ctx)
     }

     /* Tunnel Type - GRE/ERSPAN */
-    error = parse_type(ctx->argv[2], &type);
+    error = parse_mirror_tunnel_type(ctx->argv[2], &type);
     if (error) {
         ctx->error = error;
         return;
@@ -7469,7 +7465,7 @@ nbctl_mirror_add(struct ctl_context *ctx)
     }

     /* Filter for mirroring */
-    error = parse_filter(ctx->argv[4], &filter);
+    error = parse_mirror_filter(ctx->argv[4], &filter);
     if (error) {
         ctx->error = error;
         return;
@@ -7484,12 +7480,11 @@ nbctl_mirror_add(struct ctl_context *ctx)
         new_sink_ip = normalize_ipv6_addr_str(sink_ip);
     }

-    if (new_sink_ip) {
-        free(new_sink_ip);
-    } else {
+    if (!new_sink_ip) {
         ctl_error(ctx, "Invalid sink ip: %s", sink_ip);
         return;
     }
+    free(new_sink_ip);

     /* Create the mirror. */
     struct nbrec_mirror *mirror = nbrec_mirror_insert(ctx->txn);
@@ -7505,7 +7500,6 @@ static void
 nbctl_pre_mirror_del(struct ctl_context *ctx)
 {
     ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_name);
-    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_src);
 }

 static void
@@ -7540,7 +7534,6 @@ nbctl_pre_mirror_list(struct ctl_context *ctx)
     ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_index);
     ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_sink);
     ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_type);
-    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_src);
 }

 static void
@@ -7573,24 +7566,7 @@ nbctl_mirror_list(struct ctl_context *ctx)
         ds_put_format(&ctx->output, "  Sink     :  %s\n", mirror->sink);
         ds_put_format(&ctx->output, "  Filter   :  %s\n", mirror->filter);
         ds_put_format(&ctx->output, "  Index/Key:  %ld\n",
-                                                (long int) mirror->index);
-        ds_put_cstr(&ctx->output,   "  Sources  :");
-        if (mirror->n_src > 0) {
-            struct svec srcs;
-            const char *src;
-            size_t j;
-            svec_init(&srcs);
-            for (j = 0; j < mirror->n_src; j++) {
-                svec_add(&srcs, mirror->src[j]->name);
-            }
-            svec_sort(&srcs);
-            SVEC_FOR_EACH (j, src, &srcs) {
-                ds_put_format(&ctx->output, "  %s", src);
-            }
-            svec_destroy(&srcs);
-        } else {
-            ds_put_cstr(&ctx->output, "  None attached");
-        }
+                      (long int) mirror->index);
         ds_put_cstr(&ctx->output, "\n");
     }

@@ -7757,7 +7733,7 @@ static const struct ctl_command_syntax
nbctl_commands[] = {
     { "lsp-get-ls", 1, 1, "PORT", nbctl_pre_lsp_get_ls, nbctl_lsp_get_ls,
       NULL, "", RO },
     { "lsp-attach-mirror", 2, 2, "PORT MIRROR", nbctl_pre_lsp_mirror,
-      nbctl_lsp_attach_mirror, NULL, "", RW },
+      nbctl_lsp_attach_mirror, NULL, "--may-exist", RW },
     { "lsp-detach-mirror", 2, 2, "PORT MIRROR", nbctl_pre_lsp_mirror,
       nbctl_lsp_detach_mirror, NULL, "", RW },

diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
index ca6734ba3..00b2f785a 100644
--- a/utilities/ovn-sbctl.c
+++ b/utilities/ovn-sbctl.c
@@ -307,7 +307,6 @@ pre_get_info(struct ctl_context *ctx)
     ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_chassis);
     ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_datapath);
     ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_up);
-    ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_mirror_rules);

     ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_logical_datapath);
     ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_logical_dp_group);
@@ -1432,9 +1431,6 @@ static const struct ctl_table_class
tables[SBREC_N_TABLES] = {
     [SBREC_TABLE_HA_CHASSIS_GROUP].row_ids[0]
     = {&sbrec_ha_chassis_group_col_name, NULL, NULL},

-    [SBREC_TABLE_MIRROR].row_ids[0]
-    = {&sbrec_mirror_col_name, NULL, NULL},
-
     [SBREC_TABLE_METER].row_ids[0]
     = {&sbrec_meter_col_name, NULL, NULL},

---------------------------------------------------------


And I did the below changes to your patch 2.  Not many changes here.
Just moved the OVN SB schema related changes from patch 1 to patch 2.



------------------------------------------------------------
diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
index 95c7c2d7e3..1986066eeb 100644
--- a/ovn-sb.ovsschema
+++ b/ovn-sb.ovsschema
@@ -1,7 +1,7 @@
 {
     "name": "OVN_Southbound",
-    "version": "20.26.0",
-    "cksum": "3311869408 29176",
+    "version": "20.27.0",
+    "cksum": "2610590441 30335",
     "tables": {
         "SB_Global": {
             "columns": {
@@ -142,6 +142,23 @@
             "indexes": [["datapath", "tunnel_key"],
                         ["datapath", "name"]],
             "isRoot": true},
+        "Mirror": {
+            "columns": {
+                "name": {"type": "string"},
+                "filter": {"type": {"key": {"type": "string",
+                                            "enum": ["set",
+                                                     ["from-lport",
+                                                      "to-lport","both"]]}}},
+                "sink":{"type": "string"},
+                "type": {"type": {"key": {"type": "string",
+                                            "enum": ["set",
+                                                     ["gre", "erspan"]]}}},
+                "index": {"type": "integer"},
+                "external_ids": {
+                    "type": {"key": "string", "value": "string",
+                             "min": 0, "max": "unlimited"}}},
+            "indexes": [["name"]],
+            "isRoot": true},
         "Meter": {
             "columns": {
                 "name": {"type": "string"},
@@ -230,6 +247,11 @@
                                                       "refTable": "Encap",
                                                       "refType": "weak"},
                                     "min": 0, "max": "unlimited"}},
+                "mirror_rules": {"type": {"key": {"type": "uuid",
+                                          "refTable": "Mirror",
+                                          "refType": "weak"},
+                                  "min": 0,
+                                  "max": "unlimited"}},
                 "mac": {"type": {"key": "string",
                                  "min": 0,
                                  "max": "unlimited"}},
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 4f485b8605..ee0c5f77ff 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -2881,6 +2881,51 @@ tcp.flags = RST;
     </column>
   </table>

+  <table name="Mirror" title="Mirror Entry">
+    <p>
+      Each row in this table represents one Mirror that can be used for
+      port mirroring. These Mirrors are referenced by the
+      <ref column="mirror_rules" table="Port_Binding"/> column in
+      the <ref table="Port_Binding"/> table.
+    </p>
+
+    <column name="name">
+      <p>
+        Represents the name of the mirror.
+      </p>
+    </column>
+
+    <column name="filter">
+      <p>
+        The value of this field represents selection criteria of the mirror.
+      </p>
+    </column>
+
+    <column name="sink">
+      <p>
+        The value of this field represents the destination/sink of the mirror.
+      </p>
+    </column>
+
+    <column name="type">
+      <p>
+        The value of this field represents the type of the tunnel used for
+        sending the mirrored packets
+      </p>
+    </column>
+
+    <column name="index">
+      <p>
+        The value of this field represents the key/idx depending on the
+        tunnel type configured
+      </p>
+    </column>
+
+    <column name="external_ids">
+      See <em>External IDs</em> at the beginning of this document.
+    </column>
+  </table>
+
   <table name="Meter" title="Meter entry">
     <p>
       Each row in this table represents a meter that can be used for QoS or
@@ -3383,6 +3428,11 @@ tcp.flags = RST;
       </column>
     </group>

+    <column name="mirror_rules">
+        Mirror rules that apply to the port binding.
+        Please see the <ref table="Mirror"/> table.
+    </column>
+
     <group title="Patch Options">
       <p>
         These options apply to logical ports with <ref column="type"/> of
diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
index 00b2f785a5..ca6734ba32 100644
--- a/utilities/ovn-sbctl.c
+++ b/utilities/ovn-sbctl.c
@@ -307,6 +307,7 @@ pre_get_info(struct ctl_context *ctx)
     ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_chassis);
     ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_datapath);
     ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_up);
+    ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_mirror_rules);

     ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_logical_datapath);
     ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_logical_dp_group);
@@ -1431,6 +1432,9 @@ static const struct ctl_table_class
tables[SBREC_N_TABLES] = {
     [SBREC_TABLE_HA_CHASSIS_GROUP].row_ids[0]
     = {&sbrec_ha_chassis_group_col_name, NULL, NULL},

+    [SBREC_TABLE_MIRROR].row_ids[0]
+    = {&sbrec_mirror_col_name, NULL, NULL},
+
     [SBREC_TABLE_METER].row_ids[0]
     = {&sbrec_meter_col_name, NULL, NULL},

diff --git a/northd/northd.c b/northd/northd.c
index 42dc226e3e..6ada6eb689 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -15442,7 +15442,7 @@ sync_meters(struct northd_input *input_data,

 static bool
 mirror_needs_update(const struct nbrec_mirror *nb_mirror,
-                  const struct sbrec_mirror *sb_mirror)
+                    const struct sbrec_mirror *sb_mirror)
 {

     if (nb_mirror->index != sb_mirror->index) {
@@ -15460,9 +15460,9 @@ mirror_needs_update(const struct nbrec_mirror
*nb_mirror,

 static void
 sync_mirrors_iterate_nb_mirror(struct ovsdb_idl_txn *ovnsb_txn,
-                             const char *mirror_name,
-                             const struct nbrec_mirror *nb_mirror,
-                             struct shash *sb_mirrors)
+                               const char *mirror_name,
+                               const struct nbrec_mirror *nb_mirror,
+                               struct shash *sb_mirrors)
 {
     const struct sbrec_mirror *sb_mirror;
     bool new_sb_mirror = false;
@@ -15475,8 +15475,8 @@ sync_mirrors_iterate_nb_mirror(struct
ovsdb_idl_txn *ovnsb_txn,
         new_sb_mirror = true;
     }

-    if ((new_sb_mirror) || mirror_needs_update(nb_mirror, sb_mirror)) {
-        sbrec_mirror_set_filter(sb_mirror,nb_mirror->filter);
+    if (new_sb_mirror || mirror_needs_update(nb_mirror, sb_mirror)) {
+        sbrec_mirror_set_filter(sb_mirror, nb_mirror->filter);
         sbrec_mirror_set_index(sb_mirror, nb_mirror->index);
         sbrec_mirror_set_sink(sb_mirror, nb_mirror->sink);
         sbrec_mirror_set_type(sb_mirror, nb_mirror->type);
@@ -15485,7 +15485,7 @@ sync_mirrors_iterate_nb_mirror(struct
ovsdb_idl_txn *ovnsb_txn,

 static void
 sync_mirrors(struct northd_input *input_data,
-            struct ovsdb_idl_txn *ovnsb_txn)
+             struct ovsdb_idl_txn *ovnsb_txn)
 {
     struct shash sb_mirrors = SHASH_INITIALIZER(&sb_mirrors);

@@ -15497,7 +15497,7 @@ sync_mirrors(struct northd_input *input_data,
     const struct nbrec_mirror *nb_mirror;
     NBREC_MIRROR_TABLE_FOR_EACH (nb_mirror, input_data->nbrec_mirror_table) {
         sync_mirrors_iterate_nb_mirror(ovnsb_txn, nb_mirror->name, nb_mirror,
-                                     &sb_mirrors);
+                                       &sb_mirrors);
         shash_find_and_delete(&sb_mirrors, nb_mirror->name);
     }

diff --git a/ovn-sb.xml b/ovn-sb.xml
index ee0c5f77ff..97d4c5c793 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -2883,8 +2883,8 @@ tcp.flags = RST;

   <table name="Mirror" title="Mirror Entry">
     <p>
-      Each row in this table represents one Mirror that can be used for
-      port mirroring. These Mirrors are referenced by the
+      Each row in this table represents a mirror that can be used for
+      port mirroring. These mirrors are referenced by the
       <ref column="mirror_rules" table="Port_Binding"/> column in
       the <ref table="Port_Binding"/> table.
     </p>
-------------------------------------------------------------------


Please enhance the test case covering the "both" filter as suggested
by Ihar in the patch 3.


Thanks
Numan

> Acked-By: Ihar Hrachyshka <[email protected]>
>
> On Thu, Dec 1, 2022 at 12:04 PM Abhiram R N <[email protected]> wrote:
> >
> > In order to support Remote Port Mirroring
> > added the required schemas in NB and SB.
> > Also, ovn-nbctl.c and ovn-sbctl.c changes are added.
> > Futher added test cases to test nbctl commands.
> >
> > Co-authored-by: Veda Barrenkala <[email protected]>
> > Signed-off-by: Veda Barrenkala <[email protected]>
> > Signed-off-by: Abhiram R N <[email protected]>
> > ---
> > v17-->v18: Addressed comments of Mark from v17
> >
> > Modifications related to comments are in
> > ovn-nbctl.c, ovn-nbctl.at
> > Other files changed to just fix rebase conflicts
> >
> >  ovn-nb.ovsschema      |  31 +++-
> >  ovn-nb.xml            |  63 ++++++++
> >  ovn-sb.ovsschema      |  26 ++-
> >  ovn-sb.xml            |  50 ++++++
> >  tests/ovn-nbctl.at    | 124 +++++++++++++++
> >  utilities/ovn-nbctl.c | 360 ++++++++++++++++++++++++++++++++++++++++++
> >  utilities/ovn-sbctl.c |   4 +
> >  7 files changed, 654 insertions(+), 4 deletions(-)
> >
> > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> > index 6f9d38f47..bb747c187 100644
> > --- a/ovn-nb.ovsschema
> > +++ b/ovn-nb.ovsschema
> > @@ -1,7 +1,7 @@
> >  {
> >      "name": "OVN_Northbound",
> > -    "version": "6.4.0",
> > -    "cksum": "3512158873 32360",
> > +    "version": "6.5.0",
> > +    "cksum": "3702451195 33843",
> >      "tables": {
> >          "NB_Global": {
> >              "columns": {
> > @@ -132,6 +132,11 @@
> >                                              "refType": "weak"},
> >                                   "min": 0,
> >                                   "max": 1}},
> > +                "mirror_rules": {"type": {"key": {"type": "uuid",
> > +                                          "refTable": "Mirror",
> > +                                          "refType": "weak"},
> > +                                  "min": 0,
> > +                                  "max": "unlimited"}},
> >                  "ha_chassis_group": {
> >                      "type": {"key": {"type": "uuid",
> >                                       "refTable": "HA_Chassis_Group",
> > @@ -301,6 +306,28 @@
> >                      "type": {"key": "string", "value": "string",
> >                               "min": 0, "max": "unlimited"}}},
> >              "isRoot": false},
> > +        "Mirror": {
> > +            "columns": {
> > +                "name": {"type": "string"},
> > +                "filter": {"type": {"key": {"type": "string",
> > +                                            "enum": ["set", ["from-lport",
> > +                                                             "to-lport",
> > +                                                             "both"]]}}},
> > +                "sink":{"type": "string"},
> > +                "type": {"type": {"key": {"type": "string",
> > +                                            "enum": ["set", ["gre",
> > +                                                             "erspan"]]}}},
> > +                "index": {"type": "integer"},
> > +                "src": {"type": {"key": {"type": "uuid",
> > +                                           "refTable": 
> > "Logical_Switch_Port",
> > +                                           "refType": "weak"},
> > +                                   "min": 0,
> > +                                   "max": "unlimited"}},
> > +                "external_ids": {
> > +                    "type": {"key": "string", "value": "string",
> > +                             "min": 0, "max": "unlimited"}}},
> > +            "indexes": [["name"]],
> > +            "isRoot": true},
> >          "Meter": {
> >              "columns": {
> >                  "name": {"type": "string"},
> > diff --git a/ovn-nb.xml b/ovn-nb.xml
> > index 0edc3da96..4e0013cc1 100644
> > --- a/ovn-nb.xml
> > +++ b/ovn-nb.xml
> > @@ -1582,6 +1582,11 @@
> >        </column>
> >      </group>
> >
> > +    <column name="mirror_rules">
> > +        Mirror rules that apply to logical switch port which is the source.
> > +        Please see the <ref table="Mirror"/> table.
> > +    </column>
> > +
> >      <column name="ha_chassis_group">
> >        References a row in the OVN Northbound database's
> >        <ref table="HA_Chassis_Group" db="OVN_Northbound"/> table.
> > @@ -2587,6 +2592,64 @@ or
> >      </column>
> >    </table>
> >
> > +  <table name="Mirror" title="Mirror Entry">
> > +    <p>
> > +      Each row in this table represents one Mirror that can be used for
> > +      port mirroring. These Mirrors are referenced by the
> > +      <ref column="mirror_rules" table="Logical_Switch_Port"/> column in
> > +      the <ref table="Logical_Switch_Port"/> table.
> > +    </p>
> > +
> > +    <column name="name">
> > +      <p>
> > +        Represents the name of the mirror.
> > +      </p>
> > +    </column>
> > +
> > +    <column name="filter">
> > +      <p>
> > +        The value of this field represents selection criteria of the 
> > mirror.
> > +        Supported values for filter to-lport / from-lport / both
> > +        to-lport - to mirror packets coming into logical port
> > +        from-lport - to mirror packets going out of logical port
> > +        both - to mirror packets coming into and going out of logical port.
> > +      </p>
> > +    </column>
> > +
> > +    <column name="sink">
> > +      <p>
> > +        The value of this field represents the destination/sink of the 
> > mirror.
> > +        The value it takes is an IP address of the sink port.
> > +      </p>
> > +    </column>
> > +
> > +    <column name="type">
> > +      <p>
> > +        The value of this field represents the type of the tunnel used for
> > +        sending the mirrored packets. Supported Tunnel types gre and erspan
> > +      </p>
> > +    </column>
> > +
> > +    <column name="index">
> > +      <p>
> > +        The value of this field represents the tunnel ID. Depending on the
> > +        tunnel type configured, GRE key value if type GRE and erspan_idx 
> > value
> > +        if ERSPAN
> > +      </p>
> > +    </column>
> > +
> > +    <column name="src">
> > +      <p>
> > +        The value of this field represents a list of source ports for the
> > +        mirror. Please see the <ref table="Logical_Switch_Port"/> table.
> > +      </p>
> > +    </column>
> > +
> > +    <column name="external_ids">
> > +      See <em>External IDs</em> at the beginning of this document.
> > +    </column>
> > +  </table>
> > +
> >    <table name="Meter" title="Meter entry">
> >      <p>
> >        Each row in this table represents a meter that can be used for QoS or
> > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> > index 95c7c2d7e..1986066ee 100644
> > --- a/ovn-sb.ovsschema
> > +++ b/ovn-sb.ovsschema
> > @@ -1,7 +1,7 @@
> >  {
> >      "name": "OVN_Southbound",
> > -    "version": "20.26.0",
> > -    "cksum": "3311869408 29176",
> > +    "version": "20.27.0",
> > +    "cksum": "2610590441 30335",
> >      "tables": {
> >          "SB_Global": {
> >              "columns": {
> > @@ -142,6 +142,23 @@
> >              "indexes": [["datapath", "tunnel_key"],
> >                          ["datapath", "name"]],
> >              "isRoot": true},
> > +        "Mirror": {
> > +            "columns": {
> > +                "name": {"type": "string"},
> > +                "filter": {"type": {"key": {"type": "string",
> > +                                            "enum": ["set",
> > +                                                     ["from-lport",
> > +                                                      
> > "to-lport","both"]]}}},
> > +                "sink":{"type": "string"},
> > +                "type": {"type": {"key": {"type": "string",
> > +                                            "enum": ["set",
> > +                                                     ["gre", "erspan"]]}}},
> > +                "index": {"type": "integer"},
> > +                "external_ids": {
> > +                    "type": {"key": "string", "value": "string",
> > +                             "min": 0, "max": "unlimited"}}},
> > +            "indexes": [["name"]],
> > +            "isRoot": true},
> >          "Meter": {
> >              "columns": {
> >                  "name": {"type": "string"},
> > @@ -230,6 +247,11 @@
> >                                                        "refTable": "Encap",
> >                                                        "refType": "weak"},
> >                                      "min": 0, "max": "unlimited"}},
> > +                "mirror_rules": {"type": {"key": {"type": "uuid",
> > +                                          "refTable": "Mirror",
> > +                                          "refType": "weak"},
> > +                                  "min": 0,
> > +                                  "max": "unlimited"}},
> >                  "mac": {"type": {"key": "string",
> >                                   "min": 0,
> >                                   "max": "unlimited"}},
> > diff --git a/ovn-sb.xml b/ovn-sb.xml
> > index 4f485b860..ee0c5f77f 100644
> > --- a/ovn-sb.xml
> > +++ b/ovn-sb.xml
> > @@ -2881,6 +2881,51 @@ tcp.flags = RST;
> >      </column>
> >    </table>
> >
> > +  <table name="Mirror" title="Mirror Entry">
> > +    <p>
> > +      Each row in this table represents one Mirror that can be used for
> > +      port mirroring. These Mirrors are referenced by the
> > +      <ref column="mirror_rules" table="Port_Binding"/> column in
> > +      the <ref table="Port_Binding"/> table.
> > +    </p>
> > +
> > +    <column name="name">
> > +      <p>
> > +        Represents the name of the mirror.
> > +      </p>
> > +    </column>
> > +
> > +    <column name="filter">
> > +      <p>
> > +        The value of this field represents selection criteria of the 
> > mirror.
> > +      </p>
> > +    </column>
> > +
> > +    <column name="sink">
> > +      <p>
> > +        The value of this field represents the destination/sink of the 
> > mirror.
> > +      </p>
> > +    </column>
> > +
> > +    <column name="type">
> > +      <p>
> > +        The value of this field represents the type of the tunnel used for
> > +        sending the mirrored packets
> > +      </p>
> > +    </column>
> > +
> > +    <column name="index">
> > +      <p>
> > +        The value of this field represents the key/idx depending on the
> > +        tunnel type configured
> > +      </p>
> > +    </column>
> > +
> > +    <column name="external_ids">
> > +      See <em>External IDs</em> at the beginning of this document.
> > +    </column>
> > +  </table>
> > +
> >    <table name="Meter" title="Meter entry">
> >      <p>
> >        Each row in this table represents a meter that can be used for QoS or
> > @@ -3383,6 +3428,11 @@ tcp.flags = RST;
> >        </column>
> >      </group>
> >
> > +    <column name="mirror_rules">
> > +        Mirror rules that apply to the port binding.
> > +        Please see the <ref table="Mirror"/> table.
> > +    </column>
> > +
> >      <group title="Patch Options">
> >        <p>
> >          These options apply to logical ports with <ref column="type"/> of
> > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> > index 9da7c26b3..df63a0f83 100644
> > --- a/tests/ovn-nbctl.at
> > +++ b/tests/ovn-nbctl.at
> > @@ -435,6 +435,130 @@ AT_CHECK([ovn-nbctl meter-list], [0], [dnl
> >
> >  dnl ---------------------------------------------------------------------
> >
> > +OVN_NBCTL_TEST([ovn_nbctl_mirrors], [mirrors], [
> > +check ovn-nbctl mirror-add mirror1 gre 0 from-lport 10.10.10.1
> > +check ovn-nbctl mirror-add mirror2 erspan 1 both 10.10.10.2
> > +check ovn-nbctl mirror-add mirror3 gre 2 to-lport 10.10.10.3
> > +check ovn-nbctl ls-add sw0
> > +check ovn-nbctl lsp-add sw0 sw0-port1
> > +check ovn-nbctl lsp-add sw0 sw0-port2
> > +check ovn-nbctl lsp-add sw0 sw0-port3
> > +
> > +dnl Add duplicate mirror name
> > +AT_CHECK([ovn-nbctl mirror-add mirror1 gre 0 from-lport 10.10.10.5], [1], 
> > [], [stderr])
> > +AT_CHECK([grep 'already exists' stderr], [0], [ignore])
> > +
> > +dnl Attach invalid source port to mirror
> > +AT_CHECK([ovn-nbctl lsp-attach-mirror sw0-port4 mirror3], [1], [], 
> > [stderr])
> > +AT_CHECK([grep 'port name not found' stderr], [0], [ignore])
> > +
> > +dnl Attach source port to invalid mirror
> > +AT_CHECK([ovn-nbctl lsp-attach-mirror sw0-port3 mirror4], [1], [], 
> > [stderr])
> > +AT_CHECK([grep 'mirror name not found' stderr], [0], [ignore])
> > +
> > +mirror3uuid=$(fetch_column nb:Mirror _uuid name=mirror3)
> > +dnl Attach source port to mirror
> > +check ovn-nbctl lsp-attach-mirror sw0-port1 mirror3
> > +check_column "$mirror3uuid" nb:Logical_Switch_Port mirror_rules 
> > name=sw0-port1
> > +
> > +dnl Attach one more source port to mirror
> > +check ovn-nbctl lsp-attach-mirror sw0-port3 mirror3
> > +check_column "$mirror3uuid" nb:Logical_Switch_Port mirror_rules 
> > name=sw0-port3
> > +
> > +dnl Verify if multiple ports are attached to the same mirror properly
> > +AT_CHECK([ovn-nbctl mirror-list], [0], [dnl
> > +mirror1:
> > +  Type     :  gre
> > +  Sink     :  10.10.10.1
> > +  Filter   :  from-lport
> > +  Index/Key:  0
> > +  Sources  :  None attached
> > +mirror2:
> > +  Type     :  erspan
> > +  Sink     :  10.10.10.2
> > +  Filter   :  both
> > +  Index/Key:  1
> > +  Sources  :  None attached
> > +mirror3:
> > +  Type     :  gre
> > +  Sink     :  10.10.10.3
> > +  Filter   :  to-lport
> > +  Index/Key:  2
> > +  Sources  :  sw0-port1  sw0-port3
> > +])
> > +
> > +dnl Detach one source port from mirror
> > +check ovn-nbctl lsp-detach-mirror sw0-port3 mirror3
> > +
> > +dnl Check if the detach happened from source properly
> > +check_column "" nb:Logical_Switch_Port mirror_rules name=sw0-port3
> > +
> > +dnl Verify if detach source port from mirror happens properly
> > +AT_CHECK([ovn-nbctl mirror-list], [0], [dnl
> > +mirror1:
> > +  Type     :  gre
> > +  Sink     :  10.10.10.1
> > +  Filter   :  from-lport
> > +  Index/Key:  0
> > +  Sources  :  None attached
> > +mirror2:
> > +  Type     :  erspan
> > +  Sink     :  10.10.10.2
> > +  Filter   :  both
> > +  Index/Key:  1
> > +  Sources  :  None attached
> > +mirror3:
> > +  Type     :  gre
> > +  Sink     :  10.10.10.3
> > +  Filter   :  to-lport
> > +  Index/Key:  2
> > +  Sources  :  sw0-port1
> > +])
> > +
> > +dnl Delete a single mirror which has source attached.
> > +check ovn-nbctl mirror-del mirror3
> > +
> > +dnl Check if the detach happened from source properly
> > +check_column "" nb:Logical_Switch_Port mirror_rules name=sw0-port1
> > +
> > +dnl Check if the mirror deleted properly
> > +AT_CHECK([ovn-nbctl mirror-list], [0], [dnl
> > +mirror1:
> > +  Type     :  gre
> > +  Sink     :  10.10.10.1
> > +  Filter   :  from-lport
> > +  Index/Key:  0
> > +  Sources  :  None attached
> > +mirror2:
> > +  Type     :  erspan
> > +  Sink     :  10.10.10.2
> > +  Filter   :  both
> > +  Index/Key:  1
> > +  Sources  :  None attached
> > +])
> > +
> > +dnl Delete another mirror
> > +check ovn-nbctl mirror-del mirror2
> > +
> > +dnl Update the Sink address
> > +check ovn-nbctl set mirror . sink=192.168.1.13
> > +
> > +AT_CHECK([ovn-nbctl mirror-list], [0], [dnl
> > +mirror1:
> > +  Type     :  gre
> > +  Sink     :  192.168.1.13
> > +  Filter   :  from-lport
> > +  Index/Key:  0
> > +  Sources  :  None attached
> > +])
> > +
> > +dnl Delete all mirrors
> > +check ovn-nbctl mirror-del
> > +AT_CHECK([ovn-nbctl mirror-list], [0], [dnl
> > +])])
> > +
> > +dnl ---------------------------------------------------------------------
> > +
> >  OVN_NBCTL_TEST([ovn_nbctl_nats], [NATs], [
> >  AT_CHECK([ovn-nbctl lr-add lr0])
> >  AT_CHECK([ovn-nbctl lr-nat-add lr0 snatt 30.0.0.2 192.168.1.2], [1], [],
> > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> > index 9e9b83ef1..4cd0b2961 100644
> > --- a/utilities/ovn-nbctl.c
> > +++ b/utilities/ovn-nbctl.c
> > @@ -272,6 +272,19 @@ QoS commands:\n\
> >                              remove QoS rules from SWITCH\n\
> >    qos-list SWITCH           print QoS rules for SWITCH\n\
> >  \n\
> > +Mirror commands:\n\
> > +  mirror-add NAME TYPE INDEX FILTER IP\n\
> > +                            add a mirror with given name\n\
> > +                            specify TYPE 'gre' or 'erspan'\n\
> > +                            specify the tunnel INDEX value\n\
> > +                                (indicates key if GRE\n\
> > +                                 erpsan_idx if ERSPAN)\n\
> > +                            specify FILTER for mirroring selection\n\
> > +                                'to-lport' / 'from-lport' / 'both'\n\
> > +                            specify Sink / Destination i.e. Remote IP\n\
> > +  mirror-del [NAME]         remove mirrors\n\
> > +  mirror-list               print mirrors\n\
> > +\n\
> >  Meter commands:\n\
> >    [--fair]\n\
> >    meter-add NAME ACTION RATE UNIT [BURST]\n\
> > @@ -312,6 +325,8 @@ Logical switch port commands:\n\
> >                              set dhcpv6 options for PORT\n\
> >    lsp-get-dhcpv6-options PORT  get the dhcpv6 options for PORT\n\
> >    lsp-get-ls PORT           get the logical switch which the port belongs 
> > to\n\
> > +  lsp-attach-mirror PORT MIRROR   attach source PORT to MIRROR\n\
> > +  lsp-detach-mirror PORT MIRROR   detach source PORT from MIRROR\n\
> >  \n\
> >  Forwarding group commands:\n\
> >    [--liveness]\n\
> > @@ -1686,6 +1701,130 @@ nbctl_pre_lsp_type(struct ctl_context *ctx)
> >      ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_port_col_type);
> >  }
> >
> > +static void
> > +nbctl_pre_lsp_mirror(struct ctl_context *ctx)
> > +{
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_port_col_name);
> > +    ovsdb_idl_add_column(ctx->idl,
> > +                         &nbrec_logical_switch_port_col_mirror_rules);
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_name);
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_src);
> > +}
> > +
> > +static int
> > +mirror_cmp(const void *mirror1_, const void *mirror2_)
> > +{
> > +    const struct nbrec_mirror *const *mirror_1 = mirror1_;
> > +    const struct nbrec_mirror *const *mirror_2 = mirror2_;
> > +
> > +    const struct nbrec_mirror *mirror1 = *mirror_1;
> > +    const struct nbrec_mirror *mirror2 = *mirror_2;
> > +
> > +    return strcmp(mirror1->name,mirror2->name);
> > +}
> > +
> > +static char * OVS_WARN_UNUSED_RESULT
> > +mirror_by_name_or_uuid(struct ctl_context *ctx, const char *id,
> > +                    bool must_exist,
> > +                    const struct nbrec_mirror **mirror_p)
> > +{
> > +    const struct nbrec_mirror *mirror = NULL;
> > +    *mirror_p = NULL;
> > +
> > +    struct uuid mirror_uuid;
> > +    bool is_uuid = uuid_from_string(&mirror_uuid, id);
> > +    if (is_uuid) {
> > +        mirror = nbrec_mirror_get_for_uuid(ctx->idl, &mirror_uuid);
> > +    }
> > +
> > +    if (!mirror) {
> > +        NBREC_MIRROR_FOR_EACH (mirror, ctx->idl) {
> > +            if (!strcmp(mirror->name, id)) {
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > +    if (!mirror && must_exist) {
> > +        return xasprintf("%s: mirror %s not found",
> > +                         id, is_uuid ? "UUID" : "name");
> > +    }
> > +
> > +    *mirror_p = mirror;
> > +    return NULL;
> > +}
> > +
> > +static void
> > +nbctl_lsp_attach_mirror(struct ctl_context *ctx)
> > +{
> > +    const char *port = ctx->argv[1];
> > +    const char *mirror_name = ctx->argv[2];
> > +    const struct nbrec_logical_switch_port *lsp = NULL;
> > +    const struct nbrec_mirror *mirror;
> > +
> > +    char *error;
> > +
> > +    error = lsp_by_name_or_uuid(ctx, port, true, &lsp);
> > +    if (error) {
> > +        ctx->error = error;
> > +        return;
> > +    }
> > +
> > +
> > +    /*check if a mirror rule actually exists on that name or not*/
> > +    error = mirror_by_name_or_uuid(ctx, mirror_name, true, &mirror);
> > +    if (error) {
> > +        ctx->error = error;
> > +        return;
> > +    }
> > +
> > +    /* Check if same mirror rule already exists for the lsp */
> > +    for (size_t i = 0; i < lsp->n_mirror_rules; i++) {
> > +        if (!mirror_cmp(&lsp->mirror_rules[i], &mirror)) {
> > +            bool may_exist = shash_find(&ctx->options, "--may-exist") != 
> > NULL;
> > +            if (!may_exist) {
> > +                ctl_error(ctx, "Same mirror already existed on the lsp 
> > %s.",
> > +                          ctx->argv[1]);
> > +                return;
> > +            }
> > +            return;
> > +        }
> > +    }
> > +
> > +    nbrec_logical_switch_port_update_mirror_rules_addvalue(lsp, mirror);
> > +    nbrec_mirror_update_src_addvalue(mirror,lsp);
> > +
> > +}
> > +
> > +static void
> > +nbctl_lsp_detach_mirror(struct ctl_context *ctx)
> > +{
> > +    const char *port = ctx->argv[1];
> > +    const char *mirror_name = ctx->argv[2];
> > +    const struct nbrec_logical_switch_port *lsp = NULL;
> > +    const struct nbrec_mirror *mirror;
> > +
> > +    char *error;
> > +
> > +    error = lsp_by_name_or_uuid(ctx, port, true, &lsp);
> > +    if (error) {
> > +        ctx->error = error;
> > +        return;
> > +    }
> > +
> > +
> > +    /*check if a mirror rule actually exists on that name or not*/
> > +    error = mirror_by_name_or_uuid(ctx, mirror_name, true, &mirror);
> > +    if (error) {
> > +        ctx->error = error;
> > +        return;
> > +    }
> > +
> > +    nbrec_logical_switch_port_update_mirror_rules_delvalue(lsp, mirror);
> > +    nbrec_mirror_update_src_delvalue(mirror,lsp);
> > +
> > +}
> > +
> >  static void
> >  nbctl_lsp_set_type(struct ctl_context *ctx)
> >  {
> > @@ -7244,6 +7383,214 @@ cmd_ha_ch_grp_set_chassis_prio(struct ctl_context 
> > *ctx)
> >      nbrec_ha_chassis_set_priority(ha_chassis, priority);
> >  }
> >
> > +static char * OVS_WARN_UNUSED_RESULT
> > +parse_filter(const char *arg, const char **selection_p)
> > +{
> > +    /* Validate selection.  Only require the first letter. */
> > +    if (arg[0] == 't') {
> > +        *selection_p = "to-lport";
> > +    } else if (arg[0] == 'f') {
> > +        *selection_p = "from-lport";
> > +    } else if (arg[0] == 'b') {
> > +        *selection_p = "both";
> > +    } else {
> > +        *selection_p = NULL;
> > +        return xasprintf("%s: selection must be \"to-lport\" or "
> > +                         "\"from-lport\" or \"both\" ", arg);
> > +    }
> > +    return NULL;
> > +}
> > +
> > +static char * OVS_WARN_UNUSED_RESULT
> > +parse_type(const char *arg, const char **type_p)
> > +{
> > +    /* Validate type.  Only require the first letter. */
> > +    if (arg[0] == 'g') {
> > +        *type_p = "gre";
> > +    } else if (arg[0] == 'e') {
> > +        *type_p = "erspan";
> > +    } else {
> > +        *type_p = NULL;
> > +        return xasprintf("%s: type must be \"gre\" or "
> > +                         "\"erspan\"", arg);
> > +    }
> > +    return NULL;
> > +}
> > +
> > +static void
> > +nbctl_pre_mirror_add(struct ctl_context *ctx)
> > +{
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_name);
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_filter);
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_index);
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_sink);
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_type);
> > +}
> > +
> > +static void
> > +nbctl_mirror_add(struct ctl_context *ctx)
> > +{
> > +    const char *filter = NULL;
> > +    const char *sink_ip = NULL;
> > +    const char *type = NULL;
> > +    const char *name = NULL;
> > +    char *new_sink_ip = NULL;
> > +    int64_t index;
> > +    char *error = NULL;
> > +    const struct nbrec_mirror *mirror_check = NULL;
> > +
> > +    /* Mirror Name */
> > +    name = ctx->argv[1];
> > +    NBREC_MIRROR_FOR_EACH (mirror_check, ctx->idl) {
> > +        if (!strcmp(mirror_check->name, name)) {
> > +            ctl_error(ctx, "Mirror with %s name already exists.",
> > +                      name);
> > +            return;
> > +        }
> > +    }
> > +
> > +    /* Tunnel Type - GRE/ERSPAN */
> > +    error = parse_type(ctx->argv[2], &type);
> > +    if (error) {
> > +        ctx->error = error;
> > +        return;
> > +    }
> > +
> > +    /* tunnel index / GRE key / ERSPAN idx */
> > +    if (!str_to_long(ctx->argv[3], 10, (long int *) &index)) {
> > +        ctl_error(ctx, "Invalid Index");
> > +        return;
> > +    }
> > +
> > +    /* Filter for mirroring */
> > +    error = parse_filter(ctx->argv[4], &filter);
> > +    if (error) {
> > +        ctx->error = error;
> > +        return;
> > +    }
> > +
> > +    /* Destination / Sink details */
> > +    sink_ip = ctx->argv[5];
> > +
> > +    /* check if it is a valid ip */
> > +    new_sink_ip = normalize_ipv4_addr_str(sink_ip);
> > +    if (!new_sink_ip) {
> > +        new_sink_ip = normalize_ipv6_addr_str(sink_ip);
> > +    }
> > +
> > +    if (new_sink_ip) {
> > +        free(new_sink_ip);
> > +    } else {
> > +        ctl_error(ctx, "Invalid sink ip: %s", sink_ip);
> > +        return;
> > +    }
> > +
> > +    /* Create the mirror. */
> > +    struct nbrec_mirror *mirror = nbrec_mirror_insert(ctx->txn);
> > +    nbrec_mirror_set_name(mirror, name);
> > +    nbrec_mirror_set_index(mirror, index);
> > +    nbrec_mirror_set_filter(mirror, filter);
> > +    nbrec_mirror_set_type(mirror, type);
> > +    nbrec_mirror_set_sink(mirror, sink_ip);
> > +
> > +}
> > +
> > +static void
> > +nbctl_pre_mirror_del(struct ctl_context *ctx)
> > +{
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_name);
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_src);
> > +}
> > +
> > +static void
> > +nbctl_mirror_del(struct ctl_context *ctx)
> > +{
> > +    const struct nbrec_mirror *mirror, *next;
> > +
> > +    /* If a name is not specified, delete all mirrors. */
> > +    if (ctx->argc == 1) {
> > +        NBREC_MIRROR_FOR_EACH_SAFE (mirror, next, ctx->idl) {
> > +            nbrec_mirror_delete(mirror);
> > +        }
> > +        return;
> > +    }
> > +
> > +    /* Remove the matching mirror. */
> > +    NBREC_MIRROR_FOR_EACH (mirror, ctx->idl) {
> > +        if (strcmp(ctx->argv[1], mirror->name)) {
> > +            continue;
> > +        }
> > +        nbrec_mirror_delete(mirror);
> > +        return;
> > +    }
> > +}
> > +
> > +static void
> > +nbctl_pre_mirror_list(struct ctl_context *ctx)
> > +{
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_port_col_name);
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_name);
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_filter);
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_index);
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_sink);
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_type);
> > +    ovsdb_idl_add_column(ctx->idl, &nbrec_mirror_col_src);
> > +}
> > +
> > +static void
> > +nbctl_mirror_list(struct ctl_context *ctx)
> > +{
> > +
> > +    const struct nbrec_mirror **mirrors = NULL;
> > +    const struct nbrec_mirror *mirror;
> > +    size_t n_capacity = 0;
> > +    size_t n_mirrors = 0;
> > +
> > +    NBREC_MIRROR_FOR_EACH (mirror, ctx->idl) {
> > +        if (n_mirrors == n_capacity) {
> > +            mirrors = x2nrealloc(mirrors, &n_capacity, sizeof *mirrors);
> > +        }
> > +
> > +        mirrors[n_mirrors] = mirror;
> > +        n_mirrors++;
> > +    }
> > +
> > +    if (n_mirrors) {
> > +        qsort(mirrors, n_mirrors, sizeof *mirrors, mirror_cmp);
> > +    }
> > +
> > +    for (size_t i = 0; i < n_mirrors; i++) {
> > +        mirror = mirrors[i];
> > +        ds_put_format(&ctx->output, "%s:\n", mirror->name);
> > +        /* print all the values */
> > +        ds_put_format(&ctx->output, "  Type     :  %s\n", mirror->type);
> > +        ds_put_format(&ctx->output, "  Sink     :  %s\n", mirror->sink);
> > +        ds_put_format(&ctx->output, "  Filter   :  %s\n", mirror->filter);
> > +        ds_put_format(&ctx->output, "  Index/Key:  %ld\n",
> > +                                                (long int) mirror->index);
> > +        ds_put_cstr(&ctx->output,   "  Sources  :");
> > +        if (mirror->n_src > 0) {
> > +            struct svec srcs;
> > +            const char *src;
> > +            size_t j;
> > +            svec_init(&srcs);
> > +            for (j = 0; j < mirror->n_src; j++) {
> > +                svec_add(&srcs, mirror->src[j]->name);
> > +            }
> > +            svec_sort(&srcs);
> > +            SVEC_FOR_EACH (j, src, &srcs) {
> > +                ds_put_format(&ctx->output, "  %s", src);
> > +            }
> > +            svec_destroy(&srcs);
> > +        } else {
> > +            ds_put_cstr(&ctx->output, "  None attached");
> > +        }
> > +        ds_put_cstr(&ctx->output, "\n");
> > +    }
> > +
> > +    free(mirrors);
> > +}
> > +
> >  static const struct ctl_table_class tables[NBREC_N_TABLES] = {
> >      [NBREC_TABLE_DHCP_OPTIONS].row_ids
> >      = {{&nbrec_logical_switch_port_col_name, NULL,
> > @@ -7340,6 +7687,15 @@ static const struct ctl_command_syntax 
> > nbctl_commands[] = {
> >      { "qos-list", 1, 1, "SWITCH", nbctl_pre_qos_list, nbctl_qos_list,
> >        NULL, "", RO },
> >
> > +    /* mirror commands. */
> > +    { "mirror-add", 5, 5,
> > +      "NAME TYPE INDEX FILTER IP",
> > +      nbctl_pre_mirror_add, nbctl_mirror_add, NULL, "--may-exist", RW },
> > +    { "mirror-del", 0, 1, "[NAME]",
> > +      nbctl_pre_mirror_del, nbctl_mirror_del, NULL, "", RW },
> > +    { "mirror-list", 0, 0, "", nbctl_pre_mirror_list, nbctl_mirror_list,
> > +      NULL, "", RO },
> > +
> >      /* meter commands. */
> >      { "meter-add", 4, 5, "NAME ACTION RATE UNIT [BURST]", 
> > nbctl_pre_meter_add,
> >        nbctl_meter_add, NULL, "--fair,--may-exist", RW },
> > @@ -7394,6 +7750,10 @@ static const struct ctl_command_syntax 
> > nbctl_commands[] = {
> >        nbctl_lsp_get_dhcpv6_options, NULL, "", RO },
> >      { "lsp-get-ls", 1, 1, "PORT", nbctl_pre_lsp_get_ls, nbctl_lsp_get_ls,
> >        NULL, "", RO },
> > +    { "lsp-attach-mirror", 2, 2, "PORT MIRROR", nbctl_pre_lsp_mirror,
> > +      nbctl_lsp_attach_mirror, NULL, "", RW },
> > +    { "lsp-detach-mirror", 2, 2, "PORT MIRROR", nbctl_pre_lsp_mirror,
> > +      nbctl_lsp_detach_mirror, NULL, "", RW },
> >
> >      /* forwarding group commands. */
> >      { "fwd-group-add", 4, INT_MAX, "SWITCH GROUP VIP VMAC PORT...",
> > diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
> > index 00b2f785a..ca6734ba3 100644
> > --- a/utilities/ovn-sbctl.c
> > +++ b/utilities/ovn-sbctl.c
> > @@ -307,6 +307,7 @@ pre_get_info(struct ctl_context *ctx)
> >      ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_chassis);
> >      ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_datapath);
> >      ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_up);
> > +    ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_mirror_rules);
> >
> >      ovsdb_idl_add_column(ctx->idl, 
> > &sbrec_logical_flow_col_logical_datapath);
> >      ovsdb_idl_add_column(ctx->idl, 
> > &sbrec_logical_flow_col_logical_dp_group);
> > @@ -1431,6 +1432,9 @@ static const struct ctl_table_class 
> > tables[SBREC_N_TABLES] = {
> >      [SBREC_TABLE_HA_CHASSIS_GROUP].row_ids[0]
> >      = {&sbrec_ha_chassis_group_col_name, NULL, NULL},
> >
> > +    [SBREC_TABLE_MIRROR].row_ids[0]
> > +    = {&sbrec_mirror_col_name, NULL, NULL},
> > +
> >      [SBREC_TABLE_METER].row_ids[0]
> >      = {&sbrec_meter_col_name, NULL, NULL},
> >
> > --
> > 2.31.1
> >
> > _______________________________________________
> > dev mailing list
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to