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

In order to support Remote Port Mirroring
added the required schemas in NB and SB.
Also, nbctl.c and sbctl.c changes are added.
Futher added test case 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>
---
v16-->v17: No changes

 ovn-nb.ovsschema      |  31 +++-
 ovn-nb.xml            |  63 ++++++++
 ovn-sb.ovsschema      |  26 ++-
 ovn-sb.xml            |  50 ++++++
 tests/ovn-nbctl.at    | 120 ++++++++++++++
 utilities/ovn-nbctl.c | 357 ++++++++++++++++++++++++++++++++++++++++++
 utilities/ovn-sbctl.c |   4 +
 7 files changed, 647 insertions(+), 4 deletions(-)

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
#418 FILE: utilities/ovn-nbctl.c:275:
  mirror-add NAME TYPE INDEX FILTER IP\n\

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

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

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

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

Lines checked: 837, 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

Hi, since this patch series doesn't have a cover letter, I figure I 
would make some top-level comments on the series here on patch 1. First 
off, I suggest running all of the patches through 
utilities/checkpatch.py in OVN. There are quite a few nitpicky coding 
guidelines violations I noticed that it will point out to you.

Overall, I'm not all that familiar with the ins and outs of port 
mirroring (i.e. what the requirements are), so I'm focusing mostly on 
the mechanics in this review. With all that said, I have comments down 
below.

On 11/27/22 15:14, Abhiram R N wrote:
> In order to support Remote Port Mirroring
> added the required schemas in NB and SB.
> Also, nbctl.c and sbctl.c changes are added.
> Futher added test case 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>
> ---
> v16-->v17: No changes
> 
>   ovn-nb.ovsschema      |  31 +++-
>   ovn-nb.xml            |  63 ++++++++
>   ovn-sb.ovsschema      |  26 ++-
>   ovn-sb.xml            |  50 ++++++
>   tests/ovn-nbctl.at    | 120 ++++++++++++++
>   utilities/ovn-nbctl.c | 357 ++++++++++++++++++++++++++++++++++++++++++
>   utilities/ovn-sbctl.c |   4 +
>   7 files changed, 647 insertions(+), 4 deletions(-)
> 
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index 174364c8b..01de55222 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>   {
>       "name": "OVN_Northbound",
> -    "version": "6.3.0",
> -    "cksum": "4042813038 31869",
> +    "version": "6.4.0",
> +    "cksum": "589874483 33352",
>       "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"}},

IMO, there is no reason for both the "mirror_rules" column to exist in 
the logical switch port and for the "src" column to exist in the Mirror 
table. This results in a circular reference that doesn't really seem 
necessary based on my reading of the changes in northd.c . The 
southbound DB doesn't have this problem since there is no "src" column 
in the Port_Binding table.

Also, this is a bit nitpicky of me, but why is the column called 
"mirror_rules"? Should it just be called "mirrors"?

