On Tue, Nov 29, 2022 at 7:50 AM Abhiram R N <[email protected]> wrote:
>
> Hi Mark,
>
> Thanks for your review.
> Please see replies inline below
>
> On Tue, Nov 29, 2022 at 3:24 AM Mark Michelson <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Veda Barrenkala <[email protected]>
> > > Signed-off-by: Abhiram R N <[email protected]>
> > > ---
> > > 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
