diff mbox series

[ovs-dev,v18,1/3] OVN Remote Port Mirroring: Add new Schemas in NB and SB

Message ID 20221201170358.433414-2-abhiramrn@gmail.com
State Changes Requested
Headers show
Series OVN Remote Port Mirroring | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Abhiram RN Dec. 1, 2022, 5:03 p.m. UTC 
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 <vedabarrenkala@gmail.com>
Signed-off-by: Veda Barrenkala <vedabarrenkala@gmail.com>
Signed-off-by: Abhiram R N <abhiramrn@gmail.com>
---
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(-)

Comments

0-day Robot Dec. 1, 2022, 5:19 p.m. UTC | #1 
Bleep bloop.  Greetings Abhiram R N, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line lacks whitespace around operator
#422 FILE: utilities/ovn-nbctl.c:276:
  mirror-add NAME TYPE INDEX FILTER IP\n\

WARNING: Line lacks whitespace around operator
#431 FILE: utilities/ovn-nbctl.c:285:
  mirror-del [NAME]         remove mirrors\n\

WARNING: Line lacks whitespace around operator
#432 FILE: utilities/ovn-nbctl.c:286:
  mirror-list               print mirrors\n\

WARNING: Line lacks whitespace around operator
#441 FILE: utilities/ovn-nbctl.c:328:
  lsp-attach-mirror PORT MIRROR   attach source PORT to MIRROR\n\

WARNING: Line lacks whitespace around operator
#442 FILE: utilities/ovn-nbctl.c:329:
  lsp-detach-mirror PORT MIRROR   detach source PORT from MIRROR\n\

Lines checked: 844, Warnings: 5, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Ihar Hrachyshka Dec. 12, 2022, 7:16 p.m. UTC | #2 
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...

Acked-By: Ihar Hrachyshka <ihrachys@redhat.com>

On Thu, Dec 1, 2022 at 12:04 PM Abhiram R N <abhiramrn@gmail.com> 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 <vedabarrenkala@gmail.com>
> Signed-off-by: Veda Barrenkala <vedabarrenkala@gmail.com>
> Signed-off-by: Abhiram R N <abhiramrn@gmail.com>
> ---
> 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
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Numan Siddique Dec. 13, 2022, 4:40 a.m. UTC | #3 
On Mon, Dec 12, 2022 at 2:17 PM Ihar Hrachyshka <ihrachys@redhat.com> 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 <ihrachys@redhat.com>
>
> On Thu, Dec 1, 2022 at 12:04 PM Abhiram R N <abhiramrn@gmail.com> 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 <vedabarrenkala@gmail.com>
> > Signed-off-by: Veda Barrenkala <vedabarrenkala@gmail.com>
> > Signed-off-by: Abhiram R N <abhiramrn@gmail.com>
> > ---
> > 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
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

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},