>                   "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 7f207a413..671692b49 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.
> @@ -2527,6 +2532,64 @@
>       </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 576ebbdeb..b83134416 100644
> --- a/ovn-sb.ovsschema
> +++ b/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
>   {
>       "name": "OVN_Southbound",
> -    "version": "20.25.0",
> -    "cksum": "53184112 28845",
> +    "version": "20.26.0",
> +    "cksum": "2344151793 30004",
>       "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 16f57b374..2dd2e304f 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -2869,6 +2869,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
> @@ -3371,6 +3416,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 4d480e357..d79f9d929 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -435,6 +435,126 @@ AT_CHECK([ovn-nbctl meter-list], [0], [dnl
>   
>   dnl ---------------------------------------------------------------------
>   
> +OVN_NBCTL_TEST([ovn_nbctl_mirrors], [mirrors], [
> +AT_CHECK([ovn-nbctl mirror-add mirror1 gre 0 from-lport 10.10.10.1])
> +AT_CHECK([ovn-nbctl mirror-add mirror2 erspan 1 both 10.10.10.2])
> +AT_CHECK([ovn-nbctl mirror-add mirror3 gre 2 to-lport 10.10.10.3])
> +AT_CHECK([ovn-nbctl ls-add sw0])
> +AT_CHECK([ovn-nbctl lsp-add sw0 sw0-port1])
> +AT_CHECK([ovn-nbctl lsp-add sw0 sw0-port2])
> +AT_CHECK([ovn-nbctl lsp-add sw0 sw0-port3])

Suggestion: Replace uses of AT_CHECK above with "check". It's a bit less 
cluttered and it has the benefit of automatically echoing the command to 
stdout. That can really help when debugging.

You can use "check" any time that a command should execute with a 
0-return value and is expected to output nothing to stdout or stderr.

> +
> +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])
> +
> +dnl Attach source port to mirror
> +AT_CHECK([ovn-nbctl lsp-attach-mirror sw0-port1 mirror3])
> +
> +dnl Attach one more source port to mirror
> +AT_CHECK([ovn-nbctl lsp-attach-mirror sw0-port3 mirror3])
> +
> +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
> +AT_CHECK([ovn-nbctl lsp-detach-mirror sw0-port3 mirror3])
> +
> +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.
> +AT_CHECK([ovn-nbctl mirror-del mirror3])
> +
> +dnl Check if the detach happened from source properly
> +AT_CHECK([ovn-nbctl get Logical_Switch_Port sw0-port1 mirror_rules |  cut -b 3], [0], [dnl
> +
> +])

There are some handy functions in ovn-macros.at that can simplify this 
sort of query. In this case, you can use the "check_column" function 
like so:

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
> +AT_CHECK([ovn-nbctl mirror-del mirror2])
> +
> +dnl Update the Sink address
> +AT_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
> +AT_CHECK([ovn-nbctl mirror-del])
> +AT_CHECK([ovn-nbctl mirror-list], [0], [dnl
> +])])
> +
> +dnl ---------------------------------------------------------------------

Overall this is a decent test but it should also ensure that the 
mirror_rules columns in the affected logical switch ports have the 
expected data in them.

> +
>   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 811468dc6..af2e61435 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -271,6 +271,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\
> @@ -311,6 +324,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\
> @@ -1685,6 +1700,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)
>   {
> @@ -7241,6 +7380,211 @@ 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 */
> +    index = atoi(ctx->argv[3]);

It's preferable to use the OVS function str_to_int() here instead of 
atoi(). The reason is that atoi() returns 0 for both an invalid input 
and for the literal "0". str_to_int() returns a bool to indicate whether 
the input was valid or not.

> +
> +    /* 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,
> @@ -7334,6 +7678,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 },
> @@ -7388,6 +7741,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 f60dde1b6..3d73e9e25 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},
>

Hi Mark,

Thanks for your review.
Please see replies inline below

On Tue, Nov 29, 2022 at 3:24 AM Mark Michelson <mmichels@redhat.com> wrote:

> Hi, since this patch series doesn't have a cover letter, I figure I
> would make some top-level comments on the series here on patch 1. First
> off, I suggest running all of the patches through
> utilities/checkpatch.py in OVN. There are quite a few nitpicky coding
> guidelines violations I noticed that it will point out to you.
>
I had indeed run all the patches using the utilities. For the patches 2 and
3
it said "no obvious problems found"
But only for the patch 1/3 it showed me 5 warnings
But those are bit wired ones.
For example...
WARNING: Line lacks whitespace around operator

#1496 FILE: utilities/ovn-nbctl.c:275:

  mirror-add NAME TYPE INDEX FILTER IP\n\
Here ... it expects a space between the hyphen.. but its similar to other
commands.

All the 5 warnings I got were similar ones. Other than those I didn't get
any warnings, so I have
ignored those.
Is there any way to tell the checkpatch to ignore these? Because I don't
see these warnings
for other commands.( like meter-add, meter-list etc)


>
> Overall, I'm not all that familiar with the ins and outs of port
> mirroring (i.e. what the requirements are), so I'm focusing mostly on
> the mechanics in this review. With all that said, I have comments down
> below.
>
> On 11/27/22 15:14, Abhiram R N wrote:
> > In order to support Remote Port Mirroring
> > added the required schemas in NB and SB.
> > Also, nbctl.c and sbctl.c changes are added.
> > Futher added test case 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>
> > ---
> > v16-->v17: No changes
> >
> >   ovn-nb.ovsschema      |  31 +++-
> >   ovn-nb.xml            |  63 ++++++++
> >   ovn-sb.ovsschema      |  26 ++-
> >   ovn-sb.xml            |  50 ++++++
> >   tests/ovn-nbctl.at    | 120 ++++++++++++++
> >   utilities/ovn-nbctl.c | 357 ++++++++++++++++++++++++++++++++++++++++++
> >   utilities/ovn-sbctl.c |   4 +
> >   7 files changed, 647 insertions(+), 4 deletions(-)
> >
> > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> > index 174364c8b..01de55222 100644
> > --- a/ovn-nb.ovsschema
> > +++ b/ovn-nb.ovsschema
> > @@ -1,7 +1,7 @@
> >   {
> >       "name": "OVN_Northbound",
> > -    "version": "6.3.0",
> > -    "cksum": "4042813038 31869",
> > +    "version": "6.4.0",
> > +    "cksum": "589874483 33352",
> >       "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"}},
>
> IMO, there is no reason for both the "mirror_rules" column to exist in
> the logical switch port and for the "src" column to exist in the Mirror
> table. This results in a circular reference that doesn't really seem
> necessary based on my reading of the changes in northd.c . The
> southbound DB doesn't have this problem since there is no "src" column
> in the Port_Binding table.
>
This cross dependency we kept only in NB DB. And it is not there in SB DB.
Numan had asked this point in an earlier version itself.
This is needed for mirror-list. Since lsp can be attached to multiple
mirrors
and also multiple lsps can be attached to 1 mirror this helps while
executing
'mirror-list'. Else to list mirrors with its attachments(srcs) we have to
go through
for each mirror through the list of lsps which is expensive we felt.


> Also, this is a bit nitpicky of me, but why is the column called
> "mirror_rules"? Should it just be called "mirrors"?
>
Nothing specific. I guess we had referred to qos_rules as reference.


>
> >                   "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 7f207a413..671692b49 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.
> > @@ -2527,6 +2532,64 @@
> >       </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 576ebbdeb..b83134416 100644
> > --- a/ovn-sb.ovsschema
> > +++ b/ovn-sb.ovsschema
> > @@ -1,7 +1,7 @@
> >   {
> >       "name": "OVN_Southbound",
> > -    "version": "20.25.0",
> > -    "cksum": "53184112 28845",
> > +    "version": "20.26.0",
> > +    "cksum": "2344151793 30004",
> >       "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 16f57b374..2dd2e304f 100644
> > --- a/ovn-sb.xml
> > +++ b/ovn-sb.xml
> > @@ -2869,6 +2869,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
> > @@ -3371,6 +3416,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 4d480e357..d79f9d929 100644
> > --- a/tests/ovn-nbctl.at
> > +++ b/tests/ovn-nbctl.at
> > @@ -435,6 +435,126 @@ AT_CHECK([ovn-nbctl meter-list], [0], [dnl
> >
> >   dnl
> ---------------------------------------------------------------------
> >
> > +OVN_NBCTL_TEST([ovn_nbctl_mirrors], [mirrors], [
> > +AT_CHECK([ovn-nbctl mirror-add mirror1 gre 0 from-lport 10.10.10.1])
> > +AT_CHECK([ovn-nbctl mirror-add mirror2 erspan 1 both 10.10.10.2])
> > +AT_CHECK([ovn-nbctl mirror-add mirror3 gre 2 to-lport 10.10.10.3])
> > +AT_CHECK([ovn-nbctl ls-add sw0])
> > +AT_CHECK([ovn-nbctl lsp-add sw0 sw0-port1])
> > +AT_CHECK([ovn-nbctl lsp-add sw0 sw0-port2])
> > +AT_CHECK([ovn-nbctl lsp-add sw0 sw0-port3])
>
> Suggestion: Replace uses of AT_CHECK above with "check". It's a bit less
> cluttered and it has the benefit of automatically echoing the command to
> stdout. That can really help when debugging.
>
Ok. Will replace it.

>
> You can use "check" any time that a command should execute with a
> 0-return value and is expected to output nothing to stdout or stderr.
>
Sure.

>
> > +
> > +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])
> > +
> > +dnl Attach source port to mirror
> > +AT_CHECK([ovn-nbctl lsp-attach-mirror sw0-port1 mirror3])
> > +
> > +dnl Attach one more source port to mirror
> > +AT_CHECK([ovn-nbctl lsp-attach-mirror sw0-port3 mirror3])
> > +
> > +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
> > +AT_CHECK([ovn-nbctl lsp-detach-mirror sw0-port3 mirror3])
> > +
> > +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.
> > +AT_CHECK([ovn-nbctl mirror-del mirror3])
> > +
> > +dnl Check if the detach happened from source properly
> > +AT_CHECK([ovn-nbctl get Logical_Switch_Port sw0-port1 mirror_rules |
> cut -b 3], [0], [dnl
> > +
> > +])
>
> There are some handy functions in ovn-macros.at that can simplify this
> sort of query. In this case, you can use the "check_column" function
> like so:
>
> check_column "" nb:Logical_Switch_Port mirror_rules name=sw0-port1
>
Sure. Will try to use them.

>
> > +
> > +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
> > +AT_CHECK([ovn-nbctl mirror-del mirror2])
> > +
> > +dnl Update the Sink address
> > +AT_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
> > +AT_CHECK([ovn-nbctl mirror-del])
> > +AT_CHECK([ovn-nbctl mirror-list], [0], [dnl
> > +])])
> > +
> > +dnl
> ---------------------------------------------------------------------
>
> Overall this is a decent test but it should also ensure that the
> mirror_rules columns in the affected logical switch ports have the
> expected data in them.
>
Sure.

>
> > +
> >   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 811468dc6..af2e61435 100644
> > --- a/utilities/ovn-nbctl.c
> > +++ b/utilities/ovn-nbctl.c
> > @@ -271,6 +271,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\
> > @@ -311,6 +324,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\
> > @@ -1685,6 +1700,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)
> >   {
> > @@ -7241,6 +7380,211 @@ 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 */
> > +    index = atoi(ctx->argv[3]);
>
> It's preferable to use the OVS function str_to_int() here instead of
> atoi(). The reason is that atoi() returns 0 for both an invalid input
> and for the literal "0". str_to_int() returns a bool to indicate whether
> the input was valid or not.
>
Ok. Will use str_to_int.

>
> > +
> > +    /* 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,
> > @@ -7334,6 +7678,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 },
> > @@ -7388,6 +7741,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 f60dde1b6..3d73e9e25 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},
> >
>
> Thanks & Regards,
Abhiram R N

On Tue, Nov 29, 2022 at 7:50 AM Abhiram R N <arn@redhat.com> wrote:
>
> Hi Mark,
>
> Thanks for your review.
> Please see replies inline below
>
> On Tue, Nov 29, 2022 at 3:24 AM Mark Michelson <mmichels@redhat.com> wrote:
>
> > Hi, since this patch series doesn't have a cover letter, I figure I
> > would make some top-level comments on the series here on patch 1. First
> > off, I suggest running all of the patches through
> > utilities/checkpatch.py in OVN. There are quite a few nitpicky coding
> > guidelines violations I noticed that it will point out to you.
> >
> I had indeed run all the patches using the utilities. For the patches 2 and
> 3
> it said "no obvious problems found"
> But only for the patch 1/3 it showed me 5 warnings
> But those are bit wired ones.
> For example...
> WARNING: Line lacks whitespace around operator
>
> #1496 FILE: utilities/ovn-nbctl.c:275:
>
>   mirror-add NAME TYPE INDEX FILTER IP\n\
> Here ... it expects a space between the hyphen.. but its similar to other
> commands.
>
> All the 5 warnings I got were similar ones. Other than those I didn't get
> any warnings, so I have
> ignored those.
> Is there any way to tell the checkpatch to ignore these? Because I don't
> see these warnings
> for other commands.( like meter-add, meter-list etc)
>
>
> >
> > Overall, I'm not all that familiar with the ins and outs of port
> > mirroring (i.e. what the requirements are), so I'm focusing mostly on
> > the mechanics in this review. With all that said, I have comments down
> > below.
> >
> > On 11/27/22 15:14, Abhiram R N wrote:
> > > In order to support Remote Port Mirroring
> > > added the required schemas in NB and SB.
> > > Also, nbctl.c and sbctl.c changes are added.
> > > Futher added test case 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>
> > > ---
> > > v16-->v17: No changes
> > >
> > >   ovn-nb.ovsschema      |  31 +++-
> > >   ovn-nb.xml            |  63 ++++++++
> > >   ovn-sb.ovsschema      |  26 ++-
> > >   ovn-sb.xml            |  50 ++++++
> > >   tests/ovn-nbctl.at    | 120 ++++++++++++++
> > >   utilities/ovn-nbctl.c | 357 ++++++++++++++++++++++++++++++++++++++++++
> > >   utilities/ovn-sbctl.c |   4 +
> > >   7 files changed, 647 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> > > index 174364c8b..01de55222 100644
> > > --- a/ovn-nb.ovsschema
> > > +++ b/ovn-nb.ovsschema
> > > @@ -1,7 +1,7 @@
> > >   {
> > >       "name": "OVN_Northbound",
> > > -    "version": "6.3.0",
> > > -    "cksum": "4042813038 31869",
> > > +    "version": "6.4.0",
> > > +    "cksum": "589874483 33352",
> > >       "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"}},
> >
> > IMO, there is no reason for both the "mirror_rules" column to exist in
> > the logical switch port and for the "src" column to exist in the Mirror
> > table. This results in a circular reference that doesn't really seem
> > necessary based on my reading of the changes in northd.c . The
> > southbound DB doesn't have this problem since there is no "src" column
> > in the Port_Binding table.
> >
> This cross dependency we kept only in NB DB. And it is not there in SB DB.
> Numan had asked this point in an earlier version itself.
> This is needed for mirror-list. Since lsp can be attached to multiple
> mirrors
> and also multiple lsps can be attached to 1 mirror this helps while
> executing
> 'mirror-list'. Else to list mirrors with its attachments(srcs) we have to
> go through
> for each mirror through the list of lsps which is expensive we felt.
>

I agree with Mark and I had the same concerns.  I don't think we
should have circular
dependency just for the sake of displaying in ovn-nbctl.c

I'd suggest removing the "src" column from the Mirror table.
If you think the admin/CMS wants to know the attached ports to a
mirror, then I'd suggest implement it by
looping through all the ports and figuring out the mirrors to which a
port is associated.

I've some comments and suggestions in v18.  Please take a look there.

Numan


>
> > Also, this is a bit nitpicky of me, but why is the column called
> > "mirror_rules"? Should it just be called "mirrors"?
> >
> Nothing specific. I guess we had referred to qos_rules as reference.
>
>
> >
> > >                   "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 7f207a413..671692b49 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.
> > > @@ -2527,6 +2532,64 @@
> > >       </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 576ebbdeb..b83134416 100644
> > > --- a/ovn-sb.ovsschema
> > > +++ b/ovn-sb.ovsschema
> > > @@ -1,7 +1,7 @@
> > >   {
> > >       "name": "OVN_Southbound",
> > > -    "version": "20.25.0",
> > > -    "cksum": "53184112 28845",
> > > +    "version": "20.26.0",
> > > +    "cksum": "2344151793 30004",
> > >       "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 16f57b374..2dd2e304f 100644
> > > --- a/ovn-sb.xml
> > > +++ b/ovn-sb.xml
> > > @@ -2869,6 +2869,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
> > > @@ -3371,6 +3416,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 4d480e357..d79f9d929 100644
> > > --- a/tests/ovn-nbctl.at
> > > +++ b/tests/ovn-nbctl.at
> > > @@ -435,6 +435,126 @@ AT_CHECK([ovn-nbctl meter-list], [0], [dnl
> > >
> > >   dnl
> > ---------------------------------------------------------------------
> > >
> > > +OVN_NBCTL_TEST([ovn_nbctl_mirrors], [mirrors], [
> > > +AT_CHECK([ovn-nbctl mirror-add mirror1 gre 0 from-lport 10.10.10.1])
> > > +AT_CHECK([ovn-nbctl mirror-add mirror2 erspan 1 both 10.10.10.2])
> > > +AT_CHECK([ovn-nbctl mirror-add mirror3 gre 2 to-lport 10.10.10.3])
> > > +AT_CHECK([ovn-nbctl ls-add sw0])
> > > +AT_CHECK([ovn-nbctl lsp-add sw0 sw0-port1])
> > > +AT_CHECK([ovn-nbctl lsp-add sw0 sw0-port2])
> > > +AT_CHECK([ovn-nbctl lsp-add sw0 sw0-port3])
> >
> > Suggestion: Replace uses of AT_CHECK above with "check". It's a bit less
> > cluttered and it has the benefit of automatically echoing the command to
> > stdout. That can really help when debugging.
> >
> Ok. Will replace it.
>
> >
> > You can use "check" any time that a command should execute with a
> > 0-return value and is expected to output nothing to stdout or stderr.
> >
> Sure.
>
> >
> > > +
> > > +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])
> > > +
> > > +dnl Attach source port to mirror
> > > +AT_CHECK([ovn-nbctl lsp-attach-mirror sw0-port1 mirror3])
> > > +
> > > +dnl Attach one more source port to mirror
> > > +AT_CHECK([ovn-nbctl lsp-attach-mirror sw0-port3 mirror3])
> > > +
> > > +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
> > > +AT_CHECK([ovn-nbctl lsp-detach-mirror sw0-port3 mirror3])
> > > +
> > > +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.
> > > +AT_CHECK([ovn-nbctl mirror-del mirror3])
> > > +
> > > +dnl Check if the detach happened from source properly
> > > +AT_CHECK([ovn-nbctl get Logical_Switch_Port sw0-port1 mirror_rules |
> > cut -b 3], [0], [dnl
> > > +
> > > +])
> >
> > There are some handy functions in ovn-macros.at that can simplify this
> > sort of query. In this case, you can use the "check_column" function
> > like so:
> >
> > check_column "" nb:Logical_Switch_Port mirror_rules name=sw0-port1
> >
> Sure. Will try to use them.
>
> >
> > > +
> > > +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
> > > +AT_CHECK([ovn-nbctl mirror-del mirror2])
> > > +
> > > +dnl Update the Sink address
> > > +AT_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
> > > +AT_CHECK([ovn-nbctl mirror-del])
> > > +AT_CHECK([ovn-nbctl mirror-list], [0], [dnl
> > > +])])
> > > +
> > > +dnl
> > ---------------------------------------------------------------------
> >
> > Overall this is a decent test but it should also ensure that the
> > mirror_rules columns in the affected logical switch ports have the
> > expected data in them.
> >
> Sure.
>
> >
> > > +
> > >   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 811468dc6..af2e61435 100644
> > > --- a/utilities/ovn-nbctl.c
> > > +++ b/utilities/ovn-nbctl.c
> > > @@ -271,6 +271,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\
> > > @@ -311,6 +324,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\
> > > @@ -1685,6 +1700,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)
> > >   {
> > > @@ -7241,6 +7380,211 @@ 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 */
> > > +    index = atoi(ctx->argv[3]);
> >
> > It's preferable to use the OVS function str_to_int() here instead of
> > atoi(). The reason is that atoi() returns 0 for both an invalid input
> > and for the literal "0". str_to_int() returns a bool to indicate whether
> > the input was valid or not.
> >
> Ok. Will use str_to_int.
>
> >
> > > +
> > > +    /* 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,
> > > @@ -7334,6 +7678,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 },
> > > @@ -7388,6 +7741,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 f60dde1b6..3d73e9e25 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},
> > >
> >
> > Thanks & Regards,
> Abhiram R N
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>